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

[BALATON Zoltan <balaton@eik.bme.hu>]

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

On Wed, 13 Oct 2021, Philippe Mathieu-Daudé wrote:
> 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 <balaton@eik.bme.hu>
>> Reviewed-by: Jiaxun Yang <jiaxun.yang@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.

The reason for that may be that those have sysbus versions and pci 
versions and maybe there was no need for a sysbus UHCI version as all of 
those we emulate are PCI? I think those were probably also like UHCI 
before but when a sysbus version was needed they were split.

Regards,
BALATON Zoltan