[PATCH RFC] i2c: omap: Fix the revision register read

[PATCH RFC] i2c: omap: Fix the revision register read

[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.

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 <shubhrajyoti@ti.com>
---
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(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index db31eae..651a7f7 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -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);
-- 
1.7.5.4

[PATCH RFC] i2c: omap: Fix the revision register read

[Shubhrajyoti Datta]

On Wed, Oct 31, 2012 at 2:29 PM, Shubhrajyoti D <shubhrajyoti@ti.com> wrote:
> 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.
>
tested on omap4sdp, omap3630 based beagle , omap3430sdp.

[PATCH RFC] i2c: omap: Fix the revision register read

[Felipe Balbi]

Hi,

On Wed, Oct 31, 2012 at 02:29:19PM +0530, Shubhrajyoti D wrote:
> 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 ?

> 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

> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> ---
> 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(-)
> 
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index db31eae..651a7f7 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -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 ;-)

>  /* 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.

>  	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.

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

> +	* On omap3 Offset 4 is IE Reg the bit 14 is XDR_IE which is 0 at reset.
> +	*/

please align the * characters.

> +	rev_hi = __raw_readw(dev->base + 0x04);

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

> +
> +	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.

> +		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;

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

-- 
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>

[PATCH RFC] i2c: omap: Fix the revision register read

[Felipe Balbi]

Hi,

On Wed, Oct 31, 2012 at 03:02:57PM +0530, Shubhrajyoti Datta wrote:
> On Wed, Oct 31, 2012 at 2:29 PM, Shubhrajyoti D <shubhrajyoti@ti.com> wrote:
> > 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.
> >
> tested on omap4sdp, omap3630 based beagle , omap3430sdp.

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

-- 
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>

[PATCH RFC] i2c: omap: Fix the revision register read

[Shubhrajyoti]

On Wednesday 31 October 2012 03:42 PM, Felipe Balbi wrote:
> Hi,
>
> On Wed, Oct 31, 2012 at 02:29:19PM +0530, Shubhrajyoti D wrote:
>> 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 ?
>
>> 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
>
>> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
>> ---
>> 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(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
>> index db31eae..651a7f7 100644
>> --- a/drivers/i2c/busses/i2c-omap.c
>> +++ b/drivers/i2c/busses/i2c-omap.c
>> @@ -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;
you mean

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

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

>
> For OMAP4/5:
>
> well, that's a lot more complex, but you have that data ;-)
>
>>  /* 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.
>
>>  	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.
> 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.
>
>> +	* 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
>
>> +	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.
>
>> +
>> +	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.
OK.
>
>> +		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;
> this is wrong, you need to make sure dev->regs is set early enough.

[PATCH RFC] i2c: omap: Fix the revision register read

[Shubhrajyoti]

On Wednesday 31 October 2012 03:42 PM, Felipe Balbi wrote:
>>  
>> > +	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?

[PATCH RFC] i2c: omap: Fix the revision register read

[Felipe Balbi]

Hi,

On Wed, Oct 31, 2012 at 04:18:15PM +0530, Shubhrajyoti wrote:
> On Wednesday 31 October 2012 03:42 PM, Felipe Balbi wrote:
> > Hi,
> >
> > On Wed, Oct 31, 2012 at 02:29:19PM +0530, Shubhrajyoti D wrote:
> >> 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 ?
> >
> >> 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
> >
> >> Signed-off-by: Shubhrajyoti D <shubhrajyoti@ti.com>
> >> ---
> >> 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(-)
> >>
> >> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> >> index db31eae..651a7f7 100644
> >> --- a/drivers/i2c/busses/i2c-omap.c
> >> +++ b/drivers/i2c/busses/i2c-omap.c
> >> @@ -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;
> you mean
> 
> rev major = rev >> 4;
> rev minor = rev & 0xf;

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

> 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.

> > For OMAP4/5:
> >
> > well, that's a lot more complex, but you have that data ;-)
> >
> >>  /* 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.
> >
> >>  	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.
> > 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.
> >
> >> +	* 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
> >
> >> +	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.


-- 
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>

[PATCH RFC] i2c: omap: Fix the revision register read

[Felipe Balbi]

Hi,

On Wed, Oct 31, 2012 at 04:28:38PM +0530, Shubhrajyoti wrote:
> On Wednesday 31 October 2012 03:42 PM, Felipe Balbi wrote:
> >>  
> >> > +	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?

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

cheers

-- 
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>