On Thu, Jan 18, 2018 at 07:55:41PM -0600, miny...@acm.org wrote:
> From: Corey Minyard <cminy...@mvista.com>
>
> This reverts commit 880b1ffe6ec2f0ae25cc4175716227ad275e8b8a.
>
> The commit being reverted says:
>
> 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.
>
> Emulating bad hardware so badly written software will work doesn't sound
> like a good idea to me. I have patches that add interrupt capability
> to pm_smbus, but this change breaks that because the Linux driver
> starts the transaction then waits for interrupts before reading the
> status register. That obviously won't work with these changes.
>
> The right way to fix this in AMIBIOS is to ignore the host busy bit
> and use the other bits in the host status register to tell if the
> transaction has completed. Using host busy is racy, anyway, if you
> get interrupted or something while processing, you may miss step (c)
> in your algorithm and fail.
>
> Cc: Hervé Poussineau <hpous...@reactos.org>
> Cc: Philippe Mathieu-Daudé <f4...@amsat.org>
> Signed-off-by: Corey Minyard <cminy...@mvista.com>
Would it be possible to limit the change to when guest uses
interrupts?
> ---
> hw/i2c/pm_smbus.c | 16 +---------------
> 1 file changed, 1 insertion(+), 15 deletions(-)
>
> diff --git a/hw/i2c/pm_smbus.c b/hw/i2c/pm_smbus.c
> index 0d26e0f..a044dd1 100644
> --- a/hw/i2c/pm_smbus.c
> +++ b/hw/i2c/pm_smbus.c
> @@ -62,9 +62,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) {
> @@ -137,13 +134,6 @@ 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)
> {
> @@ -159,7 +149,7 @@ static void smb_ioport_writeb(void *opaque, hwaddr addr,
> uint64_t val,
> case SMBHSTCNT:
> s->smb_ctl = val;
> if (val & 0x40)
> - smb_transaction_start(s);
> + smb_transaction(s);
> break;
> case SMBHSTCMD:
> s->smb_cmd = val;
> @@ -191,10 +181,6 @@ 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.7.4
