[PATCH 0/6] hw/usb/vt82c686-uhci-pci: Use ISA instead of PCI interrupts

[PATCH 0/6] hw/usb/vt82c686-uhci-pci: Use ISA instead of PCI interrupts

[BALATON Zoltan]

Hello,

I'm posting this again as a new series because I'm a bit lost on where
we are after Philippe sent a pull requst for a series that was
originally based on this. These are the remaining patches rebased on
Philippe's pull request that do the same to vt82c686-uhci-pci that was
done for via-ide to fix USB on pegasos2. First and last patches are
optional based on the outcome if we can access the parent directly in
this case or not but I think this should be OK in an internal
interrupt handler part of one device model and we're not poking around
in foreign objects.

Regards,

BALATON Zoltan (6):
  via-ide: Avoid expensive operations in irq handler
  usb/uhci: Misc clean up
  usb/uhci: Disallow user creating a vt82c686-uhci-pci device
  usb/uhci: Replace pci_set_irq with qemu_set_irq
  hw/usb/vt82c686-uhci-pci: Use ISA instead of PCI interrupts
  hw/usb/vt82c686-uhci-pci: Optimise itq handler

 hw/ide/via.c               | 11 ++++++-----
 hw/usb/hcd-uhci.c          | 14 ++++++++------
 hw/usb/hcd-uhci.h          |  4 +++-
 hw/usb/vt82c686-uhci-pci.c | 16 ++++++++++++++++
 include/hw/ide/pci.h       |  1 +
 5 files changed, 34 insertions(+), 12 deletions(-)

-- 
2.21.4

[PATCH 3/6] usb/uhci: Disallow user creating a vt82c686-uhci-pci device

[BALATON Zoltan]

Because this device only works as part of VIA superio chips set user
creatable to false. Since the class init method is common for UHCI
variants introduce a flag in UHCIInfo for this.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-uhci.c          | 3 +++
 hw/usb/hcd-uhci.h          | 1 +
 hw/usb/vt82c686-uhci-pci.c | 2 ++
 3 files changed, 6 insertions(+)

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index c557566ec2..7d26e35194 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -1282,6 +1282,9 @@ void uhci_data_class_init(ObjectClass *klass, void *data)
     } else {
         device_class_set_props(dc, uhci_properties_standalone);
     }
+    if (info->notuser) {
+        dc->user_creatable = false;
+    }
     u->info = *info;
 }
 
diff --git a/hw/usb/hcd-uhci.h b/hw/usb/hcd-uhci.h
index e61d8fcb19..316693f80b 100644
--- a/hw/usb/hcd-uhci.h
+++ b/hw/usb/hcd-uhci.h
@@ -85,6 +85,7 @@ typedef struct UHCIInfo {
     uint8_t    irq_pin;
     void       (*realize)(PCIDevice *dev, Error **errp);
     bool       unplug;
+    bool       notuser; /* disallow user_creatable */
 } UHCIInfo;
 
 void uhci_data_class_init(ObjectClass *klass, void *data);
diff --git a/hw/usb/vt82c686-uhci-pci.c b/hw/usb/vt82c686-uhci-pci.c
index b109c21603..ea262e6d70 100644
--- a/hw/usb/vt82c686-uhci-pci.c
+++ b/hw/usb/vt82c686-uhci-pci.c
@@ -25,6 +25,8 @@ static UHCIInfo uhci_info[] = {
         .irq_pin   = 3,
         .realize   = usb_uhci_vt82c686b_realize,
         .unplug    = true,
+        /* Reason: only works as USB function of VT82xx superio chips */
+        .notuser   = true,
     }
 };
 
-- 
2.21.4

[PATCH 2/6] usb/uhci: Misc clean up

[BALATON Zoltan]

Fix a comment for coding style so subsequent patch will not get
checkpatch error and simplify and shorten uhci_update_irq().

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/usb/hcd-uhci.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 0cb02a6432..c557566ec2 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -290,7 +290,7 @@ static UHCIAsync *uhci_async_find_td(UHCIState *s, uint32_t td_addr)
 
 static void uhci_update_irq(UHCIState *s)
 {
-    int level;
+    int level = 0;
     if (((s->status2 & 1) && (s->intr & (1 << 2))) ||
         ((s->status2 & 2) && (s->intr & (1 << 3))) ||
         ((s->status & UHCI_STS_USBERR) && (s->intr & (1 << 0))) ||
@@ -298,8 +298,6 @@ static void uhci_update_irq(UHCIState *s)
         (s->status & UHCI_STS_HSERR) ||
         (s->status & UHCI_STS_HCPERR)) {
         level = 1;
-    } else {
-        level = 0;
     }
     pci_set_irq(&s->dev, level);
 }
@@ -1170,8 +1168,7 @@ void usb_uhci_common_realize(PCIDevice *dev, Error **errp)
 
     pci_conf[PCI_CLASS_PROG] = 0x00;
     /* TODO: reset value should be 0. */
-    pci_conf[USB_SBRN] = USB_RELEASE_1; // release number
-
+    pci_conf[USB_SBRN] = USB_RELEASE_1; /* release number */
     pci_config_set_interrupt_pin(pci_conf, u->info.irq_pin + 1);
 
     if (s->masterbus) {
-- 
2.21.4

[PATCH 1/6] via-ide: Avoid expensive operations in irq handler

[BALATON Zoltan]

Cache the pointer to PCI function 0 (ISA bridge, that this IDE device
has to use for IRQs) in the PCIIDEState and pass that as the opaque
data for the interrupt handler to eliminate both the need to look up
function 0 at every interrupt and also a QOM type cast of the opaque
pointer as that's also expensive (mainly due to qom-debug being
enabled by default).

Suggested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/ide/via.c         | 11 ++++++-----
 include/hw/ide/pci.h |  1 +
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/hw/ide/via.c b/hw/ide/via.c
index 82def819c4..30566bc409 100644
--- a/hw/ide/via.c
+++ b/hw/ide/via.c
@@ -104,15 +104,15 @@ static void bmdma_setup_bar(PCIIDEState *d)
 
 static void via_ide_set_irq(void *opaque, int n, int level)
 {
-    PCIDevice *d = PCI_DEVICE(opaque);
+    PCIIDEState *d = opaque;
 
     if (level) {
-        d->config[0x70 + n * 8] |= 0x80;
+        d->parent_obj.config[0x70 + n * 8] |= 0x80;
     } else {
-        d->config[0x70 + n * 8] &= ~0x80;
+        d->parent_obj.config[0x70 + n * 8] &= ~0x80;
     }
 
-    via_isa_set_irq(pci_get_function_0(d), 14 + n, level);
+    via_isa_set_irq(d->func0, 14 + n, level);
 }
 
 static void via_ide_reset(DeviceState *dev)
@@ -188,7 +188,8 @@ static void via_ide_realize(PCIDevice *dev, Error **errp)
     bmdma_setup_bar(d);
     pci_register_bar(dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &d->bmdma_bar);
 
-    qdev_init_gpio_in(ds, via_ide_set_irq, 2);
+    d->func0 = pci_get_function_0(dev);
+    qdev_init_gpio_in_named_with_opaque(ds, via_ide_set_irq, d, NULL, 2);
     for (i = 0; i < 2; i++) {
         ide_bus_init(&d->bus[i], sizeof(d->bus[i]), ds, i, 2);
         ide_init2(&d->bus[i], qdev_get_gpio_in(ds, i));
diff --git a/include/hw/ide/pci.h b/include/hw/ide/pci.h
index d8384e1c42..89d14abf95 100644
--- a/include/hw/ide/pci.h
+++ b/include/hw/ide/pci.h
@@ -50,6 +50,7 @@ struct PCIIDEState {
     IDEBus bus[2];
     BMDMAState bmdma[2];
     uint32_t secondary; /* used only for cmd646 */
+    PCIDevice *func0; /* used only by IDE functions of superio chips */
     MemoryRegion bmdma_bar;
     MemoryRegion cmd_bar[2];
     MemoryRegion data_bar[2];
-- 
2.21.4

[PATCH 6/6] hw/usb/vt82c686-uhci-pci: Optimise itq handler

[BALATON Zoltan]

Cache the pointer to PCI function 0 (ISA bridge, that this device has
to use for IRQs) in the state struct and use that in the interrupt
handler to avoid needing to look up function 0 at every interrupt
which can be expensive.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/usb/hcd-uhci.h          | 1 +
 hw/usb/vt82c686-uhci-pci.c | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/usb/hcd-uhci.h b/hw/usb/hcd-uhci.h
index c85ab7868e..ac4e912a3f 100644
--- a/hw/usb/hcd-uhci.h
+++ b/hw/usb/hcd-uhci.h
@@ -44,6 +44,7 @@ typedef struct UHCIPort {
 
 typedef struct UHCIState {
     PCIDevice dev;
+    PCIDevice *func0; /* used when part of a superio chip */
     MemoryRegion io_bar;
     USBBus bus; /* Note unused when we're a companion controller */
     uint16_t cmd; /* cmd register */
diff --git a/hw/usb/vt82c686-uhci-pci.c b/hw/usb/vt82c686-uhci-pci.c
index 0bf2b72ff0..c500936af9 100644
--- a/hw/usb/vt82c686-uhci-pci.c
+++ b/hw/usb/vt82c686-uhci-pci.c
@@ -8,7 +8,7 @@ static void uhci_isa_set_irq(void *opaque, int irq_num, int level)
     UHCIState *s = opaque;
     uint8_t irq = pci_get_byte(s->dev.config + PCI_INTERRUPT_LINE);
     if (irq > 0 && irq < 15) {
-        via_isa_set_irq(pci_get_function_0(&s->dev), irq, level);
+        via_isa_set_irq(s->func0, irq, level);
     }
 }
 
@@ -25,6 +25,7 @@ static void usb_uhci_vt82c686b_realize(PCIDevice *dev, Error **errp)
     pci_set_long(pci_conf + 0xc0, 0x00002000);
 
     usb_uhci_common_realize(dev, errp);
+    s->func0 = pci_get_function_0(&s->dev);
     object_unref(s->irq);
     s->irq = qemu_allocate_irq(uhci_isa_set_irq, s, 0);
 }
-- 
2.21.4

[PATCH 4/6] usb/uhci: Replace pci_set_irq with qemu_set_irq

[BALATON Zoltan]

Instead of using pci_set_irq, store the irq in the device state and
use it explicitly so variants having different interrupt handling can
use their own.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
---
 hw/usb/hcd-uhci.c | 4 +++-
 hw/usb/hcd-uhci.h | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 7d26e35194..d1b5657d72 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -31,6 +31,7 @@
 #include "hw/usb/uhci-regs.h"
 #include "migration/vmstate.h"
 #include "hw/pci/pci.h"
+#include "hw/irq.h"
 #include "hw/qdev-properties.h"
 #include "qapi/error.h"
 #include "qemu/timer.h"
@@ -299,7 +300,7 @@ static void uhci_update_irq(UHCIState *s)
         (s->status & UHCI_STS_HCPERR)) {
         level = 1;
     }
-    pci_set_irq(&s->dev, level);
+    qemu_set_irq(s->irq, level);
 }
 
 static void uhci_reset(DeviceState *dev)
@@ -1170,6 +1171,7 @@ void usb_uhci_common_realize(PCIDevice *dev, Error **errp)
     /* TODO: reset value should be 0. */
     pci_conf[USB_SBRN] = USB_RELEASE_1; /* release number */
     pci_config_set_interrupt_pin(pci_conf, u->info.irq_pin + 1);
+    s->irq = pci_allocate_irq(dev);
 
     if (s->masterbus) {
         USBPort *ports[NB_PORTS];
diff --git a/hw/usb/hcd-uhci.h b/hw/usb/hcd-uhci.h
index 316693f80b..c85ab7868e 100644
--- a/hw/usb/hcd-uhci.h
+++ b/hw/usb/hcd-uhci.h
@@ -60,7 +60,7 @@ typedef struct UHCIState {
     uint32_t frame_bandwidth;
     bool completions_only;
     UHCIPort ports[NB_PORTS];
-
+    qemu_irq irq;
     /* Interrupts that should be raised at the end of the current frame.  */
     uint32_t pending_int_mask;
 
-- 
2.21.4

[PATCH 5/6] hw/usb/vt82c686-uhci-pci: Use ISA instead of PCI interrupts

[BALATON Zoltan]

This device is part of a superio/ISA bridge chip and IRQs from it are
routed to an ISA interrupt set by the Interrupt Line PCI config
register. Implement this in a vt82c686-uhci-pci specific irq handler
Using via_isa_set_irq().

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
Reviewed-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
This is merging adding uhci_isa_set_irq and using via_isa_set_irq in
it which were separate patches and got R-b that way but hopefully they
still apply as one patch so I kept the R-b tags. Say so if you don't agree.

 hw/usb/vt82c686-uhci-pci.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/hw/usb/vt82c686-uhci-pci.c b/hw/usb/vt82c686-uhci-pci.c
index ea262e6d70..0bf2b72ff0 100644
--- a/hw/usb/vt82c686-uhci-pci.c
+++ b/hw/usb/vt82c686-uhci-pci.c
@@ -1,6 +1,17 @@
 #include "qemu/osdep.h"
+#include "hw/irq.h"
+#include "hw/isa/vt82c686.h"
 #include "hcd-uhci.h"
 
+static void uhci_isa_set_irq(void *opaque, int irq_num, int level)
+{
+    UHCIState *s = opaque;
+    uint8_t irq = pci_get_byte(s->dev.config + PCI_INTERRUPT_LINE);
+    if (irq > 0 && irq < 15) {
+        via_isa_set_irq(pci_get_function_0(&s->dev), irq, level);
+    }
+}
+
 static void usb_uhci_vt82c686b_realize(PCIDevice *dev, Error **errp)
 {
     UHCIState *s = UHCI(dev);
@@ -14,6 +25,8 @@ static void usb_uhci_vt82c686b_realize(PCIDevice *dev, Error **errp)
     pci_set_long(pci_conf + 0xc0, 0x00002000);
 
     usb_uhci_common_realize(dev, errp);
+    object_unref(s->irq);
+    s->irq = qemu_allocate_irq(uhci_isa_set_irq, s, 0);
 }
 
 static UHCIInfo uhci_info[] = {
-- 
2.21.4