Re: [PATCH v2 03/13] vt82c686: Fix SMBus IO base and configuration registers
在 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
---
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(>smb.io, smbase);
+ memory_region_set_enabled(>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
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
+ }
+ 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
Re: [PATCH v2 03/13] vt82c686: Fix SMBus IO base and configuration registers
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
---
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(>smb.io, smbase);
+memory_region_set_enabled(>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 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.
+}
+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.
Regards,
BALATON Zoltan
Re: [PATCH v2 03/13] vt82c686: Fix SMBus IO base and configuration registers
在 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
---
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(>smb.io, smbase);
+memory_region_set_enabled(>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 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.
[...]
[PATCH v2 03/13] vt82c686: Fix SMBus IO base and configuration registers
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.
Signed-off-by: BALATON Zoltan
---
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(>smb.io, smbase);
+memory_region_set_enabled(>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);
+}
}
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, 0x0001);
-/* 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), >smb, false);
-memory_region_add_subregion(get_system_io(), s->smb_io_base, >smb.io);
+memory_region_add_subregion(pci_address_space_io(dev), 0, >smb.io);
+memory_region_set_enabled(>smb.io, false);
apm_init(dev, >apm, NULL, s);
@@ -135,11 +163,6 @@ static void vt82c686b_pm_realize(PCIDevice *dev, Error
**errp)
acpi_pm1_cnt_init(>ar, >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 = _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 29805242ca..fbdd6122b3 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -251,9 +251,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, _fatal);
+dev =
