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

Hervé Poussineau posted 1 patch 6 years, 4 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20171210172703.27866-1-hpoussin@reactos.org
Test checkpatch passed
Test docker passed
Test ppc passed
Test s390x passed
hw/i2c/pm_smbus.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
[Qemu-devel] [PATCH] smbus: do not immediately complete commands
Posted by Hervé Poussineau 6 years, 4 months ago
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
Posted by Philippe Mathieu-Daudé 6 years, 4 months ago
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;
>