Re: [PATCH v2 08/13] vt82c686: Move creation of ISA devices to the ISA bridge

[Philippe Mathieu-Daudé]

Hi Zoltan,

On 1/9/21 9:16 PM, BALATON Zoltan wrote:
> Currently the ISA devices that are part of the VIA south bridge,
> superio chip are wired up by board code. Move creation of these ISA
> devices to the VIA ISA bridge model so that board code does not need
> to access ISA bus. This also allows vt82c686b-superio to be made
> internal to vt82c686 which allows implementing its configuration via
> registers in subseqent commits.

Is this patch dependent of the VT82C686B_PM changes
or can it be applied before them?

> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/isa/vt82c686.c   | 20 ++++++++++++++++++++
>  hw/mips/fuloong2e.c | 29 +++++------------------------
>  2 files changed, 25 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/isa/vt82c686.c b/hw/isa/vt82c686.c
> index 58c0bba1d0..5df9be8ff4 100644
> --- a/hw/isa/vt82c686.c
> +++ b/hw/isa/vt82c686.c
> @@ -16,6 +16,11 @@
>  #include "hw/qdev-properties.h"
>  #include "hw/isa/isa.h"
>  #include "hw/isa/superio.h"
> +#include "hw/intc/i8259.h"
> +#include "hw/irq.h"
> +#include "hw/dma/i8257.h"
> +#include "hw/timer/i8254.h"
> +#include "hw/rtc/mc146818rtc.h"
>  #include "migration/vmstate.h"
>  #include "hw/isa/apm.h"
>  #include "hw/acpi/acpi.h"
> @@ -307,9 +312,16 @@ OBJECT_DECLARE_SIMPLE_TYPE(VT82C686BISAState, 
> VT82C686B_ISA)
>  
>  struct VT82C686BISAState {
>      PCIDevice dev;
> +    qemu_irq cpu_intr;
>      SuperIOConfig superio_cfg;
>  };
>  
> +static void via_isa_request_i8259_irq(void *opaque, int irq, int level)
> +{
> +    VT82C686BISAState *s = opaque;
> +    qemu_set_irq(s->cpu_intr, level);
> +}
> +
>  static void vt82c686b_write_config(PCIDevice *d, uint32_t addr,
>                                     uint32_t val, int len)
>  {
> @@ -365,10 +377,18 @@ static void vt82c686b_realize(PCIDevice *d, Error 
> **errp)
>      VT82C686BISAState *s = VT82C686B_ISA(d);
>      DeviceState *dev = DEVICE(d);
>      ISABus *isa_bus;
> +    qemu_irq *isa_irq;
>      int i;
>  
> +    qdev_init_gpio_out(dev, &s->cpu_intr, 1);

Why not use the SysBus API?

> +    isa_irq = qemu_allocate_irqs(via_isa_request_i8259_irq, s, 1);
>      isa_bus = isa_bus_new(dev, get_system_memory(), pci_address_space_io(d),
>                            &error_fatal);

Isn't it get_system_memory() -> pci_address_space(d)?

> +    isa_bus_irqs(isa_bus, i8259_init(isa_bus, *isa_irq));
> +    i8254_pit_init(isa_bus, 0x40, 0, NULL);
> +    i8257_dma_init(isa_bus, 0);
> +    isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO);
> +    mc146818_rtc_init(isa_bus, 2000, NULL);
>  
>      for (i = 0; i < PCI_CONFIG_HEADER_SIZE; i++) {
>          if (i < PCI_COMMAND || i >= PCI_REVISION_ID) {
> diff --git a/hw/mips/fuloong2e.c b/hw/mips/fuloong2e.c
> index fbdd6122b3..0fc3288556 100644
> --- a/hw/mips/fuloong2e.c
> +++ b/hw/mips/fuloong2e.c
> @@ -25,9 +25,6 @@
>  #include "qapi/error.h"
>  #include "cpu.h"
>  #include "hw/clock.h"
> -#include "hw/intc/i8259.h"
> -#include "hw/dma/i8257.h"
> -#include "hw/isa/superio.h"
>  #include "net/net.h"
>  #include "hw/boards.h"
>  #include "hw/i2c/smbus_eeprom.h"
> @@ -38,13 +35,13 @@
>  #include "qemu/log.h"
>  #include "hw/loader.h"
>  #include "hw/ide/pci.h"
> +#include "hw/qdev-properties.h"
>  #include "elf.h"
>  #include "hw/isa/vt82c686.h"
> -#include "hw/rtc/mc146818rtc.h"
> -#include "hw/timer/i8254.h"
>  #include "exec/address-spaces.h"
>  #include "sysemu/qtest.h"
>  #include "sysemu/reset.h"
> +#include "sysemu/sysemu.h"
>  #include "qemu/error-report.h"
>  
>  #define ENVP_PADDR              0x2000
> @@ -224,26 +221,13 @@ static void main_cpu_reset(void *opaque)
>  }
>  
>  static void vt82c686b_southbridge_init(PCIBus *pci_bus, int slot, qemu_irq 
> intc,
> -                                       I2CBus **i2c_bus, ISABus **p_isa_bus)
> +                                       I2CBus **i2c_bus)
>  {
> -    qemu_irq *i8259;
> -    ISABus *isa_bus;
>      PCIDevice *dev;
>  
>      dev = pci_create_simple_multifunction(pci_bus, PCI_DEVFN(slot, 0), true,
>                                            TYPE_VT82C686B_ISA);
> -    isa_bus = ISA_BUS(qdev_get_child_bus(DEVICE(dev), "isa.0"));
> -    assert(isa_bus);
> -    *p_isa_bus = isa_bus;
> -    /* Interrupt controller */
> -    /* The 8259 -> IP5  */
> -    i8259 = i8259_init(isa_bus, intc);
> -    isa_bus_irqs(isa_bus, i8259);
> -    /* init other devices */
> -    i8254_pit_init(isa_bus, 0x40, 0, NULL);
> -    i8257_dma_init(isa_bus, 0);
> -    /* Super I/O */
> -    isa_create_simple(isa_bus, TYPE_VT82C686B_SUPERIO);
> +    qdev_connect_gpio_out(DEVICE(dev), 0, intc);
>  
>      dev = pci_create_simple(pci_bus, PCI_DEVFN(slot, 1), "via-ide");
>      pci_ide_create_devs(dev);
> @@ -290,7 +274,6 @@ static void mips_fuloong2e_init(MachineState *machine)
>      uint64_t kernel_entry;
>      PCIDevice *pci_dev;
>      PCIBus *pci_bus;
> -    ISABus *isa_bus;
>      I2CBus *smbus;
>      Clock *cpuclk;
>      MIPSCPU *cpu;
> @@ -357,7 +340,7 @@ static void mips_fuloong2e_init(MachineState *machine)
>  
>      /* South bridge -> IP5 */
>      vt82c686b_southbridge_init(pci_bus, FULOONG2E_VIA_SLOT, env->irq[5],
> -                               &smbus, &isa_bus);
> +                               &smbus);
>  
>      /* GPU */
>      if (vga_interface_type != VGA_NONE) {
> @@ -372,8 +355,6 @@ static void mips_fuloong2e_init(MachineState *machine)
>      spd_data = spd_data_generate(DDR, machine->ram_size);
>      smbus_eeprom_init_one(smbus, 0x50, spd_data);
>  
> -    mc146818_rtc_init(isa_bus, 2000, NULL);
> -
>      /* Network card: RTL8139D */
>      network_init(pci_bus);
>  }
>