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

[Jiaxun Yang <jiaxun.yang@flygoat.com>]

在 2021/1/13 上午6:25, BALATON Zoltan 写道:
> On Tue, 12 Jan 2021, Jiaxun Yang wrote:
>> 在 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 :-/
>
> It says that in the summary table but later in data sheet there's also 
> detailed description of registers for each part where it says:
>
> Offset 93-90 – SMBus I/O Base ... RW
> 3-0 Fixed ... always reads 0001b
>
> The above mask and | 1 ensures this. I don't know why lowest bit is 
> always 1 but that seems to be the case for all such regs. Maybe 
> internally these are implemented like PCI BARs where lowest bit means 
> IO space.

Thanks!

In this case:

Reviewed-by: Jiaxun Yang <jiaxun.yang@flygoat.com>

>
>>> +    }
>>> +    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.
>
> I could add defines for register offsets but did not think that would 
> make it much more readable to have random names instead of random 
> numbers. Likely you'll have to consult the data sheet to find out 
> their meaning anyway.

Agreed.

- Jiaxun

>
> Regards,
> BALATON Zoltan