[Qemu-devel] [PATCH] smbus: do not immediately complete commands

[Qemu-devel] [PATCH] smbus: do not immediately complete commands

[Hervé Poussineau]

PIIX4 errata says that "immediate polling of the Host Status Register BUSY
bit may indicate that the SMBus is NOT busy."
Due to this, some code does the following steps:
(a) set parameters
(b) start command
(c) check for smbus busy bit set (to know that command started)
(d) check for smbus busy bit not set (to know that command finished)

Let (c) happen, by immediately setting the busy bit, and really executing
the command when status register has been read once.

This fixes a problem with AMIBIOS, which can now properly initialize the PIIX4.

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>

---
 hw/i2c/pm_smbus.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index 6fc3923f56..ec060d58cc 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -63,6 +63,9 @@ 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)  {
@@ -135,6 +138,13 @@ 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;
+}
+
 static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
                               unsigned width)
 {
@@ -150,7 +160,7 @@ static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
     case SMBHSTCNT:
         s->smb_ctl = val;
         if (val & 0x40)
-            smb_transaction(s);
+            smb_transaction_start(s);
         break;
     case SMBHSTCMD:
         s->smb_cmd = val;
@@ -182,6 +192,10 @@ static uint64_t smb_ioport_readb(void *opaque, hwaddr addr, unsigned width)
     switch(addr) {
     case SMBHSTSTS:
         val = s->smb_stat;
+        if (s->smb_stat & STS_HOST_BUSY) {
+            /* execute command now */
+            smb_transaction(s);
+        }
         break;
     case SMBHSTCNT:
         s->smb_index = 0;
-- 
2.11.0

Re: [Qemu-devel] [PATCH] smbus: do not immediately complete commands

[Philippe Mathieu-Daudé]

On 12/10/2017 02:27 PM, Hervé Poussineau wrote:
> PIIX4 errata says that "immediate polling of the Host Status Register BUSY
> bit may indicate that the SMBus is NOT busy."
> Due to this, some code does the following steps:
> (a) set parameters
> (b) start command
> (c) check for smbus busy bit set (to know that command started)
> (d) check for smbus busy bit not set (to know that command finished)
> 
> Let (c) happen, by immediately setting the busy bit, and really executing
> the command when status register has been read once.
> 
> This fixes a problem with AMIBIOS, which can now properly initialize the PIIX4.
> 
> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>

This looks sane, so:

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

> ---
>  hw/i2c/pm_smbus.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
> index 6fc3923f56..ec060d58cc 100644
> --- a/hw/i2c/pm_smbus.c
> +++ b/hw/i2c/pm_smbus.c
> @@ -63,6 +63,9 @@ 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)  {
> @@ -135,6 +138,13 @@ 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;
> +}
> +
>  static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
>                                unsigned width)
>  {
> @@ -150,7 +160,7 @@ static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
>      case SMBHSTCNT:
>          s->smb_ctl = val;
>          if (val & 0x40)
> -            smb_transaction(s);
> +            smb_transaction_start(s);
>          break;
>      case SMBHSTCMD:
>          s->smb_cmd = val;
> @@ -182,6 +192,10 @@ static uint64_t smb_ioport_readb(void *opaque, hwaddr addr, unsigned width)
>      switch(addr) {
>      case SMBHSTSTS:
>          val = s->smb_stat;
> +        if (s->smb_stat & STS_HOST_BUSY) {
> +            /* execute command now */
> +            smb_transaction(s);
> +        }
>          break;
>      case SMBHSTCNT:
>          s->smb_index = 0;
> 

[Qemu-devel] [PATCH] smbus: do not immediately complete commands

[Hervé Poussineau]

PIIX4 errata says that "immediate polling of the Host Status Register BUSY
bit may indicate that the SMBus is NOT busy."

This fixes some code which does the following steps:
(a) set parameters
(b) start command
(c) check for smbus busy bit set (to know that command started)
(d) check for smbus busy bit not set (to know that command finished)

Fix (c), by immediately setting the busy bit, and completing the command
a little bit later.

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>

---

This fixes a problem with a real bios, which can now properly initialize the PIIX4.
Note also that load/save support is not yet implemented for this device.

 hw/i2c/pm_smbus.c         |   16 +++++++++++++++-
 include/hw/i2c/pm_smbus.h |    1 +
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
index 7d249ad..9de860e 100644
--- a/hw/i2c/pm_smbus.c
+++ b/hw/i2c/pm_smbus.c
@@ -53,14 +53,18 @@
 #endif
 
 
-static void smb_transaction(PMSMBus *s)
+static void smb_transaction_timer(void *opaque)
 {
+    PMSMBus *s = opaque;
     uint8_t prot = (s->smb_ctl >> 2) & 0x07;
     uint8_t read = s->smb_addr & 0x01;
     uint8_t cmd = s->smb_cmd;
     uint8_t addr = s->smb_addr >> 1;
     i2c_bus *bus = s->smbus;
 
+    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)  {
@@ -115,6 +119,14 @@ static void smb_transaction(PMSMBus *s)
     s->smb_stat |= STS_DEV_ERR;
 }
 
+static void smb_transaction(PMSMBus *s)
+{
+    /* Do not execute immediately the command */
+    s->smb_stat |= STS_HOST_BUSY;
+    timer_mod(s->result_timer, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)
+              + (get_ticks_per_sec() / 1000));
+}
+
 static void smb_ioport_writeb(void *opaque, hwaddr addr, uint64_t val,
                               unsigned width)
 {
@@ -204,4 +216,6 @@ void pm_smbus_init(DeviceState *parent, PMSMBus *smb)
     smb->smbus = i2c_init_bus(parent, "i2c");
     memory_region_init_io(&smb->io, OBJECT(parent), &pm_smbus_ops, smb,
                           "pm-smbus", 64);
+    smb->result_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
+                                     smb_transaction_timer, smb);
 }
diff --git a/include/hw/i2c/pm_smbus.h b/include/hw/i2c/pm_smbus.h
index e3069bf..f0db1d9 100644
--- a/include/hw/i2c/pm_smbus.h
+++ b/include/hw/i2c/pm_smbus.h
@@ -4,6 +4,7 @@
 typedef struct PMSMBus {
     i2c_bus *smbus;
     MemoryRegion io;
+    QEMUTimer *result_timer;
 
     uint8_t smb_stat;
     uint8_t smb_ctl;
-- 
1.7.10.4