[PATCH 00/12] Misc vt82c686b clean ups

[PATCH 00/12] Misc vt82c686b clean ups

[BALATON Zoltan via]

This series are some small clean ups to the vt82c686b south bridge and
superio chip model that is only used by the mips/fuloong2e machine.
These are also in preparation to add emulation of the very similar
vt8231 later that will be used by ppc/pegasos2.

Regards,
BALATON Zoltan

BALATON Zoltan (12):
  vt82c686: Add APM and ACPI dependencies for VT82C686
  vt82c686: Rename AC97/MC97 parts from VT82C686B to VIA
  vt82c686: Remove unnecessary _DEVICE suffix from type macros
  vt82c686: Remove vt82c686b_[am]c97_init() functions
  vt82c686: Split off via-[am]c97 into separate file in hw/audio
  audio/via-ac97: Simplify code and set user_creatable to false
  vt82c686: Remove vt82c686b_isa_init() function
  vt82c686: Remove vt82c686b_pm_init() function
  vt82c686: Convert debug printf to trace points
  vt82c686: Remove unneeded includes and defines
  vt82c686: Rename some functions to better show where they belong
  vt82c686: Do not add floppy

 hw/audio/meson.build      |   1 +
 hw/audio/via-ac97.c       |  93 ++++++++++++++
 hw/isa/Kconfig            |   2 +
 hw/isa/trace-events       |   6 +
 hw/isa/vt82c686.c         | 258 +++++++-------------------------------
 hw/mips/fuloong2e.c       |  13 +-
 include/hw/isa/vt82c686.h |  12 +-
 7 files changed, 161 insertions(+), 224 deletions(-)
 create mode 100644 hw/audio/via-ac97.c

-- 
2.21.3

[PATCH 12/12] vt82c686: Do not add floppy

[BALATON Zoltan via]

The floppy is inaccessible because its ports are shadowed by the
config registers of the superio part (switchable on the real chip but
we don't model that) so disable adding the floppy matching the
existing comment in vt82c686b_isa_realize() as it's not usable.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/isa/vt82c686.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 698627d1b5..2b7a3bdba1 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -348,14 +348,19 @@ static const TypeInfo via_isa_info = {
     },
 };
 
+static bool vt82c686b_superio_floppy_is_enabled(ISASuperIODevice *sio, uint8_t index)
+{
+        return false; /* Disabled due to clash with SuperIO Config reg ports */
+}
+
 static void vt82c686b_superio_class_init(ObjectClass *klass, void *data)
 {
     ISASuperIOClass *sc = ISA_SUPERIO_CLASS(klass);
 
     sc->serial.count = 2;
     sc->parallel.count = 1;
-    sc->ide.count = 0;
-    sc->floppy.count = 1;
+    sc->ide.count = 0; /* Emulated by via-ide */
+    sc->floppy.is_enabled = vt82c686b_superio_floppy_is_enabled;
 }
 
 static const TypeInfo via_superio_info = {
-- 
2.21.3

[PATCH 07/12] vt82c686: Remove vt82c686b_isa_init() function

[BALATON Zoltan via]

Also rename VT82C686B type to lower case to match other device names.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/isa/vt82c686.c         | 9 ---------
 hw/mips/fuloong2e.c       | 4 +++-
 include/hw/isa/vt82c686.h | 3 +--
 3 files changed, 4 insertions(+), 12 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 758c54172b..1c1239cade 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -49,7 +49,6 @@ struct VT82C686BState {
     SuperIOConfig superio_conf;
 };
 
-#define TYPE_VT82C686B "VT82C686B"
 OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BState, VT82C686B)
 
 static void superio_ioport_writeb(void *opaque, hwaddr addr, uint64_t data,
@@ -367,14 +366,6 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
                                 &vt82c->superio);
 }
 
-ISABus *vt82c686b_isa_init(PCIBus *bus, int devfn)
-{
-    PCIDevice *d;
-
-    d = pci_create_simple_multifunction(bus, devfn, true, TYPE_VT82C686B);
-    return ISA_BUS(qdev_get_child_bus(DEVICE(d), "isa.0"));
-}
-
 static void via_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index 3b0489f781..60d2020033 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -240,7 +240,9 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
     ISABus *isa_bus;
     PCIDevice *dev;
 
-    isa_bus = vt82c686b_isa_init(pci_bus, PCI_DEVFN(slot, 0));
+    dev = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(slot, 0), true,
+                                          TYPE_VT82C686B);
+    isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(dev), "isa.0"));
     assert(isa_bus);
     *p_isa_bus = isa_bus;
     /* Interrupt controller */
diff --git a/include/hw/isa/vt82c686.h b/include/hw/isa/vt82c686.h
index ff80a926dc..89e205a1be 100644
--- a/include/hw/isa/vt82c686.h
+++ b/include/hw/isa/vt82c686.h
@@ -1,13 +1,12 @@
 #ifndef HW_VT82C686_H
 #define HW_VT82C686_H
 
-
+#define TYPE_VT82C686B "vt82c686b"
 #define TYPE_VT82C686B_SUPERIO "vt82c686b-superio"
 #define TYPE_VIA_AC97 "via-ac97"
 #define TYPE_VIA_MC97 "via-mc97"
 
 /* vt82c686.c */
-ISABus *vt82c686b_isa_init(PCIBus * bus, int devfn);
 I2CBus *vt82c686b_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
                           qemu_irq sci_irq);
 
-- 
2.21.3

Re: [PATCH 07/12] vt82c686: Remove vt82c686b_isa_init() function

[Philippe Mathieu-Daudé]

On 12/27/20 2:10 AM, BALATON Zoltan via wrote:
> Also rename VT82C686B type to lower case to match other device names.

If possible do not split the commit description in 2 (one part in
subject and the other part here) as this is annoying to read.

> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/isa/vt82c686.c         | 9 ---------
>  hw/mips/fuloong2e.c       | 4 +++-
>  include/hw/isa/vt82c686.h | 3 +--
>  3 files changed, 4 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 758c54172b..1c1239cade 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -49,7 +49,6 @@ struct VT82C686BState {
>      SuperIOConfig superio_conf;
>  };
>  
> -#define TYPE_VT82C686B "VT82C686B"
>  OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BState, VT82C686B)
>  
>  static void superio_ioport_writeb(void *opaque, hwaddr addr, uint64_t data,
> @@ -367,14 +366,6 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
>                                  &vt82c->superio);
>  }
>  
> -ISABus *vt82c686b_isa_init(PCIBus *bus, int devfn)
> -{
> -    PCIDevice *d;
> -
> -    d = pci_create_simple_multifunction(bus, devfn, true, TYPE_VT82C686B);
> -    return ISA_BUS(qdev_get_child_bus(DEVICE(d), "isa.0"));
> -}
> -
>  static void via_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
> index 3b0489f781..60d2020033 100644
> --- a/hw/mips/fuloong2e.c
> +++ b/hw/mips/fuloong2e.c
> @@ -240,7 +240,9 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
>      ISABus *isa_bus;
>      PCIDevice *dev;
>  
> -    isa_bus = vt82c686b_isa_init(pci_bus, PCI_DEVFN(slot, 0));
> +    dev = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(slot, 0), true,
> +                                          TYPE_VT82C686B);

Good cleanup.

Preferably using TYPE_VT82C686B_ISA:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> +    isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(dev), "isa.0"));
>      assert(isa_bus);
>      *p_isa_bus = isa_bus;
>      /* Interrupt controller */
> diff --git a/include/hw/isa/vt82c686.h b/include/hw/isa/vt82c686.h
> index ff80a926dc..89e205a1be 100644
> --- a/include/hw/isa/vt82c686.h
> +++ b/include/hw/isa/vt82c686.h
> @@ -1,13 +1,12 @@
>  #ifndef HW_VT82C686_H
>  #define HW_VT82C686_H
>  
> -
> +#define TYPE_VT82C686B "vt82c686b"
>  #define TYPE_VT82C686B_SUPERIO "vt82c686b-superio"
>  #define TYPE_VIA_AC97 "via-ac97"
>  #define TYPE_VIA_MC97 "via-mc97"
>  
>  /* vt82c686.c */
> -ISABus *vt82c686b_isa_init(PCIBus * bus, int devfn);
>  I2CBus *vt82c686b_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
>                            qemu_irq sci_irq);
>  
> 

Re: [PATCH 07/12] vt82c686: Remove vt82c686b_isa_init() function

[BALATON Zoltan via]

[-- Attachment #1: Type: text/plain, Size: 3217 bytes --]

On Sun, 27 Dec 2020, Philippe Mathieu-Daudé wrote:
> On 12/27/20 2:10 AM, BALATON Zoltan via wrote:
>> Also rename VT82C686B type to lower case to match other device names.
>
> If possible do not split the commit description in 2 (one part in
> subject and the other part here) as this is annoying to read.

Not so much in the commit log but maybe indeed on the list in email. I did 
not want to repeat subject in the description but can if you prefer.

Thanks for reviewing these, I'll wait a few days to see if there are any 
other comments then will send v2 with suggested changes.

Regards,
BALATON Zoltan

>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/isa/vt82c686.c         | 9 ---------
>>  hw/mips/fuloong2e.c       | 4 +++-
>>  include/hw/isa/vt82c686.h | 3 +--
>>  3 files changed, 4 insertions(+), 12 deletions(-)
>>
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index 758c54172b..1c1239cade 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -49,7 +49,6 @@ struct VT82C686BState {
>>      SuperIOConfig superio_conf;
>>  };
>>
>> -#define TYPE_VT82C686B "VT82C686B"
>>  OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BState, VT82C686B)
>>
>>  static void superio_ioport_writeb(void *opaque, hwaddr addr, uint64_t data,
>> @@ -367,14 +366,6 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
>>                                  &vt82c->superio);
>>  }
>>
>> -ISABus *vt82c686b_isa_init(PCIBus *bus, int devfn)
>> -{
>> -    PCIDevice *d;
>> -
>> -    d = pci_create_simple_multifunction(bus, devfn, true, TYPE_VT82C686B);
>> -    return ISA_BUS(qdev_get_child_bus(DEVICE(d), "isa.0"));
>> -}
>> -
>>  static void via_class_init(ObjectClass *klass, void *data)
>>  {
>>      DeviceClass *dc = DEVICE_CLASS(klass);
>> diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
>> index 3b0489f781..60d2020033 100644
>> --- a/hw/mips/fuloong2e.c
>> +++ b/hw/mips/fuloong2e.c
>> @@ -240,7 +240,9 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
>>      ISABus *isa_bus;
>>      PCIDevice *dev;
>>
>> -    isa_bus = vt82c686b_isa_init(pci_bus, PCI_DEVFN(slot, 0));
>> +    dev = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(slot, 0), true,
>> +                                          TYPE_VT82C686B);
>
> Good cleanup.
>
> Preferably using TYPE_VT82C686B_ISA:
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
>> +    isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(dev), "isa.0"));
>>      assert(isa_bus);
>>      *p_isa_bus = isa_bus;
>>      /* Interrupt controller */
>> diff --git a/include/hw/isa/vt82c686.h b/include/hw/isa/vt82c686.h
>> index ff80a926dc..89e205a1be 100644
>> --- a/include/hw/isa/vt82c686.h
>> +++ b/include/hw/isa/vt82c686.h
>> @@ -1,13 +1,12 @@
>>  #ifndef HW_VT82C686_H
>>  #define HW_VT82C686_H
>>
>> -
>> +#define TYPE_VT82C686B "vt82c686b"
>>  #define TYPE_VT82C686B_SUPERIO "vt82c686b-superio"
>>  #define TYPE_VIA_AC97 "via-ac97"
>>  #define TYPE_VIA_MC97 "via-mc97"
>>
>>  /* vt82c686.c */
>> -ISABus *vt82c686b_isa_init(PCIBus * bus, int devfn);
>>  I2CBus *vt82c686b_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
>>                            qemu_irq sci_irq);
>>
>>
>
>

[PATCH 02/12] vt82c686: Rename AC97/MC97 parts from VT82C686B to VIA

[BALATON Zoltan via]

These parts are common between VT82C686B and VT8231 so can be shared
in the future. Rename them to VIA prefix accordingly.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/isa/vt82c686.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index b3170c70c3..2a0f85dea9 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -168,22 +168,22 @@ struct VT686PMState {
     uint32_t smb_io_base;
 };
 
-struct VT686AC97State {
+struct VIAAC97State {
     PCIDevice dev;
 };
 
-struct VT686MC97State {
+struct VIAMC97State {
     PCIDevice dev;
 };
 
 #define TYPE_VT82C686B_PM_DEVICE "VT82C686B_PM"
 OBJECT_DECLARE_SIMPLE_TYPE(VT686PMState, VT82C686B_PM_DEVICE)
 
-#define TYPE_VT82C686B_MC97_DEVICE "VT82C686B_MC97"
-OBJECT_DECLARE_SIMPLE_TYPE(VT686MC97State, VT82C686B_MC97_DEVICE)
+#define TYPE_VIA_MC97_DEVICE "VIA_MC97"
+OBJECT_DECLARE_SIMPLE_TYPE(VIAMC97State, VIA_MC97_DEVICE)
 
-#define TYPE_VT82C686B_AC97_DEVICE "VT82C686B_AC97"
-OBJECT_DECLARE_SIMPLE_TYPE(VT686AC97State, VT82C686B_AC97_DEVICE)
+#define TYPE_VIA_AC97_DEVICE "VIA_AC97"
+OBJECT_DECLARE_SIMPLE_TYPE(VIAAC97State, VIA_AC97_DEVICE)
 
 static void pm_update_sci(VT686PMState *s)
 {
@@ -260,7 +260,7 @@ static const VMStateDescription vmstate_acpi = {
 
 static void vt82c686b_ac97_realize(PCIDevice *dev, Error **errp)
 {
-    VT686AC97State *s = VT82C686B_AC97_DEVICE(dev);
+    VIAAC97State *s = VIA_AC97_DEVICE(dev);
     uint8_t *pci_conf = s->dev.config;
 
     pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_INVALIDATE |
@@ -274,7 +274,7 @@ void vt82c686b_ac97_init(PCIBus *bus, int devfn)
 {
     PCIDevice *dev;
 
-    dev = pci_new(devfn, TYPE_VT82C686B_AC97_DEVICE);
+    dev = pci_new(devfn, TYPE_VIA_AC97_DEVICE);
     pci_realize_and_unref(dev, bus, &error_fatal);
 }
 
@@ -293,9 +293,9 @@ static void via_ac97_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo via_ac97_info = {
-    .name          = TYPE_VT82C686B_AC97_DEVICE,
+    .name          = TYPE_VIA_AC97_DEVICE,
     .parent        = TYPE_PCI_DEVICE,
-    .instance_size = sizeof(VT686AC97State),
+    .instance_size = sizeof(VIAAC97State),
     .class_init    = via_ac97_class_init,
     .interfaces = (InterfaceInfo[]) {
         { INTERFACE_CONVENTIONAL_PCI_DEVICE },
@@ -305,7 +305,7 @@ static const TypeInfo via_ac97_info = {
 
 static void vt82c686b_mc97_realize(PCIDevice *dev, Error **errp)
 {
-    VT686MC97State *s = VT82C686B_MC97_DEVICE(dev);
+    VIAMC97State *s = VIA_MC97_DEVICE(dev);
     uint8_t *pci_conf = s->dev.config;
 
     pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_INVALIDATE |
@@ -318,7 +318,7 @@ void vt82c686b_mc97_init(PCIBus *bus, int devfn)
 {
     PCIDevice *dev;
 
-    dev = pci_new(devfn, TYPE_VT82C686B_MC97_DEVICE);
+    dev = pci_new(devfn, TYPE_VIA_MC97_DEVICE);
     pci_realize_and_unref(dev, bus, &error_fatal);
 }
 
@@ -337,9 +337,9 @@ static void via_mc97_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo via_mc97_info = {
-    .name          = TYPE_VT82C686B_MC97_DEVICE,
+    .name          = TYPE_VIA_MC97_DEVICE,
     .parent        = TYPE_PCI_DEVICE,
-    .instance_size = sizeof(VT686MC97State),
+    .instance_size = sizeof(VIAMC97State),
     .class_init    = via_mc97_class_init,
     .interfaces = (InterfaceInfo[]) {
         { INTERFACE_CONVENTIONAL_PCI_DEVICE },
-- 
2.21.3

Re: [PATCH 02/12] vt82c686: Rename AC97/MC97 parts from VT82C686B to VIA

[Philippe Mathieu-Daudé]

On 12/27/20 2:10 AM, BALATON Zoltan via wrote:
> These parts are common between VT82C686B and VT8231 so can be shared
> in the future. Rename them to VIA prefix accordingly.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/isa/vt82c686.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[PATCH 08/12] vt82c686: Remove vt82c686b_pm_init() function

[BALATON Zoltan via]

Also rename VT82C686B_PM to match other device names.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/isa/vt82c686.c         | 17 -----------------
 hw/mips/fuloong2e.c       |  5 ++++-
 include/hw/isa/vt82c686.h |  5 +----
 3 files changed, 5 insertions(+), 22 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 1c1239cade..b138838400 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -167,7 +167,6 @@ struct VT686PMState {
     uint32_t smb_io_base;
 };
 
-#define TYPE_VT82C686B_PM "VT82C686B_PM"
 OBJECT_DECLARE_SIMPLE_TYPE(VT686PMState, VT82C686B_PM)
 
 static void pm_update_sci(VT686PMState *s)
@@ -271,22 +270,6 @@ static void vt82c686b_pm_realize(PCIDevice *dev, Error **errp)
     acpi_pm1_cnt_init(&s->ar, &s->io, false, false, 2);
 }
 
-I2CBus *vt82c686b_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
-                          qemu_irq sci_irq)
-{
-    PCIDevice *dev;
-    VT686PMState *s;
-
-    dev = pci_new(devfn, TYPE_VT82C686B_PM);
-    qdev_prop_set_uint32(&dev->qdev, "smb_io_base", smb_io_base);
-
-    s = VT82C686B_PM(dev);
-
-    pci_realize_and_unref(dev, bus, &error_fatal);
-
-    return s->smb.smbus;
-}
-
 static Property via_pm_properties[] = {
     DEFINE_PROP_UINT32("smb_io_base", VT686PMState, smb_io_base, 0),
     DEFINE_PROP_END_OF_LIST(),
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index 60d2020033..d275038830 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -263,7 +263,10 @@ 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");
 
-    *i2c_bus = vt82c686b_pm_init(pci_bus, PCI_DEVFN(slot, 4), 0xeee1, NULL);
+    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);
+    *i2c_bus = I2C_BUS(qdev_get_child_bus(DEVICE(dev), "i2c"));
 
     /* Audio support */
     pci_create_simple(pci_bus, PCI_DEVFN(slot, 5), TYPE_VIA_AC97);
diff --git a/include/hw/isa/vt82c686.h b/include/hw/isa/vt82c686.h
index 89e205a1be..080ee8fc59 100644
--- a/include/hw/isa/vt82c686.h
+++ b/include/hw/isa/vt82c686.h
@@ -3,11 +3,8 @@
 
 #define TYPE_VT82C686B "vt82c686b"
 #define TYPE_VT82C686B_SUPERIO "vt82c686b-superio"
+#define TYPE_VT82C686B_PM "vt82c686b-pm"
 #define TYPE_VIA_AC97 "via-ac97"
 #define TYPE_VIA_MC97 "via-mc97"
 
-/* vt82c686.c */
-I2CBus *vt82c686b_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
-                          qemu_irq sci_irq);
-
 #endif
-- 
2.21.3

Re: [PATCH 08/12] vt82c686: Remove vt82c686b_pm_init() function

[Philippe Mathieu-Daudé]

On 12/27/20 2:10 AM, BALATON Zoltan via wrote:
> Also rename VT82C686B_PM to match other device names.

s/Also/Remove vt82c686b_pm_init() function and/

Preferably copying the subject in the description to
ease reading:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/isa/vt82c686.c         | 17 -----------------
>  hw/mips/fuloong2e.c       |  5 ++++-
>  include/hw/isa/vt82c686.h |  5 +----
>  3 files changed, 5 insertions(+), 22 deletions(-)

[PATCH 01/12] vt82c686: Add APM and ACPI dependencies for VT82C686

[BALATON Zoltan via]

Compiling vt82c686.c fails without APM and ACPI_PM functions. Add
dependency on these in Kconfig to fix this.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/isa/Kconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
index c7f07854f7..2ca2593ee6 100644
--- a/hw/isa/Kconfig
+++ b/hw/isa/Kconfig
@@ -47,6 +47,8 @@ config VT82C686
     select ACPI_SMBUS
     select SERIAL_ISA
     select FDC
+    select APM
+    select ACPI_X86
 
 config SMC37C669
     bool
-- 
2.21.3

Re: [PATCH 01/12] vt82c686: Add APM and ACPI dependencies for VT82C686

[Huacai Chen]

Hi, BALATON

On Sun, Dec 27, 2020 at 9:21 AM BALATON Zoltan <balaton@eik.bme.hu> wrote:
>
> Compiling vt82c686.c fails without APM and ACPI_PM functions. Add
> dependency on these in Kconfig to fix this.
>
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/isa/Kconfig | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
> index c7f07854f7..2ca2593ee6 100644
> --- a/hw/isa/Kconfig
> +++ b/hw/isa/Kconfig
> @@ -47,6 +47,8 @@ config VT82C686
>      select ACPI_SMBUS
>      select SERIAL_ISA
>      select FDC
> +    select APM
> +    select ACPI_X86
I feel a bit uncomfortable with ACPI_X86 in the MIPS code, can we just
select ACPI? And if that is not enough, can we select more options?

Huacai

>
>  config SMC37C669
>      bool
> --
> 2.21.3
>

Re: [PATCH 01/12] vt82c686: Add APM and ACPI dependencies for VT82C686

[BALATON Zoltan via]

Hello,

On Mon, 28 Dec 2020, Huacai Chen wrote:
> Hi, BALATON
>
> On Sun, Dec 27, 2020 at 9:21 AM BALATON Zoltan <balaton@eik.bme.hu> wrote:
>>
>> Compiling vt82c686.c fails without APM and ACPI_PM functions. Add
>> dependency on these in Kconfig to fix this.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/isa/Kconfig | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
>> index c7f07854f7..2ca2593ee6 100644
>> --- a/hw/isa/Kconfig
>> +++ b/hw/isa/Kconfig
>> @@ -47,6 +47,8 @@ config VT82C686
>>      select ACPI_SMBUS
>>      select SERIAL_ISA
>>      select FDC
>> +    select APM
>> +    select ACPI_X86
> I feel a bit uncomfortable with ACPI_X86 in the MIPS code, can we just
> select ACPI? And if that is not enough, can we select more options?

This patch is not new, I've tried submitting it before but got rejeceted 
for similar reason:

https://lists.nongnu.org/archive/html/qemu-devel/2019-03/msg03428.html

Then Philippe said he had a better alternative but it's still not fixed in 
master so this patch is needed and you likely already depend on X86 
without knowing as something is pulling these in for MIPS. This can be 
reproduced e,g, by adding this device to PPC as:

diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
index d235a096c6..90b53d40c2 100644
--- a/hw/ppc/Kconfig
+++ b/hw/ppc/Kconfig
@@ -64,6 +64,7 @@ config SAM460EX
      select SMBUS_EEPROM
      select USB_EHCI_SYSBUS
      select USB_OHCI
+    select VT82C686

  config PREP
      bool

then compiling --target-list=ppc-softmmu
Even after:

diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
index c7f07854f7..75986671b9 100644
--- a/hw/isa/Kconfig
+++ b/hw/isa/Kconfig
@@ -47,6 +47,8 @@ config VT82C686
      select ACPI_SMBUS
      select SERIAL_ISA
      select FDC
+    select APM
+    select ACPI

  config SMC37C669
      bool

I get:

[] Linking target qemu-system-ppc
FAILED: qemu-system-ppc
ld: libcommon.fa.p/hw_isa_vt82c686.c.o: in function `vt82c686b_pm_realize':
hw/isa/vt82c686.c:378: undefined reference to `acpi_pm_tmr_init'
ld: hw/isa/vt82c686.c:379: undefined reference to `acpi_pm1_evt_init'
ld: libcommon.fa.p/hw_isa_vt82c686.c.o: in function `pm_update_sci':
hw/isa/vt82c686.c:192: undefined reference to `acpi_pm1_evt_get_sts'
ld: libcommon.fa.p/hw_isa_vt82c686.c.o: in function `vt82c686b_pm_realize':
hw/isa/vt82c686.c:380: undefined reference to `acpi_pm1_cnt_init'
ld: libcommon.fa.p/hw_isa_vt82c686.c.o: in function `pm_update_sci':
hw/isa/vt82c686.c:200: undefined reference to `acpi_pm_tmr_update'
collect2: error: ld returned 1 exit status

So my patch just makes existing dependencies explicit and allows this to 
build but I'm OK with any other fix you propose that fixes the above case 
as that's how I'll try to use this in the future. (I did look at this when 
first found it and concluded that I could not make a better fix than 
depending on ACPI_X86 here. I forgot the details but it was way more work 
than I want to take up for this so please propose a better fix if you 
can't accept this patch.)

Maybe Philippe remembers some more.

Regards,
BALATON Zoltan

Re: [PATCH 01/12] vt82c686: Add APM and ACPI dependencies for VT82C686

[chen huacai]

OK, just do it as Philippe suggested.

Huacai

On Mon, Dec 28, 2020 at 9:42 AM BALATON Zoltan via
<qemu-devel@nongnu.org> wrote:
>
> Hello,
>
> On Mon, 28 Dec 2020, Huacai Chen wrote:
> > Hi, BALATON
> >
> > On Sun, Dec 27, 2020 at 9:21 AM BALATON Zoltan <balaton@eik.bme.hu> wrote:
> >>
> >> Compiling vt82c686.c fails without APM and ACPI_PM functions. Add
> >> dependency on these in Kconfig to fix this.
> >>
> >> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> >> ---
> >>  hw/isa/Kconfig | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
> >> index c7f07854f7..2ca2593ee6 100644
> >> --- a/hw/isa/Kconfig
> >> +++ b/hw/isa/Kconfig
> >> @@ -47,6 +47,8 @@ config VT82C686
> >>      select ACPI_SMBUS
> >>      select SERIAL_ISA
> >>      select FDC
> >> +    select APM
> >> +    select ACPI_X86
> > I feel a bit uncomfortable with ACPI_X86 in the MIPS code, can we just
> > select ACPI? And if that is not enough, can we select more options?
>
> This patch is not new, I've tried submitting it before but got rejeceted
> for similar reason:
>
> https://lists.nongnu.org/archive/html/qemu-devel/2019-03/msg03428.html
>
> Then Philippe said he had a better alternative but it's still not fixed in
> master so this patch is needed and you likely already depend on X86
> without knowing as something is pulling these in for MIPS. This can be
> reproduced e,g, by adding this device to PPC as:
>
> diff --git a/hw/ppc/Kconfig b/hw/ppc/Kconfig
> index d235a096c6..90b53d40c2 100644
> --- a/hw/ppc/Kconfig
> +++ b/hw/ppc/Kconfig
> @@ -64,6 +64,7 @@ config SAM460EX
>       select SMBUS_EEPROM
>       select USB_EHCI_SYSBUS
>       select USB_OHCI
> +    select VT82C686
>
>   config PREP
>       bool
>
> then compiling --target-list=ppc-softmmu
> Even after:
>
> diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
> index c7f07854f7..75986671b9 100644
> --- a/hw/isa/Kconfig
> +++ b/hw/isa/Kconfig
> @@ -47,6 +47,8 @@ config VT82C686
>       select ACPI_SMBUS
>       select SERIAL_ISA
>       select FDC
> +    select APM
> +    select ACPI
>
>   config SMC37C669
>       bool
>
> I get:
>
> [] Linking target qemu-system-ppc
> FAILED: qemu-system-ppc
> ld: libcommon.fa.p/hw_isa_vt82c686.c.o: in function `vt82c686b_pm_realize':
> hw/isa/vt82c686.c:378: undefined reference to `acpi_pm_tmr_init'
> ld: hw/isa/vt82c686.c:379: undefined reference to `acpi_pm1_evt_init'
> ld: libcommon.fa.p/hw_isa_vt82c686.c.o: in function `pm_update_sci':
> hw/isa/vt82c686.c:192: undefined reference to `acpi_pm1_evt_get_sts'
> ld: libcommon.fa.p/hw_isa_vt82c686.c.o: in function `vt82c686b_pm_realize':
> hw/isa/vt82c686.c:380: undefined reference to `acpi_pm1_cnt_init'
> ld: libcommon.fa.p/hw_isa_vt82c686.c.o: in function `pm_update_sci':
> hw/isa/vt82c686.c:200: undefined reference to `acpi_pm_tmr_update'
> collect2: error: ld returned 1 exit status
>
> So my patch just makes existing dependencies explicit and allows this to
> build but I'm OK with any other fix you propose that fixes the above case
> as that's how I'll try to use this in the future. (I did look at this when
> first found it and concluded that I could not make a better fix than
> depending on ACPI_X86 here. I forgot the details but it was way more work
> than I want to take up for this so please propose a better fix if you
> can't accept this patch.)
>
> Maybe Philippe remembers some more.
>
> Regards,
> BALATON Zoltan
>


-- 
Huacai Chen

[PATCH 06/12] audio/via-ac97: Simplify code and set user_creatable to false

[BALATON Zoltan via]

Remove some unneded, empty code and set user_creatable to false
(besides being not implemented yet, so does nothing anyway) it's also
normally part of VIA south bridge chips so no need to confuse users
showing them these devices.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/audio/via-ac97.c | 51 +++++++++++++++++----------------------------
 1 file changed, 19 insertions(+), 32 deletions(-)

diff --git a/hw/audio/via-ac97.c b/hw/audio/via-ac97.c
index e617416ff7..6d556f74fc 100644
--- a/hw/audio/via-ac97.c
+++ b/hw/audio/via-ac97.c
@@ -13,27 +13,13 @@
 #include "hw/isa/vt82c686.h"
 #include "hw/pci/pci.h"
 
-struct VIAAC97State {
-    PCIDevice dev;
-};
-
-struct VIAMC97State {
-    PCIDevice dev;
-};
-
-OBJECT_DECLARE_SIMPLE_TYPE(VIAAC97State, VIA_AC97)
-OBJECT_DECLARE_SIMPLE_TYPE(VIAMC97State, VIA_MC97)
-
-static void via_ac97_realize(PCIDevice *dev, Error **errp)
+static void via_ac97_realize(PCIDevice *pci_dev, Error **errp)
 {
-    VIAAC97State *s = VIA_AC97(dev);
-    uint8_t *pci_conf = s->dev.config;
-
-    pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_INVALIDATE |
-                 PCI_COMMAND_PARITY);
-    pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_CAP_LIST |
-                 PCI_STATUS_DEVSEL_MEDIUM);
-    pci_set_long(pci_conf + PCI_INTERRUPT_PIN, 0x03);
+    pci_set_word(pci_dev->config + PCI_COMMAND,
+                 PCI_COMMAND_INVALIDATE | PCI_COMMAND_PARITY);
+    pci_set_word(pci_dev->config + PCI_STATUS,
+                 PCI_STATUS_CAP_LIST | PCI_STATUS_DEVSEL_MEDIUM);
+    pci_set_long(pci_dev->config + PCI_INTERRUPT_PIN, 0x03);
 }
 
 static void via_ac97_class_init(ObjectClass *klass, void *data)
@@ -47,13 +33,15 @@ static void via_ac97_class_init(ObjectClass *klass, void *data)
     k->revision = 0x50;
     k->class_id = PCI_CLASS_MULTIMEDIA_AUDIO;
     set_bit(DEVICE_CATEGORY_SOUND, dc->categories);
-    dc->desc = "AC97";
+    dc->desc = "VIA AC97";
+    /* Reason: Part of a south bridge chip */
+    dc->user_creatable = false;
 }
 
 static const TypeInfo via_ac97_info = {
     .name          = TYPE_VIA_AC97,
     .parent        = TYPE_PCI_DEVICE,
-    .instance_size = sizeof(VIAAC97State),
+    .instance_size = sizeof(PCIDevice),
     .class_init    = via_ac97_class_init,
     .interfaces = (InterfaceInfo[]) {
         { INTERFACE_CONVENTIONAL_PCI_DEVICE },
@@ -61,15 +49,12 @@ static const TypeInfo via_ac97_info = {
     },
 };
 
-static void via_mc97_realize(PCIDevice *dev, Error **errp)
+static void via_mc97_realize(PCIDevice *pci_dev, Error **errp)
 {
-    VIAMC97State *s = VIA_MC97(dev);
-    uint8_t *pci_conf = s->dev.config;
-
-    pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_INVALIDATE |
-                 PCI_COMMAND_VGA_PALETTE);
-    pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM);
-    pci_set_long(pci_conf + PCI_INTERRUPT_PIN, 0x03);
+    pci_set_word(pci_dev->config + PCI_COMMAND,
+                 PCI_COMMAND_INVALIDATE | PCI_COMMAND_VGA_PALETTE);
+    pci_set_word(pci_dev->config + PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM);
+    pci_set_long(pci_dev->config + PCI_INTERRUPT_PIN, 0x03);
 }
 
 static void via_mc97_class_init(ObjectClass *klass, void *data)
@@ -83,13 +68,15 @@ static void via_mc97_class_init(ObjectClass *klass, void *data)
     k->class_id = PCI_CLASS_COMMUNICATION_OTHER;
     k->revision = 0x30;
     set_bit(DEVICE_CATEGORY_NETWORK, dc->categories);
-    dc->desc = "MC97";
+    dc->desc = "VIA MC97";
+    /* Reason: Part of a south bridge chip */
+    dc->user_creatable = false;
 }
 
 static const TypeInfo via_mc97_info = {
     .name          = TYPE_VIA_MC97,
     .parent        = TYPE_PCI_DEVICE,
-    .instance_size = sizeof(VIAMC97State),
+    .instance_size = sizeof(PCIDevice),
     .class_init    = via_mc97_class_init,
     .interfaces = (InterfaceInfo[]) {
         { INTERFACE_CONVENTIONAL_PCI_DEVICE },
-- 
2.21.3

Re: [PATCH 06/12] audio/via-ac97: Simplify code and set user_creatable to false

[Philippe Mathieu-Daudé]

Hi Zoltan,

On 12/27/20 2:10 AM, BALATON Zoltan via wrote:
> Remove some unneded, empty code and set user_creatable to false
> (besides being not implemented yet, so does nothing anyway) it's also
> normally part of VIA south bridge chips so no need to confuse users
> showing them these devices.

After contributing during more than 8 years you should know we try
to avoid to do multiples changes in the same patch ;)

> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/audio/via-ac97.c | 51 +++++++++++++++++----------------------------
>  1 file changed, 19 insertions(+), 32 deletions(-)

Re: [PATCH 06/12] audio/via-ac97: Simplify code and set user_creatable to false

[BALATON Zoltan via]

[-- Attachment #1: Type: text/plain, Size: 1198 bytes --]

On Sun, 27 Dec 2020, Philippe Mathieu-Daudé wrote:
> Hi Zoltan,
>
> On 12/27/20 2:10 AM, BALATON Zoltan via wrote:
>> Remove some unneded, empty code and set user_creatable to false
>> (besides being not implemented yet, so does nothing anyway) it's also
>> normally part of VIA south bridge chips so no need to confuse users
>> showing them these devices.
>
> After contributing during more than 8 years you should know we try
> to avoid to do multiples changes in the same patch ;)

Yes, in my understanding patches should be split if

- it makes bisecting easier or
- makes reviewing easier

Which of the above appies in this case? I think these are changes that 
should neither brake anything (as this device doesn't don't do anyting 
yet) and adding user_creatable false is a one line change that's easy to 
review even with the other changes. If you insist I can split this into 
two but I didn't think that would be any better and the series was long 
enough already.

Regards,
BALATON Zoltan

>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/audio/via-ac97.c | 51 +++++++++++++++++----------------------------
>>  1 file changed, 19 insertions(+), 32 deletions(-)
>
>

[PATCH 04/12] vt82c686: Remove vt82c686b_[am]c97_init() functions

[BALATON Zoltan via]

These are legacy init functions that are just equivalent to directly
calling pci_create_simple so do that instead. Also rename objects to
lower case via-ac97 and via-mc97 matching naming of other devices.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/isa/vt82c686.c         | 27 ++++-----------------------
 hw/mips/fuloong2e.c       |  4 ++--
 include/hw/isa/vt82c686.h |  4 ++--
 3 files changed, 8 insertions(+), 27 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 1be1169f83..de772262bc 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -179,12 +179,6 @@ struct VIAMC97State {
 #define TYPE_VT82C686B_PM "VT82C686B_PM"
 OBJECT_DECLARE_SIMPLE_TYPE(VT686PMState, VT82C686B_PM)
 
-#define TYPE_VIA_MC97 "VIA_MC97"
-OBJECT_DECLARE_SIMPLE_TYPE(VIAMC97State, VIA_MC97)
-
-#define TYPE_VIA_AC97 "VIA_AC97"
-OBJECT_DECLARE_SIMPLE_TYPE(VIAAC97State, VIA_AC97)
-
 static void pm_update_sci(VT686PMState *s)
 {
     int sci_level, pmsts;
@@ -254,10 +248,13 @@ static const VMStateDescription vmstate_acpi = {
 };
 
 /*
- * TODO: vt82c686b_ac97_init() and vt82c686b_mc97_init()
+ * TODO: VIA_AC97 and VIA_MC97
  * just register a PCI device now, functionalities will be implemented later.
  */
 
+OBJECT_DECLARE_SIMPLE_TYPE(VIAMC97State, VIA_MC97)
+OBJECT_DECLARE_SIMPLE_TYPE(VIAAC97State, VIA_AC97)
+
 static void vt82c686b_ac97_realize(PCIDevice *dev, Error **errp)
 {
     VIAAC97State *s = VIA_AC97(dev);
@@ -270,14 +267,6 @@ static void vt82c686b_ac97_realize(PCIDevice *dev, Error **errp)
     pci_set_long(pci_conf + PCI_INTERRUPT_PIN, 0x03);
 }
 
-void vt82c686b_ac97_init(PCIBus *bus, int devfn)
-{
-    PCIDevice *dev;
-
-    dev = pci_new(devfn, TYPE_VIA_AC97);
-    pci_realize_and_unref(dev, bus, &error_fatal);
-}
-
 static void via_ac97_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
@@ -314,14 +303,6 @@ static void vt82c686b_mc97_realize(PCIDevice *dev, Error **errp)
     pci_set_long(pci_conf + PCI_INTERRUPT_PIN, 0x03);
 }
 
-void vt82c686b_mc97_init(PCIBus *bus, int devfn)
-{
-    PCIDevice *dev;
-
-    dev = pci_new(devfn, TYPE_VIA_MC97);
-    pci_realize_and_unref(dev, bus, &error_fatal);
-}
-
 static void via_mc97_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index f0733e87b7..3b0489f781 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -264,8 +264,8 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
     *i2c_bus = vt82c686b_pm_init(pci_bus, PCI_DEVFN(slot, 4), 0xeee1, NULL);
 
     /* Audio support */
-    vt82c686b_ac97_init(pci_bus, PCI_DEVFN(slot, 5));
-    vt82c686b_mc97_init(pci_bus, PCI_DEVFN(slot, 6));
+    pci_create_simple(pci_bus, PCI_DEVFN(slot, 5), TYPE_VIA_AC97);
+    pci_create_simple(pci_bus, PCI_DEVFN(slot, 6), TYPE_VIA_MC97);
 }
 
 /* Network support */
diff --git a/include/hw/isa/vt82c686.h b/include/hw/isa/vt82c686.h
index f23f45dfb1..ff80a926dc 100644
--- a/include/hw/isa/vt82c686.h
+++ b/include/hw/isa/vt82c686.h
@@ -3,11 +3,11 @@
 
 
 #define TYPE_VT82C686B_SUPERIO "vt82c686b-superio"
+#define TYPE_VIA_AC97 "via-ac97"
+#define TYPE_VIA_MC97 "via-mc97"
 
 /* vt82c686.c */
 ISABus *vt82c686b_isa_init(PCIBus * bus, int devfn);
-void vt82c686b_ac97_init(PCIBus *bus, int devfn);
-void vt82c686b_mc97_init(PCIBus *bus, int devfn);
 I2CBus *vt82c686b_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
                           qemu_irq sci_irq);
 
-- 
2.21.3

Re: [PATCH 04/12] vt82c686: Remove vt82c686b_[am]c97_init() functions

[Philippe Mathieu-Daudé]

On 12/27/20 2:10 AM, BALATON Zoltan via wrote:
> These are legacy init functions that are just equivalent to directly
> calling pci_create_simple so do that instead. Also rename objects to
> lower case via-ac97 and via-mc97 matching naming of other devices.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/isa/vt82c686.c         | 27 ++++-----------------------
>  hw/mips/fuloong2e.c       |  4 ++--
>  include/hw/isa/vt82c686.h |  4 ++--
>  3 files changed, 8 insertions(+), 27 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[PATCH 03/12] vt82c686: Remove unnecessary _DEVICE suffix from type macros

[BALATON Zoltan via]

There's no reason to suffix everything with _DEVICE when the names are
already unique without it and shorter names are more readable.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/isa/vt82c686.c | 48 +++++++++++++++++++++++------------------------
 1 file changed, 23 insertions(+), 25 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 2a0f85dea9..1be1169f83 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -49,8 +49,8 @@ struct VT82C686BState {
     SuperIOConfig superio_conf;
 };
 
-#define TYPE_VT82C686B_DEVICE "VT82C686B"
-OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BState, VT82C686B_DEVICE)
+#define TYPE_VT82C686B "VT82C686B"
+OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BState, VT82C686B)
 
 static void superio_ioport_writeb(void *opaque, hwaddr addr, uint64_t data,
                                   unsigned size)
@@ -117,7 +117,7 @@ static const MemoryRegionOps superio_ops = {
 
 static void vt82c686b_isa_reset(DeviceState *dev)
 {
-    VT82C686BState *vt82c = VT82C686B_DEVICE(dev);
+    VT82C686BState *vt82c = VT82C686B(dev);
     uint8_t *pci_conf = vt82c->dev.config;
 
     pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x000000c0);
@@ -146,7 +146,7 @@ static void vt82c686b_isa_reset(DeviceState *dev)
 static void vt82c686b_write_config(PCIDevice *d, uint32_t address,
                                    uint32_t val, int len)
 {
-    VT82C686BState *vt686 = VT82C686B_DEVICE(d);
+    VT82C686BState *vt686 = VT82C686B(d);
 
     DPRINTF("vt82c686b_write_config  address 0x%x  val 0x%x len 0x%x\n",
            address, val, len);
@@ -176,14 +176,14 @@ struct VIAMC97State {
     PCIDevice dev;
 };
 
-#define TYPE_VT82C686B_PM_DEVICE "VT82C686B_PM"
-OBJECT_DECLARE_SIMPLE_TYPE(VT686PMState, VT82C686B_PM_DEVICE)
+#define TYPE_VT82C686B_PM "VT82C686B_PM"
+OBJECT_DECLARE_SIMPLE_TYPE(VT686PMState, VT82C686B_PM)
 
-#define TYPE_VIA_MC97_DEVICE "VIA_MC97"
-OBJECT_DECLARE_SIMPLE_TYPE(VIAMC97State, VIA_MC97_DEVICE)
+#define TYPE_VIA_MC97 "VIA_MC97"
+OBJECT_DECLARE_SIMPLE_TYPE(VIAMC97State, VIA_MC97)
 
-#define TYPE_VIA_AC97_DEVICE "VIA_AC97"
-OBJECT_DECLARE_SIMPLE_TYPE(VIAAC97State, VIA_AC97_DEVICE)
+#define TYPE_VIA_AC97 "VIA_AC97"
+OBJECT_DECLARE_SIMPLE_TYPE(VIAAC97State, VIA_AC97)
 
 static void pm_update_sci(VT686PMState *s)
 {
@@ -260,7 +260,7 @@ static const VMStateDescription vmstate_acpi = {
 
 static void vt82c686b_ac97_realize(PCIDevice *dev, Error **errp)
 {
-    VIAAC97State *s = VIA_AC97_DEVICE(dev);
+    VIAAC97State *s = VIA_AC97(dev);
     uint8_t *pci_conf = s->dev.config;
 
     pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_INVALIDATE |
@@ -274,7 +274,7 @@ void vt82c686b_ac97_init(PCIBus *bus, int devfn)
 {
     PCIDevice *dev;
 
-    dev = pci_new(devfn, TYPE_VIA_AC97_DEVICE);
+    dev = pci_new(devfn, TYPE_VIA_AC97);
     pci_realize_and_unref(dev, bus, &error_fatal);
 }
 
@@ -293,7 +293,7 @@ static void via_ac97_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo via_ac97_info = {
-    .name          = TYPE_VIA_AC97_DEVICE,
+    .name          = TYPE_VIA_AC97,
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(VIAAC97State),
     .class_init    = via_ac97_class_init,
@@ -305,7 +305,7 @@ static const TypeInfo via_ac97_info = {
 
 static void vt82c686b_mc97_realize(PCIDevice *dev, Error **errp)
 {
-    VIAMC97State *s = VIA_MC97_DEVICE(dev);
+    VIAMC97State *s = VIA_MC97(dev);
     uint8_t *pci_conf = s->dev.config;
 
     pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_INVALIDATE |
@@ -318,7 +318,7 @@ void vt82c686b_mc97_init(PCIBus *bus, int devfn)
 {
     PCIDevice *dev;
 
-    dev = pci_new(devfn, TYPE_VIA_MC97_DEVICE);
+    dev = pci_new(devfn, TYPE_VIA_MC97);
     pci_realize_and_unref(dev, bus, &error_fatal);
 }
 
@@ -337,7 +337,7 @@ static void via_mc97_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo via_mc97_info = {
-    .name          = TYPE_VIA_MC97_DEVICE,
+    .name          = TYPE_VIA_MC97,
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(VIAMC97State),
     .class_init    = via_mc97_class_init,
@@ -350,7 +350,7 @@ static const TypeInfo via_mc97_info = {
 /* vt82c686 pm init */
 static void vt82c686b_pm_realize(PCIDevice *dev, Error **errp)
 {
-    VT686PMState *s = VT82C686B_PM_DEVICE(dev);
+    VT686PMState *s = VT82C686B_PM(dev);
     uint8_t *pci_conf;
 
     pci_conf = s->dev.config;
@@ -386,10 +386,10 @@ I2CBus *vt82c686b_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,
     PCIDevice *dev;
     VT686PMState *s;
 
-    dev = pci_new(devfn, TYPE_VT82C686B_PM_DEVICE);
+    dev = pci_new(devfn, TYPE_VT82C686B_PM);
     qdev_prop_set_uint32(&dev->qdev, "smb_io_base", smb_io_base);
 
-    s = VT82C686B_PM_DEVICE(dev);
+    s = VT82C686B_PM(dev);
 
     pci_realize_and_unref(dev, bus, &error_fatal);
 
@@ -419,7 +419,7 @@ static void via_pm_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo via_pm_info = {
-    .name          = TYPE_VT82C686B_PM_DEVICE,
+    .name          = TYPE_VT82C686B_PM,
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(VT686PMState),
     .class_init    = via_pm_class_init,
@@ -442,7 +442,7 @@ static const VMStateDescription vmstate_via = {
 /* init the PCI-to-ISA bridge */
 static void vt82c686b_realize(PCIDevice *d, Error **errp)
 {
-    VT82C686BState *vt82c = VT82C686B_DEVICE(d);
+    VT82C686BState *vt82c = VT82C686B(d);
     uint8_t *pci_conf;
     ISABus *isa_bus;
     uint8_t *wmask;
@@ -479,9 +479,7 @@ ISABus *vt82c686b_isa_init(PCIBus *bus, int devfn)
 {
     PCIDevice *d;
 
-    d = pci_create_simple_multifunction(bus, devfn, true,
-                                        TYPE_VT82C686B_DEVICE);
-
+    d = pci_create_simple_multifunction(bus, devfn, true, TYPE_VT82C686B);
     return ISA_BUS(qdev_get_child_bus(DEVICE(d), "isa.0"));
 }
 
@@ -507,7 +505,7 @@ static void via_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo via_info = {
-    .name          = TYPE_VT82C686B_DEVICE,
+    .name          = TYPE_VT82C686B,
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(VT82C686BState),
     .class_init    = via_class_init,
-- 
2.21.3

Re: [PATCH 03/12] vt82c686: Remove unnecessary _DEVICE suffix from type macros

[Philippe Mathieu-Daudé]

On 12/27/20 2:10 AM, BALATON Zoltan via wrote:
> There's no reason to suffix everything with _DEVICE when the names are
> already unique without it and shorter names are more readable.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/isa/vt82c686.c | 48 +++++++++++++++++++++++------------------------
>  1 file changed, 23 insertions(+), 25 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 03/12] vt82c686: Remove unnecessary _DEVICE suffix from type macros

[Philippe Mathieu-Daudé]

On 12/27/20 2:10 AM, BALATON Zoltan via wrote:
> There's no reason to suffix everything with _DEVICE when the names are
> already unique without it and shorter names are more readable.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/isa/vt82c686.c | 48 +++++++++++++++++++++++------------------------
>  1 file changed, 23 insertions(+), 25 deletions(-)
> 
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 2a0f85dea9..1be1169f83 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -49,8 +49,8 @@ struct VT82C686BState {
>      SuperIOConfig superio_conf;
>  };
>  
> -#define TYPE_VT82C686B_DEVICE "VT82C686B"
> -OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BState, VT82C686B_DEVICE)
> +#define TYPE_VT82C686B "VT82C686B"
> +OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BState, VT82C686B)

Can we name this one VT82C686B_ISA?

Re: [PATCH 03/12] vt82c686: Remove unnecessary _DEVICE suffix from type macros

[BALATON Zoltan via]

[-- Attachment #1: Type: text/plain, Size: 1116 bytes --]

On Sun, 27 Dec 2020, Philippe Mathieu-Daudé wrote:
> On 12/27/20 2:10 AM, BALATON Zoltan via wrote:
>> There's no reason to suffix everything with _DEVICE when the names are
>> already unique without it and shorter names are more readable.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/isa/vt82c686.c | 48 +++++++++++++++++++++++------------------------
>>  1 file changed, 23 insertions(+), 25 deletions(-)
>>
>> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
>> index 2a0f85dea9..1be1169f83 100644
>> --- a/hw/isa/vt82c686.c
>> +++ b/hw/isa/vt82c686.c
>> @@ -49,8 +49,8 @@ struct VT82C686BState {
>>      SuperIOConfig superio_conf;
>>  };
>>
>> -#define TYPE_VT82C686B_DEVICE "VT82C686B"
>> -OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BState, VT82C686B_DEVICE)
>> +#define TYPE_VT82C686B "VT82C686B"
>> +OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BState, VT82C686B)
>
> Can we name this one VT82C686B_ISA?

Yes, actually this is coming up in later patches adding VT8231 but I still 
need to think about that. Once I clean that up but maybe I can do the 
rename already here.

Regards,
BALATON Zoltan

[PATCH 11/12] vt82c686: Rename some functions to better show where they belong

[BALATON Zoltan via]

This groups identifiers related to the ISA bridge part and superio
part also in their naming.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/isa/vt82c686.c         | 48 ++++++++++++++++++---------------------
 hw/mips/fuloong2e.c       |  2 +-
 include/hw/isa/vt82c686.h |  2 +-
 3 files changed, 24 insertions(+), 28 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 6dff2bc67d..698627d1b5 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -36,10 +36,10 @@ struct VT82C686BState {
     SuperIOConfig superio_conf;
 };
 
-OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BState, VT82C686B)
+OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BState, VT82C686B_ISA)
 
-static void superio_ioport_writeb(void *opaque, hwaddr addr, uint64_t data,
-                                  unsigned size)
+static void vt82c686b_superio_writeb(void *opaque, hwaddr addr, uint64_t data,
+                                     unsigned size)
 {
     SuperIOConfig *superio_conf = opaque;
 
@@ -72,7 +72,8 @@ static void superio_ioport_writeb(void *opaque, hwaddr addr, uint64_t data,
     }
 }
 
-static uint64_t superio_ioport_readb(void *opaque, hwaddr addr, unsigned size)
+static uint64_t vt82c686b_superio_readb(void *opaque, hwaddr addr,
+                                        unsigned size)
 {
     SuperIOConfig *superio_conf = opaque;
     uint8_t val = superio_conf->config[superio_conf->index];
@@ -81,9 +82,9 @@ static uint64_t superio_ioport_readb(void *opaque, hwaddr addr, unsigned size)
     return val;
 }
 
-static const MemoryRegionOps superio_ops = {
-    .read = superio_ioport_readb,
-    .write = superio_ioport_writeb,
+static const MemoryRegionOps vt82c686b_superio_ops = {
+    .read = vt82c686b_superio_readb,
+    .write = vt82c686b_superio_writeb,
     .endianness = DEVICE_NATIVE_ENDIAN,
     .impl = {
         .min_access_size = 1,
@@ -93,7 +94,7 @@ static const MemoryRegionOps superio_ops = {
 
 static void vt82c686b_isa_reset(DeviceState *dev)
 {
-    VT82C686BState *vt82c = VT82C686B(dev);
+    VT82C686BState *vt82c = VT82C686B_ISA(dev);
     uint8_t *pci_conf = vt82c->dev.config;
 
     pci_set_long(pci_conf + PCI_CAPABILITY_LIST, 0x000000c0);
@@ -118,11 +119,10 @@ static void vt82c686b_isa_reset(DeviceState *dev)
     vt82c->superio_conf.config[0xe8] = 0xbe;
 }
 
-/* write config pci function0 registers. PCI-ISA bridge */
-static void vt82c686b_write_config(PCIDevice *d, uint32_t addr,
+static void vt82c686b_isa_write_config(PCIDevice *d, uint32_t addr,
                                    uint32_t val, int len)
 {
-    VT82C686BState *vt686 = VT82C686B(d);
+    VT82C686BState *vt686 = VT82C686B_ISA(d);
 
     trace_via_isa_write(addr, val, len);
     pci_default_write_config(d, addr, val, len);
@@ -284,10 +284,9 @@ static const VMStateDescription vmstate_via = {
     }
 };
 
-/* init the PCI-to-ISA bridge */
-static void vt82c686b_realize(PCIDevice *d, Error **errp)
+static void vt82c686b_isa_realize(PCIDevice *d, Error **errp)
 {
-    VT82C686BState *vt82c = VT82C686B(d);
+    VT82C686BState *vt82c = VT82C686B_ISA(d);
     uint8_t *pci_conf;
     ISABus *isa_bus;
     uint8_t *wmask;
@@ -309,7 +308,7 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
         }
     }
 
-    memory_region_init_io(&vt82c->superio, OBJECT(d), &superio_ops,
+    memory_region_init_io(&vt82c->superio, OBJECT(d), &vt82c686b_superio_ops,
                           &vt82c->superio_conf, "superio", 2);
     memory_region_set_enabled(&vt82c->superio, false);
     /*
@@ -320,13 +319,13 @@ static void vt82c686b_realize(PCIDevice *d, Error **errp)
                                 &vt82c->superio);
 }
 
-static void via_class_init(ObjectClass *klass, void *data)
+static void via_isa_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
 
-    k->realize = vt82c686b_realize;
-    k->config_write = vt82c686b_write_config;
+    k->realize = vt82c686b_isa_realize;
+    k->config_write = vt82c686b_isa_write_config;
     k->vendor_id = PCI_VENDOR_ID_VIA;
     k->device_id = PCI_DEVICE_ID_VIA_ISA_BRIDGE;
     k->class_id = PCI_CLASS_BRIDGE_ISA;
@@ -334,18 +333,15 @@ static void via_class_init(ObjectClass *klass, void *data)
     dc->reset = vt82c686b_isa_reset;
     dc->desc = "ISA bridge";
     dc->vmsd = &vmstate_via;
-    /*
-     * Reason: part of VIA VT82C686 southbridge, needs to be wired up,
-     * e.g. by mips_fuloong2e_init()
-     */
+    /* Reason: Part of VIA southbridge, needs to be wired up by board code */
     dc->user_creatable = false;
 }
 
-static const TypeInfo via_info = {
-    .name          = TYPE_VT82C686B,
+static const TypeInfo via_isa_info = {
+    .name          = TYPE_VT82C686B_ISA,
     .parent        = TYPE_PCI_DEVICE,
     .instance_size = sizeof(VT82C686BState),
-    .class_init    = via_class_init,
+    .class_init    = via_isa_class_init,
     .interfaces = (InterfaceInfo[]) {
         { INTERFACE_CONVENTIONAL_PCI_DEVICE },
         { },
@@ -374,7 +370,7 @@ static void vt82c686b_register_types(void)
 {
     type_register_static(&via_pm_info);
     type_register_static(&via_superio_info);
-    type_register_static(&via_info);
+    type_register_static(&via_isa_info);
 }
 
 type_init(vt82c686b_register_types)
diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
index d275038830..a2b69a3a7a 100644
--- a/hw/mips/fuloong2e.c
+++ b/hw/mips/fuloong2e.c
@@ -241,7 +241,7 @@ static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq intc,
     PCIDevice *dev;
 
     dev = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(slot, 0), true,
-                                          TYPE_VT82C686B);
+                                          TYPE_VT82C686B_ISA);
     isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(dev), "isa.0"));
     assert(isa_bus);
     *p_isa_bus = isa_bus;
diff --git a/include/hw/isa/vt82c686.h b/include/hw/isa/vt82c686.h
index 080ee8fc59..5b0a1ffe72 100644
--- a/include/hw/isa/vt82c686.h
+++ b/include/hw/isa/vt82c686.h
@@ -1,7 +1,7 @@
 #ifndef HW_VT82C686_H
 #define HW_VT82C686_H
 
-#define TYPE_VT82C686B "vt82c686b"
+#define TYPE_VT82C686B_ISA "vt82c686b-isa"
 #define TYPE_VT82C686B_SUPERIO "vt82c686b-superio"
 #define TYPE_VT82C686B_PM "vt82c686b-pm"
 #define TYPE_VIA_AC97 "via-ac97"
-- 
2.21.3

Re: [PATCH 11/12] vt82c686: Rename some functions to better show where they belong

[Philippe Mathieu-Daudé]

On 12/27/20 2:10 AM, BALATON Zoltan via wrote:
> This groups identifiers related to the ISA bridge part and superio
> part also in their naming.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/isa/vt82c686.c         | 48 ++++++++++++++++++---------------------
>  hw/mips/fuloong2e.c       |  2 +-
>  include/hw/isa/vt82c686.h |  2 +-
>  3 files changed, 24 insertions(+), 28 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[PATCH 10/12] vt82c686: Remove unneeded includes and defines

[BALATON Zoltan via]

These are not used or not needed.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/isa/vt82c686.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index 789459bcae..6dff2bc67d 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -12,22 +12,16 @@
 
 #include "qemu/osdep.h"
 #include "hw/isa/vt82c686.h"
-#include "hw/i2c/i2c.h"
 #include "hw/pci/pci.h"
 #include "hw/qdev-properties.h"
-#include "hw/isa/isa.h"
 #include "hw/isa/superio.h"
-#include "hw/sysbus.h"
 #include "migration/vmstate.h"
-#include "hw/mips/mips.h"
 #include "hw/isa/apm.h"
 #include "hw/acpi/acpi.h"
 #include "hw/i2c/pm_smbus.h"
 #include "qapi/error.h"
-#include "qemu/module.h"
 #include "qemu/timer.h"
 #include "exec/address-spaces.h"
-#include "qom/object.h"
 #include "trace.h"
 
 typedef struct SuperIOConfig {
@@ -137,8 +131,6 @@ static void vt82c686b_write_config(PCIDevice *d, uint32_t addr,
     }
 }
 
-#define ACPI_DBG_IO_ADDR  0xb044
-
 struct VT686PMState {
     PCIDevice dev;
     MemoryRegion io;
-- 
2.21.3

Re: [PATCH 10/12] vt82c686: Remove unneeded includes and defines

[Philippe Mathieu-Daudé]

On 12/27/20 2:10 AM, BALATON Zoltan via wrote:
> These are not used or not needed.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/isa/vt82c686.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 789459bcae..6dff2bc67d 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -12,22 +12,16 @@
>  
>  #include "qemu/osdep.h"
>  #include "hw/isa/vt82c686.h"
> -#include "hw/i2c/i2c.h"

Maybe squash this one in patch 8 "Remove vt82c686b_pm_init"?

>  #include "hw/pci/pci.h"
>  #include "hw/qdev-properties.h"
> -#include "hw/isa/isa.h"

Used for isa_bus_new().

>  #include "hw/isa/superio.h"
> -#include "hw/sysbus.h"

OK.

>  #include "migration/vmstate.h"
> -#include "hw/mips/mips.h"

Indeed I can't see anything in commit edf79e66c02 ("Initial support
of vt82686b south bridge used by fulong mini pc") requiring it.

>  #include "hw/isa/apm.h"
>  #include "hw/acpi/acpi.h"
>  #include "hw/i2c/pm_smbus.h"
>  #include "qapi/error.h"
> -#include "qemu/module.h"

type_init().

>  #include "qemu/timer.h"
>  #include "exec/address-spaces.h"
> -#include "qom/object.h"

OK.

>  #include "trace.h"
>  
>  typedef struct SuperIOConfig {
> @@ -137,8 +131,6 @@ static void vt82c686b_write_config(PCIDevice *d, uint32_t addr,
>      }
>  }
>  
> -#define ACPI_DBG_IO_ADDR  0xb044
> -
>  struct VT686PMState {
>      PCIDevice dev;
>      MemoryRegion io;
> 

[PATCH 05/12] vt82c686: Split off via-[am]c97 into separate file in hw/audio

[BALATON Zoltan via]

These supposed to implement audio part used in VIA south bridges so
they are better placed under hw/audio.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/audio/meson.build |   1 +
 hw/audio/via-ac97.c  | 106 +++++++++++++++++++++++++++++++++++++++++++
 hw/isa/vt82c686.c    |  91 -------------------------------------
 3 files changed, 107 insertions(+), 91 deletions(-)
 create mode 100644 hw/audio/via-ac97.c

diff --git a/hw/audio/meson.build b/hw/audio/meson.build
index 549e9a0396..32c42bdebe 100644
--- a/hw/audio/meson.build
+++ b/hw/audio/meson.build
@@ -11,4 +11,5 @@ softmmu_ss.add(when: 'CONFIG_MILKYMIST', if_true: files('milkymist-ac97.c'))
 softmmu_ss.add(when: 'CONFIG_PCSPK', if_true: files('pcspk.c'))
 softmmu_ss.add(when: 'CONFIG_PL041', if_true: files('pl041.c', 'lm4549.c'))
 softmmu_ss.add(when: 'CONFIG_SB16', if_true: files('sb16.c'))
+softmmu_ss.add(when: 'CONFIG_VT82C686', if_true: files('via-ac97.c'))
 softmmu_ss.add(when: 'CONFIG_WM8750', if_true: files('wm8750.c'))
diff --git a/hw/audio/via-ac97.c b/hw/audio/via-ac97.c
new file mode 100644
index 0000000000..e617416ff7
--- /dev/null
+++ b/hw/audio/via-ac97.c
@@ -0,0 +1,106 @@
+/*
+ * VIA south bridges sound support
+ *
+ * This work is licensed under the GNU GPL license version 2 or later.
+ */
+
+/*
+ * TODO: This is entirely boiler plate just registering empty PCI devices
+ * with the right ID guests expect, functionality should be added here.
+ */
+
+#include "qemu/osdep.h"
+#include "hw/isa/vt82c686.h"
+#include "hw/pci/pci.h"
+
+struct VIAAC97State {
+    PCIDevice dev;
+};
+
+struct VIAMC97State {
+    PCIDevice dev;
+};
+
+OBJECT_DECLARE_SIMPLE_TYPE(VIAAC97State, VIA_AC97)
+OBJECT_DECLARE_SIMPLE_TYPE(VIAMC97State, VIA_MC97)
+
+static void via_ac97_realize(PCIDevice *dev, Error **errp)
+{
+    VIAAC97State *s = VIA_AC97(dev);
+    uint8_t *pci_conf = s->dev.config;
+
+    pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_INVALIDATE |
+                 PCI_COMMAND_PARITY);
+    pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_CAP_LIST |
+                 PCI_STATUS_DEVSEL_MEDIUM);
+    pci_set_long(pci_conf + PCI_INTERRUPT_PIN, 0x03);
+}
+
+static void via_ac97_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+    k->realize = via_ac97_realize;
+    k->vendor_id = PCI_VENDOR_ID_VIA;
+    k->device_id = PCI_DEVICE_ID_VIA_AC97;
+    k->revision = 0x50;
+    k->class_id = PCI_CLASS_MULTIMEDIA_AUDIO;
+    set_bit(DEVICE_CATEGORY_SOUND, dc->categories);
+    dc->desc = "AC97";
+}
+
+static const TypeInfo via_ac97_info = {
+    .name          = TYPE_VIA_AC97,
+    .parent        = TYPE_PCI_DEVICE,
+    .instance_size = sizeof(VIAAC97State),
+    .class_init    = via_ac97_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { INTERFACE_CONVENTIONAL_PCI_DEVICE },
+        { },
+    },
+};
+
+static void via_mc97_realize(PCIDevice *dev, Error **errp)
+{
+    VIAMC97State *s = VIA_MC97(dev);
+    uint8_t *pci_conf = s->dev.config;
+
+    pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_INVALIDATE |
+                 PCI_COMMAND_VGA_PALETTE);
+    pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM);
+    pci_set_long(pci_conf + PCI_INTERRUPT_PIN, 0x03);
+}
+
+static void via_mc97_class_init(ObjectClass *klass, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(klass);
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+
+    k->realize = via_mc97_realize;
+    k->vendor_id = PCI_VENDOR_ID_VIA;
+    k->device_id = PCI_DEVICE_ID_VIA_MC97;
+    k->class_id = PCI_CLASS_COMMUNICATION_OTHER;
+    k->revision = 0x30;
+    set_bit(DEVICE_CATEGORY_NETWORK, dc->categories);
+    dc->desc = "MC97";
+}
+
+static const TypeInfo via_mc97_info = {
+    .name          = TYPE_VIA_MC97,
+    .parent        = TYPE_PCI_DEVICE,
+    .instance_size = sizeof(VIAMC97State),
+    .class_init    = via_mc97_class_init,
+    .interfaces = (InterfaceInfo[]) {
+        { INTERFACE_CONVENTIONAL_PCI_DEVICE },
+        { },
+    },
+};
+
+static void via_ac97_register_types(void)
+{
+    type_register_static(&via_ac97_info);
+    type_register_static(&via_mc97_info);
+}
+
+type_init(via_ac97_register_types)
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index de772262bc..758c54172b 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -168,14 +168,6 @@ struct VT686PMState {
     uint32_t smb_io_base;
 };
 
-struct VIAAC97State {
-    PCIDevice dev;
-};
-
-struct VIAMC97State {
-    PCIDevice dev;
-};
-
 #define TYPE_VT82C686B_PM "VT82C686B_PM"
 OBJECT_DECLARE_SIMPLE_TYPE(VT686PMState, VT82C686B_PM)
 
@@ -247,87 +239,6 @@ static const VMStateDescription vmstate_acpi = {
     }
 };
 
-/*
- * TODO: VIA_AC97 and VIA_MC97
- * just register a PCI device now, functionalities will be implemented later.
- */
-
-OBJECT_DECLARE_SIMPLE_TYPE(VIAMC97State, VIA_MC97)
-OBJECT_DECLARE_SIMPLE_TYPE(VIAAC97State, VIA_AC97)
-
-static void vt82c686b_ac97_realize(PCIDevice *dev, Error **errp)
-{
-    VIAAC97State *s = VIA_AC97(dev);
-    uint8_t *pci_conf = s->dev.config;
-
-    pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_INVALIDATE |
-                 PCI_COMMAND_PARITY);
-    pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_CAP_LIST |
-                 PCI_STATUS_DEVSEL_MEDIUM);
-    pci_set_long(pci_conf + PCI_INTERRUPT_PIN, 0x03);
-}
-
-static void via_ac97_class_init(ObjectClass *klass, void *data)
-{
-    DeviceClass *dc = DEVICE_CLASS(klass);
-    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
-
-    k->realize = vt82c686b_ac97_realize;
-    k->vendor_id = PCI_VENDOR_ID_VIA;
-    k->device_id = PCI_DEVICE_ID_VIA_AC97;
-    k->revision = 0x50;
-    k->class_id = PCI_CLASS_MULTIMEDIA_AUDIO;
-    set_bit(DEVICE_CATEGORY_SOUND, dc->categories);
-    dc->desc = "AC97";
-}
-
-static const TypeInfo via_ac97_info = {
-    .name          = TYPE_VIA_AC97,
-    .parent        = TYPE_PCI_DEVICE,
-    .instance_size = sizeof(VIAAC97State),
-    .class_init    = via_ac97_class_init,
-    .interfaces = (InterfaceInfo[]) {
-        { INTERFACE_CONVENTIONAL_PCI_DEVICE },
-        { },
-    },
-};
-
-static void vt82c686b_mc97_realize(PCIDevice *dev, Error **errp)
-{
-    VIAMC97State *s = VIA_MC97(dev);
-    uint8_t *pci_conf = s->dev.config;
-
-    pci_set_word(pci_conf + PCI_COMMAND, PCI_COMMAND_INVALIDATE |
-                 PCI_COMMAND_VGA_PALETTE);
-    pci_set_word(pci_conf + PCI_STATUS, PCI_STATUS_DEVSEL_MEDIUM);
-    pci_set_long(pci_conf + PCI_INTERRUPT_PIN, 0x03);
-}
-
-static void via_mc97_class_init(ObjectClass *klass, void *data)
-{
-    DeviceClass *dc = DEVICE_CLASS(klass);
-    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
-
-    k->realize = vt82c686b_mc97_realize;
-    k->vendor_id = PCI_VENDOR_ID_VIA;
-    k->device_id = PCI_DEVICE_ID_VIA_MC97;
-    k->class_id = PCI_CLASS_COMMUNICATION_OTHER;
-    k->revision = 0x30;
-    set_bit(DEVICE_CATEGORY_NETWORK, dc->categories);
-    dc->desc = "MC97";
-}
-
-static const TypeInfo via_mc97_info = {
-    .name          = TYPE_VIA_MC97,
-    .parent        = TYPE_PCI_DEVICE,
-    .instance_size = sizeof(VIAMC97State),
-    .class_init    = via_mc97_class_init,
-    .interfaces = (InterfaceInfo[]) {
-        { INTERFACE_CONVENTIONAL_PCI_DEVICE },
-        { },
-    },
-};
-
 /* vt82c686 pm init */
 static void vt82c686b_pm_realize(PCIDevice *dev, Error **errp)
 {
@@ -516,8 +427,6 @@ static const TypeInfo via_superio_info = {
 
 static void vt82c686b_register_types(void)
 {
-    type_register_static(&via_ac97_info);
-    type_register_static(&via_mc97_info);
     type_register_static(&via_pm_info);
     type_register_static(&via_superio_info);
     type_register_static(&via_info);
-- 
2.21.3

Re: [PATCH 05/12] vt82c686: Split off via-[am]c97 into separate file in hw/audio

[Philippe Mathieu-Daudé]

On 12/27/20 2:10 AM, BALATON Zoltan via wrote:
> These supposed to implement audio part used in VIA south bridges so
> they are better placed under hw/audio.

"The via-[am]c97 code is supposed to implement the audio part of
VIA south bridge chipset so is better placed under hw/audio/.
Split it off into a separate file."

Good idea!

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/audio/meson.build |   1 +
>  hw/audio/via-ac97.c  | 106 +++++++++++++++++++++++++++++++++++++++++++
>  hw/isa/vt82c686.c    |  91 -------------------------------------
>  3 files changed, 107 insertions(+), 91 deletions(-)
>  create mode 100644 hw/audio/via-ac97.c

[PATCH 09/12] vt82c686: Convert debug printf to trace points

[BALATON Zoltan via]

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/isa/trace-events |  6 ++++++
 hw/isa/vt82c686.c   | 51 +++++++++++++--------------------------------
 2 files changed, 21 insertions(+), 36 deletions(-)

diff --git a/hw/isa/trace-events b/hw/isa/trace-events
index 3544c6213c..d267d3e652 100644
--- a/hw/isa/trace-events
+++ b/hw/isa/trace-events
@@ -13,3 +13,9 @@ pc87312_io_write(uint32_t addr, uint32_t val) "write addr=0x%x val=0x%x"
 # apm.c
 apm_io_read(uint8_t addr, uint8_t val) "read addr=0x%x val=0x%02x"
 apm_io_write(uint8_t addr, uint8_t val) "write addr=0x%x val=0x%02x"
+
+# vt82c686.c
+via_isa_write(uint32_t addr, uint32_t val, int len) "addr 0x%x val 0x%x len 0x%x"
+via_pm_write(uint32_t addr, uint32_t val, int len) "addr 0x%x val 0x%x len 0x%x"
+via_superio_read(uint8_t addr, uint8_t val) "addr 0x%x val 0x%x"
+via_superio_write(uint8_t addr, uint32_t val) "addr 0x%x val 0x%x"
diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
index b138838400..789459bcae 100644
--- a/hw/isa/vt82c686.c
+++ b/hw/isa/vt82c686.c
@@ -28,14 +28,7 @@
 #include "qemu/timer.h"
 #include "exec/address-spaces.h"
 #include "qom/object.h"
-
-/* #define DEBUG_VT82C686B */
-
-#ifdef DEBUG_VT82C686B
-#define DPRINTF(fmt, ...) fprintf(stderr, "%s: " fmt, __func__, ##__VA_ARGS__)
-#else
-#define DPRINTF(fmt, ...)
-#endif
+#include "trace.h"
 
 typedef struct SuperIOConfig {
     uint8_t config[0x100];
@@ -56,16 +49,17 @@ static void superio_ioport_writeb(void *opaque, hwaddr addr, uint64_t data,
 {
     SuperIOConfig *superio_conf = opaque;
 
-    DPRINTF("superio_ioport_writeb  address 0x%x  val 0x%x\n", addr, data);
-    if (addr == 0x3f0) {
+    if (addr == 0x3f0) { /* config index register */
         superio_conf->index = data & 0xff;
     } else {
         bool can_write = true;
-        /* 0x3f1 */
+        /* 0x3f1, config data register */
+        trace_via_superio_write(superio_conf->index, data & 0xff);
         switch (superio_conf->index) {
         case 0x00 ... 0xdf:
         case 0xe4:
         case 0xe5:
+        case 0xe6 ... 0xe8: /* Should set base port of parallel and serial */
         case 0xe9 ... 0xed:
         case 0xf3:
         case 0xf5:
@@ -74,18 +68,6 @@ static void superio_ioport_writeb(void *opaque, hwaddr addr, uint64_t data,
         case 0xfd ... 0xff:
             can_write = false;
             break;
-        case 0xe7:
-            if ((data & 0xff) != 0xfe) {
-                DPRINTF("change uart 1 base. unsupported yet\n");
-                can_write = false;
-            }
-            break;
-        case 0xe8:
-            if ((data & 0xff) != 0xbe) {
-                DPRINTF("change uart 2 base. unsupported yet\n");
-                can_write = false;
-            }
-            break;
         default:
             break;
 
@@ -99,9 +81,10 @@ static void superio_ioport_writeb(void *opaque, hwaddr addr, uint64_t data,
 static uint64_t superio_ioport_readb(void *opaque, hwaddr addr, unsigned size)
 {
     SuperIOConfig *superio_conf = opaque;
+    uint8_t val = superio_conf->config[superio_conf->index];
 
-    DPRINTF("superio_ioport_readb  address 0x%x\n", addr);
-    return superio_conf->config[superio_conf->index];
+    trace_via_superio_read(superio_conf->index, val);
+    return val;
 }
 
 static const MemoryRegionOps superio_ops = {
@@ -142,16 +125,14 @@ static void vt82c686b_isa_reset(DeviceState *dev)
 }
 
 /* write config pci function0 registers. PCI-ISA bridge */
-static void vt82c686b_write_config(PCIDevice *d, uint32_t address,
+static void vt82c686b_write_config(PCIDevice *d, uint32_t addr,
                                    uint32_t val, int len)
 {
     VT82C686BState *vt686 = VT82C686B(d);
 
-    DPRINTF("vt82c686b_write_config  address 0x%x  val 0x%x len 0x%x\n",
-           address, val, len);
-
-    pci_default_write_config(d, address, val, len);
-    if (address == 0x85) {  /* enable or disable super IO configure */
+    trace_via_isa_write(addr, val, len);
+    pci_default_write_config(d, addr, val, len);
+    if (addr == 0x85) {  /* enable or disable super IO configure */
         memory_region_set_enabled(&vt686->superio, val & 0x2);
     }
 }
@@ -204,12 +185,10 @@ static void pm_io_space_update(VT686PMState *s)
     memory_region_transaction_commit();
 }
 
-static void pm_write_config(PCIDevice *d,
-                            uint32_t address, uint32_t val, int len)
+static void pm_write_config(PCIDevice *d, uint32_t addr, uint32_t val, int len)
 {
-    DPRINTF("pm_write_config  address 0x%x  val 0x%x len 0x%x\n",
-           address, val, len);
-    pci_default_write_config(d, address, val, len);
+    trace_via_pm_write(addr, val, len);
+    pci_default_write_config(d, addr, val, len);
 }
 
 static int vmstate_acpi_post_load(void *opaque, int version_id)
-- 
2.21.3

Re: [PATCH 09/12] vt82c686: Convert debug printf to trace points

[Philippe Mathieu-Daudé]

On 12/27/20 2:10 AM, BALATON Zoltan via wrote:
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/isa/trace-events |  6 ++++++
>  hw/isa/vt82c686.c   | 51 +++++++++++++--------------------------------
>  2 files changed, 21 insertions(+), 36 deletions(-)
...

>          switch (superio_conf->index) {
>          case 0x00 ... 0xdf:
>          case 0xe4:
>          case 0xe5:
> +        case 0xe6 ... 0xe8: /* Should set base port of parallel and serial */
>          case 0xe9 ... 0xed:
>          case 0xf3:
>          case 0xf5:
> @@ -74,18 +68,6 @@ static void superio_ioport_writeb(void *opaque, hwaddr addr, uint64_t data,
>          case 0xfd ... 0xff:
>              can_write = false;
>              break;
> -        case 0xe7:
> -            if ((data & 0xff) != 0xfe) {
> -                DPRINTF("change uart 1 base. unsupported yet\n");
> -                can_write = false;
> -            }
> -            break;
> -        case 0xe8:
> -            if ((data & 0xff) != 0xbe) {
> -                DPRINTF("change uart 2 base. unsupported yet\n");
> -                can_write = false;
> -            }
> -            break;
>          default:
>              break;

This hunk belong to a different patch (does not match the patch
description).

Re: [PATCH 09/12] vt82c686: Convert debug printf to trace points

[BALATON Zoltan via]

[-- Attachment #1: Type: text/plain, Size: 1590 bytes --]

On Sun, 27 Dec 2020, Philippe Mathieu-Daudé wrote:
> On 12/27/20 2:10 AM, BALATON Zoltan via wrote:
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/isa/trace-events |  6 ++++++
>>  hw/isa/vt82c686.c   | 51 +++++++++++++--------------------------------
>>  2 files changed, 21 insertions(+), 36 deletions(-)
> ...
>
>>          switch (superio_conf->index) {
>>          case 0x00 ... 0xdf:
>>          case 0xe4:
>>          case 0xe5:
>> +        case 0xe6 ... 0xe8: /* Should set base port of parallel and serial */
>>          case 0xe9 ... 0xed:
>>          case 0xf3:
>>          case 0xf5:
>> @@ -74,18 +68,6 @@ static void superio_ioport_writeb(void *opaque, hwaddr addr, uint64_t data,
>>          case 0xfd ... 0xff:
>>              can_write = false;
>>              break;
>> -        case 0xe7:
>> -            if ((data & 0xff) != 0xfe) {
>> -                DPRINTF("change uart 1 base. unsupported yet\n");
>> -                can_write = false;
>> -            }
>> -            break;
>> -        case 0xe8:
>> -            if ((data & 0xff) != 0xbe) {
>> -                DPRINTF("change uart 2 base. unsupported yet\n");
>> -                can_write = false;
>> -            }
>> -            break;
>>          default:
>>              break;
>
> This hunk belong to a different patch (does not match the patch
> description).

In a way does, in that it removes two DPRINTFs instead of converting them. 
Maybe I should mention this in the commit message or could make it a 
separate patch but don't know if that's worth it.

Regards,
BALATON Zoltan