Re: [PATCH 1/2] i2c: omap: Fix the revision register read
On Fri, Nov 2, 2012 at 4:36 PM, Felipe Balbi wrote:
> Hi,
>
> On Fri, Nov 02, 2012 at 04:09:44PM +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.
>>
>> 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.
>>
>> Also since the omap_i2c_read_reg uses reg_map_ip_* a raw_readw is done
>> to fetch the revision register.
>>
>> 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 which platforms ?
omap4430 , 4460 and omap3.
>
>> Signed-off-by: Shubhrajyoti D
>> ---
>> drivers/i2c/busses/i2c-omap.c | 59
>> -
>> 1 files changed, 46 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
>> index db31eae..d8e7709 100644
>> --- a/drivers/i2c/busses/i2c-omap.c
>> +++ b/drivers/i2c/busses/i2c-omap.c
>> @@ -49,9 +49,10 @@
>> #define OMAP_I2C_OMAP1_REV_2 0x20
>>
>> /* I2C controller revisions present on specific hardware */
>> -#define OMAP_I2C_REV_ON_2430 0x36
>> -#define OMAP_I2C_REV_ON_3430_35300x3C
>> -#define OMAP_I2C_REV_ON_3630_44300x40
>> +#define OMAP_I2C_REV_ON_2430 0x0036
>
> are you sure this are the contents on 2430 ? Have you actually ran this
> code on that platform ?
I did not run on 2430. Will try to get a platform and run.
However the current code already has and uses the same value so
I feel it should be fine.:-)
>
>> +#define OMAP_I2C_REV_ON_3430_35300x003C
>> +#define OMAP_I2C_REV_ON_3630 0x0040
>
> likewise for these two.
I verified that the 3630 returns 0x40 on my beaglexM.
>
>> +#define OMAP_I2C_REV_ON_4430_PLUS0x5042
>
> what about 4460, 4470, 3530, etc etc etc ?
>
>> @@ -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 */
>> @@ -1052,6 +1053,14 @@ static const struct of_device_id omap_i2c_of_match[]
>> = {
>> MODULE_DEVICE_TABLE(of, omap_i2c_of_match);
>> #endif
>>
>> +#define OMAP_I2C_SCHEME(rev) (rev & 0xc000) >> 14
>
> you miss () there to make sure no other operations will take higher
> precedence when using this macro.
Indeed. Thanks.
>
>> @@ -1130,7 +1136,31 @@ 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-[15:14] ie scheme this is 1 indicates ver2.
>> + * On omap3 Offset 4 is IE Reg the bit [15:14] is XDR_IE which is 0
>> + * at reset. Also since the omap_i2c_read_reg uses reg_map_ip_* a
>> + * raw_readw is done.
>> + */
>> + rev = __raw_readw(dev->base + 0x04);
>> +
>> + switch (OMAP_I2C_SCHEME(rev)) {
>> + case 0:
>
> define macros for these two cases.
OK
>
>> + dev->regs = (u8 *)reg_map_ip_v1;
>> + dev->rev = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG) & 0xff;
>> + minor = OMAP_I2C_REV_SCHEME_0_MAJOR(dev->rev);
>> + major = OMAP_I2C_REV_SCHEME_0_MAJOR(dev->rev);
>> + break;
>
> wrong indentation.
Yes will fix.
>
> --
> balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] i2c: omap: Fix the revision register read
Hi,
On Fri, Nov 02, 2012 at 04:09:44PM +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.
>
> 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.
>
> Also since the omap_i2c_read_reg uses reg_map_ip_* a raw_readw is done
> to fetch the revision register.
>
> 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 which platforms ?
> Signed-off-by: Shubhrajyoti D
> ---
> drivers/i2c/busses/i2c-omap.c | 59
> -
> 1 files changed, 46 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index db31eae..d8e7709 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -49,9 +49,10 @@
> #define OMAP_I2C_OMAP1_REV_2 0x20
>
> /* I2C controller revisions present on specific hardware */
> -#define OMAP_I2C_REV_ON_2430 0x36
> -#define OMAP_I2C_REV_ON_3430_35300x3C
> -#define OMAP_I2C_REV_ON_3630_44300x40
> +#define OMAP_I2C_REV_ON_2430 0x0036
are you sure this are the contents on 2430 ? Have you actually ran this
code on that platform ?
> +#define OMAP_I2C_REV_ON_3430_35300x003C
> +#define OMAP_I2C_REV_ON_3630 0x0040
likewise for these two.
> +#define OMAP_I2C_REV_ON_4430_PLUS0x5042
what about 4460, 4470, 3530, etc etc etc ?
> @@ -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 */
> @@ -1052,6 +1053,14 @@ static const struct of_device_id omap_i2c_of_match[] =
> {
> MODULE_DEVICE_TABLE(of, omap_i2c_of_match);
> #endif
>
> +#define OMAP_I2C_SCHEME(rev) (rev & 0xc000) >> 14
you miss () there to make sure no other operations will take higher
precedence when using this macro.
> @@ -1130,7 +1136,31 @@ 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-[15:14] ie scheme this is 1 indicates ver2.
> + * On omap3 Offset 4 is IE Reg the bit [15:14] is XDR_IE which is 0
> + * at reset. Also since the omap_i2c_read_reg uses reg_map_ip_* a
> + * raw_readw is done.
> + */
> + rev = __raw_readw(dev->base + 0x04);
> +
> + switch (OMAP_I2C_SCHEME(rev)) {
> + case 0:
define macros for these two cases.
> + dev->regs = (u8 *)reg_map_ip_v1;
> + dev->rev = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG) & 0xff;
> + minor = OMAP_I2C_REV_SCHEME_0_MAJOR(dev->rev);
> + major = OMAP_I2C_REV_SCHEME_0_MAJOR(dev->rev);
> + break;
wrong indentation.
--
balbi
signature.asc
Description: Digital signature
[PATCH 1/2] 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.
Also since the omap_i2c_read_reg uses reg_map_ip_* a raw_readw is done
to fetch the revision register.
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
---
drivers/i2c/busses/i2c-omap.c | 59 -
1 files changed, 46 insertions(+), 13 deletions(-)
diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index db31eae..d8e7709 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -49,9 +49,10 @@
#define OMAP_I2C_OMAP1_REV_2 0x20
/* 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_2430 0x0036
+#define OMAP_I2C_REV_ON_3430_3530 0x003C
+#define OMAP_I2C_REV_ON_3630 0x0040
+#define OMAP_I2C_REV_ON_4430_PLUS 0x5042
/* 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;
+ u32 rev;
unsignedb_hw:1; /* bad h/w fixes */
unsignedreceiver: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 */
@@ -1052,6 +1053,14 @@ static const struct of_device_id omap_i2c_of_match[] = {
MODULE_DEVICE_TABLE(of, omap_i2c_of_match);
#endif
+#define OMAP_I2C_SCHEME(rev) (rev & 0xc000) >> 14
+
+#define OMAP_I2C_REV_SCHEME_0_MAJOR(rev) (rev >> 4)
+#define OMAP_I2C_REV_SCHEME_0_MINOR(rev) (rev & 0xf)
+
+#define OMAP_I2C_REV_SCHEME_1_MAJOR(rev) ((rev & 0x0700) >> 7)
+#define OMAP_I2C_REV_SCHEME_1_MINOR(rev) (rev & 0x1f)
+
static int __devinit
omap_i2c_probe(struct platform_device *pdev)
{
@@ -1064,6 +1073,8 @@ omap_i2c_probe(struct platform_device *pdev)
const struct of_device_id *match;
int irq;
int r;
+ u32 rev;
+ u16 minor, major;
/* NOTE: driver uses the static register mapping */
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -1117,11 +1128,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 +1136,31 @@ 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-[15:14] ie scheme this is 1 indicates ver2.
+* On omap3 Offset 4 is IE Reg the bit [15:14] is XDR_IE which is 0
+* at reset. Also since the omap_i2c_read_reg uses reg_map_ip_* a
+* raw_readw is done.
+*/
+ rev = __raw_readw(dev->base + 0x04);
+
+ switch (OMAP_I2C_SCHEME(rev)) {
+ case 0:
+ dev->regs = (u8 *)reg_map_ip_v1;
+ dev->rev = omap_i2c_read_reg(dev, OMAP_I2C_REV_REG) & 0xff;
+ minor = OMAP_I2C_REV_SCHEME_0_MAJOR(dev->rev);
+ major = OMAP_I2C_REV_SCHEME_0_MAJOR(dev->rev);
+ break;
+ case 1:
+ /* FALLTHROUGH */
+ default:
+ dev->regs = (u8 *)reg_map_ip_v2;
+ rev = (rev << 16) |
+ omap_i2c_read_reg(dev, OMAP_I2C_IP_V2_REVNB_LO);
+ minor = OMAP_I2C_REV_SCHEME_1_MINOR(rev);
+ major = OMAP_I2C_REV_SCHEME_1_MAJOR(rev);
+
