[Qemu-devel] [PATCH 0/3] Add PIIX4 SMBus to qtest and related changes

[Qemu-devel] [PATCH 0/3] Add PIIX4 SMBus to qtest and related changes

[Marc Marí]

Add PIIX4 SMBus to testing libraries, add a test case for those libraries, and
solve problems in SMBus and PIIX4 codes.

Marc Marí (2):
  x86 piix4: Correct SMBus base address
  qtest: Add PIIX4 SMBus support for libqos

Paolo Bonzini (1):
  smbus: fix writes

 hw/acpi/piix4.c            |    2 +-
 hw/i2c/smbus.c             |    7 +-
 tests/Makefile             |    5 +
 tests/eeprom-test.c        |   58 ++++++++++++
 tests/libqos/i2c.h         |    5 +-
 tests/libqos/smbus-piix4.c |  220 ++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 293 insertions(+), 4 deletions(-)
 create mode 100644 tests/eeprom-test.c
 create mode 100644 tests/libqos/smbus-piix4.c

-- 
1.7.10.4

[Qemu-devel] [PATCH 1/3] smbus: fix writes

[Marc Marí]

From: Paolo Bonzini <pbonzini@redhat.com>

SMBus protocol sends offset and length before the actual data that
is transferred.  So we need to skip two bytes rather than one.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/i2c/smbus.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/hw/i2c/smbus.c b/hw/i2c/smbus.c
index 6e27ae8..173a533 100644
--- a/hw/i2c/smbus.c
+++ b/hw/i2c/smbus.c
@@ -59,9 +59,12 @@ static void smbus_do_write(SMBusDevice *dev)
     } else {
         dev->command = dev->data_buf[0];
         DPRINTF("Command %d len %d\n", dev->command, dev->data_len - 1);
+        if (dev->data_buf[1] > dev->data_len - 2) {
+            fprintf(stderr, "SMBus data transfer overrun!\n");
+        }
         if (sc->write_data) {
-            sc->write_data(dev, dev->command, dev->data_buf + 1,
-                           dev->data_len - 1);
+            sc->write_data(dev, dev->command, dev->data_buf + 2,
+                           MIN(dev->data_buf[1], dev->data_len - 2));
         }
     }
 }
-- 
1.7.10.4

[Qemu-devel] [PATCH 2/3] x86 piix4: Correct SMBus base address

[Marc Marí]

As defined in the PIIX4 datasheet in page 135, the base address is set
on bits [15:4]. The mask was changed to match those bits.

Datasheet: http://www.intel.com/assets/pdf/datasheet/290562.pdf

Found by Stefan Hajnoczi

Signed-off-by: Marc Marí <marc.mari.barcelo@gmail.com>
---
 hw/acpi/piix4.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 67dc075..5f263da 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -133,7 +133,7 @@ static void smbus_io_space_update(PIIX4PMState *s)
     PCIDevice *d = PCI_DEVICE(s);
 
     s->smb_io_base = le32_to_cpu(*(uint32_t *)(d->config + 0x90));
-    s->smb_io_base &= 0xffc0;
+    s->smb_io_base &= 0xfff0;
 
     memory_region_transaction_begin();
     memory_region_set_enabled(&s->smb.io, d->config[0xd2] & 1);
-- 
1.7.10.4

[Qemu-devel] [PATCH 3/3] qtest: Add PIIX4 SMBus support for libqos

[Marc Marí]

Add support for the SMBus protocol on the PIIX4 chipset. Add a test for
EEPROMs using this interface.

Signed-off-by: Marc Marí <marc.mari.barcelo@gmail.com>
---
 tests/Makefile             |    5 +
 tests/eeprom-test.c        |   58 ++++++++++++
 tests/libqos/i2c.h         |    5 +-
 tests/libqos/smbus-piix4.c |  220 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 287 insertions(+), 1 deletion(-)
 create mode 100644 tests/eeprom-test.c
 create mode 100644 tests/libqos/smbus-piix4.c

diff --git a/tests/Makefile b/tests/Makefile
index 361bb7b..e49c779 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -152,6 +152,8 @@ gcov-files-i386-y += hw/pci-bridge/i82801b11.c
 check-qtest-i386-y += tests/ioh3420-test$(EXESUF)
 gcov-files-i386-y += hw/pci-bridge/ioh3420.c
 check-qtest-i386-y += tests/usb-hcd-ehci-test$(EXESUF)
+gcov-files-i386-y += hw/i2c/smbus_eeprom.c
+check-qtest-i386-y += tests/eeprom-test$(EXESUF)
 gcov-files-i386-y += hw/usb/hcd-ehci.c
 gcov-files-i386-y += hw/usb/hcd-uhci.c
 gcov-files-i386-y += hw/usb/dev-hid.c
@@ -281,6 +283,7 @@ libqos-obj-y += tests/libqos/i2c.o
 libqos-pc-obj-y = $(libqos-obj-y) tests/libqos/pci-pc.o
 libqos-pc-obj-y += tests/libqos/malloc-pc.o
 libqos-omap-obj-y = $(libqos-obj-y) tests/libqos/i2c-omap.o
+libqos-piix4-obj-y = $(libqos-obj-y) tests/libqos/smbus-piix4.o
 
 tests/rtc-test$(EXESUF): tests/rtc-test.o
 tests/m48t59-test$(EXESUF): tests/m48t59-test.o
@@ -322,9 +325,11 @@ tests/es1370-test$(EXESUF): tests/es1370-test.o
 tests/intel-hda-test$(EXESUF): tests/intel-hda-test.o
 tests/ioh3420-test$(EXESUF): tests/ioh3420-test.o
 tests/usb-hcd-ehci-test$(EXESUF): tests/usb-hcd-ehci-test.o $(libqos-pc-obj-y)
+tests/eeprom-test$(EXESUF): tests/eeprom-test.o $(libqos-piix4-obj-y)
 tests/qemu-iotests/socket_scm_helper$(EXESUF): tests/qemu-iotests/socket_scm_helper.o
 tests/test-qemu-opts$(EXESUF): tests/test-qemu-opts.o libqemuutil.a libqemustub.a
 
+
 # QTest rules
 
 TARGETS=$(patsubst %-softmmu,%, $(filter %-softmmu,$(TARGET_DIRS)))
diff --git a/tests/eeprom-test.c b/tests/eeprom-test.c
new file mode 100644
index 0000000..6f01dd2
--- /dev/null
+++ b/tests/eeprom-test.c
@@ -0,0 +1,58 @@
+/*
+ * QTest testcase for the SMBus EEPROM device
+ *
+ * Copyright (c) 2014 Marc Marí
+ *
+ * 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 <glib.h>
+#include <stdio.h>
+
+#include "libqtest.h"
+#include "libqos/i2c.h"
+
+#define PIIX4_SMBUS_BASE 0x0000b100
+
+#define EEPROM_TEST_ID   "eeprom-test"
+#define EEPROM_TEST_ADDR 0x50
+
+static I2CAdapter *i2c;
+
+static void send_and_receive(void)
+{
+    uint8_t i;
+    uint8_t buf = 0;
+    uint64_t big_buf = 0;
+
+    for (i = EEPROM_TEST_ADDR; i < EEPROM_TEST_ADDR+8; ++i) {
+        i2c_recv(i2c, i, &buf, 1);
+        g_assert_cmphex(buf, ==, 0);
+
+        i2c_recv(i2c, i, (uint8_t *)&big_buf, 4);
+        g_assert_cmphex(big_buf, ==, 0);
+    }
+}
+
+int main(int argc, char **argv)
+{
+    QTestState *s = NULL;
+    int ret;
+
+    g_test_init(&argc, &argv, NULL);
+
+    s = qtest_start(NULL);
+    i2c = piix4_smbus_create(PIIX4_SMBUS_BASE);
+
+    qtest_add_func("/eeprom/tx-rx", send_and_receive);
+
+    ret = g_test_run();
+
+    if (s) {
+        qtest_quit(s);
+    }
+    g_free(i2c);
+
+    return ret;
+}
diff --git a/tests/libqos/i2c.h b/tests/libqos/i2c.h
index 1ce9af4..9fd3a1c 100644
--- a/tests/libqos/i2c.h
+++ b/tests/libqos/i2c.h
@@ -24,7 +24,10 @@ void i2c_send(I2CAdapter *i2c, uint8_t addr,
 void i2c_recv(I2CAdapter *i2c, uint8_t addr,
               uint8_t *buf, uint16_t len);
 
-/* libi2c-omap.c */
+/* i2c-omap.c */
 I2CAdapter *omap_i2c_create(uint64_t addr);
 
+/* smbus-piix4.c */
+I2CAdapter *piix4_smbus_create(uint64_t addr);
+
 #endif
diff --git a/tests/libqos/smbus-piix4.c b/tests/libqos/smbus-piix4.c
new file mode 100644
index 0000000..2e04968
--- /dev/null
+++ b/tests/libqos/smbus-piix4.c
@@ -0,0 +1,220 @@
+/*
+ * QTest I2C driver
+ *
+ * Copyright (c) 2014 Marc Marí
+ *
+ * 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 "libqos/i2c.h"
+
+#include <glib.h>
+
+#include "libqtest.h"
+
+#define SMBUS_TIMEOUT 100000
+
+enum PIIX4SMBUSRegisters {
+    PIIX4_SMBUS_BA = 0x90, /* Base address (in [15:4]). 32 bits */
+    PIIX4_SMBUS_STAT = 0x00, /* Status. 8 bits */
+    PIIX4_SMBUS_CNT = 0x02, /* Control. 8 bits */
+    PIIX4_SMBUS_CMD = 0x03, /* Command. 8 bits */
+    PIIX4_SMBUS_ADD = 0x04, /* Address. 8 bits */
+    PIIX4_SMBUS_DAT0 = 0x05, /* Data 0. 8 bits */
+    PIIX4_SMBUS_DAT1 = 0x06, /* Data 1. 8 bits */
+    PIIX4_SMBUS_BLKDAT = 0x07, /* Block data. 8 bits */
+};
+
+enum PIIX4SMBUSSTATBits {
+    PIIX4_SMBUS_CNT_INTEREN = 1 << 0, /* Enable interrupts */
+    PIIX4_SMBUS_CNT_KILL = 1 << 1, /* Stop the current transaction */
+    PIIX4_SMBUS_CNT_PROT = 7 << 2, /* Type of command */
+    PIIX4_SMBUS_CNT_START = 1 << 6, /* Start execution */
+    PIIX4_SMBUS_STAT_BUSY = 1 << 0, /* Host busy */
+};
+
+enum PIIX4SMBUSCommands {
+    PIIX4_SMBUS_QRW = 0x00, /* Quick read or write */
+    PIIX4_SMBUS_BRW = 0x04, /* Byte read or write */
+    PIIX4_SMBUS_BDRW = 0x08, /* Byte data read or write */
+    PIIX4_SMBUS_WDRW = 0x0C, /* Word data read or write */
+    PIIX4_SMBUS_BLRW = 0x14, /* Block read or write */
+    PIIX4_SMBUS_WR = 0x0, /* Write */
+    PIIX4_SMBUS_RD = 0x1, /* Read */
+};
+
+typedef struct {
+    I2CAdapter parent;
+    uint64_t addr;
+} PIIX4I2C;
+
+static int piix4_smbus_wait_ready(uint64_t addr)
+{
+    uint64_t loops;
+    uint8_t status;
+    loops = SMBUS_TIMEOUT;
+    do {
+        clock_step(10);
+        status = inb(addr + PIIX4_SMBUS_STAT);
+        if ((status & 0x1) == 0) {
+            break; /* It has ended */
+        }
+    } while (--loops);
+
+    if (loops != 0) {
+        return 0;
+    } else {
+        return -1;
+    }
+}
+
+static int piix4_smbus_wait_done(uint64_t addr)
+{
+    uint64_t loops;
+    uint8_t status;
+    loops = SMBUS_TIMEOUT;
+    do {
+        clock_step(10);
+        status = inb(addr + PIIX4_SMBUS_STAT);
+        if ((status & 0x1) != 0) {
+            continue; /* It has not started yet */
+        }
+        if (status & 0xfe) {
+            break; /* One of the interrupt flags is set */
+        }
+    } while (--loops);
+
+    if (loops != 0) {
+        return 0;
+    } else {
+        return -1;
+    }
+}
+
+static void piix4_smbus_recv(I2CAdapter *i2c, uint8_t addr,
+                          uint8_t *buf, uint16_t len)
+{
+    uint8_t stat;
+    uint8_t size;
+    PIIX4I2C *s = (PIIX4I2C *)i2c;
+
+    /* Wait to be ready */
+    g_assert_cmpint(piix4_smbus_wait_ready(s->addr), == , 0);
+
+    /* Clear interrupts */
+    outb(s->addr + PIIX4_SMBUS_STAT, 0x1e);
+
+    /* Write device */
+    outb(s->addr + PIIX4_SMBUS_ADD, (addr << 1) | PIIX4_SMBUS_RD);
+
+    /* Clear data */
+    outb(s->addr + PIIX4_SMBUS_DAT0, 0);
+
+    /* Start */
+    if (len == 1) {
+        outb(s->addr + PIIX4_SMBUS_CNT,
+                PIIX4_SMBUS_BRW | PIIX4_SMBUS_CNT_START);
+    } else {
+        outb(s->addr + PIIX4_SMBUS_CNT,
+                PIIX4_SMBUS_BLRW | PIIX4_SMBUS_CNT_START);
+    }
+
+    /* Wait to be done */
+    piix4_smbus_wait_done(s->addr);
+
+    /* Wait end */
+    stat = inb(s->addr + PIIX4_SMBUS_STAT);
+    g_assert_cmphex((stat & 0x3e), ==, 2); /* Only interrupt enabled
+
+    /* Read */
+    if (len == 1) {
+        buf[0] = inb(s->addr + PIIX4_SMBUS_DAT0);
+    } else {
+        while (len > 0) {
+            size = inb(s->addr + PIIX4_SMBUS_DAT0);
+            if (size == 0) {
+                break;
+            }
+            g_assert_cmpuint((len-size), <, 0);
+            while (size > 0) {
+                buf[0] = readb(s->addr + PIIX4_SMBUS_BLKDAT);
+                ++buf;
+                --size;
+            }
+
+            len -= size;
+        }
+    }
+}
+
+static void piix4_smbus_send(I2CAdapter *i2c, uint8_t addr,
+                          const uint8_t *buf, uint16_t len)
+{
+    uint8_t stat;
+    uint8_t size;
+    PIIX4I2C *s = (PIIX4I2C *)i2c;
+
+    /* Wait to be ready */
+    g_assert_cmpint(piix4_smbus_wait_ready(s->addr), ==, 0);
+
+    /* Clear interrupts */
+    outb(s->addr + PIIX4_SMBUS_STAT, 0x1e);
+
+    /* Write device */
+    outb(s->addr + PIIX4_SMBUS_ADD, (addr << 1) | PIIX4_SMBUS_WR);
+
+    if (len == 1) {
+        /* Write */
+        outb(s->addr + PIIX4_SMBUS_DAT0, buf[0]);
+
+        stat = inb(s->addr + PIIX4_SMBUS_DAT0);
+        g_assert_cmphex(buf[0], ==, stat);
+
+        /* Start */
+        outb(s->addr + PIIX4_SMBUS_CNT,
+                PIIX4_SMBUS_BRW | PIIX4_SMBUS_CNT_START);
+
+        /* Wait to be done */
+        piix4_smbus_wait_done(s->addr);
+
+        /* Wait end */
+        stat = inb(s->addr + PIIX4_SMBUS_STAT);
+        g_assert_cmphex((stat & 0x3e), ==, 2); /* Only interrupt enabled
+    } else {
+        /* Write 32 bytes */
+        while (len > 0) {
+            size = 0;
+            while (len > 0 && size < 32) {
+                outb(s->addr + PIIX4_SMBUS_BLKDAT, buf[0]);
+                ++buf;
+                ++size;
+                --len;
+            }
+            outb(s->addr + PIIX4_SMBUS_DAT0, size);
+
+            /* Start */
+            outb(s->addr + PIIX4_SMBUS_CNT,
+                    PIIX4_SMBUS_BLRW | PIIX4_SMBUS_CNT_START);
+
+            /* Wait to be done */
+            piix4_smbus_wait_done(s->addr);
+
+            /* Wait end */
+            stat = inb(s->addr + PIIX4_SMBUS_STAT);
+            g_assert_cmphex((stat & 0x3e), ==, 2);
+        }
+    }
+}
+
+I2CAdapter *piix4_smbus_create(uint64_t addr)
+{
+    PIIX4I2C *s = g_malloc0(sizeof(*s));
+    I2CAdapter *i2c = (I2CAdapter *)s;
+
+    s->addr = addr;
+
+    i2c->send = piix4_smbus_send;
+    i2c->recv = piix4_smbus_recv;
+
+    return i2c;
+}
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH 3/3] qtest: Add PIIX4 SMBus support for libqos

[Paolo Bonzini]

Il 09/06/2014 10:55, Marc Marí ha scritto:
>
> +static void send_and_receive(void)
> +{
> +    uint8_t i;
> +    uint8_t buf = 0;
> +    uint64_t big_buf = 0;

Probably uint32_t since you only read 4 bytes.

> +    for (i = EEPROM_TEST_ADDR; i < EEPROM_TEST_ADDR+8; ++i) {
> +        i2c_recv(i2c, i, &buf, 1);
> +        g_assert_cmphex(buf, ==, 0);
> +
> +        i2c_recv(i2c, i, (uint8_t *)&big_buf, 4);
> +        g_assert_cmphex(big_buf, ==, 0);
> +    }

Here you should be sending something before.  The protocol is that you 
send the address, then send the offset, then receive bytes.

In fact, right now you're sending a zero here:

+    /* Clear data */
+    outb(s->addr + PIIX4_SMBUS_DAT0, 0);

in libqos.

The problem is that the piix4 smbus interface includes more "magic" than 
the i2c interface that is currently part of libqos.  Basically libqos 
would have to implement the same state machine as hw/i2c/smbus.c in 
order to convert i2c commands (from the test case) to smbus commands 
(for the device).  The opposite also makes sense if you want to have an 
smbus testcase API and you want to make it talk to "basic" i2c devices.

So this testcase by itself is not really meaningful.  This is not your 
fault---I and Stefan aren't 100% comfortable with I2C either. :)  Let's 
discuss it today on the chat.

> +        while (len > 0) {
> +            size = inb(s->addr + PIIX4_SMBUS_DAT0);
> +            if (size == 0) {
> +                break;
> +            }
> +            g_assert_cmpuint((len-size), <, 0);

Asserting len < size does not make much sense.  Should you be asserting 
instead that size <= len here?  Or even len == size (without the outer 
"while (len > 0)" loop)?

> +            while (size > 0) {
> +                buf[0] = readb(s->addr + PIIX4_SMBUS_BLKDAT);
> +                ++buf;
> +                --size;
> +            }
> +
> +            len -= size;
> +        }

size is zero here; the decrement should be before the inner while loop.

Paolo