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:
>>
>> mr_ops_write mr 0x5583912b2e00 (south-bridge-pci-config) addr 0x1fe80490
>> value 0xeee1 size 4
>> mr_ops_write mr 0x5583912b2e00 (south-bridge-pci-config) addr 0x1fe804d2
>> value 0x1 size 2
>> mr_ops_write mr 0x5583912b2e00 (south-bridge-pci-config) addr 0x1fe80404
>> value 0x1 size 1
>> mr_ops_write mr 0x5583912b2e00 (south-bridge-pci-config) addr 0x1fe80004
>> value 0x7 size 4
>> mr_ops_read mr 0x5583912b2e00 (south-bridge-pci-config) addr 0x1fe80081
>> value 0x0 size 1
>> mr_ops_write mr 0x5583912b2e00 (south-bridge-pci-config) addr 0x1fe80081
>> value 0x80 size 1
>> mr_ops_write mr 0x5583912b2e00 (south-bridge-pci-config) addr 0x1fe80083
>> value 0x89 size 1
>> mr_ops_write mr 0x5583912b2e00 (south-bridge-pci-config) addr 0x1fe80085
>> value 0x3 size 1
>> mr_ops_write mr 0x5583912b2e00 (south-bridge-pci-config) addr 0x1fe8005a
>> value 0x7 size 1
>> mr_ops_write mr 0x558390ff4d00 (io) addr 0x3f0 value 0xe2 size 1
>> mr_ops_write mr 0x558390ff4d00 (io) addr 0x3f0 value 0xe3 size 1
>> mr_ops_write mr 0x558390ff4d00 (io) addr 0x3f0 value 0xe6 size 1
>> mr_ops_write mr 0x558390ff4d00 (io) addr 0x3f0 value 0xe7 size 1
>> mr_ops_write mr 0x558390ff4d00 (io) addr 0x3f0 value 0xe8 size 1
>> mr_ops_write mr 0x558390ff4d00 (io) addr 0x3f0 value 0xee size 1
>> mr_ops_write mr 0x5583912b2e00 (south-bridge-pci-config) addr 0x1fe80085
>> value 0x1 size 1
>
> These are:
> 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 [*].
> pci_cfg_write vt82c686b-pm 05:4 @0x4 <- 0x1
(this one is PCI_COMMAND)
> pci_cfg_write vt82c686b-isa 05:0 @0x4 <- 0x7
> pci_cfg_read vt82c686b-isa 05:0 @0x80 -> 0x0
> pci_cfg_write vt82c686b-isa 05:0 @0x80 <- 0x80
> pci_cfg_write vt82c686b-isa 05:0 @0x80 <- 0x89
> pci_cfg_write vt82c686b-isa 05:0 @0x84 <- 0x3
> pci_cfg_write vt82c686b-isa 05:0 @0x58 <- 0x7
> pci_cfg_write vt82c686b-isa 05:0 @0x84 <- 0x1
>
>> mr_ops_write mr 0x558390ff4d00 (io) addr 0xeee4 value 0xa1 size 1
>> mr_ops_write mr 0x558390ff4d00 (io) addr 0xeee3 value 0x0 size 1
>> mr_ops_write mr 0x558390ff4d00 (io) addr 0xeee2 value 0x8 size 1
>> mr_ops_read mr 0x558390ff4d00 (io) addr 0xeee0 value 0xffffffffffffffff
>> size 1
>> mr_ops_write mr 0x558390ff4d00 (io) addr 0xeee0 value 0x1f size 1
>> mr_ops_read mr 0x558390ff4d00 (io) addr 0xeee0 value 0xffffffffffffffff
>> size 1
>> mr_ops_read mr 0x558390ff4d00 (io) addr 0xeee2 value 0xffffffffffffffff
>> size 1
>> mr_ops_write mr 0x558390ff4d00 (io) addr 0xeee2 value 0xff size 1
>> mr_ops_read mr 0x558390ff4d00 (io) addr 0xeee0 value 0xffffffffffffffff
>> size 1
>> mr_ops_read mr 0x558390ff4d00 (io) addr 0xeee0 value 0xffffffffffffffff
>> size 1
>> mr_ops_read mr 0x558390ff4d00 (io) addr 0xeee0 value 0xffffffffffffffff
>> size 1
>> mr_ops_read mr 0x558390ff4d00 (io) addr 0xeee0 value 0xffffffffffffffff
>> size 1
>> mr_ops_read mr 0x558390ff4d00 (io) addr 0xeee0 value 0xffffffffffffffff
>> size 1
>> mr_ops_read mr 0x558390ff4d00 (io) addr 0xeee0 value 0xffffffffffffffff
>> size 1
>>
>>> 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;
>>>> 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 */
>>>>
>>>
>>>
>>
>
