OMAP 3 and 4 i2c fixes
Subsystems (7)
cover OMAP 3 and 4 i2c fixes
The following series fixes two issues with OMAP 3 and 4 i2c support.
First, hwmod tables don't have the i2c units marked up as being for 16-bit access only, which is mandatory.
Second, the i2c peripheral unit init code is confused about using cpu_is...() and probed peripheral unit version, leading to OMAP3 i2c code doing the wrong thing and accessing nonexistant registers.
---
Andy Green (4):
OMAP3 and 4 I2C use cpu type consistently for new register availability OMAP3 and 4 i2c mark extended reg enums as extended only OMAP3 I2C document why cpu type and not peripheral unit ID used to probe OMAP3 and 4 hwmod I2C units only allow 16 bit access
arch/arm/mach-omap2/omap_hwmod_3xxx_data.c | 3 ++ arch/arm/mach-omap2/omap_hwmod_44xx_data.c | 8 +++--- drivers/i2c/busses/i2c-omap.c | 36 +++++++++++++++++++--------- 3 files changed, 31 insertions(+), 16 deletions(-)
Signature · 1 line
Signature
PATCH 1/4 OMAP3 and 4 hwmod I2C units only allow 16 bit access
Peter Maydell noticed when running under QEMU he was getting errors reporting 32-bit access to I2C peripheral unit registers that are documented to be 8 or 16-bit only[1][2]
The I2C driver is blameless as it wraps its accesses in a function using __raw_writew and __raw_readw, it turned out it is the hwmod stuff.
However the hwmod code already has a flag to force a perhipheral unit to only be accessed using 16-bit operations.
This patch applies the 16-bit only flag to the OMAP3xxx and OMAP44xx hwmod structs.
[1] OMAP4430 Technical reference manual section 23.1.6.2
[2] OMAP3530 Techincal reference manual section 18.6
---
arch/arm/mach-omap2/omap_hwmod_3xxx_data.c | 3 +++ arch/arm/mach-omap2/omap_hwmod_44xx_data.c | 8 ++++---- 2 files changed, 7 insertions(+), 4 deletions(-)
arch/arm/mach-omap2/omap_hwmod_3xxx_data.c +3 -0
@@ -1170,6 +1170,7 @@ static struct omap_hwmod_ocp_if *omap3xxx_i2c1_slaves[] = {static struct omap_hwmod omap3xxx_i2c1_hwmod = { .name ="i2c1" , + .flags =HWMOD_16BIT_REG , .mpu_irqs = i2c1_mpu_irqs, .mpu_irqs_cnt =ARRAY_SIZE (i2c1_mpu_irqs), .sdma_reqs = i2c1_sdma_reqs,
@@ -1212,6 +1213,7 @@ static struct omap_hwmod_ocp_if *omap3xxx_i2c2_slaves[] = {static struct omap_hwmod omap3xxx_i2c2_hwmod = { .name ="i2c2" , + .flags =HWMOD_16BIT_REG , .mpu_irqs = i2c2_mpu_irqs, .mpu_irqs_cnt =ARRAY_SIZE (i2c2_mpu_irqs), .sdma_reqs = i2c2_sdma_reqs,
@@ -1254,6 +1256,7 @@ static struct omap_hwmod_ocp_if *omap3xxx_i2c3_slaves[] = {static struct omap_hwmod omap3xxx_i2c3_hwmod = { .name ="i2c3" , + .flags =HWMOD_16BIT_REG , .mpu_irqs = i2c3_mpu_irqs, .mpu_irqs_cnt =ARRAY_SIZE (i2c3_mpu_irqs), .sdma_reqs = i2c3_sdma_reqs,
arch/arm/mach-omap2/omap_hwmod_44xx_data.c +4 -4
@@ -2280,7 +2280,7 @@ static struct omap_hwmod_ocp_if *omap44xx_i2c1_slaves[] = {static struct omap_hwmod omap44xx_i2c1_hwmod = { .name ="i2c1" , .class = &omap44xx_i2c_hwmod_class, - .flags =HWMOD_INIT_NO_RESET , + .flags =HWMOD_INIT_NO_RESET |HWMOD_16BIT_REG , .mpu_irqs = omap44xx_i2c1_irqs, .mpu_irqs_cnt =ARRAY_SIZE (omap44xx_i2c1_irqs), .sdma_reqs = omap44xx_i2c1_sdma_reqs,
@@ -2396,7 +2396,7 @@ static struct omap_hwmod_ocp_if *omap44xx_i2c2_slaves[] = {static struct omap_hwmod omap44xx_i2c2_hwmod = { .name ="i2c2" , .class = &omap44xx_i2c_hwmod_class, - .flags =HWMOD_INIT_NO_RESET , + .flags =HWMOD_INIT_NO_RESET |HWMOD_16BIT_REG , .mpu_irqs = omap44xx_i2c2_irqs, .mpu_irqs_cnt =ARRAY_SIZE (omap44xx_i2c2_irqs), .sdma_reqs = omap44xx_i2c2_sdma_reqs,
@@ -2449,7 +2449,7 @@ static struct omap_hwmod_ocp_if *omap44xx_i2c3_slaves[] = {static struct omap_hwmod omap44xx_i2c3_hwmod = { .name ="i2c3" , .class = &omap44xx_i2c_hwmod_class, - .flags =HWMOD_INIT_NO_RESET , + .flags =HWMOD_INIT_NO_RESET |HWMOD_16BIT_REG , .mpu_irqs = omap44xx_i2c3_irqs, .mpu_irqs_cnt =ARRAY_SIZE (omap44xx_i2c3_irqs), .sdma_reqs = omap44xx_i2c3_sdma_reqs,
@@ -2502,7 +2502,7 @@ static struct omap_hwmod_ocp_if *omap44xx_i2c4_slaves[] = {static struct omap_hwmod omap44xx_i2c4_hwmod = { .name ="i2c4" , .class = &omap44xx_i2c_hwmod_class, - .flags =HWMOD_INIT_NO_RESET , + .flags =HWMOD_INIT_NO_RESET |HWMOD_16BIT_REG , .mpu_irqs = omap44xx_i2c4_irqs, .mpu_irqs_cnt =ARRAY_SIZE (omap44xx_i2c4_irqs), .sdma_reqs = omap44xx_i2c4_sdma_reqs,
PATCH 1/4 OMAP3 and 4 hwmod I2C units only allow 16 bit access
Quoted 3 lines · Andy Green
Peter Maydell noticed when running under QEMU he was getting errors reporting 32-bit access to I2C peripheral unit registers that are documented to be 8 or 16-bit only[1][2]
Well, in that case, it is more a QEMU bug since the HW is working fine with 32 bits access to sysconfig :-)
Quoted 6 lines
The I2C driver is blameless as it wraps its accesses in a function using
__raw_writewand__raw_readw, it turned out it is the hwmod stuff.However the hwmod code already has a flag to force a perhipheral unit to only be accessed using 16-bit operations..
In fact that flag was added because 32 bits access to I2C sysconfig was generating bus abort on 2420 only:
(2004290f55f03c52e22044a5843928cf0f6cc56a).
Since 2430, OMAP3 and OMAP4 are working fine with 32 bits, we were lazy and didn't add that flag.
Did you check this patch on a real HW? Since this was reported using QEMU only.
Otherwise, I'm fine with that patch, it will not change anything but will improve the consistency across SoC version.
BTW, It will be good if you could update the omap_hwmod_2430_data.c file as well.
Thanks,
Benoit
Quoted 88 lines
This patch applies the 16-bit only flag to the OMAP3xxx and OMAP44xx hwmod structs.
[1] OMAP4430 Technical reference manual section 23.1.6.2
[2] OMAP3530 Techincal reference manual section 18.6Cc patches at linaro.orgReported-by Peter Maydell<[email protected]>Signed-off-by Andy Green<[email protected]>---
arch/arm/mach-omap2/omap_hwmod_3xxx_data.c | 3 +++ arch/arm/mach-omap2/omap_hwmod_44xx_data.c | 8 ++++---- 2 files changed, 7 insertions(+), 4 deletions(-)arch/arm/mach-omap2/omap_hwmod_3xxx_data.c +3
-0@@ -1170,6 +1170,7 @@ static struct omap_hwmod_ocp_if *omap3xxx_i2c1_slaves[] = {static struct omap_hwmod omap3xxx_i2c1_hwmod = { .name ="i2c1" , + .flags =HWMOD_16BIT_REG , .mpu_irqs = i2c1_mpu_irqs, .mpu_irqs_cnt =ARRAY_SIZE (i2c1_mpu_irqs), .sdma_reqs = i2c1_sdma_reqs,@@ -1212,6 +1213,7 @@ static struct omap_hwmod_ocp_if *omap3xxx_i2c2_slaves[] = {static struct omap_hwmod omap3xxx_i2c2_hwmod = { .name ="i2c2" , + .flags =HWMOD_16BIT_REG , .mpu_irqs = i2c2_mpu_irqs, .mpu_irqs_cnt =ARRAY_SIZE (i2c2_mpu_irqs), .sdma_reqs = i2c2_sdma_reqs,@@ -1254,6 +1256,7 @@ static struct omap_hwmod_ocp_if *omap3xxx_i2c3_slaves[] = {static struct omap_hwmod omap3xxx_i2c3_hwmod = { .name ="i2c3" , + .flags =HWMOD_16BIT_REG , .mpu_irqs = i2c3_mpu_irqs, .mpu_irqs_cnt =ARRAY_SIZE (i2c3_mpu_irqs), .sdma_reqs = i2c3_sdma_reqs,arch/arm/mach-omap2/omap_hwmod_44xx_data.c +4
-5@@ -2280,7 +2280,7 @@ static struct omap_hwmod_ocp_if *omap44xx_i2c1_slaves[] = {static struct omap_hwmod omap44xx_i2c1_hwmod = { .name ="i2c1" , .class =&omap44xx_i2c_hwmod_class, - .flags =HWMOD_INIT_NO_RESET , + .flags =HWMOD_INIT_NO_RESET |HWMOD_16BIT_REG , .mpu_irqs = omap44xx_i2c1_irqs, .mpu_irqs_cnt =ARRAY_SIZE (omap44xx_i2c1_irqs), .sdma_reqs = omap44xx_i2c1_sdma_reqs,@@ -2396,7 +2396,7 @@ static struct omap_hwmod_ocp_if *omap44xx_i2c2_slaves[] = {static struct omap_hwmod omap44xx_i2c2_hwmod = { .name ="i2c2" , .class =&omap44xx_i2c_hwmod_class, - .flags =HWMOD_INIT_NO_RESET , + .flags =HWMOD_INIT_NO_RESET |HWMOD_16BIT_REG , .mpu_irqs = omap44xx_i2c2_irqs, .mpu_irqs_cnt =ARRAY_SIZE (omap44xx_i2c2_irqs), .sdma_reqs = omap44xx_i2c2_sdma_reqs,@@ -2449,7 +2449,7 @@ static struct omap_hwmod_ocp_if *omap44xx_i2c3_slaves[] = {static struct omap_hwmod omap44xx_i2c3_hwmod = { .name ="i2c3" , .class =&omap44xx_i2c_hwmod_class, - .flags =HWMOD_INIT_NO_RESET , + .flags =HWMOD_INIT_NO_RESET |HWMOD_16BIT_REG , .mpu_irqs = omap44xx_i2c3_irqs, .mpu_irqs_cnt =ARRAY_SIZE (omap44xx_i2c3_irqs), .sdma_reqs = omap44xx_i2c3_sdma_reqs,@@ -2502,7 +2502,7 @@ static struct omap_hwmod_ocp_if *omap44xx_i2c4_slaves[] = {static struct omap_hwmod omap44xx_i2c4_hwmod = { .name ="i2c4" , .class =&omap44xx_i2c_hwmod_class, - .flags =HWMOD_INIT_NO_RESET , + .flags =HWMOD_INIT_NO_RESET |HWMOD_16BIT_REG , .mpu_irqs = omap44xx_i2c4_irqs, .mpu_irqs_cnt =ARRAY_SIZE (omap44xx_i2c4_irqs), .sdma_reqs = omap44xx_i2c4_sdma_reqs, --To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 1/4 OMAP3 and 4 hwmod I2C units only allow 16 bit access
Quoted 1 line · Somebody in the thread at some point
On 3/3/2011 2:50 PM, Andy Green wrote:
Hi -
Thanks for the reply.
Quoted 6 lines
Quoted 3 lines
Peter Maydell noticed when running under QEMU he was getting errors reporting 32-bit access to I2C peripheral unit registers that are documented to be 8 or 16-bit only[1][2]
Well, in that case, it is more a QEMU bug since the HW is working fine with 32 bits access to sysconfig :-)
Actually it's documented in TI documentation, as noted:
Quoted 2 lines
Quoted 2 lines
[1] OMAP4430 Technical reference manual section 23.1.6.2
[2] OMAP3530 Techincal reference manual section 18.6
With the following warning in a nice big grey box -->
''CAUTION
The I2Ci registers are limited to 16 bit and 8 bit data accesses, 32 bit data access is not allowed and can corrupt register content.''
So, as a side-issue it can be worth confirming with the author of the warning if it still holds or not and letting Qemu guys know if it's not actually true what is written in the TI docs about that.
Quoted 6 lines
In fact that flag was added because 32 bits access to I2C sysconfig was generating bus abort on 2420 only:
(2004290f55f03c52e22044a5843928cf0f6cc56a).Since 2430, OMAP3 and OMAP4 are working fine with 32 bits, we were lazy and didn't add that flag.
There is no bus abort. However if the warning in the documentation is true, it'd be better that there was a bus abort.
Quoted 2 lines
Did you check this patch on a real HW? Since this was reported using QEMU only.
I checked my patched code works OK on both IGEP2 (OMAP3) and Panda (OMAP4), there's no visible symptom without the patch it's true.
Quoted 5 lines
Otherwise, I'm fine with that patch, it will not change anything but will improve the consistency across SoC version.
BTW, It will be good if you could update the
omap_hwmod_2430_data.cfile as well.
I left it because I can't test it, but I'll happily do it additionally if you can test it on some OMAP2 hardware.
-Andy
PATCH 1/4 OMAP3 and 4 hwmod I2C units only allow 16 bit access
Quoted 21 lines · Andy Green
Quoted 1 line · Somebody in the thread at some point
On 3/3/2011 2:50 PM, Andy Green wrote:
Hi -
Thanks for the reply.
Quoted 6 lines
Quoted 3 lines
Peter Maydell noticed when running under QEMU he was getting errors reporting 32-bit access to I2C peripheral unit registers that are documented to be 8 or 16-bit only[1][2]
Well, in that case, it is more a QEMU bug since the HW is working fine with 32 bits access to sysconfig :-)
Actually it's documented in TI documentation, as noted:
Quoted 2 lines
Quoted 2 lines
[1] OMAP4430 Technical reference manual section 23.1.6.2
[2] OMAP3530 Techincal reference manual section 18.6With the following warning in a nice big grey box -->
Quoted 7 lines
''CAUTION
The I2Ci registers are limited to 16 bit and 8 bit data accesses, 32 bit data access is not allowed and can corrupt register content.''So, as a side-issue it can be worth confirming with the author of the warning if it still holds or not and letting Qemu guys know if it's not actually true what is written in the TI docs about that.
I was able to check for OMAP4, and in fact since the I2C bus is using only the 16 LSB of the 32 bits interconnect, doing 32 bits access is harmless.
But OMAP2 & 3 were using a different interconnect, so it was probably not done like that, hence the big CAUTION in the TRM.
Quoted 7 lines
Quoted 6 lines
In fact that flag was added because 32 bits access to I2C sysconfig was generating bus abort on 2420 only:
(2004290f55f03c52e22044a5843928cf0f6cc56a).Since 2430, OMAP3 and OMAP4 are working fine with 32 bits, we were lazy and didn't add that flag.
Quoted 8 lines
There is no bus abort. However if the warning in the documentation is true, it'd be better that there was a bus abort.
Quoted 2 lines
Did you check this patch on a real HW? Since this was reported using QEMU only.
I checked my patched code works OK on both IGEP2 (OMAP3) and Panda (OMAP4), there's no visible symptom without the patch it's true.
Even if starting from OMAP4 generation we can do 32 bits access, since the whole IP is documented with 16 bits registers, it is cleaner to prevent hwmod access in 32 bits.
I will still report that to the TRM team in order to avoid unnecessary scary notes.
Quoted 8 lines
Quoted 5 lines
Otherwise, I'm fine with that patch, it will not change anything but will improve the consistency across SoC version.
BTW, It will be good if you could update the
omap_hwmod_2430_data.cfile as well.I left it because I can't test it, but I'll happily do it additionally if you can test it on some OMAP2 hardware.
Don't hesitate to do it, and clearly add in the cover-letter that it was tested on 3430 and 4430 only.
Someone from TI should be able to test it on 2430.
Thanks,
Benoit
PATCH 1/4 OMAP3 and 4 hwmod I2C units only allow 16 bit access
On 03/03/2011 08:40 PM, Somebody in the thread at some point said:
Hi -
Quoted 9 lines
Quoted 5 lines
Quoted 2 lines
BTW, It will be good if you could update the
omap_hwmod_2430_data.cfile as well.I left it because I can't test it, but I'll happily do it additionally if you can test it on some OMAP2 hardware.
Don't hesitate to do it, and clearly add in the cover-letter that it was tested on 3430 and 4430 only.
Someone from TI should be able to test it on 2430.
Alright I will extend the patch series to cover 2430 for this and note it is untested.
-Andy
PATCH 1/4 OMAP3 and 4 hwmod I2C units only allow 16 bit access
Quoted 10 lines · Andy Green
On 03/03/2011 08:40 PM, Somebody in the thread at some point said:
Hi -
Quoted 6 lines
Quoted 5 lines
Quoted 2 lines
BTW, It will be good if you could update the
omap_hwmod_2430_data.cfile as well.I left it because I can't test it, but I'll happily do it additionally if you can test it on some OMAP2 hardware.
Quoted 6 lines
Quoted 3 lines
Don't hesitate to do it, and clearly add in the cover-letter that it was tested on 3430 and 4430 only.
Someone from TI should be able to test it on 2430.Alright I will extend the patch series to cover 2430 for this and note it is untested.
Cool, thanks for that.
Benoit
PATCH 1/4 OMAP3 and 4 hwmod I2C units only allow 16 bit access
In reply to Andy GreenOne more minor comment about the order of the flags for OMAP4.
On 3/3/2011 2:50 PM, Andy Green wrote:
[...]
arch/arm/mach-omap2/omap_hwmod_44xx_data.c static struct omap_hwmod_ocp_if *omap44xx_i2c1_slaves[] = { · line 2283
Quoted 10 lines
arch/arm/mach-omap2/omap_hwmod_44xx_data.c +1
-1@@ -2280,7 +2280,7 @@ static struct omap_hwmod_ocp_if *omap44xx_i2c1_slaves[] = {static struct omap_hwmod omap44xx_i2c1_hwmod = { .name ="i2c1" , .class =&omap44xx_i2c_hwmod_class, - .flags =HWMOD_INIT_NO_RESET , + .flags =HWMOD_INIT_NO_RESET |HWMOD_16BIT_REG ,
Since this code is generated by script, the flags are sorted by name. HWMOD_16BIT_REG should then be the first one. This is of course applicable for the 4 instances.
I already updated the generator to take the sysconfig register width into account.
Thanks,
Benoit
Quoted 35 lines
arch/arm/mach-omap2/omap_hwmod_44xx_data.c +3
-4.mpu_irqs = omap44xx_i2c1_irqs, .mpu_irqs_cnt = ARRAY_SIZE(omap44xx_i2c1_irqs), .sdma_reqs = omap44xx_i2c1_sdma_reqs,@@ -2396,7 +2396,7 @@ static struct omap_hwmod_ocp_if *omap44xx_i2c2_slaves[] = {static struct omap_hwmod omap44xx_i2c2_hwmod = { .name ="i2c2" , .class =&omap44xx_i2c_hwmod_class, - .flags =HWMOD_INIT_NO_RESET , + .flags =HWMOD_INIT_NO_RESET |HWMOD_16BIT_REG , .mpu_irqs = omap44xx_i2c2_irqs, .mpu_irqs_cnt =ARRAY_SIZE (omap44xx_i2c2_irqs), .sdma_reqs = omap44xx_i2c2_sdma_reqs,@@ -2449,7 +2449,7 @@ static struct omap_hwmod_ocp_if *omap44xx_i2c3_slaves[] = {static struct omap_hwmod omap44xx_i2c3_hwmod = { .name ="i2c3" , .class =&omap44xx_i2c_hwmod_class, - .flags =HWMOD_INIT_NO_RESET , + .flags =HWMOD_INIT_NO_RESET |HWMOD_16BIT_REG , .mpu_irqs = omap44xx_i2c3_irqs, .mpu_irqs_cnt =ARRAY_SIZE (omap44xx_i2c3_irqs), .sdma_reqs = omap44xx_i2c3_sdma_reqs,@@ -2502,7 +2502,7 @@ static struct omap_hwmod_ocp_if *omap44xx_i2c4_slaves[] = {static struct omap_hwmod omap44xx_i2c4_hwmod = { .name ="i2c4" , .class =&omap44xx_i2c_hwmod_class, - .flags =HWMOD_INIT_NO_RESET , + .flags =HWMOD_INIT_NO_RESET |HWMOD_16BIT_REG , .mpu_irqs = omap44xx_i2c4_irqs, .mpu_irqs_cnt =ARRAY_SIZE (omap44xx_i2c4_irqs), .sdma_reqs = omap44xx_i2c4_sdma_reqs, --To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 2/4 OMAP3 I2C document why cpu type and not peripheral unit ID used to probe
In reply to Andy GreenDescribe why we can't simply probe the peripheral unit ID to make the decision about what register map to use
---
drivers/i2c/busses/i2c-omap.c | 11 +++++++++++ 1 files changed, 11 insertions(+), 0 deletions(-)
drivers/i2c/busses/i2c-omap.c +11 -0
@@ -1032,6 +1032,17 @@ omap_i2c_probe(struct platform_device *pdev)else dev->reg_shift =2 ; + dev->regs = (u8 *)reg_map; + +/* +* this is a bit tricky, implementation on 4430 has the active +* part of its ID register moved to +4 instead of +0 as +* previously. So, we can't probe just using the ID register +* Complicating matters the older implementation using the +* simpler register set on 3530 also reports its revision as +* 0x40, same as the 4430 newer implementation. +*/ +if (cpu_is_omap44xx ()) dev->regs = (u8 *) omap4_reg_map;else
PATCH 2/4 OMAP3 I2C document why cpu type and not peripheral unit ID used to probe
Quoted 18 lines · Andy Green
Describe why we can't simply probe the peripheral unit ID to make the decision about what register map to use
Cc patches at linaro.orgSigned-off-by Andy Green<[email protected]>---
drivers/i2c/busses/i2c-omap.c | 11 +++++++++++ 1 files changed, 11 insertions(+), 0 deletions(-)drivers/i2c/busses/i2c-omap.c +0
-0@@ -1032,6 +1032,17 @@ omap_i2c_probe(struct platform_device *pdev)else dev->reg_shift =2 ;
drivers/i2c/busses/i2c-omap.c omap_i2c_probe(struct platform_device *pdev) · line 1044
Quoted 10 lines
drivers/i2c/busses/i2c-omap.c +10
-0+ dev->regs = (u8 *)reg_map; + +/* +* this is a bit tricky, implementation on 4430 has the active +* part of its ID register moved to +4 instead of +0 as +* previously. So, we can't probe just using the ID register +* Complicating matters the older implementation using the +* simpler register set on 3530 also reports its revision as +* 0x40, same as the 4430 newer implementation. +*/
The issue is that this revision field is not really documented in OMAP4 TRM, so you should not rely on it. Moreover, as you already noticed, the revision number is not even accurate. OMAP3 and 4 are using a different programming model but this is not reflected in this field.
Since that issue is quite common in many OMAP IPs, we introduced a SW field in hwmod_class that should reflect the change in IP programming model. For the moment it is a simple integer that we increment each time there is change in a programming model that will impact the driver.
You can check how it was done for the timer for example:
https://patchwork.kernel.org/patch/587211/
Then you need to copy that info to a pdata field in order to allow the driver to access it. Check omap_timer_init in:
https://patchwork.kernel.org/patch/587241/
drivers/i2c/busses/i2c-omap.c omap_i2c_probe(struct platform_device *pdev) · line 1047
Quoted 3 lines
drivers/i2c/busses/i2c-omap.c +1
-0+if (cpu_is_omap44xx ()) dev->regs = (u8 *) omap4_reg_map;
OK, this is not part of your patch, but any call to cpu_is are forbidden from the driver. As soon as you have the revision field from hwmod you can get rid of all that code.
Regards,
Benoit
PATCH 2/4 OMAP3 I2C document why cpu type and not peripheral unit ID used to probe
On 03/03/2011 09:12 PM, Somebody in the thread at some point said:
Hi -
Thanks for your comments.
Quoted 4 lines
The issue is that this revision field is not really documented in OMAP4 TRM, so you should not rely on it. Moreover, as you already noticed, the revision number is not even accurate. OMAP3 and 4 are using a different programming model but this is not reflected in this field.
OK.
Quoted 6 lines
Since that issue is quite common in many OMAP IPs, we introduced a SW field in
hwmod_classthat should reflect the change in IP programming model. For the moment it is a simple integer that we increment each time there is change in a programming model that will impact the driver.You can check how it was done for the timer for example:
Alright. In I2C case the path is hwmod class -> platform data though since hwmod content directly is not used in the driver, but platform data is.
Quoted 7 lines
Quoted 3 lines
+
if (cpu_is_omap44xx())dev->regs= (u8 *)omap4_reg_map;OK, this is not part of your patch, but any call to
cpu_isare forbidden from the driver. As soon as you have the revision field from hwmod you can get rid of all that code.
Well, as you point out they are not forbidden enough since I was just working with what was already there.
However this hwmod scheme will cover it all AFAICS, so I will extend the patches to nuke all cpu_is... from the driver.
-Andy
PATCH 3/4 OMAP3 and 4 i2c mark extended reg enums as extended only
In reply to Andy GreenThe OMAP I2C driver dynamically chooses between two register sets of differing sizes depending on the cpu type it finds itself on.
It has been observed that the existing code references non-existing registers on OMAP3530, because while it correctly chose the smaller register layout based on cpu type, the code uses the probed register ID to decide if to execute code referencing an extra register, and both register layout devices on OMAP3530 and OMAP4430 report the same probed ID of 0x40.
This patch changes the extended register name to make it clearer they only exist in OMAP4 context
---
drivers/i2c/busses/i2c-omap.c | 23 ++++++++++++----------- 1 files changed, 12 insertions(+), 11 deletions(-)
drivers/i2c/busses/i2c-omap.c +12 -11
@@ -72,11 +72,12 @@ enum {OMAP_I2C_SCLH_REG ,OMAP_I2C_SYSTEST_REG ,OMAP_I2C_BUFSTAT_REG , -OMAP_I2C_REVNB_LO , -OMAP_I2C_REVNB_HI , -OMAP_I2C_IRQSTATUS_RAW , -OMAP_I2C_IRQENABLE_SET , - OMAP_I2C_IRQENABLE_CLR, +/* only on OMAP4430 */ +OMAP_I2C_OMAP4430_REVNB_LO , +OMAP_I2C_OMAP4430_REVNB_HI , +OMAP_I2C_OMAP4430_IRQSTATUS_RAW , +OMAP_I2C_OMAP4430_IRQENABLE_SET , + OMAP_I2C_OMAP4430_IRQENABLE_CLR, };/* I2C Interrupt Enable Register (OMAP_I2C_IE): */
@@ -244,11 +245,11 @@ const static u8 omap4_reg_map[] = { [OMAP_I2C_SCLH_REG] = 0xb8, [OMAP_I2C_SYSTEST_REG] = 0xbC, [OMAP_I2C_BUFSTAT_REG] = 0xc0, - [OMAP_I2C_REVNB_LO] = 0x00, - [OMAP_I2C_REVNB_HI] = 0x04, - [OMAP_I2C_IRQSTATUS_RAW] = 0x24, - [OMAP_I2C_IRQENABLE_SET] = 0x2c, - [OMAP_I2C_IRQENABLE_CLR] = 0x30, + [OMAP_I2C_OMAP4430_REVNB_LO] = 0x00, + [OMAP_I2C_OMAP4430_REVNB_HI] = 0x04, + [OMAP_I2C_OMAP4430_IRQSTATUS_RAW] = 0x24, + [OMAP_I2C_OMAP4430_IRQENABLE_SET] = 0x2c, + [OMAP_I2C_OMAP4430_IRQENABLE_CLR] = 0x30, }; static inline void omap_i2c_write_reg(struct omap_i2c_dev *i2c_dev,
@@ -309,7 +310,7 @@ static void omap_i2c_idle(struct omap_i2c_dev *dev) dev->iestate =omap_i2c_read_reg (dev,OMAP_I2C_IE_REG );if (dev->rev >=OMAP_I2C_REV_ON_4430 ) -omap_i2c_write_reg (dev,OMAP_I2C_IRQENABLE_CLR ,1 ); +omap_i2c_write_reg (dev,OMAP_I2C_OMAP4430_IRQENABLE_CLR ,1 );else omap_i2c_write_reg (dev,OMAP_I2C_IE_REG ,0 );
PATCH 3/4 OMAP3 and 4 i2c mark extended reg enums as extended only
Quoted 9 lines · Andy Green
The OMAP I2C driver dynamically chooses between two register sets of differing sizes depending on the cpu type it finds itself on.
It has been observed that the existing code references non-existing registers on OMAP3530, because while it correctly chose the smaller register layout based on cpu type, the code uses the probed register ID to decide if to execute code referencing an extra register, and both register layout devices on OMAP3530 and OMAP4430 report the same probed ID of
0x40.
Since it is a patch on the I2C driver, the subject should start with something like "I2C: OMAP2+: XXXXX". That comment is also applicable for the other patches of the series except the first one.
Quoted 6 lines
This patch changes the extended register name to make it clearer they only exist in OMAP4 context
Cc patches at linaro.orgReported-by Peter Maydell<[email protected]>Signed-off-by Andy Green<[email protected]>
The I2C maintainer should be in CC as well.
Quoted 13 lines
---
drivers/i2c/busses/i2c-omap.c | 23 ++++++++++++----------- 1 files changed, 12 insertions(+), 11 deletions(-)drivers/i2c/busses/i2c-omap.c +0
-0@@ -72,11 +72,12 @@ enum {OMAP_I2C_SCLH_REG ,OMAP_I2C_SYSTEST_REG , OMAP_I2C_BUFSTAT_REG,
drivers/i2c/busses/i2c-omap.c enum { · line 79
Quoted 10 lines
drivers/i2c/busses/i2c-omap.c +5
-5-OMAP_I2C_REVNB_LO , -OMAP_I2C_REVNB_HI , -OMAP_I2C_IRQSTATUS_RAW , -OMAP_I2C_IRQENABLE_SET , - OMAP_I2C_IRQENABLE_CLR, +/* only on OMAP4430 */ +OMAP_I2C_OMAP4430_REVNB_LO , +OMAP_I2C_OMAP4430_REVNB_HI , +OMAP_I2C_OMAP4430_IRQSTATUS_RAW , + OMAP_I2C_OMAP4430_IRQENABLE_SET,
I think that you should keep only the comment, because it is not really recommended to add SoC related information directly in IP register names. These new registers are just an evolution of the I2C IP. The first instances of that version are used in OMAP4 first, but OMAP4 variants (4440) and OMAP5 will use the same one.
Bottom line is that we can probably drop that patch from the series.
Regards,
Benoit
Quoted 35 lines
drivers/i2c/busses/i2c-omap.c +7
-7+ OMAP_I2C_OMAP4430_IRQENABLE_CLR, };/* I2C Interrupt Enable Register (OMAP_I2C_IE): */ @@ -244,11 +245,11 @@ const static u8 omap4_reg_map[] = { [OMAP_I2C_SCLH_REG] = 0xb8, [OMAP_I2C_SYSTEST_REG] = 0xbC, [OMAP_I2C_BUFSTAT_REG] = 0xc0, - [OMAP_I2C_REVNB_LO] = 0x00, - [OMAP_I2C_REVNB_HI] = 0x04, - [OMAP_I2C_IRQSTATUS_RAW] = 0x24, - [OMAP_I2C_IRQENABLE_SET] = 0x2c, - [OMAP_I2C_IRQENABLE_CLR] = 0x30, + [OMAP_I2C_OMAP4430_REVNB_LO] = 0x00, + [OMAP_I2C_OMAP4430_REVNB_HI] = 0x04, + [OMAP_I2C_OMAP4430_IRQSTATUS_RAW] = 0x24, + [OMAP_I2C_OMAP4430_IRQENABLE_SET] = 0x2c, + [OMAP_I2C_OMAP4430_IRQENABLE_CLR] = 0x30, }; static inline void omap_i2c_write_reg(struct omap_i2c_dev *i2c_dev,@@ -309,7 +310,7 @@ static void omap_i2c_idle(struct omap_i2c_dev *dev) dev->iestate =omap_i2c_read_reg (dev,OMAP_I2C_IE_REG );if (dev->rev>=OMAP_I2C_REV_ON_4430 ) -omap_i2c_write_reg (dev,OMAP_I2C_IRQENABLE_CLR ,1 ); +omap_i2c_write_reg (dev,OMAP_I2C_OMAP4430_IRQENABLE_CLR ,1 );else omap_i2c_write_reg (dev,OMAP_I2C_IE_REG ,0 ); --To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 3/4 OMAP3 and 4 i2c mark extended reg enums as extended only
On 03/03/2011 09:33 PM, Somebody in the thread at some point said:
Hi -
Quoted 4 lines
Since it is a patch on the I2C driver, the subject should start with something like "I2C: OMAP2+: XXXXX". That comment is also applicable for the other patches of the series except the first one.
Quoted 8 lines
Quoted 6 lines
This patch changes the extended register name to make it clearer they only exist in OMAP4 context
Cc patches at linaro.orgReported-by Peter Maydell<[email protected]>Signed-off-by Andy Green<[email protected]>The I2C maintainer should be in CC as well.
OK thanks for this correction.
Quoted 6 lines
Quoted 5 lines
+ /* only on OMAP4430 */
+OMAP_I2C_OMAP4430_REVNB_LO,
+OMAP_I2C_OMAP4430_REVNB_HI,
+OMAP_I2C_OMAP4430_IRQSTATUS_RAW,
+OMAP_I2C_OMAP4430_IRQENABLE_SET,
Quoted 7 lines
I think that you should keep only the comment, because it is not really recommended to add SoC related information directly in IP register names. These new registers are just an evolution of the I2C IP. The first instances of that version are used in OMAP4 first, but OMAP4 variants (4440) and OMAP5 will use the same one.
Bottom line is that we can probably drop that patch from the series.
The desire of this patch is to make it clear to the eye that a register that was introduced in what we will now call "IP_V2" is being touched. That is good because then code like
if (dev->rev == BLAH_IP_V1) touch(BLAH_BLAH_IP_V2);
will stand out clearly as wrong. So I will update the patch rather than drop it, since the IP_Vn scheme is a much better fit for what is actually being done. If you still don't like it we can forget about it then.
-Andy
PATCH 3/4 OMAP3 and 4 i2c mark extended reg enums as extended only
Quoted 37 lines · Andy Green
On 03/03/2011 09:33 PM, Somebody in the thread at some point said:
Hi -
Quoted 4 lines
Since it is a patch on the I2C driver, the subject should start with something like "I2C: OMAP2+: XXXXX". That comment is also applicable for the other patches of the series except the first one.
Quoted 8 lines
Quoted 6 lines
This patch changes the extended register name to make it clearer they only exist in OMAP4 context
Cc patches at linaro.orgReported-by Peter Maydell<[email protected]>Signed-off-by Andy Green<[email protected]>The I2C maintainer should be in CC as well.
OK thanks for this correction.
Quoted 6 lines
Quoted 5 lines
+ /* only on OMAP4430 */
+OMAP_I2C_OMAP4430_REVNB_LO,
+OMAP_I2C_OMAP4430_REVNB_HI,
+OMAP_I2C_OMAP4430_IRQSTATUS_RAW,
+OMAP_I2C_OMAP4430_IRQENABLE_SET,Quoted 7 lines
I think that you should keep only the comment, because it is not really recommended to add SoC related information directly in IP register names. These new registers are just an evolution of the I2C IP. The first instances of that version are used in OMAP4 first, but OMAP4 variants (4440) and OMAP5 will use the same one.
Bottom line is that we can probably drop that patch from the series.
The desire of this patch is to make it clear to the eye that a register that was introduced in what we will now call "
IP_V2" is being touched. That is good because then code like
Quoted 7 lines
if (dev->rev == BLAH_IP_V1) touch(BLAH_BLAH_IP_V2);will stand out clearly as wrong. So I will update the patch rather than drop it, since the
IP_Vnscheme is a much better fit for what is actually being done. If you still don't like it we can forget about it then.
It is a little bit better. I personally don't think it is necessary, but since it is a purely subjective opinion, you can go ahead with that fix.
Regards,
Benoit
PATCH 4/4 OMAP3 and 4 I2C use cpu type consistently for new register availability
In reply to Andy GreenThe driver makes the choice about which register layout to use based on cpu, however it then tries to use the probed peripheral unit version register to decide whether to access registers that only exist in the 4430 unit.
Unfortunately, the unit with the smaller register map on the OMAP3530 has the same peripheral unit version number, leading the OMAP3530 to dereference the register map beyond the bounds of its array, and then to access a 'random' register offset taken from whatever happens to be sitting beyond the register map array, as reported here
https://bugs.launchpad.net/linux-linaro/+bug/645324
This patch makes both the choice of register map and the decision to use a register only present in the larger map both do so based on cpu type, which correctly reflects register availability.
---
drivers/i2c/busses/i2c-omap.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/i2c/busses/i2c-omap.c +1 -1
@@ -309,7 +309,7 @@ static void omap_i2c_idle(struct omap_i2c_dev *dev) pdata = pdev->dev.platform_data; dev->iestate =omap_i2c_read_reg (dev,OMAP_I2C_IE_REG ); -if (dev->rev >=OMAP_I2C_REV_ON_4430 ) +if (cpu_is_omap44xx ())omap_i2c_write_reg (dev,OMAP_I2C_OMAP4430_IRQENABLE_CLR ,1 );else omap_i2c_write_reg (dev,OMAP_I2C_IE_REG ,0 );
PATCH 4/4 OMAP3 and 4 I2C use cpu type consistently for new register availability
Quoted 31 lines · Andy Green
The driver makes the choice about which register layout to use based on cpu, however it then tries to use the probed peripheral unit version register to decide whether to access registers that only exist in the 4430 unit.
Unfortunately, the unit with the smaller register map on the OMAP3530 has the same peripheral unit version number, leading the OMAP3530 to dereference the register map beyond the bounds of its array, and then to access a 'random' register offset taken from whatever happens to be sitting beyond the register map array, as reported here
https://bugs.launchpad.net/linux-linaro/+bug/645324
This patch makes both the choice of register map and the decision to use a register only present in the larger map both do so based on cpu type, which correctly reflects register availability.
Cc patches at linaro.orgReported-by Peter Maydell<[email protected]>Signed-off-by Andy Green<[email protected]>---
drivers/i2c/busses/i2c-omap.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)drivers/i2c/busses/i2c-omap.c +0
-0
drivers/i2c/busses/i2c-omap.c static void omap_i2c_idle(struct omap_i2c_dev *dev) · line 312
Quoted 5 lines
drivers/i2c/busses/i2c-omap.c +1
-1@@ -309,7 +309,7 @@ static void omap_i2c_idle(struct omap_i2c_dev *dev) pdata = pdev->dev.platform_data; dev->iestate =omap_i2c_read_reg (dev,OMAP_I2C_IE_REG ); -if (dev->rev>=OMAP_I2C_REV_ON_4430 ) +if (cpu_is_omap44xx ())
As explained before, you should not add any cpu_is_XXX in the driver. That rev field is the way to go, except that it should be populated using hwmod data information instead of inaccurate I2C register revision field.
Regards,
Benoit
cover OMAP 3 and 4 i2c fixes
In reply to Andy GreenTo summarize my comments on your series.
Quoted 4 lines · Andy Green
The following series fixes two issues with OMAP 3 and 4 i2c support.
First, hwmod tables don't have the i2c units marked up as being for 16-bit access only, which is mandatory.
That part is OK, and should just be extended to 2430 for the sake of completeness. A rev attribute should be added as well for the second part.
Quoted 3 lines
Second, the i2c peripheral unit init code is confused about using cpu_is...() and probed peripheral unit version, leading to OMAP3 i2c code doing the wrong thing and accessing nonexistant registers.
That part should be revisited to use a better way to identify the IP revision.
Most of the code is there, you just have to copy the correct information during I2C device init.
Just let me know if you are not comfortable with that latest part, some l-o folks, including me, can help you.
Thanks,
Benoit