hw/mips: Fix the Fuloong 2E machine with PMON bios

[PATCH 1/5] hw/isa/vt82c686: Replace magic numbers by definitions

Posted by Philippe Mathieu-Daudé 4 years, 7 months ago

Replace magic values of the Power Management / SMBus function (#4)
by definitions from the datasheet. The result is less compact, and
we can follow what the code does without having to recur to the
datasheet.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/isa/vt82c686.c | 50 +++++++++++++++++++++++++++++++----------------
 1 file changed, 33 insertions(+), 17 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index f57f3e70679..4ddcf2d398c 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -14,6 +14,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "hw/registerfields.h"
 #include "hw/isa/vt82c686.h"
 #include "hw/pci/pci.h"
 #include "hw/qdev-properties.h"
@@ -38,6 +39,16 @@
 #define TYPE_VIA_PM "via-pm"
 OBJECT_DECLARE_SIMPLE_TYPE(ViaPMState, VIA_PM)
 
+REG8(PM_GEN_CFG0,                   0x40)
+REG8(PM_GEN_CFG1,                   0x41)
+FIELD(PM_GEN_CFG1, ACPI_IO_ENABLE,  7, 1)
+REG32(PM_IO_BASE,                   0x48)
+FIELD(PM_IO_BASE, ADDR,             7, 9)
+REG32(SMBUS_IO_BASE,                0x90)
+FIELD(SMBUS_IO_BASE, ADDR,          4, 12)
+REG8(SMBUS_HOST_CONFIG,             0xd2)
+FIELD(SMBUS_HOST_CONFIG, ENABLE,    0, 1)
+
 struct ViaPMState {
     PCIDevice dev;
     MemoryRegion io;
@@ -48,21 +59,24 @@ struct ViaPMState {
 
 static void pm_io_space_update(ViaPMState *s)
 {
-    uint32_t pmbase = pci_get_long(s->dev.config + 0x48) & 0xff80UL;
+    uint32_t pmbase = pci_get_long(s->dev.config + A_PM_IO_BASE);
 
     memory_region_transaction_begin();
-    memory_region_set_address(&s->io, pmbase);
-    memory_region_set_enabled(&s->io, s->dev.config[0x41] & BIT(7));
+    memory_region_set_address(&s->io, pmbase & R_PM_IO_BASE_ADDR_MASK);
+    memory_region_set_enabled(&s->io, FIELD_EX32(s->dev.config[A_PM_GEN_CFG1],
+                                      PM_GEN_CFG1, ACPI_IO_ENABLE));
     memory_region_transaction_commit();
 }
 
 static void smb_io_space_update(ViaPMState *s)
 {
-    uint32_t smbase = pci_get_long(s->dev.config + 0x90) & 0xfff0UL;
+    uint32_t smbase = pci_get_long(s->dev.config + A_SMBUS_IO_BASE);
 
     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_set_address(&s->smb.io, smbase & R_SMBUS_IO_BASE_ADDR_MASK);
+    memory_region_set_enabled(&s->smb.io,
+                              FIELD_EX32(s->dev.config[A_SMBUS_HOST_CONFIG],
+                                         SMBUS_HOST_CONFIG, ENABLE));
     memory_region_transaction_commit();
 }
 
@@ -98,19 +112,21 @@ static void pm_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int len)
 
     trace_via_pm_write(addr, val, len);
     pci_default_write_config(d, addr, val, len);
-    if (ranges_overlap(addr, len, 0x48, 4)) {
-        uint32_t v = pci_get_long(s->dev.config + 0x48);
-        pci_set_long(s->dev.config + 0x48, (v & 0xff80UL) | 1);
+    if (ranges_overlap(addr, len, A_PM_IO_BASE, 4)) {
+        uint32_t v = pci_get_long(s->dev.config + A_PM_IO_BASE);
+        pci_set_long(s->dev.config + A_PM_IO_BASE,
+                     (v & R_PM_IO_BASE_ADDR_MASK) | 1);
     }
-    if (range_covers_byte(addr, len, 0x41)) {
+    if (range_covers_byte(addr, len, A_PM_GEN_CFG1)) {
         pm_io_space_update(s);
     }
-    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 (ranges_overlap(addr, len, A_SMBUS_IO_BASE, 4)) {
+        uint32_t v = pci_get_long(s->dev.config + A_SMBUS_IO_BASE);
+        pci_set_long(s->dev.config + A_SMBUS_IO_BASE,
+                     (v & R_SMBUS_IO_BASE_ADDR_MASK) | 1);
     }
-    if (range_covers_byte(addr, len, 0xd2)) {
-        s->dev.config[0xd2] &= 0xf;
+    if (range_covers_byte(addr, len, A_SMBUS_HOST_CONFIG)) {
+        s->dev.config[A_SMBUS_HOST_CONFIG] &= 0xf;
         smb_io_space_update(s);
     }
 }
@@ -176,9 +192,9 @@ static void via_pm_reset(DeviceState *d)
     memset(s->dev.config + PCI_CONFIG_HEADER_SIZE, 0,
            PCI_CONFIG_SPACE_SIZE - PCI_CONFIG_HEADER_SIZE);
     /* Power Management IO base */
-    pci_set_long(s->dev.config + 0x48, 1);
+    pci_set_long(s->dev.config + A_PM_IO_BASE, 1);
     /* SMBus IO base */
-    pci_set_long(s->dev.config + 0x90, 1);
+    pci_set_long(s->dev.config + A_SMBUS_IO_BASE, 1);
 
     acpi_pm1_evt_reset(&s->ar);
     acpi_pm1_cnt_reset(&s->ar);
-- 
2.31.1

Re: [PATCH 1/5] hw/isa/vt82c686: Replace magic numbers by definitions

Posted by BALATON Zoltan 4 years, 7 months ago

On Thu, 24 Jun 2021, Philippe Mathieu-Daudé wrote:
> Replace magic values of the Power Management / SMBus function (#4)
> by definitions from the datasheet. The result is less compact, and
> we can follow what the code does without having to recur to the
> datasheet.

I'm not sure this is an improvement. With the values it's clear what is 
done but I can't follow how these magic constants are defined or what they 
do so no idea if this is still correct. I think if you want to review a 
device model then you should be familiar with the device or consult the 
data sheet. Otherwise you can't spot problems in the definition of these 
constants either. I'm not a fan of hiding things behind cryptic macros 
when you could just write it in a straightforward way that could be 
understood more clearly.

Regards,
BALATON Zoltan

> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/isa/vt82c686.c | 50 +++++++++++++++++++++++++++++++----------------
> 1 file changed, 33 insertions(+), 17 deletions(-)
>
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index f57f3e70679..4ddcf2d398c 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -14,6 +14,7 @@
>  */
>
> #include "qemu/osdep.h"
> +#include "hw/registerfields.h"
> #include "hw/isa/vt82c686.h"
> #include "hw/pci/pci.h"
> #include "hw/qdev-properties.h"
> @@ -38,6 +39,16 @@
> #define TYPE_VIA_PM "via-pm"
> OBJECT_DECLARE_SIMPLE_TYPE(ViaPMState, VIA_PM)
>
> +REG8(PM_GEN_CFG0,                   0x40)
> +REG8(PM_GEN_CFG1,                   0x41)
> +FIELD(PM_GEN_CFG1, ACPI_IO_ENABLE,  7, 1)
> +REG32(PM_IO_BASE,                   0x48)
> +FIELD(PM_IO_BASE, ADDR,             7, 9)
> +REG32(SMBUS_IO_BASE,                0x90)
> +FIELD(SMBUS_IO_BASE, ADDR,          4, 12)
> +REG8(SMBUS_HOST_CONFIG,             0xd2)
> +FIELD(SMBUS_HOST_CONFIG, ENABLE,    0, 1)
> +
> struct ViaPMState {
>     PCIDevice dev;
>     MemoryRegion io;
> @@ -48,21 +59,24 @@ struct ViaPMState {
>
> static void pm_io_space_update(ViaPMState *s)
> {
> -    uint32_t pmbase = pci_get_long(s->dev.config + 0x48) & 0xff80UL;
> +    uint32_t pmbase = pci_get_long(s->dev.config + A_PM_IO_BASE);
>
>     memory_region_transaction_begin();
> -    memory_region_set_address(&s->io, pmbase);
> -    memory_region_set_enabled(&s->io, s->dev.config[0x41] & BIT(7));
> +    memory_region_set_address(&s->io, pmbase & R_PM_IO_BASE_ADDR_MASK);
> +    memory_region_set_enabled(&s->io, FIELD_EX32(s->dev.config[A_PM_GEN_CFG1],
> +                                      PM_GEN_CFG1, ACPI_IO_ENABLE));
>     memory_region_transaction_commit();
> }
>
> static void smb_io_space_update(ViaPMState *s)
> {
> -    uint32_t smbase = pci_get_long(s->dev.config + 0x90) & 0xfff0UL;
> +    uint32_t smbase = pci_get_long(s->dev.config + A_SMBUS_IO_BASE);
>
>     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_set_address(&s->smb.io, smbase & R_SMBUS_IO_BASE_ADDR_MASK);
> +    memory_region_set_enabled(&s->smb.io,
> +                              FIELD_EX32(s->dev.config[A_SMBUS_HOST_CONFIG],
> +                                         SMBUS_HOST_CONFIG, ENABLE));
>     memory_region_transaction_commit();
> }
>
> @@ -98,19 +112,21 @@ static void pm_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int len)
>
>     trace_via_pm_write(addr, val, len);
>     pci_default_write_config(d, addr, val, len);
> -    if (ranges_overlap(addr, len, 0x48, 4)) {
> -        uint32_t v = pci_get_long(s->dev.config + 0x48);
> -        pci_set_long(s->dev.config + 0x48, (v & 0xff80UL) | 1);
> +    if (ranges_overlap(addr, len, A_PM_IO_BASE, 4)) {
> +        uint32_t v = pci_get_long(s->dev.config + A_PM_IO_BASE);
> +        pci_set_long(s->dev.config + A_PM_IO_BASE,
> +                     (v & R_PM_IO_BASE_ADDR_MASK) | 1);
>     }
> -    if (range_covers_byte(addr, len, 0x41)) {
> +    if (range_covers_byte(addr, len, A_PM_GEN_CFG1)) {
>         pm_io_space_update(s);
>     }
> -    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 (ranges_overlap(addr, len, A_SMBUS_IO_BASE, 4)) {
> +        uint32_t v = pci_get_long(s->dev.config + A_SMBUS_IO_BASE);
> +        pci_set_long(s->dev.config + A_SMBUS_IO_BASE,
> +                     (v & R_SMBUS_IO_BASE_ADDR_MASK) | 1);
>     }
> -    if (range_covers_byte(addr, len, 0xd2)) {
> -        s->dev.config[0xd2] &= 0xf;
> +    if (range_covers_byte(addr, len, A_SMBUS_HOST_CONFIG)) {
> +        s->dev.config[A_SMBUS_HOST_CONFIG] &= 0xf;
>         smb_io_space_update(s);
>     }
> }
> @@ -176,9 +192,9 @@ static void via_pm_reset(DeviceState *d)
>     memset(s->dev.config + PCI_CONFIG_HEADER_SIZE, 0,
>            PCI_CONFIG_SPACE_SIZE - PCI_CONFIG_HEADER_SIZE);
>     /* Power Management IO base */
> -    pci_set_long(s->dev.config + 0x48, 1);
> +    pci_set_long(s->dev.config + A_PM_IO_BASE, 1);
>     /* SMBus IO base */
> -    pci_set_long(s->dev.config + 0x90, 1);
> +    pci_set_long(s->dev.config + A_SMBUS_IO_BASE, 1);
>
>     acpi_pm1_evt_reset(&s->ar);
>     acpi_pm1_cnt_reset(&s->ar);
>

[PATCH 1/5] hw/isa/vt82c686: Replace magic numbers by definitions
[PATCH 2/5] hw/pci-host/bonito: Trace PCI config accesses smaller than 32-bit
[PATCH 3/5] hw/pci-host/bonito: Allow PCI config accesses smaller than 32-bit
[PATCH 4/5] tests/acceptance: Test Linux on the Fuloong 2E machine
[PATCH 5/5] tests/acceptance: Test PMON on the Fuloong 2E machine