[PATCH 0/4] OMAP 3 and 4 i2c fixes

[PATCH 0/4] OMAP 3 and 4 i2c fixes

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

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

[PATCH 1/4] OMAP3 and 4 hwmod I2C units only allow 16 bit access

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

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

Cc: patches at linaro.org
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Andy Green <andy.green@linaro.org>
---

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

diff --git a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
index 541092c..1409779 100644
--- a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
@@ -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,
diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
index ce646f2..c500416 100644
--- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
@@ -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

[Cousson, Benoit]

On 3/3/2011 2:50 PM, Andy Green wrote:
> 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 :-)

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

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

> 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
>
> Cc: patches at linaro.org
> Reported-by: Peter Maydell<peter.maydell@linaro.org>
> Signed-off-by: Andy Green<andy.green@linaro.org>
> ---
>
>   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(-)
>
> diff --git a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
> index 541092c..1409779 100644
> --- a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
> +++ b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
> @@ -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,
> diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> index ce646f2..c500416 100644
> --- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> +++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> @@ -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

[Andy Green]

On 03/03/2011 05:42 PM, Somebody in the thread at some point said:
> On 3/3/2011 2:50 PM, Andy Green wrote:

Hi -

Thanks for the reply.

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

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

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

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

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

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

[Cousson, Benoit]

On 3/3/2011 6:56 PM, Andy Green wrote:
> On 03/03/2011 05:42 PM, Somebody in the thread at some point said:
>> On 3/3/2011 2:50 PM, Andy Green wrote:
>
> Hi -
>
> Thanks for the reply.
>
>>> 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:
>
>   >>  [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.

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.

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

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

[Andy Green]

On 03/03/2011 08:40 PM, Somebody in the thread at some point said:

Hi -

>>> BTW, It will be good if you could update the omap_hwmod_2430_data.c file
>>> 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

[Cousson, Benoit]

On 3/4/2011 9:33 AM, Andy Green wrote:
> On 03/03/2011 08:40 PM, Somebody in the thread at some point said:
>
> Hi -
>
>>>> BTW, It will be good if you could update the omap_hwmod_2430_data.c file
>>>> 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.

Cool, thanks for that.
Benoit

[PATCH 1/4] OMAP3 and 4 hwmod I2C units only allow 16 bit access

[Cousson, Benoit]

One more minor comment about the order of the flags for OMAP4.

On 3/3/2011 2:50 PM, Andy Green wrote:
[...]

> diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> index ce646f2..c500416 100644
> --- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> +++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> @@ -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


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

[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.org
Signed-off-by: Andy Green <andy.green@linaro.org>
---

 drivers/i2c/busses/i2c-omap.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index b605ff3..d6500ec 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -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

[Cousson, Benoit]

On 3/3/2011 2:50 PM, Andy Green wrote:
> 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.org
> Signed-off-by: Andy Green<andy.green@linaro.org>
> ---
>
>   drivers/i2c/busses/i2c-omap.c |   11 +++++++++++
>   1 files changed, 11 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index b605ff3..d6500ec 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -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.
> +	 */

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/

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

[Andy Green]

On 03/03/2011 09:12 PM, Somebody in the thread at some point said:

Hi -

Thanks for your comments.

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

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

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.

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

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

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

This patch changes the extended register name to make it clearer
they only exist in OMAP4 context

Cc: patches at linaro.org
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Andy Green <andy.green@linaro.org>
---

 drivers/i2c/busses/i2c-omap.c |   23 ++++++++++++-----------
 1 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index d6500ec..e09c62d 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -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

[Cousson, Benoit]

On 3/3/2011 2:50 PM, Andy Green wrote:
> 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.

> This patch changes the extended register name to make it clearer
> they only exist in OMAP4 context
>
> Cc: patches at linaro.org
> Reported-by: Peter Maydell<peter.maydell@linaro.org>
> Signed-off-by: Andy Green<andy.green@linaro.org>

The I2C maintainer should be in CC as well.

> ---
>
>   drivers/i2c/busses/i2c-omap.c |   23 ++++++++++++-----------
>   1 files changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index d6500ec..e09c62d 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -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,

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

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

[Andy Green]

On 03/03/2011 09:33 PM, Somebody in the thread at some point said:

Hi -

> 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.
>
>> This patch changes the extended register name to make it clearer
>> they only exist in OMAP4 context
>>
>> Cc: patches at linaro.org
>> Reported-by: Peter Maydell<peter.maydell@linaro.org>
>> Signed-off-by: Andy Green<andy.green@linaro.org>
>
> The I2C maintainer should be in CC as well.

OK thanks for this correction.

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

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

[Cousson, Benoit]

On 3/4/2011 9:32 AM, Andy Green wrote:
> On 03/03/2011 09:33 PM, Somebody in the thread at some point said:
>
> Hi -
>
>> 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.
>>
>>> This patch changes the extended register name to make it clearer
>>> they only exist in OMAP4 context
>>>
>>> Cc: patches at linaro.org
>>> Reported-by: Peter Maydell<peter.maydell@linaro.org>
>>> Signed-off-by: Andy Green<andy.green@linaro.org>
>>
>> The I2C maintainer should be in CC as well.
>
> OK thanks for this correction.
>
>>> + /* 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.
>
> 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.

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

[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.org
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Andy Green <andy.green@linaro.org>
---

 drivers/i2c/busses/i2c-omap.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index e09c62d..c82e1bb5 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -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

[Cousson, Benoit]

On 3/3/2011 2:50 PM, Andy Green wrote:
> 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.org
> Reported-by: Peter Maydell<peter.maydell@linaro.org>
> Signed-off-by: Andy Green<andy.green@linaro.org>
> ---
>
>   drivers/i2c/busses/i2c-omap.c |    2 +-
>   1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
> index e09c62d..c82e1bb5 100644
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -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

[PATCH 0/4] OMAP 3 and 4 i2c fixes

[Cousson, Benoit]

To summarize my comments on your series.

On 3/3/2011 2:50 PM, Andy Green wrote:
> 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.

> 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