i2c-omap.c:omap_i2c_idle() writes to nonexistent register on OMAP3630

Bug #645324 reported by Peter Maydell
10
This bug affects 1 person
Affects Status Importance Assigned to Milestone
linaro-landing-team-ti
Confirmed
Low
warmcat

Bug Description

If you boot the linaro beta image on qemu-maemo's beaglexm model:

qemu-maemo-system-arm -M beaglexm -sd linaro-beta.img -clock unix -nographic
then during bootup there are a large number of warning messages:
omap_i2c_write: Bad register 0x000000ec

The DM37x TRM section 17.6.2 (HS I2C Registers) only lists registers at offsets from 0x00 to 0x54, so I think the model is correct that 0xEC is bogus. We should investigate what bit of the kernel is doing this and what it thinks it's doing.

Revision history for this message
Loïc Minier (lool) wrote :

Moving to qemu-linaro, should investigate and move as appropriate

affects: qemu-maemo → qemu-linaro
Revision history for this message
Peter Maydell (pmaydell) wrote :

Still happens, although the register is 0xe8 now. This was waiting for there to be a hwpack with a kernel for which we hadn't lost the ddeb, as it's basically undebuggable without debugsyms for the kernel.

It is only a beaglexm issue, incidentally.

Peter Maydell (pmaydell)
Changed in qemu-linaro:
assignee: nobody → Peter Maydell (pmaydell)
importance: Undecided → Low
status: New → Confirmed
Revision history for this message
Peter Maydell (pmaydell) wrote :

This happens because the Linux omap_i2c driver omap_i2c_idle() routine thinks we have a register we don't have:
 if (dev->rev >= OMAP_I2C_REV_ON_4430)
  omap_i2c_write_reg(dev, OMAP_I2C_IRQENABLE_CLR, 1);
 else
  omap_i2c_write_reg(dev, OMAP_I2C_IE_REG, 0);

...because we're claiming a REVISION of 0x40 which Linux thinks means 4430. This in turn happens because qemu's beagle model initialises the omap3 code to be "omap3630", which is wrong according to the Beagle XM docs which say it has an omap3530. The qemu omap code doesn't model an omap3530.

I don't know if qemu is right about the I2C revision being 0x40 on the 3630 because for some reason TI think that the revision number is "TI internal data" despite it being freely available in the register if you have the hardware...

Revision history for this message
Peter Maydell (pmaydell) wrote :

> the Beagle XM docs which say it has an omap3530

I was looking at the wrong doc; the Beagle is 3530 (apparently more or less identical to 3430 for marketing purposes); the XM is 3730/3630 (ditto).

> I don't know if qemu is right about the I2C revision being 0x40

It is; from real XM hardware:

[ 0.045898] i2c_omap i2c_omap.1: bus 1 rev4.0 at 2600 kHz

...so this is a bug in the kernel i2c-omap.c, and qemu is legitimately complaining about it.

Revision history for this message
Peter Maydell (pmaydell) wrote :

Since this is a kernel issue I'm moving it over to "linux-linaro"; please let me know if I should have filed it somewhere else.

The specific kernel I've been looking at is:
 [ 0.000000] Linux version 2.6.37-1002-linaro-omap (buildd@actinidiaceae) (gcc version 4.5.2 (Ubuntu/Linaro 4.5.2-1ubuntu6) ) #5-Ubuntu SMP Thu Jan 20 02:19:36 UTC 2011 (Ubuntu 2.6.37-1002.5-linaro-omap 2.6.37)

from the 20110126 linaro omap3 snapshot+hwpack.

summary: - qemu-maemo: beaglexm boot complains "omap_i2c_write: Bad register
- 0x000000ec"
+ i2c-omap.c:omap_i2c_idle() writes to nonexistent register on OMAP3630
affects: qemu-linaro → linux-linaro
Peter Maydell (pmaydell)
Changed in linux-linaro:
assignee: Peter Maydell (pmaydell) → nobody
Revision history for this message
warmcat (andy-warmcat) wrote :

The underlying problem seems to be that TI extended the size of their revision register on the OMAP4430 version of the i2c peripheral unit, without maintaining the position of the revision LSB.

So, prior to OMAP4430, there was one 16-bit word at offset +0 defining the revision. On OMAP4430, there are two words and it seems the word with the same logical meaning as the one at +0 before, is now at +4.

This leads to a situation where the code can't probe just from the peripheral unit ID, because then it won't know beforehand which register(s) it is in.

In the existing driver, they go outside the peripheral unit's own ID and just in that one place to decide the register map, they use cpu_is_omap44xx() to decide if they will choose "old style" register map or "new style". The fact is, this DOES make the correct choice, because otherwise 3530 I2C would be broken since the OMAP4430 register layout is totally different.

That seems to lead to the conclusion that you can find 0x40 in the +0 register on OMAP3530 AND 0x40 in the +4 register on OMAP4430, the idea being that the extra 16 bits of revision numbering in the +0 register then lets you tell the difference.

The bug comes from an inconsistent test to use the new registers or not, by CPU type to select the register map and then by reported peripheral unit revision to decide whether to use them or not.

The attached patchset solves this and has been sent upstream.

Revision history for this message
warmcat (andy-warmcat) wrote :
Revision history for this message
warmcat (andy-warmcat) wrote :
Changed in linux-linaro:
assignee: nobody → warmcat (andy-warmcat)
Linus Walleij (triad)
affects: linux-linaro → linaro-landing-team-ti
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers