[Qemu-devel] [PATCH 0/2] tmp105: qtest support

[Qemu-devel] [PATCH 0/2] tmp105: qtest support

[Andreas Färber]

Hello,

Last night I decided to learn about I2C and derived a qtest from the omap_i2c
implementation used in the n800/n810. Since that is the first I2C device I got
to know, this is not yet a full-blown generic I2C libqos framework, but it
should be pretty easy to generalize it further as follow-up.

This test case uncovers a bug with odd-numbered I2C reads in the OMAP
implementation, for which I'm including a fix here.

The test case itself will still fail on master, a fix is being discussed in
thread "tmp105: Fix I2C protocol bug".

The proposed tmp105.h header file promises to allow sharing the enum for the
four registers.

Regards,
Andreas

Cc: Alex Horn <alex.horn@cs.ox.ac.uk>
Cc: Andrzej Zaborowski <balrogg@gmail.com>
Cc: Peter Maydell <peter.maydell@linaro.org>
Cc: Blue Swirl <blauwirbel@gmail.com>

Andreas Färber (2):
  omap_i2c: Clear SBD bit in STAT register on DATA read
  tests: Add tmp105 unit test

 hw/omap_i2c.c       |    1 +
 tests/Makefile      |    2 +
 tests/tmp105-test.c |  205 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 Dateien geändert, 208 Zeilen hinzugefügt(+)
 create mode 100644 tests/tmp105-test.c

-- 
1.7.10.4

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

[Andreas Färber]

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(+)

diff --git a/hw/omap_i2c.c b/hw/omap_i2c.c
index ba08e64..308c088 100644
--- a/hw/omap_i2c.c
+++ b/hw/omap_i2c.c
@@ -196,6 +196,7 @@ static uint32_t omap_i2c_read(void *opaque, hwaddr addr)
             s->stat |= 1 << 15;					/* SBD */
             s->rxlen = 0;
         } else if (s->rxlen > 1) {
+            s->stat &= ~(1 << 15); /* SBD */
             if (s->rxlen > 2)
                 s->fifo >>= 16;
             s->rxlen -= 2;
-- 
1.7.10.4

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

[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:

"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

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

[Andreas Färber]

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

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

[Qemu-devel] [PATCH 2/2] tests: Add tmp105 unit test

[Andreas Färber]

Exercise all four commands of the TMP105, testing for an issue in the
I2C TX path. The test case is specific to the n800's OMAP I2C for now
and is the first test case for arm.

Signed-off-by: Andreas Färber <andreas.faerber@web.de>
---
 tests/Makefile      |    2 +
 tests/tmp105-test.c |  205 +++++++++++++++++++++++++++++++++++++++++++++++++++
 2 Dateien geändert, 207 Zeilen hinzugefügt(+)
 create mode 100644 tests/tmp105-test.c

diff --git a/tests/Makefile b/tests/Makefile
index b60f0fb..69eff45 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -28,6 +28,7 @@ check-qtest-i386-y += tests/rtc-test$(EXESUF)
 check-qtest-x86_64-y = $(check-qtest-i386-y)
 check-qtest-sparc-y = tests/m48t59-test$(EXESUF)
 check-qtest-sparc64-y = tests/m48t59-test$(EXESUF)
+check-qtest-arm-y = tests/tmp105-test$(EXESUF)
 
 GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h tests/test-qmp-commands.h
 
@@ -78,6 +79,7 @@ tests/rtc-test$(EXESUF): tests/rtc-test.o $(trace-obj-y)
 tests/m48t59-test$(EXESUF): tests/m48t59-test.o $(trace-obj-y)
 tests/fdc-test$(EXESUF): tests/fdc-test.o tests/libqtest.o $(trace-obj-y)
 tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o tests/libqtest.o $(trace-obj-y)
+tests/tmp105-test$(EXESUF): tests/tmp105-test.o $(trace-obj-y)
 
 # QTest rules
 
diff --git a/tests/tmp105-test.c b/tests/tmp105-test.c
new file mode 100644
index 0000000..26d51f7
--- /dev/null
+++ b/tests/tmp105-test.c
@@ -0,0 +1,205 @@
+/*
+ * QTest testcase for the TMP105 temperature sensor
+ *
+ * Copyright (c) 2012 Andreas Färber
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+#include "libqtest.h"
+
+#include <glib.h>
+#include <string.h>
+
+enum OMAPI2CRegisters {
+    OMAP_I2C_REV  = 0x00,
+    OMAP_I2C_STAT = 0x08,
+    OMAP_I2C_CNT  = 0x18,
+    OMAP_I2C_DATA = 0x1c,
+    OMAP_I2C_CON  = 0x24,
+    OMAP_I2C_SA   = 0x2c,
+};
+
+enum OMAPI2CSTATBits {
+    OMAP_I2C_STAT_NACK = 1 << 1,
+    OMAP_I2C_STAT_ARDY = 1 << 2,
+    OMAP_I2C_STAT_RRDY = 1 << 3,
+    OMAP_I2C_STAT_XRDY = 1 << 4,
+    OMAP_I2C_STAT_ROVR = 1 << 11,
+    OMAP_I2C_STAT_SBD  = 1 << 15,
+};
+
+enum OMAPI2CCONBits {
+    OMAP_I2C_CON_STT    = 1 << 0,
+    OMAP_I2C_CON_STP    = 1 << 1,
+    OMAP_I2C_CON_TRX    = 1 << 9,
+    OMAP_I2C_CON_MST    = 1 << 10,
+    OMAP_I2C_CON_BE     = 1 << 14,
+    OMAP_I2C_CON_I2C_EN = 1 << 15,
+};
+
+#define OMAP2_I2C_1_BASE 0x48070000
+
+#define N8X0_ADDR 0x48
+
+static const uint32_t i2c_base = OMAP2_I2C_1_BASE;
+
+static void omap_i2c_init(void)
+{
+    uint16_t data;
+
+    /* verify the mmio address by looking for a known signature */
+    memread(i2c_base + OMAP_I2C_REV, &data, 2);
+    g_assert_cmphex(data, ==, 0x34);
+}
+
+static void omap_i2c_set_slave_addr(uint8_t addr)
+{
+    uint16_t data = addr;
+
+    memwrite(i2c_base + OMAP_I2C_SA, &data, 2);
+    memread(i2c_base + OMAP_I2C_SA, &data, 2);
+    g_assert_cmphex(data, ==, addr);
+}
+
+static void omap_i2c_send(uint8_t addr, const void *buf, uint16_t len)
+{
+    uint16_t data;
+
+    omap_i2c_set_slave_addr(addr);
+
+    data = len;
+    memwrite(i2c_base + OMAP_I2C_CNT, &data, 2);
+
+    data = OMAP_I2C_CON_I2C_EN |
+           OMAP_I2C_CON_TRX |
+           OMAP_I2C_CON_MST |
+           OMAP_I2C_CON_STT |
+           OMAP_I2C_CON_STP;
+    memwrite(i2c_base + OMAP_I2C_CON, &data, 2);
+    memread(i2c_base + OMAP_I2C_CON, &data, 2);
+    g_assert((data & OMAP_I2C_CON_STP) != 0);
+
+    memread(i2c_base + OMAP_I2C_STAT, &data, 2);
+    g_assert((data & OMAP_I2C_STAT_NACK) == 0);
+
+    while (len > 1) {
+        memread(i2c_base + OMAP_I2C_STAT, &data, 2);
+        g_assert((data & OMAP_I2C_STAT_XRDY) != 0);
+
+        memwrite(i2c_base + OMAP_I2C_DATA, buf, 2);
+        buf = (uint8_t *)buf + 2;
+        len -= 2;
+    }
+    if (len == 1) {
+        memread(i2c_base + OMAP_I2C_STAT, &data, 2);
+        g_assert((data & OMAP_I2C_STAT_XRDY) != 0);
+
+        memwrite(i2c_base + OMAP_I2C_DATA, buf, 1);
+    }
+
+    memread(i2c_base + OMAP_I2C_CON, &data, 2);
+    g_assert((data & OMAP_I2C_CON_STP) == 0);
+}
+
+static void omap_i2c_recv(uint8_t addr, void *buf, uint16_t len)
+{
+    uint16_t data, stat;
+
+    omap_i2c_set_slave_addr(addr);
+
+    data = len;
+    memwrite(i2c_base + OMAP_I2C_CNT, &data, 2);
+
+    data = OMAP_I2C_CON_I2C_EN |
+           OMAP_I2C_CON_MST |
+           OMAP_I2C_CON_STT |
+           OMAP_I2C_CON_STP;
+    memwrite(i2c_base + OMAP_I2C_CON, &data, 2);
+    memread(i2c_base + OMAP_I2C_CON, &data, 2);
+    g_assert((data & OMAP_I2C_CON_STP) == 0);
+
+    memread(i2c_base + OMAP_I2C_STAT, &data, 2);
+    g_assert((data & OMAP_I2C_STAT_NACK) == 0);
+
+    memread(i2c_base + OMAP_I2C_CNT, &data, 2);
+    g_assert_cmpuint(data, ==, len);
+
+    while (len > 0) {
+        memread(i2c_base + OMAP_I2C_STAT, &data, 2);
+        g_assert((data & OMAP_I2C_STAT_RRDY) != 0);
+        g_assert((data & OMAP_I2C_STAT_ROVR) == 0);
+
+        memread(i2c_base + OMAP_I2C_DATA, &data, 2);
+
+        memread(i2c_base + OMAP_I2C_STAT, &stat, 2);
+        if ((stat & OMAP_I2C_STAT_SBD) != 0) {
+            *(uint8_t *)buf = data & 0xf;
+            buf = (uint8_t *)buf + 1;
+            len--;
+        } else {
+            memcpy(buf, &data, 2);
+            buf = (uint8_t *)buf + 2;
+            len -= 2;
+        }
+    }
+
+    memread(i2c_base + OMAP_I2C_CON, &data, 2);
+    g_assert((data & OMAP_I2C_CON_STP) == 0);
+}
+
+static void send_and_receive(void)
+{
+    const uint8_t addr = N8X0_ADDR;
+    uint8_t cmd[3];
+    uint8_t resp[2];
+
+    omap_i2c_init();
+
+    cmd[0] = 0;
+    omap_i2c_send(addr, cmd, 1);
+    omap_i2c_recv(addr, resp, 2);
+    g_assert_cmpuint(((uint16_t)resp[0] << 8) | resp[1], ==, 0);
+
+    cmd[0] = 1;
+    cmd[1] = 0x0;
+    omap_i2c_send(addr, cmd, 2);
+    omap_i2c_recv(addr, resp, 1);
+    g_assert_cmphex(resp[0], ==, cmd[1]);
+
+    cmd[0] = 2;
+    cmd[1] = 0x12;
+    cmd[2] = 0x34;
+    omap_i2c_send(addr, cmd, 3);
+    omap_i2c_recv(addr, resp, 2);
+    g_assert_cmphex(resp[0], ==, cmd[1]);
+    g_assert_cmphex(resp[1], ==, cmd[2]);
+
+    cmd[0] = 3;
+    cmd[1] = 0x42;
+    cmd[2] = 0x31;
+    omap_i2c_send(addr, cmd, 3);
+    omap_i2c_recv(addr, resp, 2);
+    g_assert_cmphex(resp[0], ==, cmd[1]);
+    g_assert_cmphex(resp[1], ==, cmd[2]);
+}
+
+int main(int argc, char **argv)
+{
+    QTestState *s = NULL;
+    int ret;
+
+    g_test_init(&argc, &argv, NULL);
+
+    s = qtest_start("-display none -machine n800");
+
+    qtest_add_func("/tmp105/tx-rx", send_and_receive);
+
+    ret = g_test_run();
+
+    if (s) {
+        qtest_quit(s);
+    }
+
+    return ret;
+}
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 2/2] tests: Add tmp105 unit test

[Alex Horn]

Thanks so much for taking the initiative on creating functional tests
for the TMP105 hardware model. I am exciting to see these changes and
I am wondering if your QTests could be complemented with the following
standalone unit tests:

  https://github.com/ahorn/benchmarks/blob/965e571d92e677e8e64ad5faa0107f5dbd451981/qemu-hw/tmp105/tmp105-test.c

Their purpose is similar to yours except that unit tests focus on
checking the internal state of a hardware model without requiring a
bus implementation or a socket connection for the QTest client-server
infrastructure. That is, unit tests do not seek to replace QTests but
strengthen them (see also below).

In fact, I have tried including the above unit tests in QEMU. For
this, the following lines were added to tests/Makefile:

  check-unit-y += tests/test-tmp105$(EXESUF)
  tests/test-tmp105$(EXESUF): tests/test-tmp105.o hw/tmp105.o
$(qobject-obj-y) $(common-obj-y) $(qom-obj-y)

The last three $(...) variables are required to compile. However,
"make check" results in a NSS linking error:

/usr/bin/ld: libcacard/vcard_emul_nss.o: undefined reference to symbol
'CERT_GetDefaultCertDB@@NSS_3.2'
/usr/bin/ld: note: 'CERT_GetDefaultCertDB@@NSS_3.2' is defined in DSO
/usr/lib64/libnss3.so so try adding it to the linker command line
/usr/lib64/libnss3.so: could not read symbols: Invalid operation
collect2: ld returned 1 exit status
make: *** [tests/test-tmp105] Error 1

If these type of issues could be resolved, it would be possible to
encourage others to write unit tests for their hardware models.

The utility of these unit tests is manifold. Noteworthy, a certain
level of confidence can be achieved before integrating a hardware
model into a bus implementation (e.g. OMAP or SMBus for I2C). This
could also prove useful for overall regression testing and debugging.
Finally, unit tests encourage modular designs (e.g. [1]). Such
modularity could also be a catalyst for new QEMU use cases and a
potentially broader developer base.

With kind regards,
Alex

[1] http://en.wikipedia.org/wiki/Dependency_injection

On 12 December 2012 06:29, Andreas Färber <andreas.faerber@web.de> wrote:
> Exercise all four commands of the TMP105, testing for an issue in the
> I2C TX path. The test case is specific to the n800's OMAP I2C for now
> and is the first test case for arm.
>
> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
> ---
>  tests/Makefile      |    2 +
>  tests/tmp105-test.c |  205 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 Dateien geändert, 207 Zeilen hinzugefügt(+)
>  create mode 100644 tests/tmp105-test.c
>
> diff --git a/tests/Makefile b/tests/Makefile
> index b60f0fb..69eff45 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -28,6 +28,7 @@ check-qtest-i386-y += tests/rtc-test$(EXESUF)
>  check-qtest-x86_64-y = $(check-qtest-i386-y)
>  check-qtest-sparc-y = tests/m48t59-test$(EXESUF)
>  check-qtest-sparc64-y = tests/m48t59-test$(EXESUF)
> +check-qtest-arm-y = tests/tmp105-test$(EXESUF)
>
>  GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h tests/test-qmp-commands.h
>
> @@ -78,6 +79,7 @@ tests/rtc-test$(EXESUF): tests/rtc-test.o $(trace-obj-y)
>  tests/m48t59-test$(EXESUF): tests/m48t59-test.o $(trace-obj-y)
>  tests/fdc-test$(EXESUF): tests/fdc-test.o tests/libqtest.o $(trace-obj-y)
>  tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o tests/libqtest.o $(trace-obj-y)
> +tests/tmp105-test$(EXESUF): tests/tmp105-test.o $(trace-obj-y)
>
>  # QTest rules
>
> diff --git a/tests/tmp105-test.c b/tests/tmp105-test.c
> new file mode 100644
> index 0000000..26d51f7
> --- /dev/null
> +++ b/tests/tmp105-test.c
> @@ -0,0 +1,205 @@
> +/*
> + * QTest testcase for the TMP105 temperature sensor
> + *
> + * Copyright (c) 2012 Andreas Färber
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#include "libqtest.h"
> +
> +#include <glib.h>
> +#include <string.h>
> +
> +enum OMAPI2CRegisters {
> +    OMAP_I2C_REV  = 0x00,
> +    OMAP_I2C_STAT = 0x08,
> +    OMAP_I2C_CNT  = 0x18,
> +    OMAP_I2C_DATA = 0x1c,
> +    OMAP_I2C_CON  = 0x24,
> +    OMAP_I2C_SA   = 0x2c,
> +};
> +
> +enum OMAPI2CSTATBits {
> +    OMAP_I2C_STAT_NACK = 1 << 1,
> +    OMAP_I2C_STAT_ARDY = 1 << 2,
> +    OMAP_I2C_STAT_RRDY = 1 << 3,
> +    OMAP_I2C_STAT_XRDY = 1 << 4,
> +    OMAP_I2C_STAT_ROVR = 1 << 11,
> +    OMAP_I2C_STAT_SBD  = 1 << 15,
> +};
> +
> +enum OMAPI2CCONBits {
> +    OMAP_I2C_CON_STT    = 1 << 0,
> +    OMAP_I2C_CON_STP    = 1 << 1,
> +    OMAP_I2C_CON_TRX    = 1 << 9,
> +    OMAP_I2C_CON_MST    = 1 << 10,
> +    OMAP_I2C_CON_BE     = 1 << 14,
> +    OMAP_I2C_CON_I2C_EN = 1 << 15,
> +};
> +
> +#define OMAP2_I2C_1_BASE 0x48070000
> +
> +#define N8X0_ADDR 0x48
> +
> +static const uint32_t i2c_base = OMAP2_I2C_1_BASE;
> +
> +static void omap_i2c_init(void)
> +{
> +    uint16_t data;
> +
> +    /* verify the mmio address by looking for a known signature */
> +    memread(i2c_base + OMAP_I2C_REV, &data, 2);
> +    g_assert_cmphex(data, ==, 0x34);
> +}
> +
> +static void omap_i2c_set_slave_addr(uint8_t addr)
> +{
> +    uint16_t data = addr;
> +
> +    memwrite(i2c_base + OMAP_I2C_SA, &data, 2);
> +    memread(i2c_base + OMAP_I2C_SA, &data, 2);
> +    g_assert_cmphex(data, ==, addr);
> +}
> +
> +static void omap_i2c_send(uint8_t addr, const void *buf, uint16_t len)
> +{
> +    uint16_t data;
> +
> +    omap_i2c_set_slave_addr(addr);
> +
> +    data = len;
> +    memwrite(i2c_base + OMAP_I2C_CNT, &data, 2);
> +
> +    data = OMAP_I2C_CON_I2C_EN |
> +           OMAP_I2C_CON_TRX |
> +           OMAP_I2C_CON_MST |
> +           OMAP_I2C_CON_STT |
> +           OMAP_I2C_CON_STP;
> +    memwrite(i2c_base + OMAP_I2C_CON, &data, 2);
> +    memread(i2c_base + OMAP_I2C_CON, &data, 2);
> +    g_assert((data & OMAP_I2C_CON_STP) != 0);
> +
> +    memread(i2c_base + OMAP_I2C_STAT, &data, 2);
> +    g_assert((data & OMAP_I2C_STAT_NACK) == 0);
> +
> +    while (len > 1) {
> +        memread(i2c_base + OMAP_I2C_STAT, &data, 2);
> +        g_assert((data & OMAP_I2C_STAT_XRDY) != 0);
> +
> +        memwrite(i2c_base + OMAP_I2C_DATA, buf, 2);
> +        buf = (uint8_t *)buf + 2;
> +        len -= 2;
> +    }
> +    if (len == 1) {
> +        memread(i2c_base + OMAP_I2C_STAT, &data, 2);
> +        g_assert((data & OMAP_I2C_STAT_XRDY) != 0);
> +
> +        memwrite(i2c_base + OMAP_I2C_DATA, buf, 1);
> +    }
> +
> +    memread(i2c_base + OMAP_I2C_CON, &data, 2);
> +    g_assert((data & OMAP_I2C_CON_STP) == 0);
> +}
> +
> +static void omap_i2c_recv(uint8_t addr, void *buf, uint16_t len)
> +{
> +    uint16_t data, stat;
> +
> +    omap_i2c_set_slave_addr(addr);
> +
> +    data = len;
> +    memwrite(i2c_base + OMAP_I2C_CNT, &data, 2);
> +
> +    data = OMAP_I2C_CON_I2C_EN |
> +           OMAP_I2C_CON_MST |
> +           OMAP_I2C_CON_STT |
> +           OMAP_I2C_CON_STP;
> +    memwrite(i2c_base + OMAP_I2C_CON, &data, 2);
> +    memread(i2c_base + OMAP_I2C_CON, &data, 2);
> +    g_assert((data & OMAP_I2C_CON_STP) == 0);
> +
> +    memread(i2c_base + OMAP_I2C_STAT, &data, 2);
> +    g_assert((data & OMAP_I2C_STAT_NACK) == 0);
> +
> +    memread(i2c_base + OMAP_I2C_CNT, &data, 2);
> +    g_assert_cmpuint(data, ==, len);
> +
> +    while (len > 0) {
> +        memread(i2c_base + OMAP_I2C_STAT, &data, 2);
> +        g_assert((data & OMAP_I2C_STAT_RRDY) != 0);
> +        g_assert((data & OMAP_I2C_STAT_ROVR) == 0);
> +
> +        memread(i2c_base + OMAP_I2C_DATA, &data, 2);
> +
> +        memread(i2c_base + OMAP_I2C_STAT, &stat, 2);
> +        if ((stat & OMAP_I2C_STAT_SBD) != 0) {
> +            *(uint8_t *)buf = data & 0xf;
> +            buf = (uint8_t *)buf + 1;
> +            len--;
> +        } else {
> +            memcpy(buf, &data, 2);
> +            buf = (uint8_t *)buf + 2;
> +            len -= 2;
> +        }
> +    }
> +
> +    memread(i2c_base + OMAP_I2C_CON, &data, 2);
> +    g_assert((data & OMAP_I2C_CON_STP) == 0);
> +}
> +
> +static void send_and_receive(void)
> +{
> +    const uint8_t addr = N8X0_ADDR;
> +    uint8_t cmd[3];
> +    uint8_t resp[2];
> +
> +    omap_i2c_init();
> +
> +    cmd[0] = 0;
> +    omap_i2c_send(addr, cmd, 1);
> +    omap_i2c_recv(addr, resp, 2);
> +    g_assert_cmpuint(((uint16_t)resp[0] << 8) | resp[1], ==, 0);
> +
> +    cmd[0] = 1;
> +    cmd[1] = 0x0;
> +    omap_i2c_send(addr, cmd, 2);
> +    omap_i2c_recv(addr, resp, 1);
> +    g_assert_cmphex(resp[0], ==, cmd[1]);
> +
> +    cmd[0] = 2;
> +    cmd[1] = 0x12;
> +    cmd[2] = 0x34;
> +    omap_i2c_send(addr, cmd, 3);
> +    omap_i2c_recv(addr, resp, 2);
> +    g_assert_cmphex(resp[0], ==, cmd[1]);
> +    g_assert_cmphex(resp[1], ==, cmd[2]);
> +
> +    cmd[0] = 3;
> +    cmd[1] = 0x42;
> +    cmd[2] = 0x31;
> +    omap_i2c_send(addr, cmd, 3);
> +    omap_i2c_recv(addr, resp, 2);
> +    g_assert_cmphex(resp[0], ==, cmd[1]);
> +    g_assert_cmphex(resp[1], ==, cmd[2]);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    QTestState *s = NULL;
> +    int ret;
> +
> +    g_test_init(&argc, &argv, NULL);
> +
> +    s = qtest_start("-display none -machine n800");
> +
> +    qtest_add_func("/tmp105/tx-rx", send_and_receive);
> +
> +    ret = g_test_run();
> +
> +    if (s) {
> +        qtest_quit(s);
> +    }
> +
> +    return ret;
> +}
> --
> 1.7.10.4
>

Re: [Qemu-devel] [PATCH 2/2] tests: Add tmp105 unit test

[Andreas Färber]

Am 12.12.2012 15:44, schrieb Alex Horn:
> Thanks so much for taking the initiative on creating functional tests
> for the TMP105 hardware model. I am exciting to see these changes and
> I am wondering if your QTests could be complemented with the following
> standalone unit tests:
> 
>   https://github.com/ahorn/benchmarks/blob/965e571d92e677e8e64ad5faa0107f5dbd451981/qemu-hw/tmp105/tmp105-test.c

Honestly I consider that test a gross hack for three reasons:
* You ignore any global state that devices may rely on, i.e. some
operations like hotplug may succeed that would normally fail.
* You completely bypass QOM/qdev infrastructure by instantiating random
structs on the stack. This may lead to devices not properly being
initialized.
* You ignore any endianness swizzling our Memory API takes care of. (Did
you verify your tests on a Big Endian host?)

All this would be quite a lot of work to duplicate into a testing
environment and it occasionally changes, which is why we are reusing the
"original" infrastructure for testing in a special "qtest" mode.

What would be appreciated though is if you could add some of your tests
to our test infrastructure as a follow-up to my patches - assuming my
proposal gets adopted. For testing the alarm, Paolo's IRQs interception
API may be useful (in rtc-test IIRC).

> Their purpose is similar to yours except that unit tests focus on
> checking the internal state of a hardware model without requiring a
> bus implementation or a socket connection for the QTest client-server
> infrastructure. That is, unit tests do not seek to replace QTests but
> strengthen them (see also below).

I am the wrong person to argue about this, Anthony has been setting up
this test infrastructure. Unlike C++ with its friend classes we can't
test any static C functions anyway, so the interface for testing the way
you propose is very limited.

What I have been demonstrating is that it is too trivial to do it the
expected qtest way (one evening a prototype for bus+device plus one
evening a bus abstraction) to try working around it. :)

Regards,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH 2/2] tests: Add tmp105 unit test

[Blue Swirl]

On Wed, Dec 12, 2012 at 6:29 AM, Andreas Färber <andreas.faerber@web.de> wrote:
> Exercise all four commands of the TMP105, testing for an issue in the
> I2C TX path. The test case is specific to the n800's OMAP I2C for now
> and is the first test case for arm.
>
> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
> ---
>  tests/Makefile      |    2 +
>  tests/tmp105-test.c |  205 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 Dateien geändert, 207 Zeilen hinzugefügt(+)
>  create mode 100644 tests/tmp105-test.c
>
> diff --git a/tests/Makefile b/tests/Makefile
> index b60f0fb..69eff45 100644
> --- a/tests/Makefile
> +++ b/tests/Makefile
> @@ -28,6 +28,7 @@ check-qtest-i386-y += tests/rtc-test$(EXESUF)
>  check-qtest-x86_64-y = $(check-qtest-i386-y)
>  check-qtest-sparc-y = tests/m48t59-test$(EXESUF)
>  check-qtest-sparc64-y = tests/m48t59-test$(EXESUF)
> +check-qtest-arm-y = tests/tmp105-test$(EXESUF)
>
>  GENERATED_HEADERS += tests/test-qapi-types.h tests/test-qapi-visit.h tests/test-qmp-commands.h
>
> @@ -78,6 +79,7 @@ tests/rtc-test$(EXESUF): tests/rtc-test.o $(trace-obj-y)
>  tests/m48t59-test$(EXESUF): tests/m48t59-test.o $(trace-obj-y)
>  tests/fdc-test$(EXESUF): tests/fdc-test.o tests/libqtest.o $(trace-obj-y)
>  tests/hd-geo-test$(EXESUF): tests/hd-geo-test.o tests/libqtest.o $(trace-obj-y)
> +tests/tmp105-test$(EXESUF): tests/tmp105-test.o $(trace-obj-y)
>
>  # QTest rules
>
> diff --git a/tests/tmp105-test.c b/tests/tmp105-test.c
> new file mode 100644
> index 0000000..26d51f7
> --- /dev/null
> +++ b/tests/tmp105-test.c
> @@ -0,0 +1,205 @@
> +/*
> + * QTest testcase for the TMP105 temperature sensor
> + *
> + * Copyright (c) 2012 Andreas Färber
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + */
> +#include "libqtest.h"
> +
> +#include <glib.h>
> +#include <string.h>
> +
> +enum OMAPI2CRegisters {
> +    OMAP_I2C_REV  = 0x00,
> +    OMAP_I2C_STAT = 0x08,
> +    OMAP_I2C_CNT  = 0x18,
> +    OMAP_I2C_DATA = 0x1c,
> +    OMAP_I2C_CON  = 0x24,
> +    OMAP_I2C_SA   = 0x2c,
> +};
> +
> +enum OMAPI2CSTATBits {
> +    OMAP_I2C_STAT_NACK = 1 << 1,
> +    OMAP_I2C_STAT_ARDY = 1 << 2,
> +    OMAP_I2C_STAT_RRDY = 1 << 3,
> +    OMAP_I2C_STAT_XRDY = 1 << 4,
> +    OMAP_I2C_STAT_ROVR = 1 << 11,
> +    OMAP_I2C_STAT_SBD  = 1 << 15,
> +};
> +
> +enum OMAPI2CCONBits {
> +    OMAP_I2C_CON_STT    = 1 << 0,
> +    OMAP_I2C_CON_STP    = 1 << 1,
> +    OMAP_I2C_CON_TRX    = 1 << 9,
> +    OMAP_I2C_CON_MST    = 1 << 10,
> +    OMAP_I2C_CON_BE     = 1 << 14,
> +    OMAP_I2C_CON_I2C_EN = 1 << 15,
> +};
> +
> +#define OMAP2_I2C_1_BASE 0x48070000
> +
> +#define N8X0_ADDR 0x48
> +
> +static const uint32_t i2c_base = OMAP2_I2C_1_BASE;
> +
> +static void omap_i2c_init(void)
> +{
> +    uint16_t data;
> +
> +    /* verify the mmio address by looking for a known signature */
> +    memread(i2c_base + OMAP_I2C_REV, &data, 2);
> +    g_assert_cmphex(data, ==, 0x34);
> +}
> +
> +static void omap_i2c_set_slave_addr(uint8_t addr)
> +{
> +    uint16_t data = addr;
> +
> +    memwrite(i2c_base + OMAP_I2C_SA, &data, 2);
> +    memread(i2c_base + OMAP_I2C_SA, &data, 2);
> +    g_assert_cmphex(data, ==, addr);
> +}
> +
> +static void omap_i2c_send(uint8_t addr, const void *buf, uint16_t len)

With const uint8_t *buf you could avoid a few casts below.

> +{
> +    uint16_t data;
> +
> +    omap_i2c_set_slave_addr(addr);
> +
> +    data = len;
> +    memwrite(i2c_base + OMAP_I2C_CNT, &data, 2);
> +
> +    data = OMAP_I2C_CON_I2C_EN |
> +           OMAP_I2C_CON_TRX |
> +           OMAP_I2C_CON_MST |
> +           OMAP_I2C_CON_STT |
> +           OMAP_I2C_CON_STP;
> +    memwrite(i2c_base + OMAP_I2C_CON, &data, 2);
> +    memread(i2c_base + OMAP_I2C_CON, &data, 2);
> +    g_assert((data & OMAP_I2C_CON_STP) != 0);
> +
> +    memread(i2c_base + OMAP_I2C_STAT, &data, 2);
> +    g_assert((data & OMAP_I2C_STAT_NACK) == 0);
> +
> +    while (len > 1) {
> +        memread(i2c_base + OMAP_I2C_STAT, &data, 2);
> +        g_assert((data & OMAP_I2C_STAT_XRDY) != 0);
> +
> +        memwrite(i2c_base + OMAP_I2C_DATA, buf, 2);
> +        buf = (uint8_t *)buf + 2;
> +        len -= 2;
> +    }
> +    if (len == 1) {
> +        memread(i2c_base + OMAP_I2C_STAT, &data, 2);
> +        g_assert((data & OMAP_I2C_STAT_XRDY) != 0);
> +
> +        memwrite(i2c_base + OMAP_I2C_DATA, buf, 1);
> +    }
> +
> +    memread(i2c_base + OMAP_I2C_CON, &data, 2);
> +    g_assert((data & OMAP_I2C_CON_STP) == 0);
> +}
> +
> +static void omap_i2c_recv(uint8_t addr, void *buf, uint16_t len)
> +{
> +    uint16_t data, stat;
> +
> +    omap_i2c_set_slave_addr(addr);
> +
> +    data = len;
> +    memwrite(i2c_base + OMAP_I2C_CNT, &data, 2);
> +
> +    data = OMAP_I2C_CON_I2C_EN |
> +           OMAP_I2C_CON_MST |
> +           OMAP_I2C_CON_STT |
> +           OMAP_I2C_CON_STP;
> +    memwrite(i2c_base + OMAP_I2C_CON, &data, 2);
> +    memread(i2c_base + OMAP_I2C_CON, &data, 2);
> +    g_assert((data & OMAP_I2C_CON_STP) == 0);
> +
> +    memread(i2c_base + OMAP_I2C_STAT, &data, 2);
> +    g_assert((data & OMAP_I2C_STAT_NACK) == 0);
> +
> +    memread(i2c_base + OMAP_I2C_CNT, &data, 2);
> +    g_assert_cmpuint(data, ==, len);
> +
> +    while (len > 0) {
> +        memread(i2c_base + OMAP_I2C_STAT, &data, 2);
> +        g_assert((data & OMAP_I2C_STAT_RRDY) != 0);
> +        g_assert((data & OMAP_I2C_STAT_ROVR) == 0);
> +
> +        memread(i2c_base + OMAP_I2C_DATA, &data, 2);
> +
> +        memread(i2c_base + OMAP_I2C_STAT, &stat, 2);
> +        if ((stat & OMAP_I2C_STAT_SBD) != 0) {
> +            *(uint8_t *)buf = data & 0xf;
> +            buf = (uint8_t *)buf + 1;
> +            len--;
> +        } else {
> +            memcpy(buf, &data, 2);
> +            buf = (uint8_t *)buf + 2;
> +            len -= 2;
> +        }
> +    }
> +
> +    memread(i2c_base + OMAP_I2C_CON, &data, 2);
> +    g_assert((data & OMAP_I2C_CON_STP) == 0);
> +}
> +
> +static void send_and_receive(void)
> +{
> +    const uint8_t addr = N8X0_ADDR;
> +    uint8_t cmd[3];
> +    uint8_t resp[2];
> +
> +    omap_i2c_init();
> +
> +    cmd[0] = 0;
> +    omap_i2c_send(addr, cmd, 1);
> +    omap_i2c_recv(addr, resp, 2);
> +    g_assert_cmpuint(((uint16_t)resp[0] << 8) | resp[1], ==, 0);
> +
> +    cmd[0] = 1;
> +    cmd[1] = 0x0;
> +    omap_i2c_send(addr, cmd, 2);
> +    omap_i2c_recv(addr, resp, 1);
> +    g_assert_cmphex(resp[0], ==, cmd[1]);
> +
> +    cmd[0] = 2;
> +    cmd[1] = 0x12;
> +    cmd[2] = 0x34;
> +    omap_i2c_send(addr, cmd, 3);
> +    omap_i2c_recv(addr, resp, 2);
> +    g_assert_cmphex(resp[0], ==, cmd[1]);
> +    g_assert_cmphex(resp[1], ==, cmd[2]);
> +
> +    cmd[0] = 3;
> +    cmd[1] = 0x42;
> +    cmd[2] = 0x31;
> +    omap_i2c_send(addr, cmd, 3);
> +    omap_i2c_recv(addr, resp, 2);
> +    g_assert_cmphex(resp[0], ==, cmd[1]);
> +    g_assert_cmphex(resp[1], ==, cmd[2]);
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    QTestState *s = NULL;
> +    int ret;
> +
> +    g_test_init(&argc, &argv, NULL);
> +
> +    s = qtest_start("-display none -machine n800");
> +
> +    qtest_add_func("/tmp105/tx-rx", send_and_receive);

A register fuzzing test (like rtc-test.c) would be nice too.

> +
> +    ret = g_test_run();
> +
> +    if (s) {
> +        qtest_quit(s);
> +    }
> +
> +    return ret;
> +}
> --
> 1.7.10.4
>

Re: [Qemu-devel] [PATCH 2/2] tests: Add tmp105 unit test

[Andreas Färber]

Am 12.12.2012 20:43, schrieb Blue Swirl:
> On Wed, Dec 12, 2012 at 6:29 AM, Andreas Färber <andreas.faerber@web.de> wrote:
>> +static void omap_i2c_send(uint8_t addr, const void *buf, uint16_t len)
> 
> With const uint8_t *buf you could avoid a few casts below.

Good idea, thanks. I was working with other data types before, where
void* saved casts.

>> +    qtest_add_func("/tmp105/tx-rx", send_and_receive);
> 
> A register fuzzing test (like rtc-test.c) would be nice too.

I'm aware of your register fuzzing tests. However we do not directly
access registers of the tmp105, so those should go into an
omap_i2c-test.c file instead if someone wants to ever test that
implementation.

Alex H. has pointed to some more real-world test cases which I'd invite
him to add here as follow-ups.

My quest for tonight will be to generalize the I2C API for libqos by
comparing the tegra_i2c implementation. I hope my omap_i2c_* API is
pretty close already if we add one parameter for an adapter struct.

Andreas