+Yajin, Chenming On 6/24/21 6:46 PM, Philippe Mathieu-Daudé wrote: > On 6/24/21 6:16 PM, Philippe Mathieu-Daudé wrote: >> On 6/24/21 6:01 PM, Philippe Mathieu-Daudé wrote: >>> On 6/24/21 5:46 PM, Philippe Mathieu-Daudé wrote: >>>> Hi Zoltan, >>>> >>>> On 2/21/21 3:34 PM, Philippe Mathieu-Daudé wrote: >>>>> From: BALATON Zoltan <bala...@eik.bme.hu> >>>>> >>>>> 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. >>>> >>>> This commit broken running PMON on Fuloong2E: >>>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg752605.html >>>> console: PMON2000 MIPS Initializing. Standby... >>>> console: ERRORPC=00000000 CONFIG=00030932 >>>> console: PRID=00006302 >>>> console: DIMM read >>>> console: 000000ff >>>> console: 000000ff >>>> console: 000000ff >>>> console: 000000ff >>>> console: 000000ff >>>> console: 000000ff >>>> console: 000000ff >>>> console: 000000ff >>>> console: 000000ff >>>> console: 000000ff >>>> ... >>>> >>>> From here the console loops displaying this value... >>> >>> Tracing:
>> pci_cfg_write vt82c686b-pm 05:4 @0x90 <- 0xeee1 > > Offset 93-90 – SMBus I/O Base ....................................... RW > 15-4 I/O Base (16-byte I/O space)................ default = 00h > pci_cfg_write vt82c686b-pm 05:4 @0x90 <- 0xeee1 > >> pci_cfg_write vt82c686b-pm 05:4 @0xd0 <- 0x1 > > Offset D2 – SMBus Host Configuration ......................... RW > SMBus Host Controller Enable > 0 Disable SMB controller functions ......... default > 1 Enable SMB controller functions > pci_cfg_write vt82c686b-pm 05:4 @0xd0 <- 0x1 > > Hmm the datasheet indeed document 0xd2... why is the guest accessing > 0xd0 to enable the function? It seems this is the problem, since if > I replace d2 -> d0 PMON boots. See below [*]. >>>> Expected: >>>> >>>> console: PMON2000 MIPS Initializing. Standby... >>>> console: ERRORPC=00000000 CONFIG=00030932 >>>> console: PRID=00006302 >>>> console: DIMM read >>>> console: 00000080 >>>> console: read memory type >>>> console: read number of rows >>>> ... >>>> >>>>> Signed-off-by: BALATON Zoltan <bala...@eik.bme.hu> >>>>> Reviewed-by: Jiaxun Yang <jiaxun.y...@flygoat.com> >>>>> Message-Id: >>>>> <f2ca2ad5f08ba8cee07afd9d67b4e75cda21db09.1610223397.git.bala...@eik.bme.hu> >>>>> Signed-off-by: Philippe Mathieu-Daudé <f4...@amsat.org> >>>>> --- >>>>> 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 fe8961b0573..9c4d1530225 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); >>>>> + } >>>>> + if (range_covers_byte(addr, len, 0xd2)) { >>>>> + s->dev.config[0xd2] &= 0xf; >>>>> + smb_io_space_update(s); > > [*] So the guest writing at 0xd0, this block is skipped, the > I/O region never enabled. > >>>>> + } >>>>> } >>>>> >>>>> 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); >>>>> + s->dev.config[0xd2] = 0; >>>>> + >>>>> + smb_io_space_update(s); >>>>> +} >>>>> + >>>>> static void vt82c686b_pm_realize(PCIDevice *dev, Error **errp) >>>>> { >>>>> VT686PMState *s = VT82C686B_PM(dev); >>>>> @@ -116,13 +148,9 @@ static void vt82c686b_pm_realize(PCIDevice *dev, >>>>> Error **errp) >>>>> /* 0x48-0x4B is Power Management I/O Base */ >>>>> pci_set_long(pci_conf + 0x48, 0x00000001); >>>>> >>>>> - /* SMB ports:0xeee0~0xeeef */ >>>>> - s->smb_io_base = ((s->smb_io_base & 0xfff0) + 0x0); >>>>> - pci_conf[0x90] = s->smb_io_base | 1; >>>>> - pci_conf[0x91] = s->smb_io_base >> 8; >>>>> - pci_conf[0xd2] = 0x90; This line hasn't been modified since its introduction in commit edf79e66c02 ("Initial support of vt82686b south bridge used by fulong mini pc"), so I am not sure it was used. Beside, I don't understand 0x90; per the datasheet description only bits 0-3 are defined. Huacai, do you remember? =) >>>>> pm_smbus_init(DEVICE(s), &s->smb, false); >>>>> - memory_region_add_subregion(get_system_io(), s->smb_io_base, >>>>> &s->smb.io); >>>>> + memory_region_add_subregion(pci_address_space_io(dev), 0, >>>>> &s->smb.io); >>>>> + memory_region_set_enabled(&s->smb.io, false); >>>>> >>>>> apm_init(dev, &s->apm, NULL, s); >>>>> >>>>> @@ -135,11 +163,6 @@ static void vt82c686b_pm_realize(PCIDevice *dev, >>>>> Error **errp) >>>>> acpi_pm1_cnt_init(&s->ar, &s->io, false, false, 2); >>>>> } >>>>> >>>>> -static Property via_pm_properties[] = { >>>>> - DEFINE_PROP_UINT32("smb_io_base", VT686PMState, smb_io_base, 0), >>>>> - DEFINE_PROP_END_OF_LIST(), >>>>> -}; >>>>> - >>>>> static void via_pm_class_init(ObjectClass *klass, void *data) >>>>> { >>>>> DeviceClass *dc = DEVICE_CLASS(klass); >>>>> @@ -151,10 +174,10 @@ static void via_pm_class_init(ObjectClass *klass, >>>>> void *data) >>>>> k->device_id = PCI_DEVICE_ID_VIA_ACPI; >>>>> k->class_id = PCI_CLASS_BRIDGE_OTHER; >>>>> k->revision = 0x40; >>>>> + dc->reset = vt82c686b_pm_reset; >>>>> dc->desc = "PM"; >>>>> dc->vmsd = &vmstate_acpi; >>>>> set_bit(DEVICE_CATEGORY_BRIDGE, dc->categories); >>>>> - device_class_set_props(dc, via_pm_properties); >>>>> } >>>>> >>>>> static const TypeInfo via_pm_info = { >>>>> diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c >>>>> index 1f3680fda3e..94f4718147f 100644 >>>>> --- a/hw/mips/fuloong2e.c >>>>> +++ b/hw/mips/fuloong2e.c >>>>> @@ -230,9 +230,7 @@ static void vt82c686b_southbridge_init(PCIBus >>>>> *pci_bus, int slot, qemu_irq intc, >>>>> pci_create_simple(pci_bus, PCI_DEVFN(slot, 2), "vt82c686b-usb-uhci"); >>>>> pci_create_simple(pci_bus, PCI_DEVFN(slot, 3), "vt82c686b-usb-uhci"); >>>>> >>>>> - dev = pci_new(PCI_DEVFN(slot, 4), TYPE_VT82C686B_PM); >>>>> - qdev_prop_set_uint32(DEVICE(dev), "smb_io_base", 0xeee1); >>>>> - pci_realize_and_unref(dev, pci_bus, &error_fatal); >>>>> + dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 4), >>>>> TYPE_VT82C686B_PM); >>>>> *i2c_bus = I2C_BUS(qdev_get_child_bus(DEVICE(dev), "i2c")); >>>>> >>>>> /* Audio support */ >>>>> >>>> >>>> >>> >> >