Skip to contentSkip to conversation

i2c: omap: Fix the revision register read

RFC PATCH New Shubhrajyoti D 3 participants 8 messages Oct 31, 2012

Subsystems (4)
I2C subsystem host drivers OMAP2+ support OMAP1 support OMAP I2C driver
lore
Share & tools
lore

Shubhrajyoti D

author
#0 lore raw

RFC PATCH 1/1 i2c: omap: Fix the revision register read

The revision register on OMAP4 is a 16-bit lo and a 16-bit hi. Currently the driver reads only the lower 8-bits. Fix the same by preventing the truncating of the rev register for OMAP4.

Also use the scheme bit ie bit-14 of the hi register to know if it is OMAP_I2C_IP_VERSION_2.

On platforms previous to OMAP4 the offset 0x04 is IE register whose bit-14 reset value is 0, the code uses the same to its advantage.

The dev->regs is populated after reading the rev_hi. A NULL check has been added in the resume handler to prevent the access before the setting of the regs.

Signed-off-by Shubhrajyoti D <[email protected]>

---
todo: some of the flag checks can be removed in favour of revision check.

 drivers/i2c/busses/i2c-omap.c |   35 +++++++++++++++++++++++++----------
 1 files changed, 25 insertions(+), 10 deletions(-)
drivers/i2c/busses/i2c-omap.c +25 -10
index db31eae..651a7f7 100644
@@ -51,7 +51,8 @@
 /* I2C controller revisions present on specific hardware */
 #define OMAP_I2C_REV_ON_2430		0x36
 #define OMAP_I2C_REV_ON_3430_3530	0x3C
-#define OMAP_I2C_REV_ON_3630_4430	0x40
+#define OMAP_I2C_REV_ON_3630		0x40
+#define OMAP_I2C_REV_ON_4430_PLUS	0x5040
 
 /* timeout waiting for the controller to respond */
 #define OMAP_I2C_TIMEOUT (msecs_to_jiffies(1000))

@@ -202,7 +203,7 @@ struct omap_i2c_dev {
 						 * fifo_size==0 implies no fifo
 						 * if set, should be trsh+1
 						 */
-	u8			rev;
+	u16			rev;
 	unsigned		b_hw:1;		/* bad h/w fixes */
 	unsigned		receiver:1;	/* true when we're in receiver mode */
 	u16			iestate;	/* Saved interrupt register */

@@ -490,7 +491,7 @@ static void omap_i2c_resize_fifo(struct omap_i2c_dev *dev, u8 size, bool is_rx)
 
 	omap_i2c_write_reg(dev, OMAP_I2C_BUF_REG, buf);
 
-	if (dev->rev < OMAP_I2C_REV_ON_3630_4430)
+	if (dev->rev < OMAP_I2C_REV_ON_3630)
 		dev->b_hw = 1; /* Enable hardware fixes */
 
 	/* calculate wakeup latency constraint for MPU */

@@ -1064,6 +1065,8 @@ omap_i2c_probe(struct platform_device *pdev)
 	const struct of_device_id *match;
 	int irq;
 	int r;
+	u16 rev_lo;
+	u16 rev_hi;
 
 	/* NOTE: driver uses the static register mapping */
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);

@@ -1117,11 +1120,6 @@ omap_i2c_probe(struct platform_device *pdev)
 
 	dev->reg_shift = (dev->flags >> OMAP_I2C_FLAG_BUS_SHIFT__SHIFT) & 3;
 
-	if (dev->dtrev == OMAP_I2C_IP_VERSION_2)
-		dev->regs = (u8 *)reg_map_ip_v2;
-	else
-		dev->regs = (u8 *)reg_map_ip_v1;
-
 	pm_runtime_enable(dev->dev);
 	pm_runtime_set_autosuspend_delay(dev->dev, OMAP_I2C_PM_TIMEOUT);
 	pm_runtime_use_autosuspend(dev->dev);

@@ -1130,7 +1128,21 @@ omap_i2c_probe(struct platform_device *pdev)
 	if (IS_ERR_VALUE(r))
 		goto err_free_mem;
 
-	dev->rev = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG) & 0xff;
+	/* Read the Rev hi bit-14 ie scheme this is 1 indicates ver2 or
+	* highlander.
+	* On omap3 Offset 4 is IE Reg the bit 14 is XDR_IE which is 0 at reset.
+	*/
+	rev_hi = __raw_readw(dev->base + 0x04);
+
+	if (rev_hi & 0x4000) {/* If scheme 1*/
+		dev->regs = (u8 *)reg_map_ip_v2;
+		dev->rev = omap_i2c_read_reg(dev, OMAP_I2C_IP_V2_REVNB_HI);
+		rev_lo = omap_i2c_read_reg(dev, OMAP_I2C_IP_V2_REVNB_LO);
+		dev_info(dev->dev, "the low rev %x\n", rev_lo);
+	} else {
+		dev->regs = (u8 *)reg_map_ip_v1;
+		dev->rev = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG) & 0xff;
+	}
 
 	dev->errata = 0;
 

@@ -1155,7 +1167,7 @@ omap_i2c_probe(struct platform_device *pdev)
 
 		dev->fifo_size = (dev->fifo_size / 2);
 
-		if (dev->rev < OMAP_I2C_REV_ON_3630_4430)
+		if (dev->rev < OMAP_I2C_REV_ON_3630)
 			dev->b_hw = 1; /* Enable hardware fixes */
 
 		/* calculate wakeup latency constraint for MPU */

@@ -1264,6 +1276,9 @@ static int omap_i2c_runtime_resume(struct device *dev)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
 
+	if (!_dev->regs)
+		return 0;
+
 	if (_dev->flags & OMAP_I2C_FLAG_RESET_REGS_POSTIDLE) {
 		omap_i2c_write_reg(_dev, OMAP_I2C_CON_REG, 0);
 		omap_i2c_write_reg(_dev, OMAP_I2C_PSC_REG, _dev->pscstate);
Signature · 1 line
1.7.5.4
#1 lore raw

RFC PATCH 1/1 i2c: omap: Fix the revision register read

Quoted 8 lines · Shubhrajyoti D

The revision register on OMAP4 is a 16-bit lo and a 16-bit hi. Currently the driver reads only the lower 8-bits. Fix the same by preventing the truncating of the rev register for OMAP4.

Also use the scheme bit ie bit-14 of the hi register to know if it is OMAP_I2C_IP_VERSION_2.

Quoted 7 lines

On platforms previous to OMAP4 the offset 0x04 is IE register whose bit-14 reset value is 0, the code uses the same to its advantage.

The dev->regs is populated after reading the rev_hi. A NULL check has been added in the resume handler to prevent the access before the setting of the regs.

tested on omap4sdp, omap3630 based beagle , omap3430sdp.

#2 lore raw

RFC PATCH 1/1 i2c: omap: Fix the revision register read

Hi,

Quoted 9 lines · Shubhrajyoti Datta
Quoted 8 lines · Shubhrajyoti D

The revision register on OMAP4 is a 16-bit lo and a 16-bit hi. Currently the driver reads only the lower 8-bits. Fix the same by preventing the truncating of the rev register for OMAP4.

Also use the scheme bit ie bit-14 of the hi register to know if it is OMAP_I2C_IP_VERSION_2.

Quoted 8 lines
Quoted 7 lines

On platforms previous to OMAP4 the offset 0x04 is IE register whose bit-14 reset value is 0, the code uses the same to its advantage.

The dev->regs is populated after reading the rev_hi. A NULL check has been added in the resume handler to prevent the access before the setting of the regs.

tested on omap4sdp, omap3630 based beagle , omap3430sdp.

oh, now I see. You need to test on OMAP2 which has different revision layout.

Signature · 8 lines
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121031/feaae506/attachment.sig>
#3 lore raw

RFC PATCH 1/1 i2c: omap: Fix the revision register read

In reply to Shubhrajyoti D

Hi,

Quoted 4 lines · Shubhrajyoti D

The revision register on OMAP4 is a 16-bit lo and a 16-bit hi. Currently the driver reads only the lower 8-bits. Fix the same by preventing the truncating of the rev register for OMAP4.

very good, but you need to test this with OMAP2/3/4 (5 ??). How was this tested ?

Quoted 9 lines

Also use the scheme bit ie bit-14 of the hi register to know if it is OMAP_I2C_IP_VERSION_2.

On platforms previous to OMAP4 the offset 0x04 is IE register whose bit-14 reset value is 0, the code uses the same to its advantage.

The dev->regs is populated after reading the rev_hi. A NULL check has been added in the resume handler to prevent the access before the setting of the regs.

this could get some rephrasing, I guess. At least to me it's difficult to understand what you mean :-s

Quoted 12 lines
Signed-off-by Shubhrajyoti D <[email protected]>

---
todo: some of the flag checks can be removed in favour of revision check.

 drivers/i2c/busses/i2c-omap.c |   35 +++++++++++++++++++++++++----------
 1 files changed, 25 insertions(+), 10 deletions(-)
drivers/i2c/busses/i2c-omap.c +0 -0
index db31eae..651a7f7 100644

drivers/i2c/busses/i2c-omap.c line 55

Quoted 6 lines
drivers/i2c/busses/i2c-omap.c +2 -1
@@ -51,7 +51,8 @@
 /* I2C controller revisions present on specific hardware */
 #define OMAP_I2C_REV_ON_2430		0x36
 #define OMAP_I2C_REV_ON_3430_3530	0x3C
-#define OMAP_I2C_REV_ON_3630_4430	0x40
+#define OMAP_I2C_REV_ON_3630		0x40
+#define OMAP_I2C_REV_ON_4430_PLUS	0x5040

I would rather see a proper decoding of the revision, meaning that you would:

For omap2/3:

rev major = rev >> 8;
rev minor = rev & 0xff;

For OMAP4/5:

well, that's a lot more complex, but you have that data ;-)

drivers/i2c/busses/i2c-omap.c struct omap_i2c_dev { · line 206

Quoted 8 lines
drivers/i2c/busses/i2c-omap.c +1 -1
 /* timeout waiting for the controller to respond */
 #define OMAP_I2C_TIMEOUT (msecs_to_jiffies(1000))

@@ -202,7 +203,7 @@ struct omap_i2c_dev {
 						 * fifo_size==0 implies no fifo
 						 * if set, should be trsh+1
 						 */
-	u8			rev;
+	u16			rev;

IMHO this should be u32, so you don't need rev_lo and rev_hi below.

Quoted 34 lines
drivers/i2c/busses/i2c-omap.c +3 -6
 	unsigned		b_hw:1;		/* bad h/w fixes */
 	unsigned		receiver:1;	/* true when we're in receiver mode */
 	u16			iestate;	/* Saved interrupt register */

@@ -490,7 +491,7 @@ static void omap_i2c_resize_fifo(struct omap_i2c_dev *dev, u8 size, bool is_rx)
 
 	omap_i2c_write_reg(dev, OMAP_I2C_BUF_REG, buf);
 
-	if (dev->rev < OMAP_I2C_REV_ON_3630_4430)
+	if (dev->rev < OMAP_I2C_REV_ON_3630)
 		dev->b_hw = 1; /* Enable hardware fixes */
 
 	/* calculate wakeup latency constraint for MPU */

@@ -1064,6 +1065,8 @@ omap_i2c_probe(struct platform_device *pdev)
 	const struct of_device_id *match;
 	int irq;
 	int r;
+	u16 rev_lo;
+	u16 rev_hi;
 
 	/* NOTE: driver uses the static register mapping */
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);

@@ -1117,11 +1120,6 @@ omap_i2c_probe(struct platform_device *pdev)
 
 	dev->reg_shift = (dev->flags >> OMAP_I2C_FLAG_BUS_SHIFT__SHIFT) & 3;
 
-	if (dev->dtrev == OMAP_I2C_IP_VERSION_2)
-		dev->regs = (u8 *)reg_map_ip_v2;
-	else
-		dev->regs = (u8 *)reg_map_ip_v1;
-
 	pm_runtime_enable(dev->dev);
 	pm_runtime_set_autosuspend_delay(dev->dev, OMAP_I2C_PM_TIMEOUT);
 	pm_runtime_use_autosuspend(dev->dev);

drivers/i2c/busses/i2c-omap.c omap_i2c_probe(struct platform_device *pdev) · line 1132

Quoted 6 lines
drivers/i2c/busses/i2c-omap.c +2 -1
@@ -1130,7 +1128,21 @@ omap_i2c_probe(struct platform_device *pdev)
 	if (IS_ERR_VALUE(r))
 		goto err_free_mem;
 
-	dev->rev = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG) & 0xff;
+	/* Read the Rev hi bit-14 ie scheme this is 1 indicates ver2 or
+	* highlander.

the "scheme" in highlander is a 2 bit value. In order to make this future proof, you need to read both bits (31:30).

drivers/i2c/busses/i2c-omap.c omap_i2c_probe(struct platform_device *pdev) · line 1134

Quoted 2 lines
drivers/i2c/busses/i2c-omap.c +2 -0
+	* On omap3 Offset 4 is IE Reg the bit 14 is XDR_IE which is 0 at reset.
+	*/

please align the * characters.

drivers/i2c/busses/i2c-omap.c omap_i2c_probe(struct platform_device *pdev) · line 1135

Quoted 1 line
drivers/i2c/busses/i2c-omap.c +1 -0
+	rev_hi = __raw_readw(dev->base + 0x04);

you should make omap_i2c_read_reg() work fine for this case too.

drivers/i2c/busses/i2c-omap.c omap_i2c_probe(struct platform_device *pdev) · line 1137

Quoted 2 lines
drivers/i2c/busses/i2c-omap.c +2 -0
+
+	if (rev_hi & 0x4000) {/* If scheme 1*/

you should add a symbolic define for scheme, something like:

switch (OMAP_I2C_SCHEME(rev)) {
case OMAP_I2C_SCHEME_1:

	foo();
	break;
case OMAP_I2C_SCHEME_2:
	/* FALLTHROUGH */
default:
	bar();
}

note that this will also default to highest known scheme if another scheme is added. You need to make the driver behave like that to decrease amount of rework to support newest OMAPs.

Quoted 21 lines
drivers/i2c/busses/i2c-omap.c +9 -1
+		dev->regs = (u8 *)reg_map_ip_v2;
+		dev->rev = omap_i2c_read_reg(dev, OMAP_I2C_IP_V2_REVNB_HI);
+		rev_lo = omap_i2c_read_reg(dev, OMAP_I2C_IP_V2_REVNB_LO);
+		dev_info(dev->dev, "the low rev %x\n", rev_lo);
+	} else {
+		dev->regs = (u8 *)reg_map_ip_v1;
+		dev->rev = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG) & 0xff;
+	}
 
 	dev->errata = 0;
 

@@ -1155,7 +1167,7 @@ omap_i2c_probe(struct platform_device *pdev)
 
 		dev->fifo_size = (dev->fifo_size / 2);
 
-		if (dev->rev < OMAP_I2C_REV_ON_3630_4430)
+		if (dev->rev < OMAP_I2C_REV_ON_3630)
 			dev->b_hw = 1; /* Enable hardware fixes */
 
 		/* calculate wakeup latency constraint for MPU */

drivers/i2c/busses/i2c-omap.c static int omap_i2c_runtime_resume(struct device *dev) · line 1280

Quoted 5 lines
drivers/i2c/busses/i2c-omap.c +2 -0
@@ -1264,6 +1276,9 @@ static int omap_i2c_runtime_resume(struct device *dev)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
 
+	if (!_dev->regs)
+		return 0;

this is wrong, you need to make sure dev->regs is set early enough.

Signature · 8 lines
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121031/78efcc5c/attachment-0001.sig>
author
#4 lore raw

RFC PATCH 1/1 i2c: omap: Fix the revision register read

Quoted 43 lines · Felipe Balbi

Hi,

Quoted 4 lines · Shubhrajyoti D

The revision register on OMAP4 is a 16-bit lo and a 16-bit hi. Currently the driver reads only the lower 8-bits. Fix the same by preventing the truncating of the rev register for OMAP4.

very good, but you need to test this with OMAP2/3/4 (5 ??). How was this tested ?

Quoted 9 lines

Also use the scheme bit ie bit-14 of the hi register to know if it is OMAP_I2C_IP_VERSION_2.

On platforms previous to OMAP4 the offset 0x04 is IE register whose bit-14 reset value is 0, the code uses the same to its advantage.

The dev->regs is populated after reading the rev_hi. A NULL check has been added in the resume handler to prevent the access before the setting of the regs.

this could get some rephrasing, I guess. At least to me it's difficult to understand what you mean :-s

Quoted 12 lines
Signed-off-by Shubhrajyoti D <[email protected]>

---
todo: some of the flag checks can be removed in favour of revision check.

 drivers/i2c/busses/i2c-omap.c |   35 +++++++++++++++++++++++++----------
 1 files changed, 25 insertions(+), 10 deletions(-)
drivers/i2c/busses/i2c-omap.c +0 -0
index db31eae..651a7f7 100644

drivers/i2c/busses/i2c-omap.c line 55

Quoted 6 lines
drivers/i2c/busses/i2c-omap.c +2 -1
@@ -51,7 +51,8 @@
 /* I2C controller revisions present on specific hardware */
 #define OMAP_I2C_REV_ON_2430		0x36
 #define OMAP_I2C_REV_ON_3430_3530	0x3C
-#define OMAP_I2C_REV_ON_3630_4430	0x40
+#define OMAP_I2C_REV_ON_3630		0x40
+#define OMAP_I2C_REV_ON_4430_PLUS	0x5040

I would rather see a proper decoding of the revision, meaning that you would:

Quoted 4 lines

For omap2/3:

rev major = rev >> 8;
rev minor = rev & 0xff;

you mean

rev major = rev >> 4;
rev minor = rev & 0xf;

thats doable too. However that currently that is read together currently.

Quoted 52 lines

For OMAP4/5:

well, that's a lot more complex, but you have that data ;-)

drivers/i2c/busses/i2c-omap.c struct omap_i2c_dev { · line 206

Quoted 8 lines
drivers/i2c/busses/i2c-omap.c +1 -1
 /* timeout waiting for the controller to respond */
 #define OMAP_I2C_TIMEOUT (msecs_to_jiffies(1000))

@@ -202,7 +203,7 @@ struct omap_i2c_dev {
 						 * fifo_size==0 implies no fifo
 						 * if set, should be trsh+1
 						 */
-	u8			rev;
+	u16			rev;

IMHO this should be u32, so you don't need rev_lo and rev_hi below.

Quoted 37 lines
drivers/i2c/busses/i2c-omap.c +3 -6
 	unsigned		b_hw:1;		/* bad h/w fixes */
 	unsigned		receiver:1;	/* true when we're in receiver mode */
 	u16			iestate;	/* Saved interrupt register */

@@ -490,7 +491,7 @@ static void omap_i2c_resize_fifo(struct omap_i2c_dev *dev, u8 size, bool is_rx)
 
 	omap_i2c_write_reg(dev, OMAP_I2C_BUF_REG, buf);
 
-	if (dev->rev < OMAP_I2C_REV_ON_3630_4430)
+	if (dev->rev < OMAP_I2C_REV_ON_3630)
 		dev->b_hw = 1; /* Enable hardware fixes */
 
 	/* calculate wakeup latency constraint for MPU */

@@ -1064,6 +1065,8 @@ omap_i2c_probe(struct platform_device *pdev)
 	const struct of_device_id *match;
 	int irq;
 	int r;
+	u16 rev_lo;
+	u16 rev_hi;
 
 	/* NOTE: driver uses the static register mapping */
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);

@@ -1117,11 +1120,6 @@ omap_i2c_probe(struct platform_device *pdev)
 
 	dev->reg_shift = (dev->flags >> OMAP_I2C_FLAG_BUS_SHIFT__SHIFT) & 3;
 
-	if (dev->dtrev == OMAP_I2C_IP_VERSION_2)
-		dev->regs = (u8 *)reg_map_ip_v2;
-	else
-		dev->regs = (u8 *)reg_map_ip_v1;
-
 	pm_runtime_enable(dev->dev);
 	pm_runtime_set_autosuspend_delay(dev->dev, OMAP_I2C_PM_TIMEOUT);
 	pm_runtime_use_autosuspend(dev->dev);

@@ -1130,7 +1128,21 @@ omap_i2c_probe(struct platform_device *pdev)
 	if (IS_ERR_VALUE(r))
 		goto err_free_mem;
Quoted 5 lines

drivers/i2c/busses/i2c-omap.c omap_i2c_probe(struct platform_device *pdev) · line 1132

Quoted 3 lines
drivers/i2c/busses/i2c-omap.c +2 -1
-	dev->rev = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG) & 0xff;
+	/* Read the Rev hi bit-14 ie scheme this is 1 indicates ver2 or
+	* highlander.

the "scheme" in highlander is a 2 bit value. In order to make this future proof, you need to read both bits (31:30).

Good point will fix it.

Quoted 4 lines

drivers/i2c/busses/i2c-omap.c omap_i2c_probe(struct platform_device *pdev) · line 1134

Quoted 3 lines
drivers/i2c/busses/i2c-omap.c +2 -0
+	* On omap3 Offset 4 is IE Reg the bit 14 is XDR_IE which is 0 at reset.
+	*/

please align the * characters.

yes will repost

Quoted 3 lines

drivers/i2c/busses/i2c-omap.c omap_i2c_probe(struct platform_device *pdev) · line 1135

Quoted 2 lines
drivers/i2c/busses/i2c-omap.c +1 -0
+	rev_hi = __raw_readw(dev->base + 0x04);

you should make omap_i2c_read_reg() work fine for this case too.

Just felt it is more readlable this way also I didnt want to use the reg_shift etc.

which also may get cleaned up sometime.

Quoted 15 lines

drivers/i2c/busses/i2c-omap.c omap_i2c_probe(struct platform_device *pdev) · line 1137

Quoted 3 lines
drivers/i2c/busses/i2c-omap.c +2 -0
+
+	if (rev_hi & 0x4000) {/* If scheme 1*/

you should add a symbolic define for scheme, something like:

switch (OMAP_I2C_SCHEME(rev)) {
case OMAP_I2C_SCHEME_1:

	foo();
	break;
case OMAP_I2C_SCHEME_2:
	/* FALLTHROUGH */
default:
	bar();
}
Quoted 3 lines

note that this will also default to highest known scheme if another scheme is added. You need to make the driver behave like that to decrease amount of rework to support newest OMAPs.

OK.

Quoted 28 lines
Quoted 22 lines
drivers/i2c/busses/i2c-omap.c +9 -1
+		dev->regs = (u8 *)reg_map_ip_v2;
+		dev->rev = omap_i2c_read_reg(dev, OMAP_I2C_IP_V2_REVNB_HI);
+		rev_lo = omap_i2c_read_reg(dev, OMAP_I2C_IP_V2_REVNB_LO);
+		dev_info(dev->dev, "the low rev %x\n", rev_lo);
+	} else {
+		dev->regs = (u8 *)reg_map_ip_v1;
+		dev->rev = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG) & 0xff;
+	}
 
 	dev->errata = 0;
 

@@ -1155,7 +1167,7 @@ omap_i2c_probe(struct platform_device *pdev)
 
 		dev->fifo_size = (dev->fifo_size / 2);
 
-		if (dev->rev < OMAP_I2C_REV_ON_3630_4430)
+		if (dev->rev < OMAP_I2C_REV_ON_3630)
 			dev->b_hw = 1; /* Enable hardware fixes */
 
 		/* calculate wakeup latency constraint for MPU */

drivers/i2c/busses/i2c-omap.c static int omap_i2c_runtime_resume(struct device *dev) · line 1280

Quoted 5 lines
drivers/i2c/busses/i2c-omap.c +2 -0
@@ -1264,6 +1276,9 @@ static int omap_i2c_runtime_resume(struct device *dev)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct omap_i2c_dev *_dev = platform_get_drvdata(pdev);
 
+	if (!_dev->regs)
+		return 0;

this is wrong, you need to make sure dev->regs is set early enough.

#5 lore raw

RFC PATCH 1/1 i2c: omap: Fix the revision register read

Hi,

Quoted 44 lines · Shubhrajyoti
Quoted 43 lines · Felipe Balbi

Hi,

Quoted 4 lines · Shubhrajyoti D

The revision register on OMAP4 is a 16-bit lo and a 16-bit hi. Currently the driver reads only the lower 8-bits. Fix the same by preventing the truncating of the rev register for OMAP4.

very good, but you need to test this with OMAP2/3/4 (5 ??). How was this tested ?

Quoted 9 lines

Also use the scheme bit ie bit-14 of the hi register to know if it is OMAP_I2C_IP_VERSION_2.

On platforms previous to OMAP4 the offset 0x04 is IE register whose bit-14 reset value is 0, the code uses the same to its advantage.

The dev->regs is populated after reading the rev_hi. A NULL check has been added in the resume handler to prevent the access before the setting of the regs.

this could get some rephrasing, I guess. At least to me it's difficult to understand what you mean :-s

Quoted 12 lines
Signed-off-by Shubhrajyoti D <[email protected]>

---
todo: some of the flag checks can be removed in favour of revision check.

 drivers/i2c/busses/i2c-omap.c |   35 +++++++++++++++++++++++++----------
 1 files changed, 25 insertions(+), 10 deletions(-)
drivers/i2c/busses/i2c-omap.c +0 -0
index db31eae..651a7f7 100644

drivers/i2c/busses/i2c-omap.c line 55

Quoted 6 lines
drivers/i2c/busses/i2c-omap.c +2 -1
@@ -51,7 +51,8 @@
 /* I2C controller revisions present on specific hardware */
 #define OMAP_I2C_REV_ON_2430		0x36
 #define OMAP_I2C_REV_ON_3430_3530	0x3C
-#define OMAP_I2C_REV_ON_3630_4430	0x40
+#define OMAP_I2C_REV_ON_3630		0x40
+#define OMAP_I2C_REV_ON_4430_PLUS	0x5040

I would rather see a proper decoding of the revision, meaning that you would:

Quoted 8 lines
Quoted 4 lines

For omap2/3:

rev major = rev >> 8;
rev minor = rev & 0xff;

you mean

rev major = rev >> 4;
rev minor = rev & 0xf;

might be, I didn't look at the TRM to make sure, my bad :-)

Quoted 2 lines

thats doable too. However that currently that is read together currently.

and that's what's wrong IMHO. What's current in driver is only valid for OMAP1 IIRC.

Quoted 63 lines
Quoted 51 lines

For OMAP4/5:

well, that's a lot more complex, but you have that data ;-)

drivers/i2c/busses/i2c-omap.c struct omap_i2c_dev { · line 206

Quoted 8 lines
drivers/i2c/busses/i2c-omap.c +1 -1
 /* timeout waiting for the controller to respond */
 #define OMAP_I2C_TIMEOUT (msecs_to_jiffies(1000))

@@ -202,7 +203,7 @@ struct omap_i2c_dev {
 						 * fifo_size==0 implies no fifo
 						 * if set, should be trsh+1
 						 */
-	u8			rev;
+	u16			rev;

IMHO this should be u32, so you don't need rev_lo and rev_hi below.

Quoted 37 lines
drivers/i2c/busses/i2c-omap.c +3 -6
 	unsigned		b_hw:1;		/* bad h/w fixes */
 	unsigned		receiver:1;	/* true when we're in receiver mode */
 	u16			iestate;	/* Saved interrupt register */

@@ -490,7 +491,7 @@ static void omap_i2c_resize_fifo(struct omap_i2c_dev *dev, u8 size, bool is_rx)
 
 	omap_i2c_write_reg(dev, OMAP_I2C_BUF_REG, buf);
 
-	if (dev->rev < OMAP_I2C_REV_ON_3630_4430)
+	if (dev->rev < OMAP_I2C_REV_ON_3630)
 		dev->b_hw = 1; /* Enable hardware fixes */
 
 	/* calculate wakeup latency constraint for MPU */

@@ -1064,6 +1065,8 @@ omap_i2c_probe(struct platform_device *pdev)
 	const struct of_device_id *match;
 	int irq;
 	int r;
+	u16 rev_lo;
+	u16 rev_hi;
 
 	/* NOTE: driver uses the static register mapping */
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);

@@ -1117,11 +1120,6 @@ omap_i2c_probe(struct platform_device *pdev)
 
 	dev->reg_shift = (dev->flags >> OMAP_I2C_FLAG_BUS_SHIFT__SHIFT) & 3;
 
-	if (dev->dtrev == OMAP_I2C_IP_VERSION_2)
-		dev->regs = (u8 *)reg_map_ip_v2;
-	else
-		dev->regs = (u8 *)reg_map_ip_v1;
-
 	pm_runtime_enable(dev->dev);
 	pm_runtime_set_autosuspend_delay(dev->dev, OMAP_I2C_PM_TIMEOUT);
 	pm_runtime_use_autosuspend(dev->dev);

@@ -1130,7 +1128,21 @@ omap_i2c_probe(struct platform_device *pdev)
 	if (IS_ERR_VALUE(r))
 		goto err_free_mem;
Quoted 5 lines

drivers/i2c/busses/i2c-omap.c omap_i2c_probe(struct platform_device *pdev) · line 1132

Quoted 3 lines
drivers/i2c/busses/i2c-omap.c +2 -1
-	dev->rev = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG) & 0xff;
+	/* Read the Rev hi bit-14 ie scheme this is 1 indicates ver2 or
+	* highlander.

the "scheme" in highlander is a 2 bit value. In order to make this future proof, you need to read both bits (31:30).

Good point will fix it.

Quoted 4 lines

drivers/i2c/busses/i2c-omap.c omap_i2c_probe(struct platform_device *pdev) · line 1134

Quoted 3 lines
drivers/i2c/busses/i2c-omap.c +2 -0
+	* On omap3 Offset 4 is IE Reg the bit 14 is XDR_IE which is 0 at reset.
+	*/

please align the * characters.

yes will repost

Quoted 1 line
Quoted 7 lines
Quoted 2 lines

drivers/i2c/busses/i2c-omap.c omap_i2c_probe(struct platform_device *pdev) · line 1135

Quoted 1 line
drivers/i2c/busses/i2c-omap.c +1 -0
+	rev_hi = __raw_readw(dev->base + 0x04);

you should make omap_i2c_read_reg() work fine for this case too.

Just felt it is more readlable this way also I didnt want to use the reg_shift etc.

which also may get cleaned up sometime.

correct, but I think for now using omap_i2c_read_reg() is fine.

Maybe defining (for now) REV_HI and REV_LO on both reg_maps would do it? Just for the time being until we can get rid of reg_shift and omap_i2c_ip_version ??

I don't know, but to me using __raw_readl() with a hardcoded offset is a bit odd.

Signature · 8 lines
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121031/da098f2c/attachment.sig>
author
#6 lore raw

RFC PATCH 1/1 i2c: omap: Fix the revision register read

In reply to Felipe Balbi
Quoted 4 lines · Felipe Balbi
Quoted 3 lines

drivers/i2c/busses/i2c-omap.c static int omap_i2c_runtime_resume(struct device *dev) · line 1280

Quoted 3 lines
drivers/i2c/busses/i2c-omap.c +2 -0
+	if (!_dev->regs)
+		return 0;

this is wrong, you need to make sure dev->regs is set early enough.

to set the dev->regs I use the value read from revision register.

to read the revision register I do a get_sync first. This in turn leads to the call to resume handler.
At this point the regs is NULL and so the check.

After reading the register and checking the scheme bit. the reg map is populated.

How can this issue be solved?

#7 lore raw

RFC PATCH 1/1 i2c: omap: Fix the revision register read

Hi,

Quoted 9 lines · Shubhrajyoti
Quoted 4 lines · Felipe Balbi
Quoted 3 lines

drivers/i2c/busses/i2c-omap.c static int omap_i2c_runtime_resume(struct device *dev) · line 1280

Quoted 3 lines
drivers/i2c/busses/i2c-omap.c +2 -0
+	if (!_dev->regs)
+		return 0;

this is wrong, you need to make sure dev->regs is set early enough.

to set the dev->regs I use the value read from revision register.

Quoted 8 lines

to read the revision register I do a get_sync first. This in turn leads to the call to resume handler.
At this point the regs is NULL and so the check.

After reading the register and checking the scheme bit. the reg map is populated.

How can this issue be solved?

aaa good point, missed that. Nevermind that comment ;-)

cheers

Signature · 8 lines
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20121031/cc7b16de/attachment.sig>