From: Andy Green <hidden> Date: 2011-03-03 13:50:14
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
From: Andy Green <hidden> Date: 2011-03-03 13:50:22
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 <redacted>
Signed-off-by: Andy Green <redacted>
---
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(-)
From: Andy Green <hidden> Date: 2011-03-03 13:50:30
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 <redacted>
---
drivers/i2c/busses/i2c-omap.c | 11 +++++++++++
1 files changed, 11 insertions(+), 0 deletions(-)
From: Andy Green <hidden> Date: 2011-03-03 13:50:37
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 <redacted>
Signed-off-by: Andy Green <redacted>
---
drivers/i2c/busses/i2c-omap.c | 23 ++++++++++++-----------
1 files changed, 12 insertions(+), 11 deletions(-)
From: Andy Green <hidden> Date: 2011-03-03 13:50:44
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 <redacted>
Signed-off-by: Andy Green <redacted>
---
drivers/i2c/busses/i2c-omap.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
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
quoted hunk
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<redacted>
Signed-off-by: Andy Green<redacted>
---
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(-)
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
From: Andy Green <hidden> Date: 2011-03-03 17:56:12
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.
quoted
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 03/03/2011 05:42 PM, Somebody in the thread at some point said:
quoted
On 3/3/2011 2:50 PM, Andy Green wrote:
Hi -
Thanks for the reply.
quoted
quoted
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.
quoted
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
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
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
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<redacted>
---
drivers/i2c/busses/i2c-omap.c | 11 +++++++++++
1 files changed, 11 insertions(+), 0 deletions(-)
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
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<redacted>
Signed-off-by: Andy Green<redacted>
@@ -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
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
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<redacted>
Signed-off-by: Andy Green<redacted>
---
drivers/i2c/busses/i2c-omap.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
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
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
From: Andy Green <hidden> Date: 2011-03-04 08:25:51
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.
quoted
+
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
From: Andy Green <hidden> Date: 2011-03-04 08:32:01
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.
quoted
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<redacted>
Signed-off-by: Andy Green<redacted>
The I2C maintainer should be in CC as well.
OK thanks for this correction.
quoted
+ /* 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
From: Andy Green <hidden> Date: 2011-03-04 08:33:36
On 03/03/2011 08:40 PM, Somebody in the thread at some point said:
Hi -
quoted
quoted
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 03/03/2011 09:33 PM, Somebody in the thread at some point said:
Hi -
quoted
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
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<redacted>
Signed-off-by: Andy Green<redacted>
The I2C maintainer should be in CC as well.
OK thanks for this correction.
quoted
quoted
+ /* 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
On 03/03/2011 08:40 PM, Somebody in the thread at some point said:
Hi -
quoted
quoted
quoted
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.
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
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