| Message ID | 20131127171026.237fc859@skate |
|---|---|
| State | Not Applicable |
| Delegated to: | Tom Rini |
| Headers | show |
On 11/27/2013 06:10 PM, Thomas Petazzoni wrote: > Dear Nikita Kiryanov, > > On Wed, 27 Nov 2013 17:52:31 +0200, Nikita Kiryanov wrote: > >> The zeroing of the cnt register also happens in other places in the >> driver, and it is entirely possible that they should be removed for >> OMAP3s as well. >> >> In my patch I removed it only for i2c_write() because the original >> driver also zeroed the cnt register in I/O functions- except in >> i2c_write(), and I decided to follow the original driver's lead. >> >> What happens if you remove all the lines that zero the cnt register? > > It works. I've removed all the writew(0, ...cnt register...), and then > I've done at least 20 boots of the IGEP board, and all of them were > successful. > > I've attached the ugly patch that comments out all of those cases. Of > course, it's not a patch intended for merging, just to show which > locations I've commented. > > Since I've absolutely no idea of the background for the problem, would > you mind submitting a proper patch with a good explanation? I will be > happy to test it, of course. Sure. I'm about to leave office though, so I'll prepare it tomorrow. > > Thanks! > > Thomas >
Hi all, 2013/11/27 Nikita Kiryanov <nikita@compulab.co.il>: > On 11/27/2013 06:10 PM, Thomas Petazzoni wrote: >> >> Dear Nikita Kiryanov, >> >> On Wed, 27 Nov 2013 17:52:31 +0200, Nikita Kiryanov wrote: >> >>> The zeroing of the cnt register also happens in other places in the >>> driver, and it is entirely possible that they should be removed for >>> OMAP3s as well. >>> >>> In my patch I removed it only for i2c_write() because the original >>> driver also zeroed the cnt register in I/O functions- except in >>> i2c_write(), and I decided to follow the original driver's lead. >>> >>> What happens if you remove all the lines that zero the cnt register? >> >> >> It works. I've removed all the writew(0, ...cnt register...), and then >> I've done at least 20 boots of the IGEP board, and all of them were >> successful. >> >> I've attached the ugly patch that comments out all of those cases. Of >> course, it's not a patch intended for merging, just to show which >> locations I've commented. >> >> Since I've absolutely no idea of the background for the problem, would >> you mind submitting a proper patch with a good explanation? I will be >> happy to test it, of course. > > > Sure. I'm about to leave office though, so I'll prepare it tomorrow. > >> >> Thanks! >> >> Thomas >> > > > -- > Regards, > Nikita. Maybe I'm wrong but I think the problem could be related to that OMAP3430 and OMAP3630 has a different I2C revision and current code don't handle this correctly, As I know the I2C revision on OMAP3630 and OMAP4 is the same but different than OMAP3530. If the Tom's board uses the OMAP3530 processor this will explain why he's not affected for the issue. Others like me and Thomas, using DM3730, have the problem because the driver do not handle this, it supposes that we're using an OMAP3530. See for example the following code in drivers/i2c/omap24xx_i2c.c, #if defined(CONFIG_OMAP243X) || defined(CONFIG_OMAP34XX) /* * Have to enable interrupts for OMAP2/3, these IPs don't have * an 'irqstatus_raw' register and we shall have to poll 'stat' */ writew(I2C_IE_XRDY_IE | I2C_IE_RRDY_IE | I2C_IE_ARDY_IE | I2C_IE_NACK_IE | I2C_IE_AL_IE, &i2c_base->ie); #endif This is right for OMAP34/35xx but not for OMAP36xx and AM/DM37xx As a reference see the following patch introduced in the kernel: commit f518b482c89b3ff51804f09c14b1cedbef811b84 Author: Jon Hunter <jon-hunter@ti.com> Date: Thu Jun 28 20:41:31 2012 +0530 i2c: omap: Correct I2C revision for OMAP3 The OMAP3530 is based upon the same silicon as the OMAP3430 and so the I2C revision is the same for 3430 and 3530. However, the OMAP3630 device has the same I2C revision as OMAP4. Correct the revision definition to reflect this. This patch is based on work done by Jon Hunter <jon-hunter@ti.com> Changes from his patch - Update OMAP_I2C_REV_ON_3430 also to reflect that it is same as 3530 Reviewed-by: Felipe Balbi <balbi@ti.com> Signed-off-by: Jon Hunter <jon-hunter@ti.com> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com> Signed-off-by: Wolfram Sang <w.sang@pengutronix.de> Best regards, Enric
On 11/27/2013 07:12 PM, Enric Balletbo Serra wrote: > Hi all, > > 2013/11/27 Nikita Kiryanov <nikita@compulab.co.il>: >> On 11/27/2013 06:10 PM, Thomas Petazzoni wrote: >>> >>> Dear Nikita Kiryanov, >>> >>> On Wed, 27 Nov 2013 17:52:31 +0200, Nikita Kiryanov wrote: >>> >>>> The zeroing of the cnt register also happens in other places in the >>>> driver, and it is entirely possible that they should be removed for >>>> OMAP3s as well. >>>> >>>> In my patch I removed it only for i2c_write() because the original >>>> driver also zeroed the cnt register in I/O functions- except in >>>> i2c_write(), and I decided to follow the original driver's lead. >>>> >>>> What happens if you remove all the lines that zero the cnt register? >>> >>> >>> It works. I've removed all the writew(0, ...cnt register...), and then >>> I've done at least 20 boots of the IGEP board, and all of them were >>> successful. >>> >>> I've attached the ugly patch that comments out all of those cases. Of >>> course, it's not a patch intended for merging, just to show which >>> locations I've commented. >>> >>> Since I've absolutely no idea of the background for the problem, would >>> you mind submitting a proper patch with a good explanation? I will be >>> happy to test it, of course. >> >> >> Sure. I'm about to leave office though, so I'll prepare it tomorrow. >> >>> >>> Thanks! >>> >>> Thomas >>> >> >> >> -- >> Regards, >> Nikita. > > > Maybe I'm wrong but I think the problem could be related to that > OMAP3430 and OMAP3630 has a different I2C revision and current code > don't handle this correctly, As I know the I2C revision on OMAP3630 > and OMAP4 is the same but different than OMAP3530. If the Tom's board > uses the OMAP3530 processor this will explain why he's not affected > for the issue. Others like me and Thomas, using DM3730, have the > problem because the driver do not handle this, it supposes that we're > using an OMAP3530. Well, this issue is mentioned in the errata for both OMAP3530 and DM3730, and the TRM for OMAP4430 mentions the same thing. It seems to me that the zeroing of cnt register being problematic is something the different OMAPs have in common. Either way, these lines seem unnecessary, so I'm still in favor of removing them. > > Best regards, > Enric >
diff --git a/drivers/i2c/omap24xx_i2c.c b/drivers/i2c/omap24xx_i2c.c index ef38d71..54c36d8 100644 --- a/drivers/i2c/omap24xx_i2c.c +++ b/drivers/i2c/omap24xx_i2c.c @@ -165,7 +165,7 @@ void i2c_init(int speed, int slaveadd) udelay(1000); flush_fifo(); writew(0xFFFF, &i2c_base->stat); - writew(0, &i2c_base->cnt); + // writew(0, &i2c_base->cnt); if (gd->flags & GD_FLG_RELOC) bus_initialized[current_bus] = 1; @@ -205,7 +205,7 @@ int i2c_probe(uchar chip) return res; /* No data transfer, slave addr only */ - writew(0, &i2c_base->cnt); + //writew(0, &i2c_base->cnt); /* Set slave address */ writew(chip, &i2c_base->sa); /* Stop bit needed here */ @@ -241,7 +241,7 @@ int i2c_probe(uchar chip) pr_exit: flush_fifo(); writew(0xFFFF, &i2c_base->stat); - writew(0, &i2c_base->cnt); + //writew(0, &i2c_base->cnt); return res; } @@ -377,7 +377,7 @@ int i2c_read(uchar chip, uint addr, int alen, uchar *buffer, int len) rd_exit: flush_fifo(); writew(0xFFFF, &i2c_base->stat); - writew(0, &i2c_base->cnt); + //writew(0, &i2c_base->cnt); return i2c_error; } @@ -476,7 +476,7 @@ int i2c_write(uchar chip, uint addr, int alen, uchar *buffer, int len) wr_exit: flush_fifo(); writew(0xFFFF, &i2c_base->stat); - writew(0, &i2c_base->cnt); + //writew(0, &i2c_base->cnt); return i2c_error; }