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

[BALATON Zoltan]

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'))