Re: [PATCH v2 03/13] vt82c686: Fix SMBus IO base and configuration registers

[Jiaxun Yang]

在 2021/1/10 上午4:16, BALATON Zoltan 写道:
> The base address of the SMBus io ports and its enabled status is set
> by registers in the PCI config space but this was not correctly
> emulated. Instead the SMBus registers were mapped on realize to the
> base address set by a property to the address expected by fuloong2e
> firmware.
>
> Fix the base and config register handling to more closely model
> hardware which allows to remove the property and allows the guest to
> control this mapping. Do all this in reset instead of realize so it's
> correctly updated on reset.

Hi,

Thanks for your patch!

> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>   hw/isa/vt82c686.c   | 49 +++++++++++++++++++++++++++++++++------------
>   hw/mips/fuloong2e.c |  4 +---
>   2 files changed, 37 insertions(+), 16 deletions(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index fe8961b057..9c4d153022 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -22,6 +22,7 @@
>   #include "hw/i2c/pm_smbus.h"
>   #include "qapi/error.h"
>   #include "qemu/module.h"
> +#include "qemu/range.h"
>   #include "qemu/timer.h"
>   #include "exec/address-spaces.h"
>   #include "trace.h"
> @@ -34,7 +35,6 @@ struct VT686PMState {
>       ACPIREGS ar;
>       APMState apm;
>       PMSMBus smb;
> -    uint32_t smb_io_base;
>   };
>   
>   static void pm_io_space_update(VT686PMState *s)
> @@ -50,11 +50,22 @@ static void pm_io_space_update(VT686PMState *s)
>       memory_region_transaction_commit();
>   }
>   
> +static void smb_io_space_update(VT686PMState *s)
> +{
> +    uint32_t smbase = pci_get_long(s->dev.config + 0x90) & 0xfff0UL;
> +
> +    memory_region_transaction_begin();
> +    memory_region_set_address(&s->smb.io, smbase);
> +    memory_region_set_enabled(&s->smb.io, s->dev.config[0xd2] & BIT(0));
> +    memory_region_transaction_commit();
> +}
> +
>   static int vmstate_acpi_post_load(void *opaque, int version_id)
>   {
>       VT686PMState *s = opaque;
>   
>       pm_io_space_update(s);
> +    smb_io_space_update(s);
>       return 0;
>   }
>   
> @@ -77,8 +88,18 @@ static const VMStateDescription vmstate_acpi = {
>   
>   static void pm_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int len)
>   {
> +    VT686PMState *s = VT82C686B_PM(d);
> +
>       trace_via_pm_write(addr, val, len);
>       pci_default_write_config(d, addr, val, len);
> +    if (ranges_overlap(addr, len, 0x90, 4)) {
> +        uint32_t v = pci_get_long(s->dev.config + 0x90);
> +        pci_set_long(s->dev.config + 0x90, (v & 0xfff0UL) | 1);

What does this "or 1" do?
The datasheet I found only mentioned the default value of BASE is 0000 0001
but didn't say anything about it's function :-/

> +    }
> +    if (range_covers_byte(addr, len, 0xd2)) {
> +        s->dev.config[0xd2] &= 0xf;
> +        smb_io_space_update(s);
> +    }
>   }
>   
>   static void pm_update_sci(VT686PMState *s)
> @@ -103,6 +124,17 @@ static void pm_tmr_timer(ACPIREGS *ar)
>       pm_update_sci(s);
>   }
>   
> +static void vt82c686b_pm_reset(DeviceState *d)
> +{
> +    VT686PMState *s = VT82C686B_PM(d);
> +
> +    /* SMBus IO base */
> +    pci_set_long(s->dev.config + 0x90, 1);

Theoretically this kind of magic number should be avoided but
as the rest of the file was written in such style it seems fine for me.

[...]