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 <address@hidden>

This looks sane, so:

Signed-off-by: Philippe Mathieu-Daudé <address@hidden>

> ---
>  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;
>