[Qemu-devel] [PATCH 0/3] pm_smbus: correctly report unclaimed cycles

[Qemu-devel] [PATCH 0/3] pm_smbus: correctly report unclaimed cycles

[Paolo Bonzini]

Prodded by Alex David's i2c thread, I tried running i2cdetect on an x86
machine, and it reported all addresses as occupied.  This small series
fixes it.

Paolo Bonzini (3):
  smbus: allow returning an error from reads
  smbus: return -1 if nothing found at the given address
  pm_smbus: correctly report unclaimed cycles

 hw/i2c/pm_smbus.c      | 63 ++++++++++++++++++++++++++++++----------------
 hw/i2c/smbus.c         | 68 ++++++++++++++++++++++++++++++++++----------------
 include/hw/i2c/smbus.h | 18 ++++++-------
 3 files changed, 97 insertions(+), 52 deletions(-)

-- 
1.8.5.3

[Qemu-devel] [PATCH 1/3] smbus: allow returning an error from reads

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/i2c/smbus.c         | 6 +++---
 include/hw/i2c/smbus.h | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/i2c/smbus.c b/hw/i2c/smbus.c
index 25d2d04..a8931b7 100644
--- a/hw/i2c/smbus.c
+++ b/hw/i2c/smbus.c
@@ -214,7 +214,7 @@ void smbus_quick_command(i2c_bus *bus, uint8_t addr, int read)
     i2c_end_transfer(bus);
 }
 
-uint8_t smbus_receive_byte(i2c_bus *bus, uint8_t addr)
+int smbus_receive_byte(i2c_bus *bus, uint8_t addr)
 {
     uint8_t data;
 
@@ -232,7 +232,7 @@ void smbus_send_byte(i2c_bus *bus, uint8_t addr, uint8_t data)
     i2c_end_transfer(bus);
 }
 
-uint8_t smbus_read_byte(i2c_bus *bus, uint8_t addr, uint8_t command)
+int smbus_read_byte(i2c_bus *bus, uint8_t addr, uint8_t command)
 {
     uint8_t data;
     i2c_start_transfer(bus, addr, 0);
@@ -252,7 +252,7 @@ void smbus_write_byte(i2c_bus *bus, uint8_t addr, uint8_t command, uint8_t data)
     i2c_end_transfer(bus);
 }
 
-uint16_t smbus_read_word(i2c_bus *bus, uint8_t addr, uint8_t command)
+int smbus_read_word(i2c_bus *bus, uint8_t addr, uint8_t command)
 {
     uint16_t data;
     i2c_start_transfer(bus, addr, 0);
diff --git a/include/hw/i2c/smbus.h b/include/hw/i2c/smbus.h
index d764d75..4293733 100644
--- a/include/hw/i2c/smbus.h
+++ b/include/hw/i2c/smbus.h
@@ -67,11 +67,11 @@ struct SMBusDevice {
 
 /* Master device commands.  */
 void smbus_quick_command(i2c_bus *bus, uint8_t addr, int read);
-uint8_t smbus_receive_byte(i2c_bus *bus, uint8_t addr);
+int smbus_receive_byte(i2c_bus *bus, uint8_t addr);
 void smbus_send_byte(i2c_bus *bus, uint8_t addr, uint8_t data);
-uint8_t smbus_read_byte(i2c_bus *bus, uint8_t addr, uint8_t command);
+int smbus_read_byte(i2c_bus *bus, uint8_t addr, uint8_t command);
 void smbus_write_byte(i2c_bus *bus, uint8_t addr, uint8_t command, uint8_t data);
-uint16_t smbus_read_word(i2c_bus *bus, uint8_t addr, uint8_t command);
+int smbus_read_word(i2c_bus *bus, uint8_t addr, uint8_t command);
 void smbus_write_word(i2c_bus *bus, uint8_t addr, uint8_t command, uint16_t data);
 int smbus_read_block(i2c_bus *bus, uint8_t addr, uint8_t command, uint8_t *data);
 void smbus_write_block(i2c_bus *bus, uint8_t addr, uint8_t command, uint8_t *data,
-- 
1.8.5.3

[Qemu-devel] [PATCH 2/3] smbus: return -1 if nothing found at the given address

[Paolo Bonzini]

Fix coding standards in the meanwhile.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/i2c/smbus.c         | 62 +++++++++++++++++++++++++++++++++++---------------
 include/hw/i2c/smbus.h | 12 +++++-----
 2 files changed, 50 insertions(+), 24 deletions(-)

diff --git a/hw/i2c/smbus.c b/hw/i2c/smbus.c
index a8931b7..18fb4d1 100644
--- a/hw/i2c/smbus.c
+++ b/hw/i2c/smbus.c
@@ -208,34 +208,44 @@ static int smbus_device_init(I2CSlave *i2c)
 }
 
 /* Master device commands.  */
-void smbus_quick_command(i2c_bus *bus, uint8_t addr, int read)
+int smbus_quick_command(i2c_bus *bus, uint8_t addr, int read)
 {
-    i2c_start_transfer(bus, addr, read);
+    if (i2c_start_transfer(bus, addr, read)) {
+        return -1;
+    }
     i2c_end_transfer(bus);
+    return 0;
 }
 
 int smbus_receive_byte(i2c_bus *bus, uint8_t addr)
 {
     uint8_t data;
 
-    i2c_start_transfer(bus, addr, 1);
+    if (i2c_start_transfer(bus, addr, 1)) {
+        return -1;
+    }
     data = i2c_recv(bus);
     i2c_nack(bus);
     i2c_end_transfer(bus);
     return data;
 }
 
-void smbus_send_byte(i2c_bus *bus, uint8_t addr, uint8_t data)
+int smbus_send_byte(i2c_bus *bus, uint8_t addr, uint8_t data)
 {
-    i2c_start_transfer(bus, addr, 0);
+    if (i2c_start_transfer(bus, addr, 0)) {
+        return -1;
+    }
     i2c_send(bus, data);
     i2c_end_transfer(bus);
+    return 0;
 }
 
 int smbus_read_byte(i2c_bus *bus, uint8_t addr, uint8_t command)
 {
     uint8_t data;
-    i2c_start_transfer(bus, addr, 0);
+    if (i2c_start_transfer(bus, addr, 0)) {
+        return -1;
+    }
     i2c_send(bus, command);
     i2c_start_transfer(bus, addr, 1);
     data = i2c_recv(bus);
@@ -244,18 +254,23 @@ int smbus_read_byte(i2c_bus *bus, uint8_t addr, uint8_t command)
     return data;
 }
 
-void smbus_write_byte(i2c_bus *bus, uint8_t addr, uint8_t command, uint8_t data)
+int smbus_write_byte(i2c_bus *bus, uint8_t addr, uint8_t command, uint8_t data)
 {
-    i2c_start_transfer(bus, addr, 0);
+    if (i2c_start_transfer(bus, addr, 0)) {
+        return -1;
+    }
     i2c_send(bus, command);
     i2c_send(bus, data);
     i2c_end_transfer(bus);
+    return 0;
 }
 
 int smbus_read_word(i2c_bus *bus, uint8_t addr, uint8_t command)
 {
     uint16_t data;
-    i2c_start_transfer(bus, addr, 0);
+    if (i2c_start_transfer(bus, addr, 0)) {
+        return -1;
+    }
     i2c_send(bus, command);
     i2c_start_transfer(bus, addr, 1);
     data = i2c_recv(bus);
@@ -265,13 +280,16 @@ int smbus_read_word(i2c_bus *bus, uint8_t addr, uint8_t command)
     return data;
 }
 
-void smbus_write_word(i2c_bus *bus, uint8_t addr, uint8_t command, uint16_t data)
+int smbus_write_word(i2c_bus *bus, uint8_t addr, uint8_t command, uint16_t data)
 {
-    i2c_start_transfer(bus, addr, 0);
+    if (i2c_start_transfer(bus, addr, 0)) {
+        return -1;
+    }
     i2c_send(bus, command);
     i2c_send(bus, data & 0xff);
     i2c_send(bus, data >> 8);
     i2c_end_transfer(bus);
+    return 0;
 }
 
 int smbus_read_block(i2c_bus *bus, uint8_t addr, uint8_t command, uint8_t *data)
@@ -279,33 +297,41 @@ int smbus_read_block(i2c_bus *bus, uint8_t addr, uint8_t command, uint8_t *data)
     int len;
     int i;
 
-    i2c_start_transfer(bus, addr, 0);
+    if (i2c_start_transfer(bus, addr, 0)) {
+        return -1;
+    }
     i2c_send(bus, command);
     i2c_start_transfer(bus, addr, 1);
     len = i2c_recv(bus);
-    if (len > 32)
+    if (len > 32) {
         len = 0;
-    for (i = 0; i < len; i++)
+    }
+    for (i = 0; i < len; i++) {
         data[i] = i2c_recv(bus);
+    }
     i2c_nack(bus);
     i2c_end_transfer(bus);
     return len;
 }
 
-void smbus_write_block(i2c_bus *bus, uint8_t addr, uint8_t command, uint8_t *data,
-                       int len)
+int smbus_write_block(i2c_bus *bus, uint8_t addr, uint8_t command, uint8_t *data,
+                      int len)
 {
     int i;
 
     if (len > 32)
         len = 32;
 
-    i2c_start_transfer(bus, addr, 0);
+    if (i2c_start_transfer(bus, addr, 0)) {
+        return -1;
+    }
     i2c_send(bus, command);
     i2c_send(bus, len);
-    for (i = 0; i < len; i++)
+    for (i = 0; i < len; i++) {
         i2c_send(bus, data[i]);
+    }
     i2c_end_transfer(bus);
+    return 0;
 }
 
 static void smbus_device_class_init(ObjectClass *klass, void *data)
diff --git a/include/hw/i2c/smbus.h b/include/hw/i2c/smbus.h
index 4293733..358f35c 100644
--- a/include/hw/i2c/smbus.h
+++ b/include/hw/i2c/smbus.h
@@ -66,16 +66,16 @@ struct SMBusDevice {
 };
 
 /* Master device commands.  */
-void smbus_quick_command(i2c_bus *bus, uint8_t addr, int read);
+int smbus_quick_command(i2c_bus *bus, uint8_t addr, int read);
 int smbus_receive_byte(i2c_bus *bus, uint8_t addr);
-void smbus_send_byte(i2c_bus *bus, uint8_t addr, uint8_t data);
+int smbus_send_byte(i2c_bus *bus, uint8_t addr, uint8_t data);
 int smbus_read_byte(i2c_bus *bus, uint8_t addr, uint8_t command);
-void smbus_write_byte(i2c_bus *bus, uint8_t addr, uint8_t command, uint8_t data);
+int smbus_write_byte(i2c_bus *bus, uint8_t addr, uint8_t command, uint8_t data);
 int smbus_read_word(i2c_bus *bus, uint8_t addr, uint8_t command);
-void smbus_write_word(i2c_bus *bus, uint8_t addr, uint8_t command, uint16_t data);
+int smbus_write_word(i2c_bus *bus, uint8_t addr, uint8_t command, uint16_t data);
 int smbus_read_block(i2c_bus *bus, uint8_t addr, uint8_t command, uint8_t *data);
-void smbus_write_block(i2c_bus *bus, uint8_t addr, uint8_t command, uint8_t *data,
-                       int len);
+int smbus_write_block(i2c_bus *bus, uint8_t addr, uint8_t command, uint8_t *data,
+                      int len);
 
 void smbus_eeprom_init(i2c_bus *smbus, int nb_eeprom,
                        const uint8_t *eeprom_spd, int size);
-- 
1.8.5.3

[Qemu-devel] [PATCH 3/3] pm_smbus: correctly report unclaimed cycles

[Paolo Bonzini]

Without this patch, i2cdetect will report all addresses as present.
With it, only 0x50..0x57 are present.

Before:

         0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
    00:          03 04 05 06 07 08 09 0a 0b 0c 0d 0e 0f
    10: 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f
    20: 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f
    30: 30 31 32 33 34 35 36 37 38 39 3a 3b 3c 3d 3e 3f
    40: 40 41 42 43 44 45 46 47 48 49 4a 4b 4c 4d 4e 4f
    50: 50 51 52 53 54 55 56 57 58 59 5a 5b 5c 5d 5e 5f
    60: 60 61 62 63 64 65 66 67 68 69 6a 6b 6c 6d 6e 6f
    70: 70 71 72 73 74 75 76 77

After:

         0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
    00:          -- -- -- -- -- -- -- -- -- -- -- -- --
    10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
    20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
    30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
    40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
    50: 50 51 52 53 54 55 56 57 -- -- -- -- -- -- -- --
    60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
    70: -- -- -- -- -- -- -- --

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/i2c/pm_smbus.c | 63 ++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 41 insertions(+), 22 deletions(-)

diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index c98e447..9dccd5f 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -60,59 +60,78 @@ static void smb_transaction(PMSMBus *s)
     uint8_t cmd = s->smb_cmd;
     uint8_t addr = s->smb_addr >> 1;
     i2c_bus *bus = s->smbus;
+    int ret;
 
     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)  {
-            goto error;
-        }
+        goto error;
+    }
     switch(prot) {
     case 0x0:
-        smbus_quick_command(bus, addr, read);
-        s->smb_stat |= STS_BYTE_DONE | STS_INTR;
-        break;
+        ret = smbus_quick_command(bus, addr, read);
+        goto done;
     case 0x1:
         if (read) {
-            s->smb_data0 = smbus_receive_byte(bus, addr);
+            ret = smbus_receive_byte(bus, addr);
+            goto data0;
         } else {
-            smbus_send_byte(bus, addr, cmd);
+            ret = smbus_send_byte(bus, addr, cmd);
+            goto done;
         }
-        s->smb_stat |= STS_BYTE_DONE | STS_INTR;
-        break;
     case 0x2:
         if (read) {
-            s->smb_data0 = smbus_read_byte(bus, addr, cmd);
+            ret = smbus_read_byte(bus, addr, cmd);
+            goto data0;
         } else {
-            smbus_write_byte(bus, addr, cmd, s->smb_data0);
+            ret = smbus_write_byte(bus, addr, cmd, s->smb_data0);
+            goto done;
         }
-        s->smb_stat |= STS_BYTE_DONE | STS_INTR;
         break;
     case 0x3:
         if (read) {
-            uint16_t val;
-            val = smbus_read_word(bus, addr, cmd);
-            s->smb_data0 = val;
-            s->smb_data1 = val >> 8;
+            ret = smbus_read_word(bus, addr, cmd);
+            goto data01;
         } else {
-            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;
         }
-        s->smb_stat |= STS_BYTE_DONE | STS_INTR;
         break;
     case 0x5:
         if (read) {
-            s->smb_data0 = smbus_read_block(bus, addr, cmd, s->smb_data);
+            ret = smbus_read_block(bus, addr, cmd, s->smb_data);
+            goto data0;
         } else {
-            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);
+            goto done;
         }
-        s->smb_stat |= STS_BYTE_DONE | STS_INTR;
         break;
     default:
         goto error;
     }
+    abort();
+
+data01:
+    if (ret < 0) {
+        goto error;
+    }
+    s->smb_data1 = ret >> 8;
+data0:
+    if (ret < 0) {
+        goto error;
+    }
+    s->smb_data0 = ret;
+done:
+    if (ret < 0) {
+        goto error;
+    }
+    s->smb_stat |= STS_BYTE_DONE | STS_INTR;
     return;
 
-  error:
+error:
     s->smb_stat |= STS_DEV_ERR;
+    return;
+
 }
 
 static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
-- 
1.8.5.3