| Message ID | 1417028722-29306-1-git-send-email-al.kochet@gmail.com |
|---|---|
| State | RFC |
| Headers | show |
Alexander Kochetkov <al.kochet@gmail.com> writes: > NOT FOR UPSTREAM > > The patch checks if IP reset during probe could bring I2C bus > to a free state on omap2430 - omap3530 boards. > > I guess, IP hold one of I2C lines in a low state. > I guess, u-boot haven't sent a stop condition. > > The patch is rebased on branch 'i2c/for-next' of > git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux > (6e79807443cba7397cd855ed29d6faba51d4c893) > > Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com> > Reported-by: Kevin Hilman <khilman@kernel.org> > Tested-by: Kevin Hilman <khilman@kernel.org> Built for omap2plus_defconfig and tested on all my OMAP boards. Results here: http://people.linaro.org/~khilman/tmp/next-20141126-1-g760388ee02e4/arm-omap2plus_defconfig/ -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Kevin Hilman <khilman@kernel.org> [141126 13:27]: > Alexander Kochetkov <al.kochet@gmail.com> writes: > > > NOT FOR UPSTREAM > > > > The patch checks if IP reset during probe could bring I2C bus > > to a free state on omap2430 - omap3530 boards. > > > > I guess, IP hold one of I2C lines in a low state. > > I guess, u-boot haven't sent a stop condition. > > > > The patch is rebased on branch 'i2c/for-next' of > > git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux > > (6e79807443cba7397cd855ed29d6faba51d4c893) > > > > Signed-off-by: Alexander Kochetkov <al.kochet@gmail.com> > > Reported-by: Kevin Hilman <khilman@kernel.org> > > Tested-by: Kevin Hilman <khilman@kernel.org> > > Built for omap2plus_defconfig and tested on all my OMAP boards. Results > here: > > http://people.linaro.org/~khilman/tmp/next-20141126-1-g760388ee02e4/arm-omap2plus_defconfig/ And below is the output from 2430sdp with linux next plus Alexander's test patch. Looks like for some time 2430 i2c has not been behaving reliably unless I force compatible to ti,omap2420-i2c instead of ti,omap2430-i2c.. The output below is with ti,omap2430-i2c. Regards, Tony 8< ------------- [ 0.913574] omap_i2c 48070000.i2c: init0: STAT=0x0000; IE=0x601f; CON=0x8000; SYSTEST=0x0000; SDA=00 [OI]; SCL=00 [OI] (omap_i2c_init:493) [ 1.931365] omap_i2c 48070000.i2c: timeout waiting for bus ready [ 1.931457] omap_i2c 48070000.i2c: init1: STAT=0x0000; IE=0x601f; CON=0x8000; SYSTEST=0x0000; SDA=00 [OI]; SCL=00 [OI] (omap_i2c_init:497) [ 1.931518] omap_i2c 48070000.i2c: init1: bb_valid=0 [ 2.949249] omap_i2c 48070000.i2c: timeout waiting for bus ready [ 2.949340] omap_i2c 48070000.i2c: init2: STAT=0x0000; IE=0x601f; CON=0x8000; SYSTEST=0x0000; SDA=00 [OI]; SCL=00 [OI] (omap_i2c_init:504) [ 2.949401] omap_i2c 48070000.i2c: init2: bb_valid=0 [ 2.952941] omap_i2c 48070000.i2c: bus 0 rev3.3 at 100 kHz (rev 00000036) [ 2.969299] omap_i2c 48072000.i2c: init0: STAT=0x0000; IE=0x601f; CON=0x8000; SYSTEST=0x0000; SDA=00 [OI]; SCL=00 [OI] (omap_i2c_init:493) [ 3.987091] omap_i2c 48072000.i2c: timeout waiting for bus ready [ 3.987182] omap_i2c 48072000.i2c: init1: STAT=0x0000; IE=0x601f; CON=0x8000; SYSTEST=0x0000; SDA=00 [OI]; SCL=00 [OI] (omap_i2c_init:497) [ 3.987243] omap_i2c 48072000.i2c: init1: bb_valid=0 [ 5.004974] omap_i2c 48072000.i2c: timeout waiting for bus ready [ 5.005096] omap_i2c 48072000.i2c: init2: STAT=0x0000; IE=0x601f; CON=0x8000; SYSTEST=0x0000; SDA=00 [OI]; SCL=00 [OI] (omap_i2c_init:504) [ 5.005126] omap_i2c 48072000.i2c: init2: bb_valid=0 [ 5.017517] omap_i2c 48072000.i2c: bus 1 rev3.3 at 100 kHz (rev 00000036) [ 7.555419] twl4030_keypad 48072000.i2c:twl@48:keypad: OF: linux,keymap property not defined in /ocp/i2c@48072000/twl@48/keypad [ 7.567626] twl4030_keypad 48072000.i2c:twl@48:keypad: Failed to build keymap [ 7.575439] twl4030_keypad: probe of 48072000.i2c:twl@48:keypad failed with error -2 [ 7.599639] input: twl4030_pwrbutton as /devices/platform/ocp/48072000.i2c/i2c-1/1-0048/48072000.i2c:twl@48:pwrbutton/input/input1 [ 7.624450] twl_rtc 48072000.i2c:twl@48:rtc: Power up reset detected. [ 7.631988] twl_rtc 48072000.i2c:twl@48:rtc: Enabling TWL-RTC [ 7.655456] twl_rtc 48072000.i2c:twl@48:rtc: rtc core: registered 48072000.i2c:twl@48 as rtc0 [ 7.923187] i2c /dev entries driver [ 8.246795] twl_rtc 48072000.i2c:twl@48:rtc: hctosys: unable to read the hardware clock [ 9.675994] omap_i2c 48072000.i2c: controller timed out [ 10.704010] omap_i2c 48072000.i2c: controller timed out [ 11.734069] omap_i2c 48072000.i2c: controller timed out root@omap2430sdp:/# [ 12.823638] omap_i2c 48072000.i2c: controller timed out [ 12.834747] twl4030: I2C error -110 reading PIH ISR -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, Tony! I just want to know, is multimaster i2c feature is interesting for TI SOC, so I could send another patches? Or it's better to leave the thing without changes, as current single master version well tested and work? Also I have a draft version of mixed multimaster/slave version. But it could introduce new bugs. Are we ready for that? Thats because IP behavior, sometimes, doesn't correspond to TRMs[4][5]. It's the one of strange IP I ever seen on TI SOC. And TRM not as detailed as DSP TRMs. Yes, I test the patch. But IP handling is really tricky. So, I doubt. Looks, like you haven't seen my response in another thread[1]. So, duplicate it here. 24.11.2014 22:47, Tony Lindgren <tony@atomide.com> *: > If you post a patch, I can test boot with it. Looks like the failing > ones have i2c rev3.3 on 34xx vs rev4.4 on 36xx. > So I doubt we just want to change the test for for a higher rev number > for OMAP_I2C_REV_ON_3430_3530. You 100% right here. My fault. Thank you for giving right directions. Thanks Kevin for running test, so I could debug the reason. Functional mode bits are unimplemented in the SYSTEST register on omap3530. "10:5 Reserved Write 0s for future compatibility. Read returns 0." That was the reason. SYSTEST always 0. It is possible (and I could do it) to implement the bus check using SYSTEST SDA/SCL loopback mode. One more problem, I found that BB-check from the patch[2] sometimes (very rarely) doesn't work as expected. Sometimes the master start transfer while bus is in use by another master. It happens if another master continuously read from eeprom array of 0xff. So, one of problems on the way of running IP in a multimaster mode, is BB-bit behavior. I reported the problem to ti forum[3] and expect some response. Regards, Alexander. [1] http://www.spinics.net/lists/linux-i2c/msg17797.html [2] http://www.spinics.net/lists/linux-i2c/msg17678.html [3] http://e2e.ti.com/support/dsp/davinci_digital_media_processors/f/537/t/383876.aspx [4] omap3530 - TRM - spruf98y [5] AM-DM37x Multimedia Device Silicon Revision 1.x - sprugn4r -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
29 нояб. 2014 г., в 1:13, Tony Lindgren <tony@atomide.com> написал(а): > Looks like for some time 2430 i2c has not been behaving > reliably commit dd74548ddece4b9d68e5528287a272fa552c81d0 ("i2c: omap: resize fifos before each message") dropped check for dev->buf_len. As result, data processing loop cause dev->buf overruns for devices with 16-bit data register (omap2420 only). Found by code review. I have the patch for that. omap2420 actually broken at all. But I'm not ready to send it right now. Part of the patch: - while (num_bytes--) { + while (num_bytes) { w = omap_i2c_read_reg(dev, OMAP_I2C_DATA_REG); *dev->buf++ = w; dev->buf_len--; + num_bytes--; /* * Data reg in 2430, omap3 and * omap4 is 8 bit wide */ if (dev->flags & OMAP_I2C_FLAG_16BIT_DATA_REG) { - *dev->buf++ = w >> 8; - dev->buf_len--; + if (num_bytes) { + *dev->buf++ = w >> 8; + dev->buf_len--; + num_bytes--; + } } } - while (num_bytes--) { + while (num_bytes) { if (dev->errata & I2C_OMAP_ERRATA_I462) { int ret; @@ -931,14 +947,18 @@ static int omap_i2c_transmit_data(struct omap_i2c_dev *dev, u8 num_bytes, w = *dev->buf++; dev->buf_len--; + num_bytes--; /* * Data reg in 2430, omap3 and * omap4 is 8 bit wide */ if (dev->flags & OMAP_I2C_FLAG_16BIT_DATA_REG) { - w |= *dev->buf++ << 8; - dev->buf_len--; + if (num_bytes) { + w |= *dev->buf++ << 8; + dev->buf_len--; + num_bytes--; + } } -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* Alexander Kochetkov <al.kochet@gmail.com> [141128 15:27]: > Hello, Tony! > > I just want to know, is multimaster i2c feature is interesting for TI SOC, > so I could send another patches? Sure and thanks for looking into fixing things. > Or it's better to leave the thing without changes, as current single master version > well tested and work? Well once the fixes are in, I don't see any reason to not add multimaster support. > Also I have a draft version of mixed multimaster/slave version. But it could introduce new bugs. > Are we ready for that? Thats because IP behavior, sometimes, doesn't correspond to TRMs[4][5]. > It's the one of strange IP I ever seen on TI SOC. And TRM not as detailed as DSP TRMs. I think we can pretty easily test the i2c support before things get merged. I guess it should then be possible to loop two i2c controllers and run automated tests on them :) > Looks, like you haven't seen my response in another thread[1]. > So, duplicate it here. Sorry I guess I forgot to reply, let me know if I still missed something. I'll give your two fixes a try on Monday hopefully. Regards, Tony -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c index 4563200..865c9a1 100644 --- a/drivers/i2c/busses/i2c-omap.c +++ b/drivers/i2c/busses/i2c-omap.c @@ -282,6 +282,26 @@ static inline u16 omap_i2c_read_reg(struct omap_i2c_dev *i2c_dev, int reg) (i2c_dev->regs[reg] << i2c_dev->reg_shift)); } +static int omap_i2c_wait_for_bb_valid(struct omap_i2c_dev *dev); + +static inline void +omap_i2c_dump_state(const char *func, int line, struct omap_i2c_dev *dev, const char *msg) +{ + u16 systest = omap_i2c_read_reg(dev, OMAP_I2C_SYSTEST_REG); + dev_info(dev->dev, "%s: STAT=0x%04x; IE=0x%04x; CON=0x%04x; SYSTEST=0x%04x; SDA=%d%d [OI]; SCL=%d%d [OI] (%s:%d)\n", + msg, + omap_i2c_read_reg(dev, OMAP_I2C_STAT_REG), + omap_i2c_read_reg(dev, OMAP_I2C_IE_REG), + omap_i2c_read_reg(dev, OMAP_I2C_CON_REG), + systest, + (systest & OMAP_I2C_SYSTEST_SDA_O_FUNC) ? 1 : 0, + (systest & OMAP_I2C_SYSTEST_SDA_I_FUNC) ? 1 : 0, + (systest & OMAP_I2C_SYSTEST_SCL_O_FUNC) ? 1 : 0, + (systest & OMAP_I2C_SYSTEST_SCL_I_FUNC) ? 1 : 0, + func, line); +} +#define OMAP_I2C_DUMP_STATE(dev, msg) omap_i2c_dump_state(__func__, __LINE__, (dev), (msg)) + static void __omap_i2c_init(struct omap_i2c_dev *dev) { @@ -469,6 +489,23 @@ static int omap_i2c_init(struct omap_i2c_dev *dev) __omap_i2c_init(dev); + msleep(1); + OMAP_I2C_DUMP_STATE(dev, "init0"); + + dev->bb_valid = 0; + omap_i2c_wait_for_bb_valid(dev); + OMAP_I2C_DUMP_STATE(dev, "init1"); + dev_info(dev->dev, "init1: bb_valid=%d\n", dev->bb_valid); + + omap_i2c_reset(dev); + __omap_i2c_init(dev); + dev->bb_valid = 0; + omap_i2c_wait_for_bb_valid(dev); + OMAP_I2C_DUMP_STATE(dev, "init2"); + dev_info(dev->dev, "init2: bb_valid=%d\n", dev->bb_valid); + + dev->bb_valid = 1; + return 0; } @@ -1367,8 +1404,8 @@ omap_i2c_probe(struct platform_device *pdev) goto err_unuse_clocks; } - dev_info(dev->dev, "bus %d rev%d.%d at %d kHz\n", adap->nr, - major, minor, dev->speed); + dev_info(dev->dev, "bus %d rev%d.%d at %d kHz (rev %08x)\n", adap->nr, + major, minor, dev->speed, dev->rev); pm_runtime_mark_last_busy(dev->dev); pm_runtime_put_autosuspend(dev->dev);