On 10/13/21 14:13, BALATON Zoltan wrote:
> 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. Change uhci_update_irq() to allow this and implement it in
> vt82c686-uhci-pci.
>
> Signed-off-by: BALATON Zoltan <bala...@eik.bme.hu>
> Reviewed-by: Jiaxun Yang <jiaxun.y...@flygoat.com>
> ---
> v3: Do it more differently using qemu_irq instead as suggested by Gerd
> v2: Do it differently to confine isa reference to vt82c686-uhci-pci as
> hcd-uhci is also used on machines that don't have isa. Left Jiaxun's
> R-b there as he checked it's the same for VT82C686B and gave R-b for
> the 82c686b case which still holds but speak up if you tink otherwise.
>
> hw/usb/hcd-uhci.c | 11 +++++------
> hw/usb/hcd-uhci.h | 2 +-
> hw/usb/vt82c686-uhci-pci.c | 12 ++++++++++++
> 3 files changed, 18 insertions(+), 7 deletions(-)
>
> diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
> index 0cb02a6432..7201cd0ae7 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"
> @@ -290,7 +291,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,10 +299,8 @@ 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);
> + qemu_set_irq(s->irq, level);
> }
^ OK.
> static void uhci_reset(DeviceState *dev)
> @@ -1170,9 +1169,9 @@ 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);
> + s->irq = pci_allocate_irq(dev);
>
> if (s->masterbus) {
> USBPort *ports[NB_PORTS];
usb_uhci_common_realize() should be refactored making it PCI-agnostic.
> diff --git a/hw/usb/hcd-uhci.h b/hw/usb/hcd-uhci.h
> index e61d8fcb19..1f8ee04186 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;
OK.
> diff --git a/hw/usb/vt82c686-uhci-pci.c b/hw/usb/vt82c686-uhci-pci.c
> index b109c21603..e70e739409 100644
> --- a/hw/usb/vt82c686-uhci-pci.c
> +++ b/hw/usb/vt82c686-uhci-pci.c
> @@ -1,6 +1,16 @@
> #include "qemu/osdep.h"
> +#include "hw/irq.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) {
> + qemu_set_irq(isa_get_irq(NULL, irq), level);
> + }
> +}
OK.
> static void usb_uhci_vt82c686b_realize(PCIDevice *dev, Error **errp)
> {
> UHCIState *s = UHCI(dev);
> @@ -14,6 +24,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);
This can be avoided by refactoring usb_uhci_common_realize(),
uhci_pci_type_info and uhci_data_class_init().
Current TYPE_UHCI becomes TYPE_PCI_UHCI.
Not sure why UHCI has been implemented that way, we already
have USB_OHCI_PCI / USB_EHCI_PCI / USB_XHCI_PCI.
Maybe look at how TYPE_SYSBUS_OHCI is implemented VS TYPE_PCI_OHCI
to be able to implement the similar TYPE_SYSBUS_UHCI?
