[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

Cc: patches@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(-)

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@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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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

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

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

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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html