Re: [Qemu-devel] omap_i2c: distinction between OMAP2_INTR_REV and OMAP2_GC_REV?

[Peter Maydell]

On 20 February 2012 12:12, andrzej zaborowski <address@hidden> wrote:
> On 17 February 2012 19:21, Peter Maydell <address@hidden> wrote:
>> Does anybody know what the distinction between these two constants
>> is supposed to be? They were introduced by commit 29885477
>> back in 2008... (Also, why "INTR" and "GC"?)
>
> Apparently the intent was for the revision to be used to
> enable/disable different features of the I2C module.  OMAP2_GC_REV
> should be the minimum revision where the omap2 Global Call interrupt
> is available.  INTR seems to be the feature that enables the interrupt
> status to be read resetting some interrupt flags.  There's an
> assumption that every revision is backwards compatible and features
> never go away.

Ah, I see. The difficulty with this is that we don't have separate
docs for the I2C module which indicate what all the new features
are and when they were introduced, so we only have the TRMs.

> I think another assumption was that between the I2C module revision
> 1.1 and 3.4 other features were introduced gradually, so
> omap1/omap2/omap3 resolution would not be enough.
>
> I have neither of the TRMs at hand but I downloaded the TRM for
> omap35x which features i2c module 3.1 and it looks like the current
> code is not even handling the masking of all the interrupts of that
> version correctly (there are 14 in 3.1)

There are a lot of omap3 changes in my patchset which I'm
trying to clean up; possibly that's one of them. Incidentally
I don't think the 35x does have 3.1 of this module, that's just
an example indicating the format of the revision field. If you
boot a beagle board (36xx but that's the same as 35xx) it will
say "omap_i2c omap_i2c.1: bus 1 rev4.0 at 2600 kHz" indicating
that this is a rev 4.0 I2C module.

(I have no idea why TI don't put the actual submodule revisions
in their TRMs, it's not like you can't find it out from the hardware.)

>> The patchset I'm trying to clean up goes for:
>> #define OMAP1_INTR_REV    0x11
>> #define OMAP2_INTR_REV    0x34
>> #define OMAP3_INTR_REV    0x3c
>> #define OMAP3630_INTR_REV 0x40
>>
>> although I'm not sure whether 'INTR' makes much sense rather
>> than 'I2C' or something. I'll stick with these constants if nobody
>> has a better idea, but I was wondering if anybody could remember
>> the history behind the current constant names...
>
> I guess it would be a change of semantics rather than clean-up, but it
> may a good idea.  On the other hand I don't see much benefit from
> using those defines instead of hardcoding the revision values in the
> init functions.

Well, the #defines mean that when the code which actually implements
the "only in this rev or better" is doing an if() it isn't using a
hardcoded constant.

FWIW the patch I have which makes this a sysbus device makes the
I2C module revision be a qdev property, so the omap2 initialisation
function creates two i2c devices with the revision property set to
0x34, and so on.

-- PMM