Re: [Qemu-devel] [PATCH 1/2] omap_i2c: Clear SBD bit in STAT register on DATA read

[Peter Maydell]

On 13 December 2012 17:04, Andreas Färber <address@hidden> wrote:
> Am 13.12.2012 15:45, schrieb Peter Maydell:
>> On 12 December 2012 06:29, Andreas Färber <address@hidden> wrote:
>>> After reading a single-byte I2C response such as the tmp105's response
>>> to 0x01 0x00, the SBD status bit would not get reset on next read, still
>>> indicating validity of only a single byte. Clear it on next word read.
>>
>> This doesn't seem to correspond to what the OMAP1510 manual describes
>> as the condition for this bit to be zeroed:
>
> I don't have the manual, just Andrzej's omap_i2c code. n800/n810 seems
> to be OMAP2420 btw.

I don't have an OMAP2 TRM, alas.

>> "This bit is cleared to 0 by the core when the local host reads the
>> I2C_IV register if INTCODE is register access ready."
>
>     case 0x0c:  /* I2C_IV */
>         if (s->revision >= OMAP2_INTR_REV)
>             break;
>
> And our s->revision == OMAP2_INTR_REV (0x34), so reading IV is a no-op.
> It reads as related to interrupt handling, which I was otherwise not
> touching on.
>
> There was no other comment saying "SBD" anywhere or touching "1 << 15"
> of s->stat, so to me it seems nothing resets this bit today.

I agree that something should be resetting the bit, I'm just
questioning whether the place you've put the reset is the
right place.

Checking actual hardware behaviour might also be interesting.

> I don't get any hit for "SBD" in the Linux driver either, i2c-omap.c in
> torvalds/linux.git seems to unconditionally read both bytes in
> omap_i2c_receive_data() if the device has a 16-bit register. CC'ing the
> Linux OMAP maintainer.
>
> I could try to simply ignore SBD and rely on my own counting. I already
> moved it to its own libqos source file last night.

If we don't have a guest that's known to work on real h/w
I'm a bit reluctant to change the model to match something
that's only been run on QEMU.

>> The manual also says for I2C_DATA:
>> "In case of an odd number of bytes received to read, the upper byte of
>> the last access always reads as 0x00. The local host must check the SBD
>> status bit in I2C_STAT register to flush this null byte."
>>
>> ...which to a naive reading implies that the high byte has
>> to be jammed to all-zeroes until I2C_STAT is read, but that
>> seems a little implausible.
>
> To me that just says I need to check SBD to decide whether the high byte
> last read is valid.

That interpretation requires a very bizarre reading of the word
"flush" (though it is certainly a more plausible actual behaviour,
so it could well be that this is just a weirdly written line of
documentation).

-- PMM