qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: BALATON Zoltan
Subject: Re: [PATCH v2 08/13] vt82c686: Move creation of ISA devices to the ISA bridge
Date: Mon, 1 Feb 2021 21:04:34 +0100 (CET)
User-agent: Alpine 2.03 (LMD 1266 2009-07-14)

On Sun, 10 Jan 2021, BALATON Zoltan wrote:
On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote:
+PCI experts

On 1/10/21 1:43 AM, BALATON Zoltan wrote:
On Sun, 10 Jan 2021, Philippe Mathieu-Daudé wrote:
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?

I don't know but why would that be better? I thought it's clearer to
clean up pm related parts first before moving more stuff to this file so
that's why this patch comes after (and also because that's the order I
did it).

Not any better, but easier for me to get your patches integrated,
as I'm reviewing your patches slowly. Finding other reviewers
would certainly help.

No problem, I'll wait for your review. Merging parts of the series does not help much because the whole series is needed for vt8231 which is prerequisite for pegasos2 so eventually all of these are needed so it does not matter if this one patch gets in earlier or later.

Not sure who could help with review. Maybe Jiaxun or Huacai as this is used by fuloong2e so they might be interested and could have info on this chip. Most of these patches just cleaning up the vt82c686b and adding some missing features so these can be reused by the vt8231 model in last 3 patches (which is very similar to 686b only some reg addresses and ids seem to be different for what we are concerned).

Ping? There are still a few patches needing review:

http://patchwork.ozlabs.org/project/qemu-devel/list/?series=223512

Jiaxun, Hiacai, or anybody else could you please help reviewing or testing if this works with fuloong2e?

Thank you,
BALATON Zoltan

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?

How? This is a PCIDevice not a SysBusDevice.

Indeed :)

+    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)?

I don't really know. Most other places that create an isa bus seem to
also use get_system_memory(), only piix4 uses pci_address_space(dev) so
I thought if those others are OK this should be too.

I'm not a PCI expert but my understanding is PCI device functions are
restricted to the PCI bus address space. The host bridge may map this
space within the host.

QEMU might be using get_system_memory() because for some host bridge
the mapping is not implemented so it was easier this way?

Maybe, also one less indirection which if not really needed is a good thing for performance so unless it's found to be needed to use another address space here I'm happy with this as it matches what other similar devices do and it seems to work. Maybe a separate address space is only really needed if we have an iommu?

Regards,
BALATON Zoltan

reply via email to

[Prev in Thread] Current Thread [Next in Thread]