* [PATCH v3] hw/usb/vt82c686-uhci-pci: Use ISA instead of PCI interrupts
@ 2021-10-13 12:13 BALATON Zoltan
2021-10-13 14:14 ` Philippe Mathieu-Daudé
2021-10-14 9:10 ` Gerd Hoffmann
0 siblings, 2 replies; 12+ messages in thread
From: BALATON Zoltan @ 2021-10-13 12:13 UTC (permalink / raw)
To: qemu-devel; +Cc: Huacai Chen, Gerd Hoffmann, Philippe M-D
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);
}
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];
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;
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);
+ }
+}
+
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);
}
static UHCIInfo uhci_info[] = {
--
2.21.4
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v3] hw/usb/vt82c686-uhci-pci: Use ISA instead of PCI interrupts
2021-10-13 12:13 [PATCH v3] hw/usb/vt82c686-uhci-pci: Use ISA instead of PCI interrupts BALATON Zoltan
@ 2021-10-13 14:14 ` Philippe Mathieu-Daudé
2021-10-13 15:21 ` BALATON Zoltan
` (2 more replies)
2021-10-14 9:10 ` Gerd Hoffmann
1 sibling, 3 replies; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-10-13 14:14 UTC (permalink / raw)
To: BALATON Zoltan, qemu-devel; +Cc: Huacai Chen, Gerd Hoffmann
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.
Maybe look at how TYPE_SYSBUS_OHCI is implemented VS TYPE_PCI_OHCI
to be able to implement the similar TYPE_SYSBUS_UHCI?
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v3] hw/usb/vt82c686-uhci-pci: Use ISA instead of PCI interrupts
2021-10-13 14:14 ` Philippe Mathieu-Daudé
@ 2021-10-13 15:21 ` BALATON Zoltan
2021-10-13 15:39 ` BALATON Zoltan
2021-10-13 15:26 ` BALATON Zoltan
2021-10-14 9:12 ` Gerd Hoffmann
2 siblings, 1 reply; 12+ messages in thread
From: BALATON Zoltan @ 2021-10-13 15:21 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: Huacai Chen, qemu-devel, Gerd Hoffmann
[-- Attachment #1: Type: text/plain, Size: 5281 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.
>
> Maybe look at how TYPE_SYSBUS_OHCI is implemented VS TYPE_PCI_OHCI
> to be able to implement the similar TYPE_SYSBUS_UHCI?
That doesn't seem to be part of fixing this bug with vt82c686-uhci-pci. Do
I really have to do that much refactoring of UHCI model just to make it
work with the case I care about? If this was good up to now it should be
good enough until somebody can do this refactoring independent of this
patch as a follow up. I may not have time for that. I'd like to improve
pegasos2 emulation by fixing this bug for 6.2 but there's a limit on how
much unrelated stuff I'm willing to do for that. Let's say this bug
uncovered a possible improvement in the uhci model so note it somewhere
(like bite-sized task on wiki) then let somebody who has time handle it.
This should not be reason to block fixing a bug if the fix is otherwise
acceptable.
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v3] hw/usb/vt82c686-uhci-pci: Use ISA instead of PCI interrupts
2021-10-13 15:21 ` BALATON Zoltan
@ 2021-10-13 15:39 ` BALATON Zoltan
0 siblings, 0 replies; 12+ messages in thread
From: BALATON Zoltan @ 2021-10-13 15:39 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: Huacai Chen, qemu-devel, Gerd Hoffmann
[-- Attachment #1: Type: text/plain, Size: 5781 bytes --]
On Wed, 13 Oct 2021, BALATON Zoltan wrote:
> 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.
>>
>> Maybe look at how TYPE_SYSBUS_OHCI is implemented VS TYPE_PCI_OHCI
>> to be able to implement the similar TYPE_SYSBUS_UHCI?
>
> That doesn't seem to be part of fixing this bug with vt82c686-uhci-pci. Do I
> really have to do that much refactoring of UHCI model just to make it work
> with the case I care about? If this was good up to now it should be good
> enough until somebody can do this refactoring independent of this patch as a
> follow up. I may not have time for that. I'd like to improve pegasos2
> emulation by fixing this bug for 6.2 but there's a limit on how much
> unrelated stuff I'm willing to do for that. Let's say this bug uncovered a
> possible improvement in the uhci model so note it somewhere (like bite-sized
> task on wiki) then let somebody who has time handle it. This should not be
> reason to block fixing a bug if the fix is otherwise acceptable.
If you don't like replacing irq with object_unref another possible way is
adding it to UHCIInfo simlar to how realize is set, I can do that if you
think that would be better but changing qdev stuff is not something I want
to do as that tends to get complex and takes hours.
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] hw/usb/vt82c686-uhci-pci: Use ISA instead of PCI interrupts
2021-10-13 14:14 ` Philippe Mathieu-Daudé
2021-10-13 15:21 ` BALATON Zoltan
@ 2021-10-13 15:26 ` BALATON Zoltan
2021-10-14 9:12 ` Gerd Hoffmann
2 siblings, 0 replies; 12+ messages in thread
From: BALATON Zoltan @ 2021-10-13 15:26 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: Huacai Chen, qemu-devel, Gerd Hoffmann
[-- 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
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v3] hw/usb/vt82c686-uhci-pci: Use ISA instead of PCI interrupts
2021-10-13 14:14 ` Philippe Mathieu-Daudé
2021-10-13 15:21 ` BALATON Zoltan
2021-10-13 15:26 ` BALATON Zoltan
@ 2021-10-14 9:12 ` Gerd Hoffmann
2021-10-14 13:28 ` Philippe Mathieu-Daudé
2 siblings, 1 reply; 12+ messages in thread
From: Gerd Hoffmann @ 2021-10-14 9:12 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: Huacai Chen, qemu-devel
Hi,
> > if (s->masterbus) {
> > USBPort *ports[NB_PORTS];
>
> usb_uhci_common_realize() should be refactored making it PCI-agnostic.
Not sure this is needed here. This seems to be more a platform-specific
oddity in IRQ routing than a non-pci UHCI device.
take care,
Gerd
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v3] hw/usb/vt82c686-uhci-pci: Use ISA instead of PCI interrupts
2021-10-14 9:12 ` Gerd Hoffmann
@ 2021-10-14 13:28 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-10-14 13:28 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: Huacai Chen, qemu-devel
On 10/14/21 11:12, Gerd Hoffmann wrote:
> Hi,
>
>>> if (s->masterbus) {
>>> USBPort *ports[NB_PORTS];
>>
>> usb_uhci_common_realize() should be refactored making it PCI-agnostic.
>
> Not sure this is needed here. This seems to be more a platform-specific
> oddity in IRQ routing than a non-pci UHCI device.
OK, fine then.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] hw/usb/vt82c686-uhci-pci: Use ISA instead of PCI interrupts
2021-10-13 12:13 [PATCH v3] hw/usb/vt82c686-uhci-pci: Use ISA instead of PCI interrupts BALATON Zoltan
2021-10-13 14:14 ` Philippe Mathieu-Daudé
@ 2021-10-14 9:10 ` Gerd Hoffmann
2021-10-14 10:22 ` BALATON Zoltan
1 sibling, 1 reply; 12+ messages in thread
From: Gerd Hoffmann @ 2021-10-14 9:10 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: Huacai Chen, qemu-devel, Philippe M-D
On Wed, Oct 13, 2021 at 02:13:09PM +0200, 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.
Looks good. There are some unrelated changes in though (whitespace,
comments, ...), and the vt82c686-uhci-pci.c changes should be a
separate patch.
thanks,
Gerd
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] hw/usb/vt82c686-uhci-pci: Use ISA instead of PCI interrupts
2021-10-14 9:10 ` Gerd Hoffmann
@ 2021-10-14 10:22 ` BALATON Zoltan
2021-10-14 13:01 ` Gerd Hoffmann
0 siblings, 1 reply; 12+ messages in thread
From: BALATON Zoltan @ 2021-10-14 10:22 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: Huacai Chen, qemu-devel, Philippe M-D
On Thu, 14 Oct 2021, Gerd Hoffmann wrote:
> On Wed, Oct 13, 2021 at 02:13:09PM +0200, 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.
>
> Looks good. There are some unrelated changes in though (whitespace,
> comments, ...), and the vt82c686-uhci-pci.c changes should be a
> separate patch.
So you mean split it into a series of three small patches? Should I do a
w4 with that?
Regards,
BALATON Zoltan
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] hw/usb/vt82c686-uhci-pci: Use ISA instead of PCI interrupts
2021-10-14 10:22 ` BALATON Zoltan
@ 2021-10-14 13:01 ` Gerd Hoffmann
2021-10-14 13:08 ` BALATON Zoltan
0 siblings, 1 reply; 12+ messages in thread
From: Gerd Hoffmann @ 2021-10-14 13:01 UTC (permalink / raw)
To: BALATON Zoltan; +Cc: Huacai Chen, qemu-devel, Philippe M-D
On Thu, Oct 14, 2021 at 12:22:58PM +0200, BALATON Zoltan wrote:
> On Thu, 14 Oct 2021, Gerd Hoffmann wrote:
> > On Wed, Oct 13, 2021 at 02:13:09PM +0200, 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.
> >
> > Looks good. There are some unrelated changes in though (whitespace,
> > comments, ...), and the vt82c686-uhci-pci.c changes should be a
> > separate patch.
>
> So you mean split it into a series of three small patches? Should I do a w4
> with that?
I was thinking about two patches: drop the unrelated stuff, one patch
for the irq signaling change, and one for the vt82c686 changes.
But of course you can add more patches for the other changes, i.e.
dropping the else branch for level = 0 and other small tweaks.
take care,
Gerd
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] hw/usb/vt82c686-uhci-pci: Use ISA instead of PCI interrupts
2021-10-14 13:01 ` Gerd Hoffmann
@ 2021-10-14 13:08 ` BALATON Zoltan
2021-10-14 17:48 ` BALATON Zoltan
0 siblings, 1 reply; 12+ messages in thread
From: BALATON Zoltan @ 2021-10-14 13:08 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: Huacai Chen, qemu-devel, Philippe M-D
On Thu, 14 Oct 2021, Gerd Hoffmann wrote:
> On Thu, Oct 14, 2021 at 12:22:58PM +0200, BALATON Zoltan wrote:
>> On Thu, 14 Oct 2021, Gerd Hoffmann wrote:
>>> On Wed, Oct 13, 2021 at 02:13:09PM +0200, 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.
>>>
>>> Looks good. There are some unrelated changes in though (whitespace,
>>> comments, ...), and the vt82c686-uhci-pci.c changes should be a
>>> separate patch.
>>
>> So you mean split it into a series of three small patches? Should I do a w4
>> with that?
>
> I was thinking about two patches: drop the unrelated stuff, one patch
> for the irq signaling change, and one for the vt82c686 changes.
>
> But of course you can add more patches for the other changes, i.e.
> dropping the else branch for level = 0 and other small tweaks.
The tewak for the comment is needed for checkpach, dropping the else is
just to make the function shorter and IMO more readable. I can do this in
a previous patch so checkpatch won't complain on the pci irq change or
need at least the comment change in that patch. I'm thinking about an
alternative way for the vt82c686 part to avoid using isa_get_irq which has
a comment saying it should be dropped but not sure about that yet, I'll
try it and submit a v4 with at least breaking it up to smaller patches.
Thanks,
BALATON Zoltan
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v3] hw/usb/vt82c686-uhci-pci: Use ISA instead of PCI interrupts
2021-10-14 13:08 ` BALATON Zoltan
@ 2021-10-14 17:48 ` BALATON Zoltan
0 siblings, 0 replies; 12+ messages in thread
From: BALATON Zoltan @ 2021-10-14 17:48 UTC (permalink / raw)
To: Gerd Hoffmann; +Cc: Huacai Chen, qemu-devel, Philippe M-D
On Thu, 14 Oct 2021, BALATON Zoltan wrote:
> On Thu, 14 Oct 2021, Gerd Hoffmann wrote:
>> On Thu, Oct 14, 2021 at 12:22:58PM +0200, BALATON Zoltan wrote:
>>> On Thu, 14 Oct 2021, Gerd Hoffmann wrote:
>>>> On Wed, Oct 13, 2021 at 02:13:09PM +0200, 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.
>>>>
>>>> Looks good. There are some unrelated changes in though (whitespace,
>>>> comments, ...), and the vt82c686-uhci-pci.c changes should be a
>>>> separate patch.
>>>
>>> So you mean split it into a series of three small patches? Should I do a
>>> w4
>>> with that?
>>
>> I was thinking about two patches: drop the unrelated stuff, one patch
>> for the irq signaling change, and one for the vt82c686 changes.
>>
>> But of course you can add more patches for the other changes, i.e.
>> dropping the else branch for level = 0 and other small tweaks.
>
> The tewak for the comment is needed for checkpach, dropping the else is just
> to make the function shorter and IMO more readable. I can do this in a
> previous patch so checkpatch won't complain on the pci irq change or need at
> least the comment change in that patch. I'm thinking about an alternative way
> for the vt82c686 part to avoid using isa_get_irq which has a comment saying
> it should be dropped but not sure about that yet, I'll try it and submit a v4
> with at least breaking it up to smaller patches.
I've tried it and it's just not worth it. By moving isa reference to
vt82c686.c we end up doing the same, just with a lot more code changes
(which is harder to understand) and less efficiently (at every interrupt
need to jump through hoops and do some QOM casts and additional calls to
reach the same effect). These are basically modelling functions of the
same device just split up in parts to categorise and reuse parts of
similar models so accesing isa parts of the chip from usb part does not
seem to be that bad and it's simple this way. So I ended up just splitting
the patch for v4.
Thanks,
BALATON Zoltan
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-10-14 17:49 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-13 12:13 [PATCH v3] hw/usb/vt82c686-uhci-pci: Use ISA instead of PCI interrupts BALATON Zoltan
2021-10-13 14:14 ` Philippe Mathieu-Daudé
2021-10-13 15:21 ` BALATON Zoltan
2021-10-13 15:39 ` BALATON Zoltan
2021-10-13 15:26 ` BALATON Zoltan
2021-10-14 9:12 ` Gerd Hoffmann
2021-10-14 13:28 ` Philippe Mathieu-Daudé
2021-10-14 9:10 ` Gerd Hoffmann
2021-10-14 10:22 ` BALATON Zoltan
2021-10-14 13:01 ` Gerd Hoffmann
2021-10-14 13:08 ` BALATON Zoltan
2021-10-14 17:48 ` BALATON Zoltan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.