[1/2] omap_i2c: Clear SBD bit in STAT register on DATA read

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.

Signed-off-by: Andreas Färber <andreas.faerber@web.de>
---
 hw/omap_i2c.c |    1 +
 1 Datei geändert, 1 Zeile hinzugefügt(+)

On 12 December 2012 06:29, Andreas Färber <andreas.faerber@web.de> 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:

"This bit is cleared to 0 by the core when the local host reads the
I2C_IV register if INTCODE is register access ready."

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.

(interestingly the SBD bit is always 0 for omap3 because the data
FIFO is 8 bits wide and so data is always read byte at at time)

-- PMM

Am 13.12.2012 15:45, schrieb Peter Maydell:
> On 12 December 2012 06:29, Andreas Färber <andreas.faerber@web.de> 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.

> "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 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.

> 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.

Andreas

> (interestingly the SBD bit is always 0 for omap3 because the data
> FIFO is 8 bits wide and so data is always read byte at at time)
> 
> -- PMM

On 13 December 2012 17:04, Andreas Färber <andreas.faerber@web.de> wrote:
> Am 13.12.2012 15:45, schrieb Peter Maydell:
>> On 12 December 2012 06:29, Andreas Färber <andreas.faerber@web.de> 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