[PATCH 0/3] hw/usb: Extract Extract VT82C686 UHCI PCI function into new unit

[PATCH 0/3] hw/usb: Extract Extract VT82C686 UHCI PCI function into new unit

[Philippe Mathieu-Daudé]

The VT82C686 UHCI PCI function should not be exposed it the
VT82C686 south bridge is not present. Extract it out of hcd-uhci.c

Following this discussion:
https://lists.gnu.org/archive/html/qemu-devel/2021-03/msg02923.html

Philippe Mathieu-Daudé (3):
  hw/southbridge: Add missing Kconfig dependency VT82C686 on USB_UHCI
  hw/usb/hcd-uhci: Expose generic prototypes to local header
  hw/usb: Extract VT82C686 UHCI PCI function into new unit

 hw/usb/hcd-uhci.h          | 93 ++++++++++++++++++++++++++++++++++++++
 hw/usb/hcd-uhci.c          | 83 ++--------------------------------
 hw/usb/vt82c686-uhci-pci.c | 43 ++++++++++++++++++
 MAINTAINERS                |  1 +
 hw/isa/Kconfig             |  1 +
 hw/usb/meson.build         |  1 +
 6 files changed, 142 insertions(+), 80 deletions(-)
 create mode 100644 hw/usb/hcd-uhci.h
 create mode 100644 hw/usb/vt82c686-uhci-pci.c

-- 
2.26.2

[PATCH 1/3] hw/southbridge: Add missing Kconfig dependency VT82C686 on USB_UHCI

[Philippe Mathieu-Daudé]

The VT82C686 south bridge provides a USB UHCI bus via a PCI function.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/isa/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
index c7f07854f7e..2691eae2f0c 100644
--- a/hw/isa/Kconfig
+++ b/hw/isa/Kconfig
@@ -47,6 +47,7 @@ config VT82C686
     select ACPI_SMBUS
     select SERIAL_ISA
     select FDC
+    select USB_UHCI
 
 config SMC37C669
     bool
-- 
2.26.2

Re: [PATCH 1/3] hw/southbridge: Add missing Kconfig dependency VT82C686 on USB_UHCI

[BALATON Zoltan]

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

On Tue, 9 Mar 2021, Philippe Mathieu-Daudé wrote:
> The VT82C686 south bridge provides a USB UHCI bus via a PCI function.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Reviewed-by: BALATON Zoltan <balaton@eik.bme.hu>

> ---
> hw/isa/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/hw/isa/Kconfig b/hw/isa/Kconfig
> index c7f07854f7e..2691eae2f0c 100644
> --- a/hw/isa/Kconfig
> +++ b/hw/isa/Kconfig
> @@ -47,6 +47,7 @@ config VT82C686
>     select ACPI_SMBUS
>     select SERIAL_ISA
>     select FDC
> +    select USB_UHCI
>
> config SMC37C669
>     bool
>

Re: [PATCH 1/3] hw/southbridge: Add missing Kconfig dependency VT82C686 on USB_UHCI

[Gerd Hoffmann]

On Tue, Mar 09, 2021 at 08:08:00PM +0100, Philippe Mathieu-Daudé wrote:
> The VT82C686 south bridge provides a USB UHCI bus via a PCI function.
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Added to usb queue.

thanks,
  Gerd

[PATCH 2/3] hw/usb/hcd-uhci: Expose generic prototypes to local header

[Philippe Mathieu-Daudé]

Extract generic UHCI prototypes into a new "hcd-uhci.h" local
header so we can reuse them in other units.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/usb/hcd-uhci.h | 93 +++++++++++++++++++++++++++++++++++++++++++++++
 hw/usb/hcd-uhci.c | 60 ++----------------------------
 2 files changed, 96 insertions(+), 57 deletions(-)
 create mode 100644 hw/usb/hcd-uhci.h

diff --git a/hw/usb/hcd-uhci.h b/hw/usb/hcd-uhci.h
new file mode 100644
index 00000000000..e61d8fcb192
--- /dev/null
+++ b/hw/usb/hcd-uhci.h
@@ -0,0 +1,93 @@
+/*
+ * USB UHCI controller emulation
+ *
+ * Copyright (c) 2005 Fabrice Bellard
+ *
+ * Copyright (c) 2008 Max Krasnyansky
+ *     Magor rewrite of the UHCI data structures parser and frame processor
+ *     Support for fully async operation and multiple outstanding transactions
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+#ifndef HW_USB_HCD_UHCI_H
+#define HW_USB_HCD_UHCI_H
+
+#include "exec/memory.h"
+#include "qemu/timer.h"
+#include "hw/pci/pci.h"
+#include "hw/usb.h"
+
+typedef struct UHCIQueue UHCIQueue;
+
+#define NB_PORTS 2
+
+typedef struct UHCIPort {
+    USBPort port;
+    uint16_t ctrl;
+} UHCIPort;
+
+typedef struct UHCIState {
+    PCIDevice dev;
+    MemoryRegion io_bar;
+    USBBus bus; /* Note unused when we're a companion controller */
+    uint16_t cmd; /* cmd register */
+    uint16_t status;
+    uint16_t intr; /* interrupt enable register */
+    uint16_t frnum; /* frame number */
+    uint32_t fl_base_addr; /* frame list base address */
+    uint8_t sof_timing;
+    uint8_t status2; /* bit 0 and 1 are used to generate UHCI_STS_USBINT */
+    int64_t expire_time;
+    QEMUTimer *frame_timer;
+    QEMUBH *bh;
+    uint32_t frame_bytes;
+    uint32_t frame_bandwidth;
+    bool completions_only;
+    UHCIPort ports[NB_PORTS];
+
+    /* Interrupts that should be raised at the end of the current frame.  */
+    uint32_t pending_int_mask;
+
+    /* Active packets */
+    QTAILQ_HEAD(, UHCIQueue) queues;
+    uint8_t num_ports_vmstate;
+
+    /* Properties */
+    char *masterbus;
+    uint32_t firstport;
+    uint32_t maxframes;
+} UHCIState;
+
+#define TYPE_UHCI "pci-uhci-usb"
+DECLARE_INSTANCE_CHECKER(UHCIState, UHCI, TYPE_UHCI)
+
+typedef struct UHCIInfo {
+    const char *name;
+    uint16_t   vendor_id;
+    uint16_t   device_id;
+    uint8_t    revision;
+    uint8_t    irq_pin;
+    void       (*realize)(PCIDevice *dev, Error **errp);
+    bool       unplug;
+} UHCIInfo;
+
+void uhci_data_class_init(ObjectClass *klass, void *data);
+void usb_uhci_common_realize(PCIDevice *dev, Error **errp);
+
+#endif
diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index 5969eb86b31..d6338c33d86 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -40,6 +40,7 @@
 #include "qemu/main-loop.h"
 #include "qemu/module.h"
 #include "qom/object.h"
+#include "hcd-uhci.h"
 
 #define FRAME_TIMER_FREQ 1000
 
@@ -50,8 +51,6 @@
 
 #define MAX_FRAMES_PER_TICK    (QH_VALID / 2)
 
-#define NB_PORTS 2
-
 enum {
     TD_RESULT_STOP_FRAME = 10,
     TD_RESULT_COMPLETE,
@@ -62,20 +61,8 @@ enum {
 
 typedef struct UHCIState UHCIState;
 typedef struct UHCIAsync UHCIAsync;
-typedef struct UHCIQueue UHCIQueue;
-typedef struct UHCIInfo UHCIInfo;
 typedef struct UHCIPCIDeviceClass UHCIPCIDeviceClass;
 
-struct UHCIInfo {
-    const char *name;
-    uint16_t   vendor_id;
-    uint16_t   device_id;
-    uint8_t    revision;
-    uint8_t    irq_pin;
-    void       (*realize)(PCIDevice *dev, Error **errp);
-    bool       unplug;
-};
-
 struct UHCIPCIDeviceClass {
     PCIDeviceClass parent_class;
     UHCIInfo       info;
@@ -107,43 +94,6 @@ struct UHCIQueue {
     int8_t    valid;
 };
 
-typedef struct UHCIPort {
-    USBPort port;
-    uint16_t ctrl;
-} UHCIPort;
-
-struct UHCIState {
-    PCIDevice dev;
-    MemoryRegion io_bar;
-    USBBus bus; /* Note unused when we're a companion controller */
-    uint16_t cmd; /* cmd register */
-    uint16_t status;
-    uint16_t intr; /* interrupt enable register */
-    uint16_t frnum; /* frame number */
-    uint32_t fl_base_addr; /* frame list base address */
-    uint8_t sof_timing;
-    uint8_t status2; /* bit 0 and 1 are used to generate UHCI_STS_USBINT */
-    int64_t expire_time;
-    QEMUTimer *frame_timer;
-    QEMUBH *bh;
-    uint32_t frame_bytes;
-    uint32_t frame_bandwidth;
-    bool completions_only;
-    UHCIPort ports[NB_PORTS];
-
-    /* Interrupts that should be raised at the end of the current frame.  */
-    uint32_t pending_int_mask;
-
-    /* Active packets */
-    QTAILQ_HEAD(, UHCIQueue) queues;
-    uint8_t num_ports_vmstate;
-
-    /* Properties */
-    char *masterbus;
-    uint32_t firstport;
-    uint32_t maxframes;
-};
-
 typedef struct UHCI_TD {
     uint32_t link;
     uint32_t ctrl; /* see TD_CTRL_xxx */
@@ -160,10 +110,6 @@ static void uhci_async_cancel(UHCIAsync *async);
 static void uhci_queue_fill(UHCIQueue *q, UHCI_TD *td);
 static void uhci_resume(void *opaque);
 
-#define TYPE_UHCI "pci-uhci-usb"
-DECLARE_INSTANCE_CHECKER(UHCIState, UHCI,
-                         TYPE_UHCI)
-
 static inline int32_t uhci_queue_token(UHCI_TD *td)
 {
     if ((td->token & (0xf << 15)) == 0) {
@@ -1213,7 +1159,7 @@ static USBPortOps uhci_port_ops = {
 static USBBusOps uhci_bus_ops = {
 };
 
-static void usb_uhci_common_realize(PCIDevice *dev, Error **errp)
+void usb_uhci_common_realize(PCIDevice *dev, Error **errp)
 {
     Error *err = NULL;
     PCIDeviceClass *pc = PCI_DEVICE_GET_CLASS(dev);
@@ -1335,7 +1281,7 @@ static const TypeInfo uhci_pci_type_info = {
     },
 };
 
-static void uhci_data_class_init(ObjectClass *klass, void *data)
+void uhci_data_class_init(ObjectClass *klass, void *data)
 {
     PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
     DeviceClass *dc = DEVICE_CLASS(klass);
-- 
2.26.2

[PATCH 3/3] hw/usb: Extract VT82C686 UHCI PCI function into a new unit

[Philippe Mathieu-Daudé]

Extract the VT82C686 PCI UHCI function into a new unit so
it is only build when the VT82C686 south bridge is selected.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/usb/hcd-uhci.c          | 23 --------------------
 hw/usb/vt82c686-uhci-pci.c | 43 ++++++++++++++++++++++++++++++++++++++
 MAINTAINERS                |  1 +
 hw/usb/meson.build         |  1 +
 4 files changed, 45 insertions(+), 23 deletions(-)
 create mode 100644 hw/usb/vt82c686-uhci-pci.c

diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
index d6338c33d86..0cb02a64321 100644
--- a/hw/usb/hcd-uhci.c
+++ b/hw/usb/hcd-uhci.c
@@ -1207,21 +1207,6 @@ void usb_uhci_common_realize(PCIDevice *dev, Error **errp)
     pci_register_bar(&s->dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &s->io_bar);
 }
 
-static void usb_uhci_vt82c686b_realize(PCIDevice *dev, Error **errp)
-{
-    UHCIState *s = UHCI(dev);
-    uint8_t *pci_conf = s->dev.config;
-
-    /* USB misc control 1/2 */
-    pci_set_long(pci_conf + 0x40,0x00001000);
-    /* PM capability */
-    pci_set_long(pci_conf + 0x80,0x00020001);
-    /* USB legacy support  */
-    pci_set_long(pci_conf + 0xc0,0x00002000);
-
-    usb_uhci_common_realize(dev, errp);
-}
-
 static void usb_uhci_exit(PCIDevice *dev)
 {
     UHCIState *s = UHCI(dev);
@@ -1318,14 +1303,6 @@ static UHCIInfo uhci_info[] = {
         .revision  = 0x01,
         .irq_pin   = 3,
         .unplug    = true,
-    },{
-        .name      = "vt82c686b-usb-uhci",
-        .vendor_id = PCI_VENDOR_ID_VIA,
-        .device_id = PCI_DEVICE_ID_VIA_UHCI,
-        .revision  = 0x01,
-        .irq_pin   = 3,
-        .realize   = usb_uhci_vt82c686b_realize,
-        .unplug    = true,
     },{
         .name      = "ich9-usb-uhci1", /* 00:1d.0 */
         .vendor_id = PCI_VENDOR_ID_INTEL,
diff --git a/hw/usb/vt82c686-uhci-pci.c b/hw/usb/vt82c686-uhci-pci.c
new file mode 100644
index 00000000000..b109c216033
--- /dev/null
+++ b/hw/usb/vt82c686-uhci-pci.c
@@ -0,0 +1,43 @@
+#include "qemu/osdep.h"
+#include "hcd-uhci.h"
+
+static void usb_uhci_vt82c686b_realize(PCIDevice *dev, Error **errp)
+{
+    UHCIState *s = UHCI(dev);
+    uint8_t *pci_conf = s->dev.config;
+
+    /* USB misc control 1/2 */
+    pci_set_long(pci_conf + 0x40, 0x00001000);
+    /* PM capability */
+    pci_set_long(pci_conf + 0x80, 0x00020001);
+    /* USB legacy support  */
+    pci_set_long(pci_conf + 0xc0, 0x00002000);
+
+    usb_uhci_common_realize(dev, errp);
+}
+
+static UHCIInfo uhci_info[] = {
+    {
+        .name      = "vt82c686b-usb-uhci",
+        .vendor_id = PCI_VENDOR_ID_VIA,
+        .device_id = PCI_DEVICE_ID_VIA_UHCI,
+        .revision  = 0x01,
+        .irq_pin   = 3,
+        .realize   = usb_uhci_vt82c686b_realize,
+        .unplug    = true,
+    }
+};
+
+static const TypeInfo vt82c686b_usb_uhci_type_info = {
+    .parent         = TYPE_UHCI,
+    .name           = "vt82c686b-usb-uhci",
+    .class_init     = uhci_data_class_init,
+    .class_data     = uhci_info,
+};
+
+static void vt82c686b_usb_uhci_register_types(void)
+{
+    type_register_static(&vt82c686b_usb_uhci_type_info);
+}
+
+type_init(vt82c686b_usb_uhci_register_types)
diff --git a/MAINTAINERS b/MAINTAINERS
index f22d83c1782..6fd55c0a40c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1173,6 +1173,7 @@ S: Odd Fixes
 F: hw/mips/fuloong2e.c
 F: hw/isa/vt82c686.c
 F: hw/pci-host/bonito.c
+F: hw/usb/vt82c686-uhci-pci.c
 F: include/hw/isa/vt82c686.h
 
 Loongson-3 virtual platforms
diff --git a/hw/usb/meson.build b/hw/usb/meson.build
index 653192cff6f..6e3159798e9 100644
--- a/hw/usb/meson.build
+++ b/hw/usb/meson.build
@@ -32,6 +32,7 @@
 softmmu_ss.add(when: 'CONFIG_TUSB6010', if_true: files('tusb6010.c'))
 softmmu_ss.add(when: 'CONFIG_IMX', if_true: files('chipidea.c'))
 softmmu_ss.add(when: 'CONFIG_IMX_USBPHY', if_true: files('imx-usb-phy.c'))
+softmmu_ss.add(when: 'CONFIG_VT82C686', if_true: files('vt82c686-uhci-pci.c'))
 specific_ss.add(when: 'CONFIG_XLNX_VERSAL', if_true: files('xlnx-versal-usb2-ctrl-regs.c'))
 specific_ss.add(when: 'CONFIG_XLNX_USB_SUBSYS', if_true: files('xlnx-usb-subsystem.c'))
 
-- 
2.26.2

Re: [PATCH 3/3] hw/usb: Extract VT82C686 UHCI PCI function into a new unit

[BALATON Zoltan]

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

On Tue, 9 Mar 2021, Philippe Mathieu-Daudé wrote:
> Extract the VT82C686 PCI UHCI function into a new unit so
> it is only build when the VT82C686 south bridge is selected.

I'm not sure it's worth separating just this one device from the other 
similar usb devices when the others that are also part of south bridge 
chips are left there. Maybe you could just set user_creatable = false so 
it can only be created as part of the chips that contain it or just don't 
bother and leave it as it is which is the least likely to break anything 
that may rely on it as removing it from the device list may need to go 
through deprecation.

But I don't really mind, so if others like this approach I don't want to 
block the patch. I think it's unlikely anybody would use this device other 
than part of fuloong2e or pegasos2 so probably it's unlikely to break 
anything if it suddenly goes away from a new build.

Regards,
BALATON Zoltan

> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> hw/usb/hcd-uhci.c          | 23 --------------------
> hw/usb/vt82c686-uhci-pci.c | 43 ++++++++++++++++++++++++++++++++++++++
> MAINTAINERS                |  1 +
> hw/usb/meson.build         |  1 +
> 4 files changed, 45 insertions(+), 23 deletions(-)
> create mode 100644 hw/usb/vt82c686-uhci-pci.c
>
> diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
> index d6338c33d86..0cb02a64321 100644
> --- a/hw/usb/hcd-uhci.c
> +++ b/hw/usb/hcd-uhci.c
> @@ -1207,21 +1207,6 @@ void usb_uhci_common_realize(PCIDevice *dev, Error **errp)
>     pci_register_bar(&s->dev, 4, PCI_BASE_ADDRESS_SPACE_IO, &s->io_bar);
> }
>
> -static void usb_uhci_vt82c686b_realize(PCIDevice *dev, Error **errp)
> -{
> -    UHCIState *s = UHCI(dev);
> -    uint8_t *pci_conf = s->dev.config;
> -
> -    /* USB misc control 1/2 */
> -    pci_set_long(pci_conf + 0x40,0x00001000);
> -    /* PM capability */
> -    pci_set_long(pci_conf + 0x80,0x00020001);
> -    /* USB legacy support  */
> -    pci_set_long(pci_conf + 0xc0,0x00002000);
> -
> -    usb_uhci_common_realize(dev, errp);
> -}
> -
> static void usb_uhci_exit(PCIDevice *dev)
> {
>     UHCIState *s = UHCI(dev);
> @@ -1318,14 +1303,6 @@ static UHCIInfo uhci_info[] = {
>         .revision  = 0x01,
>         .irq_pin   = 3,
>         .unplug    = true,
> -    },{
> -        .name      = "vt82c686b-usb-uhci",
> -        .vendor_id = PCI_VENDOR_ID_VIA,
> -        .device_id = PCI_DEVICE_ID_VIA_UHCI,
> -        .revision  = 0x01,
> -        .irq_pin   = 3,
> -        .realize   = usb_uhci_vt82c686b_realize,
> -        .unplug    = true,
>     },{
>         .name      = "ich9-usb-uhci1", /* 00:1d.0 */
>         .vendor_id = PCI_VENDOR_ID_INTEL,
> diff --git a/hw/usb/vt82c686-uhci-pci.c b/hw/usb/vt82c686-uhci-pci.c
> new file mode 100644
> index 00000000000..b109c216033
> --- /dev/null
> +++ b/hw/usb/vt82c686-uhci-pci.c
> @@ -0,0 +1,43 @@
> +#include "qemu/osdep.h"
> +#include "hcd-uhci.h"
> +
> +static void usb_uhci_vt82c686b_realize(PCIDevice *dev, Error **errp)
> +{
> +    UHCIState *s = UHCI(dev);
> +    uint8_t *pci_conf = s->dev.config;
> +
> +    /* USB misc control 1/2 */
> +    pci_set_long(pci_conf + 0x40, 0x00001000);
> +    /* PM capability */
> +    pci_set_long(pci_conf + 0x80, 0x00020001);
> +    /* USB legacy support  */
> +    pci_set_long(pci_conf + 0xc0, 0x00002000);
> +
> +    usb_uhci_common_realize(dev, errp);
> +}
> +
> +static UHCIInfo uhci_info[] = {
> +    {
> +        .name      = "vt82c686b-usb-uhci",
> +        .vendor_id = PCI_VENDOR_ID_VIA,
> +        .device_id = PCI_DEVICE_ID_VIA_UHCI,
> +        .revision  = 0x01,
> +        .irq_pin   = 3,
> +        .realize   = usb_uhci_vt82c686b_realize,
> +        .unplug    = true,
> +    }
> +};
> +
> +static const TypeInfo vt82c686b_usb_uhci_type_info = {
> +    .parent         = TYPE_UHCI,
> +    .name           = "vt82c686b-usb-uhci",
> +    .class_init     = uhci_data_class_init,
> +    .class_data     = uhci_info,
> +};
> +
> +static void vt82c686b_usb_uhci_register_types(void)
> +{
> +    type_register_static(&vt82c686b_usb_uhci_type_info);
> +}
> +
> +type_init(vt82c686b_usb_uhci_register_types)
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f22d83c1782..6fd55c0a40c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1173,6 +1173,7 @@ S: Odd Fixes
> F: hw/mips/fuloong2e.c
> F: hw/isa/vt82c686.c
> F: hw/pci-host/bonito.c
> +F: hw/usb/vt82c686-uhci-pci.c
> F: include/hw/isa/vt82c686.h
>
> Loongson-3 virtual platforms
> diff --git a/hw/usb/meson.build b/hw/usb/meson.build
> index 653192cff6f..6e3159798e9 100644
> --- a/hw/usb/meson.build
> +++ b/hw/usb/meson.build
> @@ -32,6 +32,7 @@
> softmmu_ss.add(when: 'CONFIG_TUSB6010', if_true: files('tusb6010.c'))
> softmmu_ss.add(when: 'CONFIG_IMX', if_true: files('chipidea.c'))
> softmmu_ss.add(when: 'CONFIG_IMX_USBPHY', if_true: files('imx-usb-phy.c'))
> +softmmu_ss.add(when: 'CONFIG_VT82C686', if_true: files('vt82c686-uhci-pci.c'))
> specific_ss.add(when: 'CONFIG_XLNX_VERSAL', if_true: files('xlnx-versal-usb2-ctrl-regs.c'))
> specific_ss.add(when: 'CONFIG_XLNX_USB_SUBSYS', if_true: files('xlnx-usb-subsystem.c'))
>
>

Re: [PATCH 3/3] hw/usb: Extract VT82C686 UHCI PCI function into a new unit

[Philippe Mathieu-Daudé]

On 3/9/21 9:13 PM, BALATON Zoltan wrote:
> On Tue, 9 Mar 2021, Philippe Mathieu-Daudé wrote:
>> Extract the VT82C686 PCI UHCI function into a new unit so
>> it is only build when the VT82C686 south bridge is selected.
> 
> I'm not sure it's worth separating just this one device from the other
> similar usb devices when the others that are also part of south bridge
> chips are left there. Maybe you could just set user_creatable = false so
> it can only be created as part of the chips that contain it or just
> don't bother and leave it as it is which is the least likely to break
> anything that may rely on it as removing it from the device list may
> need to go through deprecation.
> 
> But I don't really mind, so if others like this approach I don't want to
> block the patch. I think it's unlikely anybody would use this device
> other than part of fuloong2e or pegasos2 so probably it's unlikely to
> break anything if it suddenly goes away from a new build.

OK. As I sent this series to help you with your Pegasos2 board, do you
mind sending a patch then?

I suppose you mean using "#include CONFIG_DEVICES" and checking for the
CONFIG_VT82C686 symbol to register the QOM type?

Thanks,

Phil.

Re: [PATCH 3/3] hw/usb: Extract VT82C686 UHCI PCI function into a new unit

[BALATON Zoltan]

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

On Wed, 10 Mar 2021, Philippe Mathieu-Daudé wrote:
> On 3/9/21 9:13 PM, BALATON Zoltan wrote:
>> On Tue, 9 Mar 2021, Philippe Mathieu-Daudé wrote:
>>> Extract the VT82C686 PCI UHCI function into a new unit so
>>> it is only build when the VT82C686 south bridge is selected.
>>
>> I'm not sure it's worth separating just this one device from the other
>> similar usb devices when the others that are also part of south bridge
>> chips are left there. Maybe you could just set user_creatable = false so
>> it can only be created as part of the chips that contain it or just
>> don't bother and leave it as it is which is the least likely to break
>> anything that may rely on it as removing it from the device list may
>> need to go through deprecation.
>>
>> But I don't really mind, so if others like this approach I don't want to
>> block the patch. I think it's unlikely anybody would use this device
>> other than part of fuloong2e or pegasos2 so probably it's unlikely to
>> break anything if it suddenly goes away from a new build.
>
> OK. As I sent this series to help you with your Pegasos2 board, do you
> mind sending a patch then?
>
> I suppose you mean using "#include CONFIG_DEVICES" and checking for the
> CONFIG_VT82C686 symbol to register the QOM type?

I don't see how this should help with the pegasos2 series as that's 
already working without this change. I really just meant I don't see this 
would improve much when you leave the piix and ich ones there that are the 
same for other south bridges so this patch makes one of these configurable 
but not the others, that makes them different when they are now similar. 
Additionally it may break something as it changes the availability of this 
device (although that's very unlikely to cause problems in practice). So 
I'd just leave this alone for now and not change it at all until all of 
these are cleaned up in a uniform way at some later point if that's deemed 
useful or desirable. Maybe I'm missing the point though.

But while pegasos2 does not need this change it should also not break it 
so I don't really mind.

Regards,
BALATON Zoltan