Re: [Qemu-devel] [PATCH v1 5/8] xilinx_zynq: add USB controllers

[Peter Crosthwaite]

On Fri, Oct 26, 2012 at 9:54 AM, Peter Crosthwaite
<address@hidden> wrote:
> Copying patch inline to make some comments on it
>
> diff --git a/hw/usb/hcd-uhci.c b/hw/usb/hcd-uhci.c
> index b6b972f..64442a4 100644
> --- a/hw/usb/hcd-uhci.c
> +++ b/hw/usb/hcd-uhci.c
> @@ -88,6 +88,13 @@ enum {
>  typedef struct UHCIState UHCIState;
>  typedef struct UHCIAsync UHCIAsync;
>  typedef struct UHCIQueue UHCIQueue;
> +typedef struct UHCIInfo UHCIInfo;
> +
> +struct UHCIInfo {
> +    uint16_t vendor_id;
> +    uint16_t device_id;
> +    uint8_t  revision;
> +};
>

Theres nothing PCI specific here. Should this be defined somewhere in
the PCI layers so PCI classes. First thing I had to do for EHCI
equivalent was copy paste this s/UHCI/EHCI with no other change.

>  /*
>   * Pending async transaction.
> @@ -1293,17 +1300,18 @@ static Property uhci_properties[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> -static void piix3_uhci_class_init(ObjectClass *klass, void *data)
> +static void uhci_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +    UHCIInfo *info = data;
>
>      k->init = usb_uhci_common_initfn;
>      k->exit = usb_uhci_exit;
> -    k->vendor_id = PCI_VENDOR_ID_INTEL;
> -    k->device_id = PCI_DEVICE_ID_INTEL_82371SB_2;
> -    k->revision = 0x01;
> -    k->class_id = PCI_CLASS_SERIAL_USB;
> +    k->vendor_id = info->vendor_id;
> +    k->device_id = info->device_id;
> +    k->revision  = info->revision;
> +    k->class_id  = PCI_CLASS_SERIAL_USB;
>      dc->vmsd = &vmstate_uhci;
>      dc->props = uhci_properties;
>  }
> @@ -1312,29 +1320,24 @@ static TypeInfo piix3_uhci_info = {
>      .name          = "piix3-usb-uhci",
>      .parent        = TYPE_PCI_DEVICE,
>      .instance_size = sizeof(UHCIState),
> -    .class_init    = piix3_uhci_class_init,
> +    .class_init    = uhci_class_init,

Therese three elements (parent, instance_size, class_init) are
repeated from one definition to the next. This will get tedious when
we eventually come to make the same fix for some of the other devices
that have large numbers of variants (pflash_cfi0x and m25p80 being
some of the angrier ones).

Regards,
Peter

> +    .class_data    = (UHCIInfo[]) { {
> +            .vendor_id = PCI_VENDOR_ID_INTEL,
> +            .device_id = PCI_DEVICE_ID_INTEL_82371SB_2,
> +            .revision  = 0x01,
> +     } },
>  };
>
> -static void piix4_uhci_class_init(ObjectClass *klass, void *data)
> -{
> -    DeviceClass *dc = DEVICE_CLASS(klass);
> -    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> -
> -    k->init = usb_uhci_common_initfn;
> -    k->exit = usb_uhci_exit;
> -    k->vendor_id = PCI_VENDOR_ID_INTEL;
> -    k->device_id = PCI_DEVICE_ID_INTEL_82371AB_2;
> -    k->revision = 0x01;
> -    k->class_id = PCI_CLASS_SERIAL_USB;
> -    dc->vmsd = &vmstate_uhci;
> -    dc->props = uhci_properties;
> -}
> -
>  static TypeInfo piix4_uhci_info = {
>      .name          = "piix4-usb-uhci",
>      .parent        = TYPE_PCI_DEVICE,
>      .instance_size = sizeof(UHCIState),
> -    .class_init    = piix4_uhci_class_init,
> +    .class_init    = uhci_class_init,
> +    .class_data    = (UHCIInfo[]) { {
> +            .vendor_id = PCI_VENDOR_ID_INTEL,
> +            .device_id = PCI_DEVICE_ID_INTEL_82371AB_2,
> +            .revision  = 0x01,
> +     } },
>  };
>
>  static void vt82c686b_uhci_class_init(ObjectClass *klass, void *data)
>
> On Fri, Oct 26, 2012 at 12:10 AM, Gerd Hoffmann <address@hidden> wrote:
>>   Hi,
>>
>>> I'll go try that to simplify uhci ...
>>
>> Seems to work ...
>>
>> cheers,
>>   Gerd
>>
>>