[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PULL 30/43] vt82c686: Fix SMBus IO base and configuration registers
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PULL 30/43] vt82c686: Fix SMBus IO base and configuration registers |
Date: |
Thu, 24 Jun 2021 19:18:50 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 |
+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 <balaton@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 <balaton@eik.bme.hu>
>>>>> Reviewed-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
>>>>> Message-Id:
>>>>> <f2ca2ad5f08ba8cee07afd9d67b4e75cda21db09.1610223397.git.balaton@eik.bme.hu>
>>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@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 */
>>>>>
>>>>
>>>>
>>>
>>
>
- Re: [PULL 30/43] vt82c686: Fix SMBus IO base and configuration registers, Philippe Mathieu-Daudé, 2021/06/24
- Re: [PULL 30/43] vt82c686: Fix SMBus IO base and configuration registers, Philippe Mathieu-Daudé, 2021/06/24
- Re: [PULL 30/43] vt82c686: Fix SMBus IO base and configuration registers, Philippe Mathieu-Daudé, 2021/06/24
- Re: [PULL 30/43] vt82c686: Fix SMBus IO base and configuration registers, Philippe Mathieu-Daudé, 2021/06/24
- Re: [PULL 30/43] vt82c686: Fix SMBus IO base and configuration registers, BALATON Zoltan, 2021/06/24
- Re: [PULL 30/43] vt82c686: Fix SMBus IO base and configuration registers, Philippe Mathieu-Daudé, 2021/06/24
- Re: [PULL 30/43] vt82c686: Fix SMBus IO base and configuration registers, BALATON Zoltan, 2021/06/24
- Re: [PULL 30/43] vt82c686: Fix SMBus IO base and configuration registers, BALATON Zoltan, 2021/06/24
- Re: [PULL 30/43] vt82c686: Fix SMBus IO base and configuration registers, Philippe Mathieu-Daudé, 2021/06/24
- Re: [PULL 30/43] vt82c686: Fix SMBus IO base and configuration registers, Philippe Mathieu-Daudé, 2021/06/24
- Re: [PULL 30/43] vt82c686: Fix SMBus IO base and configuration registers,
Philippe Mathieu-Daudé <=
Re: [PULL 30/43] vt82c686: Fix SMBus IO base and configuration registers, BALATON Zoltan, 2021/06/24
Re: [PULL 30/43] vt82c686: Fix SMBus IO base and configuration registers, BALATON Zoltan, 2021/06/24