[Qemu-devel] [PATCH v2 0/8] Make the pm_smbus code more correct

[Qemu-devel] [PATCH v2 0/8] Make the pm_smbus code more correct

[minyard]

These changes make the pm_smbus device work more like the real thing,
adding block transfers, I2C block transfers, and interrupts.

It also adds vmstate transfer for the smbus device in patch 5.
That patch could probably be left out if that is not desirable.

Patch 7 works around a change made to work around an AMIBIOS bug and
is necessary for interrupts to work.

Patches 9 and 10 add vmstate transfer for SMBus EEPROMs, and obviously
requires patch 5 to work properly.  These could also be left out.

[Qemu-devel] [PATCH v2 01/10] i2c:pm_smbus: Clean up some style issues

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Fix some spacing issues, remove extraneous comments, add some
defines instead of hard-coding numbers.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/i2c/pm_smbus.c | 58 ++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 38 insertions(+), 20 deletions(-)

diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index 0d26e0f..83c2377 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -22,8 +22,6 @@
 #include "hw/i2c/pm_smbus.h"
 #include "hw/i2c/smbus.h"
 
-/* no save/load? */
-
 #define SMBHSTSTS       0x00
 #define SMBHSTCNT       0x02
 #define SMBHSTCMD       0x03
@@ -32,19 +30,34 @@
 #define SMBHSTDAT1      0x06
 #define SMBBLKDAT       0x07
 
-#define STS_HOST_BUSY   (1)
-#define STS_INTR        (1<<1)
-#define STS_DEV_ERR     (1<<2)
-#define STS_BUS_ERR     (1<<3)
-#define STS_FAILED      (1<<4)
-#define STS_SMBALERT    (1<<5)
-#define STS_INUSE_STS   (1<<6)
-#define STS_BYTE_DONE   (1<<7)
+#define STS_HOST_BUSY   (1 << 0)
+#define STS_INTR        (1 << 1)
+#define STS_DEV_ERR     (1 << 2)
+#define STS_BUS_ERR     (1 << 3)
+#define STS_FAILED      (1 << 4)
+#define STS_SMBALERT    (1 << 5)
+#define STS_INUSE_STS   (1 << 6)
+#define STS_BYTE_DONE   (1 << 7)
 /* Signs of successfully transaction end :
 *  ByteDoneStatus = 1 (STS_BYTE_DONE) and INTR = 1 (STS_INTR )
 */
 
-//#define DEBUG
+#define CTL_INTREN      (1 << 0)
+#define CTL_KILL        (1 << 1)
+#define CTL_LAST_BYTE   (1 << 5)
+#define CTL_START       (1 << 6)
+#define CTL_PEC_EN      (1 << 7)
+#define CTL_RETURN_MASK 0x1f
+
+#define PROT_QUICK          0
+#define PROT_BYTE           1
+#define PROT_BYTE_DATA      2
+#define PROT_WORD_DATA      3
+#define PROT_PROC_CALL      4
+#define PROT_BLOCK_DATA     5
+#define PROT_I2C_BLOCK_DATA 6
+
+/*#define DEBUG*/
 
 #ifdef DEBUG
 # define SMBUS_DPRINTF(format, ...)     printf(format, ## __VA_ARGS__)
@@ -70,11 +83,12 @@ static void smb_transaction(PMSMBus *s)
     if ((s->smb_stat & STS_DEV_ERR) != 0)  {
         goto error;
     }
+
     switch(prot) {
-    case 0x0:
+    case PROT_QUICK:
         ret = smbus_quick_command(bus, addr, read);
         goto done;
-    case 0x1:
+    case PROT_BYTE:
         if (read) {
             ret = smbus_receive_byte(bus, addr);
             goto data8;
@@ -82,7 +96,7 @@ static void smb_transaction(PMSMBus *s)
             ret = smbus_send_byte(bus, addr, cmd);
             goto done;
         }
-    case 0x2:
+    case PROT_BYTE_DATA:
         if (read) {
             ret = smbus_read_byte(bus, addr, cmd);
             goto data8;
@@ -91,16 +105,17 @@ static void smb_transaction(PMSMBus *s)
             goto done;
         }
         break;
-    case 0x3:
+    case PROT_WORD_DATA:
         if (read) {
             ret = smbus_read_word(bus, addr, cmd);
             goto data16;
         } else {
-            ret = smbus_write_word(bus, addr, cmd, (s->smb_data1 << 8) | s->smb_data0);
+            ret = smbus_write_word(bus, addr, cmd,
+                                   (s->smb_data1 << 8) | s->smb_data0);
             goto done;
         }
         break;
-    case 0x5:
+    case PROT_I2C_BLOCK_DATA:
         if (read) {
             ret = smbus_read_block(bus, addr, cmd, s->smb_data);
             goto data8;
@@ -158,8 +173,9 @@ static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
         break;
     case SMBHSTCNT:
         s->smb_ctl = val;
-        if (val & 0x40)
+        if (s->smb_ctl & CTL_START) {
             smb_transaction_start(s);
+        }
         break;
     case SMBHSTCMD:
         s->smb_cmd = val;
@@ -198,7 +214,7 @@ static uint64_t smb_ioport_readb(void *opaque, hwaddr addr, unsigned width)
         break;
     case SMBHSTCNT:
         s->smb_index = 0;
-        val = s->smb_ctl & 0x1f;
+        val = s->smb_ctl & CTL_RETURN_MASK;
         break;
     case SMBHSTCMD:
         val = s->smb_cmd;
@@ -221,7 +237,9 @@ static uint64_t smb_ioport_readb(void *opaque, hwaddr addr, unsigned width)
         val = 0;
         break;
     }
-    SMBUS_DPRINTF("SMB readb port=0x%04" HWADDR_PRIx " val=0x%02x\n", addr, val);
+    SMBUS_DPRINTF("SMB readb port=0x%04" HWADDR_PRIx " val=0x%02x\n",
+                  addr, val);
+
     return val;
 }
 
-- 
2.7.4

[Qemu-devel] [PATCH v2 02/10] i2c:pm_smbus: Fix the semantics of block I2C transfers

[minyard]

From: Corey Minyard <cminyard@mvista.com>

The I2C block transfer commands was not implemented correctly, it
read a length byte and such like it was an smbus transfer.

So fix the smbus_read_block() and smbus_write_block() functions
so they can properly handle I2C transfers, and normal SMBus
transfers (for upcoming changes).  Pass in a transfer size and
a bool to know whether to use the size byte (like SMBus) or use
the length given (like I2C).

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/i2c/pm_smbus.c      | 10 ++++++++--
 hw/i2c/smbus.c         | 37 ++++++++++++++++++++++++-------------
 include/hw/i2c/smbus.h | 17 +++++++++++++++--
 3 files changed, 47 insertions(+), 17 deletions(-)

diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index 83c2377..f1fe889 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -117,10 +117,16 @@ static void smb_transaction(PMSMBus *s)
         break;
     case PROT_I2C_BLOCK_DATA:
         if (read) {
-            ret = smbus_read_block(bus, addr, cmd, s->smb_data);
+            int xfersize = s->smb_data0;
+            if (xfersize > sizeof(s->smb_data)) {
+                xfersize = sizeof(s->smb_data);
+            }
+            ret = smbus_read_block(bus, addr, s->smb_data1, s->smb_data,
+                                   xfersize, false, true);
             goto data8;
         } else {
-            ret = smbus_write_block(bus, addr, cmd, s->smb_data, s->smb_data0);
+            ret = smbus_write_block(bus, addr, cmd, s->smb_data, s->smb_data0,
+                                    false);
             goto done;
         }
         break;
diff --git a/hw/i2c/smbus.c b/hw/i2c/smbus.c
index 587ce1a..6ff77c5 100644
--- a/hw/i2c/smbus.c
+++ b/hw/i2c/smbus.c
@@ -293,33 +293,42 @@ int smbus_write_word(I2CBus *bus, uint8_t addr, uint8_t command, uint16_t data)
     return 0;
 }
 
-int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data)
+int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
+                     int len, bool recv_len, bool send_cmd)
 {
-    int len;
+    int rlen;
     int i;
 
-    if (i2c_start_transfer(bus, addr, 0)) {
-        return -1;
+    if (send_cmd) {
+        if (i2c_start_transfer(bus, addr, 0)) {
+            return -1;
+        }
+        i2c_send(bus, command);
     }
-    i2c_send(bus, command);
     if (i2c_start_transfer(bus, addr, 1)) {
-        i2c_end_transfer(bus);
+        if (send_cmd) {
+            i2c_end_transfer(bus);
+        }
         return -1;
     }
-    len = i2c_recv(bus);
-    if (len > 32) {
-        len = 0;
+    if (recv_len) {
+        rlen = i2c_recv(bus);
+    } else {
+        rlen = len;
     }
-    for (i = 0; i < len; i++) {
+    if (rlen > len) {
+        rlen = 0;
+    }
+    for (i = 0; i < rlen; i++) {
         data[i] = i2c_recv(bus);
     }
     i2c_nack(bus);
     i2c_end_transfer(bus);
-    return len;
+    return rlen;
 }
 
 int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
-                      int len)
+                      int len, bool send_len)
 {
     int i;
 
@@ -330,7 +339,9 @@ int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
         return -1;
     }
     i2c_send(bus, command);
-    i2c_send(bus, len);
+    if (send_len) {
+        i2c_send(bus, len);
+    }
     for (i = 0; i < len; i++) {
         i2c_send(bus, data[i]);
     }
diff --git a/include/hw/i2c/smbus.h b/include/hw/i2c/smbus.h
index 4fdba02..d8b1b9e 100644
--- a/include/hw/i2c/smbus.h
+++ b/include/hw/i2c/smbus.h
@@ -72,9 +72,22 @@ int smbus_read_byte(I2CBus *bus, uint8_t addr, uint8_t command);
 int smbus_write_byte(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t data);
 int smbus_read_word(I2CBus *bus, uint8_t addr, uint8_t command);
 int smbus_write_word(I2CBus *bus, uint8_t addr, uint8_t command, uint16_t data);
-int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data);
+
+/*
+ * Do a block transfer from an I2C device.  If recv_len is set, then the
+ * first received byte is a length field and is used to know how much data
+ * to receive.  Otherwise receive "len" bytes.  If send_cmd is set, send
+ * the command byte first before receiving the data.
+ */
+int smbus_read_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
+                     int len, bool recv_len, bool send_cmd);
+
+/*
+ * Do a block transfer to an I2C device.  If send_len is set, send the
+ * "len" value before the data.
+ */
 int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
-                      int len);
+                      int len, bool send_len);
 
 void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf);
 void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom,
-- 
2.7.4

[Qemu-devel] [PATCH v2 03/10] i2c:pm_smbus: Make the I2C block read command read-only

[minyard]

From: Corey Minyard <cminyard@mvista.com>

It did have write capability, but the manual says the behavior
with write enabled is undefined.  So just set an error in this
case.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/i2c/pm_smbus.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index f1fe889..dc61f2c 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -55,7 +55,7 @@
 #define PROT_WORD_DATA      3
 #define PROT_PROC_CALL      4
 #define PROT_BLOCK_DATA     5
-#define PROT_I2C_BLOCK_DATA 6
+#define PROT_I2C_BLOCK_READ 6
 
 /*#define DEBUG*/
 
@@ -115,7 +115,7 @@ static void smb_transaction(PMSMBus *s)
             goto done;
         }
         break;
-    case PROT_I2C_BLOCK_DATA:
+    case PROT_I2C_BLOCK_READ:
         if (read) {
             int xfersize = s->smb_data0;
             if (xfersize > sizeof(s->smb_data)) {
@@ -125,9 +125,8 @@ static void smb_transaction(PMSMBus *s)
                                    xfersize, false, true);
             goto data8;
         } else {
-            ret = smbus_write_block(bus, addr, cmd, s->smb_data, s->smb_data0,
-                                    false);
-            goto done;
+            /* The manual says the behavior is undefined, just set DEV_ERR. */
+            goto error;
         }
         break;
     default:
-- 
2.7.4

[Qemu-devel] [PATCH v2 04/10] i2c:pm_smbus: Add block transfer capability

[minyard]

From: Corey Minyard <cminyard@mvista.com>

There was no block transfer code in pm_smbus.c, and it is needed
for some devices.  So add it.

This adds both byte-by-byte block transfers and buffered block
transfers.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/i2c/pm_smbus.c         | 151 ++++++++++++++++++++++++++++++++++++++++++----
 hw/i2c/smbus_ich9.c       |   8 ++-
 include/hw/i2c/pm_smbus.h |  20 +++++-
 3 files changed, 164 insertions(+), 15 deletions(-)

diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index dc61f2c..32132be 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -29,6 +29,7 @@
 #define SMBHSTDAT0      0x05
 #define SMBHSTDAT1      0x06
 #define SMBBLKDAT       0x07
+#define SMBAUXCTL       0x0d
 
 #define STS_HOST_BUSY   (1 << 0)
 #define STS_INTR        (1 << 1)
@@ -57,6 +58,10 @@
 #define PROT_BLOCK_DATA     5
 #define PROT_I2C_BLOCK_READ 6
 
+#define AUX_PEC       (1 << 0)
+#define AUX_BLK       (1 << 1)
+#define AUX_MASK      0x3
+
 /*#define DEBUG*/
 
 #ifdef DEBUG
@@ -129,6 +134,51 @@ static void smb_transaction(PMSMBus *s)
             goto error;
         }
         break;
+    case PROT_BLOCK_DATA:
+        if (read) {
+            ret = smbus_read_block(bus, addr, cmd, s->smb_data,
+                                   sizeof(s->smb_data), !s->i2c_enable,
+                                   !s->i2c_enable);
+            if (ret < 0) {
+                goto error;
+            }
+            s->smb_index = 0;
+            s->op_done = false;
+            if (s->smb_auxctl & AUX_BLK) {
+                s->smb_stat |= STS_INTR;
+            } else {
+                s->smb_blkdata = s->smb_data[0];
+                s->smb_stat |= STS_HOST_BUSY | STS_BYTE_DONE;
+            }
+            s->smb_data0 = ret;
+            goto out;
+        } else {
+            if (s->smb_auxctl & AUX_BLK) {
+                if (s->smb_index != s->smb_data0) {
+                    s->smb_index = 0;
+                    goto error;
+                }
+                /* Data is already all written to the queue, just do
+                   the operation. */
+                s->smb_index = 0;
+                ret = smbus_write_block(bus, addr, cmd, s->smb_data,
+                                        s->smb_data0, !s->i2c_enable);
+                if (ret < 0) {
+                    goto error;
+                }
+                s->op_done = true;
+                s->smb_stat |= STS_INTR;
+                s->smb_stat &= ~STS_HOST_BUSY;
+            } else {
+                s->op_done = false;
+                s->smb_stat |= STS_HOST_BUSY | STS_BYTE_DONE;
+                s->smb_data[0] = s->smb_blkdata;
+                s->smb_index = 0;
+                ret = 0;
+            }
+            goto out;
+        }
+        break;
     default:
         goto error;
     }
@@ -148,13 +198,13 @@ done:
     if (ret < 0) {
         goto error;
     }
-    s->smb_stat |= STS_BYTE_DONE | STS_INTR;
+    s->smb_stat |= STS_INTR;
+out:
     return;
 
 error:
     s->smb_stat |= STS_DEV_ERR;
     return;
-
 }
 
 static void smb_transaction_start(PMSMBus *s)
@@ -173,14 +223,61 @@ static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
                   " val=0x%02" PRIx64 "\n", addr, val);
     switch(addr) {
     case SMBHSTSTS:
-        s->smb_stat = (~(val & 0xff)) & s->smb_stat;
-        s->smb_index = 0;
+        s->smb_stat &= ~(val & ~STS_HOST_BUSY);
+        if (!s->op_done && !(s->smb_auxctl & AUX_BLK)) {
+            uint8_t read = s->smb_addr & 0x01;
+
+            s->smb_index++;
+            if (!read && s->smb_index == s->smb_data0) {
+                uint8_t prot = (s->smb_ctl >> 2) & 0x07;
+                uint8_t cmd = s->smb_cmd;
+                uint8_t addr = s->smb_addr >> 1;
+                int ret;
+
+                if (prot == PROT_I2C_BLOCK_READ) {
+                    s->smb_stat |= STS_DEV_ERR;
+                    goto out;
+                }
+
+                ret = smbus_write_block(s->smbus, addr, cmd, s->smb_data,
+                                        s->smb_data0, !s->i2c_enable);
+                if (ret < 0) {
+                    s->smb_stat |= STS_DEV_ERR;
+                    goto out;
+                }
+                s->op_done = true;
+                s->smb_stat |= STS_INTR;
+                s->smb_stat &= ~STS_HOST_BUSY;
+            } else if (!read) {
+                s->smb_data[s->smb_index] = s->smb_blkdata;
+                s->smb_stat |= STS_BYTE_DONE;
+            } else if (s->smb_ctl & CTL_LAST_BYTE) {
+                s->op_done = true;
+                s->smb_blkdata = s->smb_data[s->smb_index];
+                s->smb_index = 0;
+                s->smb_stat |= STS_INTR;
+                s->smb_stat &= ~STS_HOST_BUSY;
+            } else {
+                s->smb_blkdata = s->smb_data[s->smb_index];
+                s->smb_stat |= STS_BYTE_DONE;
+            }
+        }
         break;
     case SMBHSTCNT:
-        s->smb_ctl = val;
-        if (s->smb_ctl & CTL_START) {
+        s->smb_ctl = val & ~CTL_START; /* CTL_START always reads 0 */
+        if (val & CTL_START) {
+            if (!s->op_done) {
+                s->smb_index = 0;
+                s->op_done = true;
+            }
             smb_transaction_start(s);
         }
+        if (s->smb_ctl & CTL_KILL) {
+            s->op_done = true;
+            s->smb_index = 0;
+            s->smb_stat |= STS_FAILED;
+            s->smb_stat &= ~STS_HOST_BUSY;
+        }
         break;
     case SMBHSTCMD:
         s->smb_cmd = val;
@@ -195,13 +292,24 @@ static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
         s->smb_data1 = val;
         break;
     case SMBBLKDAT:
-        s->smb_data[s->smb_index++] = val;
-        if (s->smb_index > 31)
+        if (s->smb_index >= PM_SMBUS_MAX_MSG_SIZE) {
             s->smb_index = 0;
+        }
+        if (s->smb_auxctl & AUX_BLK) {
+            s->smb_data[s->smb_index++] = val;
+        } else {
+            s->smb_blkdata = val;
+        }
+        break;
+    case SMBAUXCTL:
+        s->smb_auxctl = val & AUX_MASK;
         break;
     default:
         break;
     }
+
+ out:
+    return;
 }
 
 static uint64_t smb_ioport_readb(void *opaque, hwaddr addr, unsigned width)
@@ -218,7 +326,6 @@ static uint64_t smb_ioport_readb(void *opaque, hwaddr addr, unsigned width)
         }
         break;
     case SMBHSTCNT:
-        s->smb_index = 0;
         val = s->smb_ctl & CTL_RETURN_MASK;
         break;
     case SMBHSTCMD:
@@ -234,9 +341,22 @@ static uint64_t smb_ioport_readb(void *opaque, hwaddr addr, unsigned width)
         val = s->smb_data1;
         break;
     case SMBBLKDAT:
-        val = s->smb_data[s->smb_index++];
-        if (s->smb_index > 31)
+        if (s->smb_index >= PM_SMBUS_MAX_MSG_SIZE) {
             s->smb_index = 0;
+        }
+        if (s->smb_auxctl & AUX_BLK) {
+            val = s->smb_data[s->smb_index++];
+            if (!s->op_done && s->smb_index == s->smb_data0) {
+                s->op_done = true;
+                s->smb_index = 0;
+                s->smb_stat &= ~STS_HOST_BUSY;
+            }
+        } else {
+            val = s->smb_blkdata;
+        }
+        break;
+    case SMBAUXCTL:
+        val = s->smb_auxctl;
         break;
     default:
         val = 0;
@@ -248,6 +368,13 @@ static uint64_t smb_ioport_readb(void *opaque, hwaddr addr, unsigned width)
     return val;
 }
 
+static void pm_smbus_reset(PMSMBus *s)
+{
+    s->op_done = true;
+    s->smb_index = 0;
+    s->smb_stat = 0;
+}
+
 static const MemoryRegionOps pm_smbus_ops = {
     .read = smb_ioport_readb,
     .write = smb_ioport_writeb,
@@ -258,6 +385,8 @@ static const MemoryRegionOps pm_smbus_ops = {
 
 void pm_smbus_init(DeviceState *parent, PMSMBus *smb)
 {
+    smb->op_done = true;
+    smb->reset = pm_smbus_reset;
     smb->smbus = i2c_init_bus(parent, "i2c");
     memory_region_init_io(&smb->io, OBJECT(parent), &pm_smbus_ops, smb,
                           "pm-smbus", 64);
diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
index 007cb67..a66a114 100644
--- a/hw/i2c/smbus_ich9.c
+++ b/hw/i2c/smbus_ich9.c
@@ -61,12 +61,16 @@ static void ich9_smbus_write_config(PCIDevice *d, uint32_t address,
     pci_default_write_config(d, address, val, len);
     if (range_covers_byte(address, len, ICH9_SMB_HOSTC)) {
         uint8_t hostc = s->dev.config[ICH9_SMB_HOSTC];
-        if ((hostc & ICH9_SMB_HOSTC_HST_EN) &&
-            !(hostc & ICH9_SMB_HOSTC_I2C_EN)) {
+        if (hostc & ICH9_SMB_HOSTC_HST_EN) {
             memory_region_set_enabled(&s->smb.io, true);
         } else {
             memory_region_set_enabled(&s->smb.io, false);
         }
+        s->smb.i2c_enable = (hostc & ICH9_SMB_HOSTC_I2C_EN) != 0;
+        if (hostc & ICH9_SMB_HOSTC_SSRESET) {
+            s->smb.reset(&s->smb);
+            s->dev.config[ICH9_SMB_HOSTC] &= ~ICH9_SMB_HOSTC_SSRESET;
+        }
     }
 }
 
diff --git a/include/hw/i2c/pm_smbus.h b/include/hw/i2c/pm_smbus.h
index 2a837af..99d5489 100644
--- a/include/hw/i2c/pm_smbus.h
+++ b/include/hw/i2c/pm_smbus.h
@@ -1,6 +1,8 @@
 #ifndef PM_SMBUS_H
 #define PM_SMBUS_H
 
+#define PM_SMBUS_MAX_MSG_SIZE 32
+
 typedef struct PMSMBus {
     I2CBus *smbus;
     MemoryRegion io;
@@ -11,8 +13,22 @@ typedef struct PMSMBus {
     uint8_t smb_addr;
     uint8_t smb_data0;
     uint8_t smb_data1;
-    uint8_t smb_data[32];
-    uint8_t smb_index;
+    uint8_t smb_data[PM_SMBUS_MAX_MSG_SIZE];
+    uint8_t smb_blkdata;
+    uint8_t smb_auxctl;
+    uint32_t smb_index;
+
+    /* Set by pm_smbus.c */
+    void (*reset)(struct PMSMBus *s);
+
+    /* Set by the user. */
+    bool i2c_enable;
+
+    /* Internally used by pm_smbus. */
+
+    /* Set on block transfers after the last byte has been read, so the
+       INTR bit can be set at the right time. */
+    bool op_done;
 } PMSMBus;
 
 void pm_smbus_init(DeviceState *parent, PMSMBus *smb);
-- 
2.7.4

[Qemu-devel] [PATCH v2 05/10] i2c:pm_smbus: Fix state transfer

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Transfer the state information for the SMBus registers and
internal data so it will work on a VM transfer.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/acpi/piix4.c           |  3 ++-
 hw/i2c/pm_smbus.c         | 20 ++++++++++++++++++++
 hw/i2c/smbus_ich9.c       |  5 +++--
 include/hw/i2c/pm_smbus.h |  2 ++
 4 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index 6404af5..f8d8d2e 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -309,7 +309,7 @@ static const VMStateDescription vmstate_cpuhp_state = {
  */
 static const VMStateDescription vmstate_acpi = {
     .name = "piix4_pm",
-    .version_id = 3,
+    .version_id = 4,
     .minimum_version_id = 3,
     .minimum_version_id_old = 1,
     .load_state_old = acpi_load_old,
@@ -320,6 +320,7 @@ static const VMStateDescription vmstate_acpi = {
         VMSTATE_UINT16(ar.pm1.evt.en, PIIX4PMState),
         VMSTATE_UINT16(ar.pm1.cnt.cnt, PIIX4PMState),
         VMSTATE_STRUCT(apm, PIIX4PMState, 0, vmstate_apm, APMState),
+        VMSTATE_STRUCT(smb, PIIX4PMState, 4, pmsmb_vmstate, PMSMBus),
         VMSTATE_TIMER_PTR(ar.tmr.timer, PIIX4PMState),
         VMSTATE_INT64(ar.tmr.overflow_time, PIIX4PMState),
         VMSTATE_STRUCT(ar.gpe, PIIX4PMState, 2, vmstate_gpe, ACPIGPE),
diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index 32132be..9e11d47 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -383,6 +383,26 @@ static const MemoryRegionOps pm_smbus_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
+const VMStateDescription pmsmb_vmstate = {
+    .name = "pmsmb",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8(smb_stat, PMSMBus),
+        VMSTATE_UINT8(smb_ctl, PMSMBus),
+        VMSTATE_UINT8(smb_cmd, PMSMBus),
+        VMSTATE_UINT8(smb_addr, PMSMBus),
+        VMSTATE_UINT8(smb_data0, PMSMBus),
+        VMSTATE_UINT8(smb_data1, PMSMBus),
+        VMSTATE_UINT32(smb_index, PMSMBus),
+        VMSTATE_UINT8_ARRAY(smb_data, PMSMBus, PM_SMBUS_MAX_MSG_SIZE),
+        VMSTATE_UINT8(smb_auxctl, PMSMBus),
+        VMSTATE_BOOL(i2c_enable, PMSMBus),
+        VMSTATE_BOOL(op_done, PMSMBus),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 void pm_smbus_init(DeviceState *parent, PMSMBus *smb)
 {
     smb->op_done = true;
diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
index a66a114..c8b8413 100644
--- a/hw/i2c/smbus_ich9.c
+++ b/hw/i2c/smbus_ich9.c
@@ -45,10 +45,11 @@ typedef struct ICH9SMBState {
 
 static const VMStateDescription vmstate_ich9_smbus = {
     .name = "ich9_smb",
-    .version_id = 1,
+    .version_id = 2,
     .minimum_version_id = 1,
     .fields = (VMStateField[]) {
-        VMSTATE_PCI_DEVICE(dev, struct ICH9SMBState),
+        VMSTATE_PCI_DEVICE(dev, ICH9SMBState),
+        VMSTATE_STRUCT(smb, ICH9SMBState, 2, pmsmb_vmstate, PMSMBus),
         VMSTATE_END_OF_LIST()
     }
 };
diff --git a/include/hw/i2c/pm_smbus.h b/include/hw/i2c/pm_smbus.h
index 99d5489..b1e1970 100644
--- a/include/hw/i2c/pm_smbus.h
+++ b/include/hw/i2c/pm_smbus.h
@@ -33,4 +33,6 @@ typedef struct PMSMBus {
 
 void pm_smbus_init(DeviceState *parent, PMSMBus *smb);
 
+extern const VMStateDescription pmsmb_vmstate;
+
 #endif /* PM_SMBUS_H */
-- 
2.7.4

[Qemu-devel] [PATCH v2 06/10] i2c:pm_smbus: Add interrupt handling

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Add the necessary code so that interrupts actually work from
the pm_smbus device.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/i2c/pm_smbus.c         | 14 +++++++++++++-
 hw/i2c/smbus_ich9.c       | 19 ++++++++++++++++++-
 include/hw/i2c/pm_smbus.h |  2 ++
 3 files changed, 33 insertions(+), 2 deletions(-)

diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index 9e11d47..664a6b1 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -214,6 +214,12 @@ static void smb_transaction_start(PMSMBus *s)
     s->smb_stat |= STS_HOST_BUSY;
 }
 
+static bool
+smb_irq_value(PMSMBus *s)
+{
+    return ((s->smb_stat & ~STS_HOST_BUSY) != 0) && (s->smb_ctl & CTL_INTREN);
+}
+
 static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
                               unsigned width)
 {
@@ -309,7 +315,9 @@ static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
     }
 
  out:
-    return;
+    if (s->set_irq) {
+        s->set_irq(s, smb_irq_value(s));
+    }
 }
 
 static uint64_t smb_ioport_readb(void *opaque, hwaddr addr, unsigned width)
@@ -365,6 +373,10 @@ static uint64_t smb_ioport_readb(void *opaque, hwaddr addr, unsigned width)
     SMBUS_DPRINTF("SMB readb port=0x%04" HWADDR_PRIx " val=0x%02x\n",
                   addr, val);
 
+    if (s->set_irq) {
+        s->set_irq(s, smb_irq_value(s));
+    }
+
     return val;
 }
 
diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
index c8b8413..316c2a4 100644
--- a/hw/i2c/smbus_ich9.c
+++ b/hw/i2c/smbus_ich9.c
@@ -40,15 +40,18 @@
 typedef struct ICH9SMBState {
     PCIDevice dev;
 
+    bool irq_enabled;
+
     PMSMBus smb;
 } ICH9SMBState;
 
 static const VMStateDescription vmstate_ich9_smbus = {
     .name = "ich9_smb",
-    .version_id = 2,
+    .version_id = 3,
     .minimum_version_id = 1,
     .fields = (VMStateField[]) {
         VMSTATE_PCI_DEVICE(dev, ICH9SMBState),
+        VMSTATE_BOOL_V(irq_enabled, ICH9SMBState, 3),
         VMSTATE_STRUCT(smb, ICH9SMBState, 2, pmsmb_vmstate, PMSMBus),
         VMSTATE_END_OF_LIST()
     }
@@ -110,11 +113,25 @@ static void ich9_smb_class_init(ObjectClass *klass, void *data)
     dc->user_creatable = false;
 }
 
+static void ich9_smb_set_irq(PMSMBus *pmsmb, bool enabled)
+{
+    ICH9SMBState *s = pmsmb->opaque;
+
+    if (enabled == s->irq_enabled) {
+        return;
+    }
+
+    s->irq_enabled = enabled;
+    pci_set_irq(&s->dev, enabled);
+}
+
 I2CBus *ich9_smb_init(PCIBus *bus, int devfn, uint32_t smb_io_base)
 {
     PCIDevice *d =
         pci_create_simple_multifunction(bus, devfn, true, TYPE_ICH9_SMB_DEVICE);
     ICH9SMBState *s = ICH9_SMB_DEVICE(d);
+    s->smb.set_irq = ich9_smb_set_irq;
+    s->smb.opaque = s;
     return s->smb.smbus;
 }
 
diff --git a/include/hw/i2c/pm_smbus.h b/include/hw/i2c/pm_smbus.h
index b1e1970..cfe596f 100644
--- a/include/hw/i2c/pm_smbus.h
+++ b/include/hw/i2c/pm_smbus.h
@@ -23,6 +23,8 @@ typedef struct PMSMBus {
 
     /* Set by the user. */
     bool i2c_enable;
+    void (*set_irq)(struct PMSMBus *s, bool enabled);
+    void *opaque;
 
     /* Internally used by pm_smbus. */
 
-- 
2.7.4

[Qemu-devel] [PATCH v2 07/10] Don't delay host status register busy bit when interrupts are enabled

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Change 880b1ffe6ec2f0ae "smbus: do not immediately complete commands"
changed pm_smbus to delay setting the host busy bit until the status
register was read, to work around a bug in AMIBIOS.  Unfortunately,
when interrupts are enabled, the status register will never get read
and the processing will never happen.

Modify the code to only delay setting the host busy bit if interrupts
are not enabled.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Hervé Poussineau <hpoussin@reactos.org>
Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/i2c/pm_smbus.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index 664a6b1..10ba208 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -80,9 +80,6 @@ static void smb_transaction(PMSMBus *s)
     I2CBus *bus = s->smbus;
     int ret;
 
-    assert(s->smb_stat & STS_HOST_BUSY);
-    s->smb_stat &= ~STS_HOST_BUSY;
-
     SMBUS_DPRINTF("SMBus trans addr=0x%02x prot=0x%02x\n", addr, prot);
     /* Transaction isn't exec if STS_DEV_ERR bit set */
     if ((s->smb_stat & STS_DEV_ERR) != 0)  {
@@ -209,9 +206,18 @@ error:
 
 static void smb_transaction_start(PMSMBus *s)
 {
-    /* Do not execute immediately the command ; it will be
-     * executed when guest will read SMB_STAT register */
-    s->smb_stat |= STS_HOST_BUSY;
+    if (s->smb_ctl & CTL_INTREN) {
+        smb_transaction(s);
+    } else {
+        /* Do not execute immediately the command; it will be
+         * executed when guest will read SMB_STAT register.  This
+         * is to work around a bug in AMIBIOS (that is working
+         * around another bug in some specific hardware) where
+         * it waits for STS_HOST_BUSY to be set before waiting
+         * checking for status.  If STS_HOST_BUSY doesn't get
+         * set, it gets stuck. */
+        s->smb_stat |= STS_HOST_BUSY;
+    }
 }
 
 static bool
@@ -330,6 +336,7 @@ static uint64_t smb_ioport_readb(void *opaque, hwaddr addr, unsigned width)
         val = s->smb_stat;
         if (s->smb_stat & STS_HOST_BUSY) {
             /* execute command now */
+            s->smb_stat &= ~STS_HOST_BUSY;
             smb_transaction(s);
         }
         break;
-- 
2.7.4

[Qemu-devel] [PATCH v2 08/10] i2c:pm_smbus: Add the ability to force block transfer enable

[minyard]

From: Corey Minyard <cminyard@mvista.com>

The PIIX4 hardware has block transfer buffer always enabled in
the hardware, but the i801 does not.  Add a parameter to pm_smbus_init
to force on the block transfer so the PIIX4 handler can enable this
by default, as it was disabled by default before.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/acpi/piix4.c           | 2 +-
 hw/i2c/pm_smbus.c         | 5 ++++-
 hw/i2c/smbus_ich9.c       | 2 +-
 hw/isa/vt82c686.c         | 2 +-
 include/hw/i2c/pm_smbus.h | 2 +-
 5 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
index f8d8d2e..313305f 100644
--- a/hw/acpi/piix4.c
+++ b/hw/acpi/piix4.c
@@ -513,7 +513,7 @@ static void piix4_pm_realize(PCIDevice *dev, Error **errp)
     pci_conf[0x90] = s->smb_io_base | 1;
     pci_conf[0x91] = s->smb_io_base >> 8;
     pci_conf[0xd2] = 0x09;
-    pm_smbus_init(DEVICE(dev), &s->smb);
+    pm_smbus_init(DEVICE(dev), &s->smb, true);
     memory_region_set_enabled(&s->smb.io, pci_conf[0xd2] & 1);
     memory_region_add_subregion(pci_address_space_io(dev),
                                 s->smb_io_base, &s->smb.io);
diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index 10ba208..4d021bb 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -422,11 +422,14 @@ const VMStateDescription pmsmb_vmstate = {
     }
 };
 
-void pm_smbus_init(DeviceState *parent, PMSMBus *smb)
+void pm_smbus_init(DeviceState *parent, PMSMBus *smb, bool force_aux_blk)
 {
     smb->op_done = true;
     smb->reset = pm_smbus_reset;
     smb->smbus = i2c_init_bus(parent, "i2c");
+    if (force_aux_blk) {
+        smb->smb_auxctl |= AUX_BLK;
+    }
     memory_region_init_io(&smb->io, OBJECT(parent), &pm_smbus_ops, smb,
                           "pm-smbus", 64);
 }
diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
index 316c2a4..5860fc9 100644
--- a/hw/i2c/smbus_ich9.c
+++ b/hw/i2c/smbus_ich9.c
@@ -88,7 +88,7 @@ static void ich9_smbus_realize(PCIDevice *d, Error **errp)
     pci_set_byte(d->config + ICH9_SMB_HOSTC, 0);
     /* TODO bar0, bar1: 64bit BAR support*/
 
-    pm_smbus_init(&d->qdev, &s->smb);
+    pm_smbus_init(&d->qdev, &s->smb, false);
     pci_register_bar(d, ICH9_SMB_SMB_BASE_BAR, PCI_BASE_ADDRESS_SPACE_IO,
                      &s->smb.io);
 }
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index cff1946..7302f6d 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -370,7 +370,7 @@ static void vt82c686b_pm_realize(PCIDevice *dev, Error **errp)
     pci_conf[0x90] = s->smb_io_base | 1;
     pci_conf[0x91] = s->smb_io_base >> 8;
     pci_conf[0xd2] = 0x90;
-    pm_smbus_init(&s->dev.qdev, &s->smb);
+    pm_smbus_init(&s->dev.qdev, &s->smb, false);
     memory_region_add_subregion(get_system_io(), s->smb_io_base, &s->smb.io);
 
     apm_init(dev, &s->apm, NULL, s);
diff --git a/include/hw/i2c/pm_smbus.h b/include/hw/i2c/pm_smbus.h
index cfe596f..471345e 100644
--- a/include/hw/i2c/pm_smbus.h
+++ b/include/hw/i2c/pm_smbus.h
@@ -33,7 +33,7 @@ typedef struct PMSMBus {
     bool op_done;
 } PMSMBus;
 
-void pm_smbus_init(DeviceState *parent, PMSMBus *smb);
+void pm_smbus_init(DeviceState *parent, PMSMBus *smb, bool force_aux_blk);
 
 extern const VMStateDescription pmsmb_vmstate;
 
-- 
2.7.4

[Qemu-devel] [PATCH v2 09/10] i2c: Add an SMBus vmstate structure

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Michael S . Tsirkin" <mst@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/i2c/smbus.c         | 14 ++++++++++++++
 include/hw/i2c/smbus.h | 18 +++++++++++++++---
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/hw/i2c/smbus.c b/hw/i2c/smbus.c
index 6ff77c5..b0774d7 100644
--- a/hw/i2c/smbus.c
+++ b/hw/i2c/smbus.c
@@ -349,6 +349,20 @@ int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
     return 0;
 }
 
+const VMStateDescription vmstate_smbus_device = {
+    .name = TYPE_SMBUS_DEVICE,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields      = (VMStateField[]) {
+        VMSTATE_I2C_SLAVE(i2c, SMBusDevice),
+        VMSTATE_INT32(mode, SMBusDevice),
+        VMSTATE_INT32(data_len, SMBusDevice),
+        VMSTATE_UINT8_ARRAY(data_buf, SMBusDevice, SMBUS_DATA_MAX_LEN),
+        VMSTATE_UINT8(command, SMBusDevice),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void smbus_device_class_init(ObjectClass *klass, void *data)
 {
     I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
diff --git a/include/hw/i2c/smbus.h b/include/hw/i2c/smbus.h
index d8b1b9e..7b52020 100644
--- a/include/hw/i2c/smbus.h
+++ b/include/hw/i2c/smbus.h
@@ -53,14 +53,16 @@ typedef struct SMBusDeviceClass
     uint8_t (*read_data)(SMBusDevice *dev, uint8_t cmd, int n);
 } SMBusDeviceClass;
 
+#define SMBUS_DATA_MAX_LEN 34  /* command + len + 32 bytes of data.  */
+
 struct SMBusDevice {
     /* The SMBus protocol is implemented on top of I2C.  */
     I2CSlave i2c;
 
     /* Remaining fields for internal use only.  */
-    int mode;
-    int data_len;
-    uint8_t data_buf[34]; /* command + len + 32 bytes of data.  */
+    int32_t mode;
+    int32_t data_len;
+    uint8_t data_buf[SMBUS_DATA_MAX_LEN];
     uint8_t command;
 };
 
@@ -93,4 +95,14 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf);
 void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom,
                        const uint8_t *eeprom_spd, int size);
 
+extern const VMStateDescription vmstate_smbus_device;
+
+#define VMSTATE_SMBUS_DEVICE(_field, _state) {                       \
+    .name       = (stringify(_field)),                               \
+    .size       = sizeof(SMBusDevice),                               \
+    .vmsd       = &vmstate_smbus_device,                             \
+    .flags      = VMS_STRUCT,                                        \
+    .offset     = vmstate_offset_value(_state, _field, SMBusDevice), \
+}
+
 #endif
-- 
2.7.4

[Qemu-devel] [PATCH v2 10/10] i2c: Add vmstate handling to the smbus eeprom

[minyard]

From: Corey Minyard <cminyard@mvista.com>

Signed-off-by: Corey Minyard <cminyard@mvista.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Michael S . Tsirkin" <mst@redhat.com>
Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/i2c/smbus_eeprom.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
index f18aa3d..d4430b0 100644
--- a/hw/i2c/smbus_eeprom.c
+++ b/hw/i2c/smbus_eeprom.c
@@ -29,6 +29,8 @@
 
 //#define DEBUG
 
+#define TYPE_SMBUS_EEPROM_DEVICE "smbus-eeprom"
+
 typedef struct SMBusEEPROMDevice {
     SMBusDevice smbusdev;
     void *data;
@@ -97,6 +99,17 @@ static uint8_t eeprom_read_data(SMBusDevice *dev, uint8_t cmd, int n)
     return eeprom_receive_byte(dev);
 }
 
+static const VMStateDescription vmstate_smbus_eeprom = {
+    .name = TYPE_SMBUS_EEPROM_DEVICE,
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields      = (VMStateField[]) {
+        VMSTATE_SMBUS_DEVICE(smbusdev, SMBusEEPROMDevice),
+        VMSTATE_UINT8(offset, SMBusEEPROMDevice),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
 {
     SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *)dev;
@@ -121,12 +134,13 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
     sc->write_data = eeprom_write_data;
     sc->read_data = eeprom_read_data;
     dc->props = smbus_eeprom_properties;
+    dc->vmsd = &vmstate_smbus_eeprom;
     /* Reason: pointer property "data" */
     dc->user_creatable = false;
 }
 
 static const TypeInfo smbus_eeprom_info = {
-    .name          = "smbus-eeprom",
+    .name          = TYPE_SMBUS_EEPROM_DEVICE,
     .parent        = TYPE_SMBUS_DEVICE,
     .instance_size = sizeof(SMBusEEPROMDevice),
     .class_init    = smbus_eeprom_class_initfn,
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 01/10] i2c:pm_smbus: Clean up some style issues

[Philippe Mathieu-Daudé]

Hi Corey,

On 08/20/2018 05:26 PM, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> Fix some spacing issues, remove extraneous comments, add some
> defines instead of hard-coding numbers.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/i2c/pm_smbus.c | 58 ++++++++++++++++++++++++++++++++++++-------------------
>  1 file changed, 38 insertions(+), 20 deletions(-)
> 
> diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
> index 0d26e0f..83c2377 100644
> --- a/hw/i2c/pm_smbus.c
> +++ b/hw/i2c/pm_smbus.c
> @@ -22,8 +22,6 @@
>  #include "hw/i2c/pm_smbus.h"
>  #include "hw/i2c/smbus.h"
>  
> -/* no save/load? */
> -
>  #define SMBHSTSTS       0x00
>  #define SMBHSTCNT       0x02
>  #define SMBHSTCMD       0x03
> @@ -32,19 +30,34 @@
>  #define SMBHSTDAT1      0x06
>  #define SMBBLKDAT       0x07
>  
> -#define STS_HOST_BUSY   (1)
> -#define STS_INTR        (1<<1)
> -#define STS_DEV_ERR     (1<<2)
> -#define STS_BUS_ERR     (1<<3)
> -#define STS_FAILED      (1<<4)
> -#define STS_SMBALERT    (1<<5)
> -#define STS_INUSE_STS   (1<<6)
> -#define STS_BYTE_DONE   (1<<7)
> +#define STS_HOST_BUSY   (1 << 0)
> +#define STS_INTR        (1 << 1)
> +#define STS_DEV_ERR     (1 << 2)
> +#define STS_BUS_ERR     (1 << 3)
> +#define STS_FAILED      (1 << 4)
> +#define STS_SMBALERT    (1 << 5)
> +#define STS_INUSE_STS   (1 << 6)
> +#define STS_BYTE_DONE   (1 << 7)
>  /* Signs of successfully transaction end :
>  *  ByteDoneStatus = 1 (STS_BYTE_DONE) and INTR = 1 (STS_INTR )
>  */
>  
> -//#define DEBUG
> +#define CTL_INTREN      (1 << 0)
> +#define CTL_KILL        (1 << 1)
> +#define CTL_LAST_BYTE   (1 << 5)
> +#define CTL_START       (1 << 6)
> +#define CTL_PEC_EN      (1 << 7)
> +#define CTL_RETURN_MASK 0x1f
> +

What about using an enum { ...

> +#define PROT_QUICK          0
> +#define PROT_BYTE           1
> +#define PROT_BYTE_DATA      2
> +#define PROT_WORD_DATA      3
> +#define PROT_PROC_CALL      4
> +#define PROT_BLOCK_DATA     5
> +#define PROT_I2C_BLOCK_DATA 6

}; ?

Regardless:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> +
> +/*#define DEBUG*/
>  
>  #ifdef DEBUG
>  # define SMBUS_DPRINTF(format, ...)     printf(format, ## __VA_ARGS__)
> @@ -70,11 +83,12 @@ static void smb_transaction(PMSMBus *s)
>      if ((s->smb_stat & STS_DEV_ERR) != 0)  {
>          goto error;
>      }
> +
>      switch(prot) {
> -    case 0x0:
> +    case PROT_QUICK:
>          ret = smbus_quick_command(bus, addr, read);
>          goto done;
> -    case 0x1:
> +    case PROT_BYTE:
>          if (read) {
>              ret = smbus_receive_byte(bus, addr);
>              goto data8;
> @@ -82,7 +96,7 @@ static void smb_transaction(PMSMBus *s)
>              ret = smbus_send_byte(bus, addr, cmd);
>              goto done;
>          }
> -    case 0x2:
> +    case PROT_BYTE_DATA:
>          if (read) {
>              ret = smbus_read_byte(bus, addr, cmd);
>              goto data8;
> @@ -91,16 +105,17 @@ static void smb_transaction(PMSMBus *s)
>              goto done;
>          }
>          break;
> -    case 0x3:
> +    case PROT_WORD_DATA:
>          if (read) {
>              ret = smbus_read_word(bus, addr, cmd);
>              goto data16;
>          } else {
> -            ret = smbus_write_word(bus, addr, cmd, (s->smb_data1 << 8) | s->smb_data0);
> +            ret = smbus_write_word(bus, addr, cmd,
> +                                   (s->smb_data1 << 8) | s->smb_data0);
>              goto done;
>          }
>          break;
> -    case 0x5:
> +    case PROT_I2C_BLOCK_DATA:
>          if (read) {
>              ret = smbus_read_block(bus, addr, cmd, s->smb_data);
>              goto data8;
> @@ -158,8 +173,9 @@ static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
>          break;
>      case SMBHSTCNT:
>          s->smb_ctl = val;
> -        if (val & 0x40)
> +        if (s->smb_ctl & CTL_START) {
>              smb_transaction_start(s);
> +        }
>          break;
>      case SMBHSTCMD:
>          s->smb_cmd = val;
> @@ -198,7 +214,7 @@ static uint64_t smb_ioport_readb(void *opaque, hwaddr addr, unsigned width)
>          break;
>      case SMBHSTCNT:
>          s->smb_index = 0;
> -        val = s->smb_ctl & 0x1f;
> +        val = s->smb_ctl & CTL_RETURN_MASK;
>          break;
>      case SMBHSTCMD:
>          val = s->smb_cmd;
> @@ -221,7 +237,9 @@ static uint64_t smb_ioport_readb(void *opaque, hwaddr addr, unsigned width)
>          val = 0;
>          break;
>      }
> -    SMBUS_DPRINTF("SMB readb port=0x%04" HWADDR_PRIx " val=0x%02x\n", addr, val);
> +    SMBUS_DPRINTF("SMB readb port=0x%04" HWADDR_PRIx " val=0x%02x\n",
> +                  addr, val);
> +
>      return val;
>  }
>  
> 

Re: [Qemu-devel] [PATCH v2 07/10] Don't delay host status register busy bit when interrupts are enabled

[Philippe Mathieu-Daudé]

On 08/20/2018 05:26 PM, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> Change 880b1ffe6ec2f0ae "smbus: do not immediately complete commands"
> changed pm_smbus to delay setting the host busy bit until the status
> register was read, to work around a bug in AMIBIOS.  Unfortunately,
> when interrupts are enabled, the status register will never get read
> and the processing will never happen.
> 
> Modify the code to only delay setting the host busy bit if interrupts
> are not enabled.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Cc: Hervé Poussineau <hpoussin@reactos.org>
> Cc: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  hw/i2c/pm_smbus.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
> index 664a6b1..10ba208 100644
> --- a/hw/i2c/pm_smbus.c
> +++ b/hw/i2c/pm_smbus.c
> @@ -80,9 +80,6 @@ static void smb_transaction(PMSMBus *s)
>      I2CBus *bus = s->smbus;
>      int ret;
>  
> -    assert(s->smb_stat & STS_HOST_BUSY);
> -    s->smb_stat &= ~STS_HOST_BUSY;
> -
>      SMBUS_DPRINTF("SMBus trans addr=0x%02x prot=0x%02x\n", addr, prot);
>      /* Transaction isn't exec if STS_DEV_ERR bit set */
>      if ((s->smb_stat & STS_DEV_ERR) != 0)  {
> @@ -209,9 +206,18 @@ error:
>  
>  static void smb_transaction_start(PMSMBus *s)
>  {
> -    /* Do not execute immediately the command ; it will be
> -     * executed when guest will read SMB_STAT register */
> -    s->smb_stat |= STS_HOST_BUSY;
> +    if (s->smb_ctl & CTL_INTREN) {
> +        smb_transaction(s);
> +    } else {
> +        /* Do not execute immediately the command; it will be
> +         * executed when guest will read SMB_STAT register.  This
> +         * is to work around a bug in AMIBIOS (that is working
> +         * around another bug in some specific hardware) where
> +         * it waits for STS_HOST_BUSY to be set before waiting
> +         * checking for status.  If STS_HOST_BUSY doesn't get
> +         * set, it gets stuck. */
> +        s->smb_stat |= STS_HOST_BUSY;
> +    }
>  }
>  
>  static bool
> @@ -330,6 +336,7 @@ static uint64_t smb_ioport_readb(void *opaque, hwaddr addr, unsigned width)
>          val = s->smb_stat;
>          if (s->smb_stat & STS_HOST_BUSY) {
>              /* execute command now */
> +            s->smb_stat &= ~STS_HOST_BUSY;
>              smb_transaction(s);
>          }
>          break;
> 

Re: [Qemu-devel] [PATCH v2 08/10] i2c:pm_smbus: Add the ability to force block transfer enable

[Philippe Mathieu-Daudé]

On 08/20/2018 05:26 PM, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> The PIIX4 hardware has block transfer buffer always enabled in
> the hardware, but the i801 does not.  Add a parameter to pm_smbus_init
> to force on the block transfer so the PIIX4 handler can enable this
> by default, as it was disabled by default before.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/acpi/piix4.c           | 2 +-
>  hw/i2c/pm_smbus.c         | 5 ++++-
>  hw/i2c/smbus_ich9.c       | 2 +-
>  hw/isa/vt82c686.c         | 2 +-
>  include/hw/i2c/pm_smbus.h | 2 +-
>  5 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index f8d8d2e..313305f 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -513,7 +513,7 @@ static void piix4_pm_realize(PCIDevice *dev, Error **errp)
>      pci_conf[0x90] = s->smb_io_base | 1;
>      pci_conf[0x91] = s->smb_io_base >> 8;
>      pci_conf[0xd2] = 0x09;
> -    pm_smbus_init(DEVICE(dev), &s->smb);
> +    pm_smbus_init(DEVICE(dev), &s->smb, true);
>      memory_region_set_enabled(&s->smb.io, pci_conf[0xd2] & 1);
>      memory_region_add_subregion(pci_address_space_io(dev),
>                                  s->smb_io_base, &s->smb.io);
> diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
> index 10ba208..4d021bb 100644
> --- a/hw/i2c/pm_smbus.c
> +++ b/hw/i2c/pm_smbus.c
> @@ -422,11 +422,14 @@ const VMStateDescription pmsmb_vmstate = {
>      }
>  };
>  
> -void pm_smbus_init(DeviceState *parent, PMSMBus *smb)
> +void pm_smbus_init(DeviceState *parent, PMSMBus *smb, bool force_aux_blk)
>  {
>      smb->op_done = true;
>      smb->reset = pm_smbus_reset;
>      smb->smbus = i2c_init_bus(parent, "i2c");
> +    if (force_aux_blk) {
> +        smb->smb_auxctl |= AUX_BLK;
> +    }
>      memory_region_init_io(&smb->io, OBJECT(parent), &pm_smbus_ops, smb,
>                            "pm-smbus", 64);
>  }
> diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
> index 316c2a4..5860fc9 100644
> --- a/hw/i2c/smbus_ich9.c
> +++ b/hw/i2c/smbus_ich9.c
> @@ -88,7 +88,7 @@ static void ich9_smbus_realize(PCIDevice *d, Error **errp)
>      pci_set_byte(d->config + ICH9_SMB_HOSTC, 0);
>      /* TODO bar0, bar1: 64bit BAR support*/
>  
> -    pm_smbus_init(&d->qdev, &s->smb);
> +    pm_smbus_init(&d->qdev, &s->smb, false);
>      pci_register_bar(d, ICH9_SMB_SMB_BASE_BAR, PCI_BASE_ADDRESS_SPACE_IO,
>                       &s->smb.io);
>  }
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index cff1946..7302f6d 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -370,7 +370,7 @@ static void vt82c686b_pm_realize(PCIDevice *dev, Error **errp)
>      pci_conf[0x90] = s->smb_io_base | 1;
>      pci_conf[0x91] = s->smb_io_base >> 8;
>      pci_conf[0xd2] = 0x90;
> -    pm_smbus_init(&s->dev.qdev, &s->smb);
> +    pm_smbus_init(&s->dev.qdev, &s->smb, false);
>      memory_region_add_subregion(get_system_io(), s->smb_io_base, &s->smb.io);
>  
>      apm_init(dev, &s->apm, NULL, s);
> diff --git a/include/hw/i2c/pm_smbus.h b/include/hw/i2c/pm_smbus.h
> index cfe596f..471345e 100644
> --- a/include/hw/i2c/pm_smbus.h
> +++ b/include/hw/i2c/pm_smbus.h
> @@ -33,7 +33,7 @@ typedef struct PMSMBus {
>      bool op_done;
>  } PMSMBus;
>  
> -void pm_smbus_init(DeviceState *parent, PMSMBus *smb);
> +void pm_smbus_init(DeviceState *parent, PMSMBus *smb, bool force_aux_blk);
>  
>  extern const VMStateDescription pmsmb_vmstate;
>  
> 

Re: [Qemu-devel] [PATCH v2 0/8] Make the pm_smbus code more correct

[Paolo Bonzini]

On 20/08/2018 22:26, minyard@acm.org wrote:
> These changes make the pm_smbus device work more like the real thing,
> adding block transfers, I2C block transfers, and interrupts.
> 
> It also adds vmstate transfer for the smbus device in patch 5.
> That patch could probably be left out if that is not desirable.
> 
> Patch 7 works around a change made to work around an AMIBIOS bug and
> is necessary for interrupts to work.
> 
> Patches 9 and 10 add vmstate transfer for SMBus EEPROMs, and obviously
> requires patch 5 to work properly.  These could also be left out.


I have applied 1-6/7/8 for now.  Thanks!

Paolo

Re: [Qemu-devel] [PATCH v2 05/10] i2c:pm_smbus: Fix state transfer

[Paolo Bonzini]

On 20/08/2018 22:26, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> Transfer the state information for the SMBus registers and
> internal data so it will work on a VM transfer.

Since the device is mostly unused, I would rather have this in a
subsection.  This way the format remains backwards-compatible as long as
the registers have the default values.

Also, in case you are interested, a few years ago I was thinking of
implementing clock stretching; the plan was that clock stretching would
let you pass host I2C devices down to the guest.  Basically, the I2C
recv function was changed to accept a callback+opaque, and said callback
would be invoked later if recv returned -EAGAIN (or something like
that).  Unfortunately I didn't finish adding support to all the I2C
controllers when my laptop was stolen, so the patches are lost. :/

Paolo

> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  hw/acpi/piix4.c           |  3 ++-
>  hw/i2c/pm_smbus.c         | 20 ++++++++++++++++++++
>  hw/i2c/smbus_ich9.c       |  5 +++--
>  include/hw/i2c/pm_smbus.h |  2 ++
>  4 files changed, 27 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> index 6404af5..f8d8d2e 100644
> --- a/hw/acpi/piix4.c
> +++ b/hw/acpi/piix4.c
> @@ -309,7 +309,7 @@ static const VMStateDescription vmstate_cpuhp_state = {
>   */
>  static const VMStateDescription vmstate_acpi = {
>      .name = "piix4_pm",
> -    .version_id = 3,
> +    .version_id = 4,
>      .minimum_version_id = 3,
>      .minimum_version_id_old = 1,
>      .load_state_old = acpi_load_old,
> @@ -320,6 +320,7 @@ static const VMStateDescription vmstate_acpi = {
>          VMSTATE_UINT16(ar.pm1.evt.en, PIIX4PMState),
>          VMSTATE_UINT16(ar.pm1.cnt.cnt, PIIX4PMState),
>          VMSTATE_STRUCT(apm, PIIX4PMState, 0, vmstate_apm, APMState),
> +        VMSTATE_STRUCT(smb, PIIX4PMState, 4, pmsmb_vmstate, PMSMBus),
>          VMSTATE_TIMER_PTR(ar.tmr.timer, PIIX4PMState),
>          VMSTATE_INT64(ar.tmr.overflow_time, PIIX4PMState),
>          VMSTATE_STRUCT(ar.gpe, PIIX4PMState, 2, vmstate_gpe, ACPIGPE),
> diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
> index 32132be..9e11d47 100644
> --- a/hw/i2c/pm_smbus.c
> +++ b/hw/i2c/pm_smbus.c
> @@ -383,6 +383,26 @@ static const MemoryRegionOps pm_smbus_ops = {
>      .endianness = DEVICE_LITTLE_ENDIAN,
>  };
>  
> +const VMStateDescription pmsmb_vmstate = {
> +    .name = "pmsmb",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8(smb_stat, PMSMBus),
> +        VMSTATE_UINT8(smb_ctl, PMSMBus),
> +        VMSTATE_UINT8(smb_cmd, PMSMBus),
> +        VMSTATE_UINT8(smb_addr, PMSMBus),
> +        VMSTATE_UINT8(smb_data0, PMSMBus),
> +        VMSTATE_UINT8(smb_data1, PMSMBus),
> +        VMSTATE_UINT32(smb_index, PMSMBus),
> +        VMSTATE_UINT8_ARRAY(smb_data, PMSMBus, PM_SMBUS_MAX_MSG_SIZE),
> +        VMSTATE_UINT8(smb_auxctl, PMSMBus),
> +        VMSTATE_BOOL(i2c_enable, PMSMBus),
> +        VMSTATE_BOOL(op_done, PMSMBus),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  void pm_smbus_init(DeviceState *parent, PMSMBus *smb)
>  {
>      smb->op_done = true;
> diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
> index a66a114..c8b8413 100644
> --- a/hw/i2c/smbus_ich9.c
> +++ b/hw/i2c/smbus_ich9.c
> @@ -45,10 +45,11 @@ typedef struct ICH9SMBState {
>  
>  static const VMStateDescription vmstate_ich9_smbus = {
>      .name = "ich9_smb",
> -    .version_id = 1,
> +    .version_id = 2,
>      .minimum_version_id = 1,
>      .fields = (VMStateField[]) {
> -        VMSTATE_PCI_DEVICE(dev, struct ICH9SMBState),
> +        VMSTATE_PCI_DEVICE(dev, ICH9SMBState),
> +        VMSTATE_STRUCT(smb, ICH9SMBState, 2, pmsmb_vmstate, PMSMBus),
>          VMSTATE_END_OF_LIST()
>      }
>  };
> diff --git a/include/hw/i2c/pm_smbus.h b/include/hw/i2c/pm_smbus.h
> index 99d5489..b1e1970 100644
> --- a/include/hw/i2c/pm_smbus.h
> +++ b/include/hw/i2c/pm_smbus.h
> @@ -33,4 +33,6 @@ typedef struct PMSMBus {
>  
>  void pm_smbus_init(DeviceState *parent, PMSMBus *smb);
>  
> +extern const VMStateDescription pmsmb_vmstate;
> +
>  #endif /* PM_SMBUS_H */
> 

Re: [Qemu-devel] [PATCH v2 10/10] i2c: Add vmstate handling to the smbus eeprom

[Dr. David Alan Gilbert]

* minyard@acm.org (minyard@acm.org) wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Michael S . Tsirkin" <mst@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  hw/i2c/smbus_eeprom.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
> index f18aa3d..d4430b0 100644
> --- a/hw/i2c/smbus_eeprom.c
> +++ b/hw/i2c/smbus_eeprom.c
> @@ -29,6 +29,8 @@
>  
>  //#define DEBUG
>  
> +#define TYPE_SMBUS_EEPROM_DEVICE "smbus-eeprom"
> +
>  typedef struct SMBusEEPROMDevice {
>      SMBusDevice smbusdev;
>      void *data;
> @@ -97,6 +99,17 @@ static uint8_t eeprom_read_data(SMBusDevice *dev, uint8_t cmd, int n)
>      return eeprom_receive_byte(dev);
>  }
>  
> +static const VMStateDescription vmstate_smbus_eeprom = {
> +    .name = TYPE_SMBUS_EEPROM_DEVICE,
> +    .version_id = 1,
> +    .minimum_version_id = 1,

Can you add a .needed and wire it up to a property that we can then wire
to machine types; that way we don't break migration compatibility.

> +    .fields      = (VMStateField[]) {
> +        VMSTATE_SMBUS_DEVICE(smbusdev, SMBusEEPROMDevice),
> +        VMSTATE_UINT8(offset, SMBusEEPROMDevice),

What about the data?

Dave

> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
>  {
>      SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *)dev;
> @@ -121,12 +134,13 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
>      sc->write_data = eeprom_write_data;
>      sc->read_data = eeprom_read_data;
>      dc->props = smbus_eeprom_properties;
> +    dc->vmsd = &vmstate_smbus_eeprom;
>      /* Reason: pointer property "data" */
>      dc->user_creatable = false;
>  }
>  
>  static const TypeInfo smbus_eeprom_info = {
> -    .name          = "smbus-eeprom",
> +    .name          = TYPE_SMBUS_EEPROM_DEVICE,
>      .parent        = TYPE_SMBUS_DEVICE,
>      .instance_size = sizeof(SMBusEEPROMDevice),
>      .class_init    = smbus_eeprom_class_initfn,
> -- 
> 2.7.4
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 09/10] i2c: Add an SMBus vmstate structure

[Dr. David Alan Gilbert]

* minyard@acm.org (minyard@acm.org) wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: "Michael S . Tsirkin" <mst@redhat.com>
> Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>  hw/i2c/smbus.c         | 14 ++++++++++++++
>  include/hw/i2c/smbus.h | 18 +++++++++++++++---
>  2 files changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/i2c/smbus.c b/hw/i2c/smbus.c
> index 6ff77c5..b0774d7 100644
> --- a/hw/i2c/smbus.c
> +++ b/hw/i2c/smbus.c
> @@ -349,6 +349,20 @@ int smbus_write_block(I2CBus *bus, uint8_t addr, uint8_t command, uint8_t *data,
>      return 0;
>  }
>  
> +const VMStateDescription vmstate_smbus_device = {
> +    .name = TYPE_SMBUS_DEVICE,
> +    .version_id = 1,
> +    .minimum_version_id = 1,

As with the other patch, I suggest the .needed and property to enable it
only for newer machine types.

> +    .fields      = (VMStateField[]) {
> +        VMSTATE_I2C_SLAVE(i2c, SMBusDevice),
> +        VMSTATE_INT32(mode, SMBusDevice),
> +        VMSTATE_INT32(data_len, SMBusDevice),
> +        VMSTATE_UINT8_ARRAY(data_buf, SMBusDevice, SMBUS_DATA_MAX_LEN),
> +        VMSTATE_UINT8(command, SMBusDevice),

Do you need a post_load  to validate that data_len isn't silly?
Maybe command/mode also need checks.

Dave

> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static void smbus_device_class_init(ObjectClass *klass, void *data)
>  {
>      I2CSlaveClass *sc = I2C_SLAVE_CLASS(klass);
> diff --git a/include/hw/i2c/smbus.h b/include/hw/i2c/smbus.h
> index d8b1b9e..7b52020 100644
> --- a/include/hw/i2c/smbus.h
> +++ b/include/hw/i2c/smbus.h
> @@ -53,14 +53,16 @@ typedef struct SMBusDeviceClass
>      uint8_t (*read_data)(SMBusDevice *dev, uint8_t cmd, int n);
>  } SMBusDeviceClass;
>  
> +#define SMBUS_DATA_MAX_LEN 34  /* command + len + 32 bytes of data.  */
> +
>  struct SMBusDevice {
>      /* The SMBus protocol is implemented on top of I2C.  */
>      I2CSlave i2c;
>  
>      /* Remaining fields for internal use only.  */
> -    int mode;
> -    int data_len;
> -    uint8_t data_buf[34]; /* command + len + 32 bytes of data.  */
> +    int32_t mode;
> +    int32_t data_len;
> +    uint8_t data_buf[SMBUS_DATA_MAX_LEN];
>      uint8_t command;
>  };
>  
> @@ -93,4 +95,14 @@ void smbus_eeprom_init_one(I2CBus *smbus, uint8_t address, uint8_t *eeprom_buf);
>  void smbus_eeprom_init(I2CBus *smbus, int nb_eeprom,
>                         const uint8_t *eeprom_spd, int size);
>  
> +extern const VMStateDescription vmstate_smbus_device;
> +
> +#define VMSTATE_SMBUS_DEVICE(_field, _state) {                       \
> +    .name       = (stringify(_field)),                               \
> +    .size       = sizeof(SMBusDevice),                               \
> +    .vmsd       = &vmstate_smbus_device,                             \
> +    .flags      = VMS_STRUCT,                                        \
> +    .offset     = vmstate_offset_value(_state, _field, SMBusDevice), \
> +}
> +
>  #endif
> -- 
> 2.7.4
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 10/10] i2c: Add vmstate handling to the smbus eeprom

[Dr. David Alan Gilbert]

* Dr. David Alan Gilbert (dgilbert@redhat.com) wrote:
> * minyard@acm.org (minyard@acm.org) wrote:
> > From: Corey Minyard <cminyard@mvista.com>
> > 
> > Signed-off-by: Corey Minyard <cminyard@mvista.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: "Michael S . Tsirkin" <mst@redhat.com>
> > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  hw/i2c/smbus_eeprom.c | 16 +++++++++++++++-
> >  1 file changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/i2c/smbus_eeprom.c b/hw/i2c/smbus_eeprom.c
> > index f18aa3d..d4430b0 100644
> > --- a/hw/i2c/smbus_eeprom.c
> > +++ b/hw/i2c/smbus_eeprom.c
> > @@ -29,6 +29,8 @@
> >  
> >  //#define DEBUG
> >  
> > +#define TYPE_SMBUS_EEPROM_DEVICE "smbus-eeprom"
> > +
> >  typedef struct SMBusEEPROMDevice {
> >      SMBusDevice smbusdev;
> >      void *data;
> > @@ -97,6 +99,17 @@ static uint8_t eeprom_read_data(SMBusDevice *dev, uint8_t cmd, int n)
> >      return eeprom_receive_byte(dev);
> >  }
> >  
> > +static const VMStateDescription vmstate_smbus_eeprom = {
> > +    .name = TYPE_SMBUS_EEPROM_DEVICE,
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> 
> Can you add a .needed and wire it up to a property that we can then wire
> to machine types; that way we don't break migration compatibility.
> 
> > +    .fields      = (VMStateField[]) {
> > +        VMSTATE_SMBUS_DEVICE(smbusdev, SMBusEEPROMDevice),
> > +        VMSTATE_UINT8(offset, SMBusEEPROMDevice),
> 
> What about the data?

Also; now I'm looking - I'm confused by 'offset' being an 8bit int
when I can see hw/arm/aspeed.c calling smbus_eeprom_init_one passing 8KB
and 32KB buffers in.

Dave

> Dave
> 
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> >  static void smbus_eeprom_realize(DeviceState *dev, Error **errp)
> >  {
> >      SMBusEEPROMDevice *eeprom = (SMBusEEPROMDevice *)dev;
> > @@ -121,12 +134,13 @@ static void smbus_eeprom_class_initfn(ObjectClass *klass, void *data)
> >      sc->write_data = eeprom_write_data;
> >      sc->read_data = eeprom_read_data;
> >      dc->props = smbus_eeprom_properties;
> > +    dc->vmsd = &vmstate_smbus_eeprom;
> >      /* Reason: pointer property "data" */
> >      dc->user_creatable = false;
> >  }
> >  
> >  static const TypeInfo smbus_eeprom_info = {
> > -    .name          = "smbus-eeprom",
> > +    .name          = TYPE_SMBUS_EEPROM_DEVICE,
> >      .parent        = TYPE_SMBUS_DEVICE,
> >      .instance_size = sizeof(SMBusEEPROMDevice),
> >      .class_init    = smbus_eeprom_class_initfn,
> > -- 
> > 2.7.4
> > 
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 05/10] i2c:pm_smbus: Fix state transfer

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> On 20/08/2018 22:26, minyard@acm.org wrote:
> > From: Corey Minyard <cminyard@mvista.com>
> > 
> > Transfer the state information for the SMBus registers and
> > internal data so it will work on a VM transfer.
> 
> Since the device is mostly unused, I would rather have this in a
> subsection.  This way the format remains backwards-compatible as long as
> the registers have the default values.

Or simpler just wire it to a property and machine type.

Dave

> Also, in case you are interested, a few years ago I was thinking of
> implementing clock stretching; the plan was that clock stretching would
> let you pass host I2C devices down to the guest.  Basically, the I2C
> recv function was changed to accept a callback+opaque, and said callback
> would be invoked later if recv returned -EAGAIN (or something like
> that).  Unfortunately I didn't finish adding support to all the I2C
> controllers when my laptop was stolen, so the patches are lost. :/
> 
> Paolo
> 
> > Signed-off-by: Corey Minyard <cminyard@mvista.com>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Paolo Bonzini <pbonzini@redhat.com>
> > Cc: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > ---
> >  hw/acpi/piix4.c           |  3 ++-
> >  hw/i2c/pm_smbus.c         | 20 ++++++++++++++++++++
> >  hw/i2c/smbus_ich9.c       |  5 +++--
> >  include/hw/i2c/pm_smbus.h |  2 ++
> >  4 files changed, 27 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/acpi/piix4.c b/hw/acpi/piix4.c
> > index 6404af5..f8d8d2e 100644
> > --- a/hw/acpi/piix4.c
> > +++ b/hw/acpi/piix4.c
> > @@ -309,7 +309,7 @@ static const VMStateDescription vmstate_cpuhp_state = {
> >   */
> >  static const VMStateDescription vmstate_acpi = {
> >      .name = "piix4_pm",
> > -    .version_id = 3,
> > +    .version_id = 4,
> >      .minimum_version_id = 3,
> >      .minimum_version_id_old = 1,
> >      .load_state_old = acpi_load_old,
> > @@ -320,6 +320,7 @@ static const VMStateDescription vmstate_acpi = {
> >          VMSTATE_UINT16(ar.pm1.evt.en, PIIX4PMState),
> >          VMSTATE_UINT16(ar.pm1.cnt.cnt, PIIX4PMState),
> >          VMSTATE_STRUCT(apm, PIIX4PMState, 0, vmstate_apm, APMState),
> > +        VMSTATE_STRUCT(smb, PIIX4PMState, 4, pmsmb_vmstate, PMSMBus),
> >          VMSTATE_TIMER_PTR(ar.tmr.timer, PIIX4PMState),
> >          VMSTATE_INT64(ar.tmr.overflow_time, PIIX4PMState),
> >          VMSTATE_STRUCT(ar.gpe, PIIX4PMState, 2, vmstate_gpe, ACPIGPE),
> > diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
> > index 32132be..9e11d47 100644
> > --- a/hw/i2c/pm_smbus.c
> > +++ b/hw/i2c/pm_smbus.c
> > @@ -383,6 +383,26 @@ static const MemoryRegionOps pm_smbus_ops = {
> >      .endianness = DEVICE_LITTLE_ENDIAN,
> >  };
> >  
> > +const VMStateDescription pmsmb_vmstate = {
> > +    .name = "pmsmb",
> > +    .version_id = 1,
> > +    .minimum_version_id = 1,
> > +    .fields = (VMStateField[]) {
> > +        VMSTATE_UINT8(smb_stat, PMSMBus),
> > +        VMSTATE_UINT8(smb_ctl, PMSMBus),
> > +        VMSTATE_UINT8(smb_cmd, PMSMBus),
> > +        VMSTATE_UINT8(smb_addr, PMSMBus),
> > +        VMSTATE_UINT8(smb_data0, PMSMBus),
> > +        VMSTATE_UINT8(smb_data1, PMSMBus),
> > +        VMSTATE_UINT32(smb_index, PMSMBus),
> > +        VMSTATE_UINT8_ARRAY(smb_data, PMSMBus, PM_SMBUS_MAX_MSG_SIZE),
> > +        VMSTATE_UINT8(smb_auxctl, PMSMBus),
> > +        VMSTATE_BOOL(i2c_enable, PMSMBus),
> > +        VMSTATE_BOOL(op_done, PMSMBus),
> > +        VMSTATE_END_OF_LIST()
> > +    }
> > +};
> > +
> >  void pm_smbus_init(DeviceState *parent, PMSMBus *smb)
> >  {
> >      smb->op_done = true;
> > diff --git a/hw/i2c/smbus_ich9.c b/hw/i2c/smbus_ich9.c
> > index a66a114..c8b8413 100644
> > --- a/hw/i2c/smbus_ich9.c
> > +++ b/hw/i2c/smbus_ich9.c
> > @@ -45,10 +45,11 @@ typedef struct ICH9SMBState {
> >  
> >  static const VMStateDescription vmstate_ich9_smbus = {
> >      .name = "ich9_smb",
> > -    .version_id = 1,
> > +    .version_id = 2,
> >      .minimum_version_id = 1,
> >      .fields = (VMStateField[]) {
> > -        VMSTATE_PCI_DEVICE(dev, struct ICH9SMBState),
> > +        VMSTATE_PCI_DEVICE(dev, ICH9SMBState),
> > +        VMSTATE_STRUCT(smb, ICH9SMBState, 2, pmsmb_vmstate, PMSMBus),
> >          VMSTATE_END_OF_LIST()
> >      }
> >  };
> > diff --git a/include/hw/i2c/pm_smbus.h b/include/hw/i2c/pm_smbus.h
> > index 99d5489..b1e1970 100644
> > --- a/include/hw/i2c/pm_smbus.h
> > +++ b/include/hw/i2c/pm_smbus.h
> > @@ -33,4 +33,6 @@ typedef struct PMSMBus {
> >  
> >  void pm_smbus_init(DeviceState *parent, PMSMBus *smb);
> >  
> > +extern const VMStateDescription pmsmb_vmstate;
> > +
> >  #endif /* PM_SMBUS_H */
> > 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK