Patchwork [v1,5/8] xilinx_zynq: add USB controllers

Copying patch inline to make some comments on it


On Fri, Oct 26, 2012 at 12:10 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>   Hi,
>
>> I'll go try that to simplify uhci ...
>
> Seems to work ...
>
> cheers,
>   Gerd
>
>

On Fri, Oct 26, 2012 at 9:54 AM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> 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 <kraxel@redhat.com> wrote:
>>   Hi,
>>
>>> I'll go try that to simplify uhci ...
>>
>> Seems to work ...
>>
>> cheers,
>>   Gerd
>>
>>

Hi,

>> @@ -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).

I think we can also create TypeInfo at runtime (uhci_register_types in
the uhci case).  We'll just have to stick the name into UHCIInfo, build
a static uhciinfo array, then generate & register TypeInfo from that.

cheers,
  Gerd

Next version up on list, Ive changed the approach a bit.

On Fri, Oct 26, 2012 at 4:49 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>   Hi,
>
>>> @@ -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).
>
> I think we can also create TypeInfo at runtime (uhci_register_types in
> the uhci case).
Im comming I think this approach is ok for a small number of variants,
but for the large ones creating type_infos dynamically may be the way
to go.

> We'll just have to stick the name into UHCIInfo, build
> a static uhciinfo array, then generate & register TypeInfo from that.
>
Instead of create a new struct definition (e.g. UHCIInfo), I just used
the parent class definition (you would use PCIDeviceClass in the UHCI
case). EHCI was a little more complex cos of the multiple parents, but
UHCI you should just be able to pass a partially populated
PCIDeviceClass to class_data that class_init can pick fields out of.

> cheers,
>   Gerd
>
>