From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:56221) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TSpHS-00025M-Ez for qemu-devel@nongnu.org; Mon, 29 Oct 2012 09:21:32 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TSpHL-0000Yc-9Z for qemu-devel@nongnu.org; Mon, 29 Oct 2012 09:21:26 -0400 Received: from cantor2.suse.de ([195.135.220.15]:59147 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TSpHK-0000YF-SX for qemu-devel@nongnu.org; Mon, 29 Oct 2012 09:21:19 -0400 Message-ID: <508E82C9.5070303@suse.de> Date: Mon, 29 Oct 2012 14:21:13 +0100 From: =?ISO-8859-1?Q?Andreas_F=E4rber?= MIME-Version: 1.0 References: <508E4DD6.2030407@suse.de> In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v3 2/8] usb/ehci: Use class_data to init PCI variations List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Crosthwaite Cc: vineshp@xilinx.com, peter.maydell@linaro.org, Jason Baron , qemu-devel@nongnu.org, john.williams@xilinx.com, Gerd Hoffmann , Anthony Liguori , edgar.iglesias@gmail.com Am 29.10.2012 12:43, schrieb Peter Crosthwaite: >=20 > On Oct 29, 2012 7:35 PM, "Andreas F=E4rber" > wrote: >> >> Am 29.10.2012 02:34, schrieb Peter Crosthwaite: >> > Got rid of the duplication of the class init functions for the two > PCI EHCI >> > variants. The PCI specifics are passed in as as class_data and set > by a common >> > class_init function. >> > >> > Premeptively defined a new Class "EHCICLass" for the upcomming > addition of new >> >> "Preemptively", "upcoming" >> >> > fields. The class_data is an instance of EHCICLass that forms a > template for the >> > class to generate. >> >> Using "EHCI[PCI]Class" to template itself seems a bit awkward, Anthony >> do you have any thoughts on this? The usual way would be to have a >> dedicated EHCIInfo struct or so. >=20 > Why? The class struct defines the exactly all information needed. Seems > redundant and error prone to have to maintain two structs with the same > fields? This was a general discussion, involving Anthony and others. One reason I can think of is that PCIDeviceClass is much larger than the actual values you are interested in. I once did such a hack involving XtensaCPUClass and worked around it for applying I believe. >> > >> > Signed-off-by: Peter Crosthwaite > >> > --- >> > Got rid of union for sharing EHCIClassDefinition - made PCI specific >> > Simplified literal class_data arrays in ehci_info accordingly >> > removed null sentinel from ehci_info and used ARRAY_SIZE for > type_regsiter loop >> > bound instead >> > >> > hw/usb/hcd-ehci.c | 76 > ++++++++++++++++++++++++++++------------------------ >> > 1 files changed, 41 insertions(+), 35 deletions(-) >> > >> > diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c >> > index 6c65a73..274225b 100644 >> > --- a/hw/usb/hcd-ehci.c >> > +++ b/hw/usb/hcd-ehci.c >> > @@ -2641,46 +2641,49 @@ static Property ehci_properties[] =3D { >> > DEFINE_PROP_END_OF_LIST(), >> > }; >> > >> > -static void ehci_class_init(ObjectClass *klass, void *data) >> > -{ >> > - DeviceClass *dc =3D DEVICE_CLASS(klass); >> > - PCIDeviceClass *k =3D PCI_DEVICE_CLASS(klass); >> > - >> > - k->init =3D usb_ehci_initfn; >> > - k->vendor_id =3D PCI_VENDOR_ID_INTEL; >> > - k->device_id =3D PCI_DEVICE_ID_INTEL_82801D; /* ich4 */ >> > - k->revision =3D 0x10; >> > - k->class_id =3D PCI_CLASS_SERIAL_USB; >> > - dc->vmsd =3D &vmstate_ehci; >> > - dc->props =3D ehci_properties; >> > -} >> > +typedef struct EHCIPCIClass { >> > + PCIDeviceClass pci; >> > +} EHCIPCIClass; >> > >> > -static TypeInfo ehci_info =3D { >> > - .name =3D "usb-ehci", >> > - .parent =3D TYPE_PCI_DEVICE, >> > - .instance_size =3D sizeof(EHCIState), >> > - .class_init =3D ehci_class_init, >> > -}; >> > - >> > -static void ich9_ehci_class_init(ObjectClass *klass, void *data) >> > +static void ehci_class_init(ObjectClass *klass, void *data) >> > { >> > DeviceClass *dc =3D DEVICE_CLASS(klass); >> > - PCIDeviceClass *k =3D PCI_DEVICE_CLASS(klass); >> > - >> > - k->init =3D usb_ehci_initfn; >> > - k->vendor_id =3D PCI_VENDOR_ID_INTEL; >> > - k->device_id =3D PCI_DEVICE_ID_INTEL_82801I_EHCI1; >> > - k->revision =3D 0x03; >> > - k->class_id =3D PCI_CLASS_SERIAL_USB; >> > + EHCIPCIClass *k =3D (EHCIPCIClass *)klass; >> >> Please use a proper QOM cast macro: EHCI_PCI_CLASS(klass) >> >=20 > How is this possible whe TYPE_EHCI_PCI doesn't exist ? The FOO_CLASS > macros require the name of the class but that does not exist as its a > dynamic class. Well, why doesn't it exist? Surely a type exists that uses your EHCIPCIClass struct and if we don't have an abstract base type for individual models then adding a TypeInfo alongside the struct surely is clean and trivial. >> In this case however, please keep using PCIDeviceClass rather than >> trying to access it through a named member. If we need to access any >> dedicated EHCIPCIClass fields later in the series we can add additiona= l >> variables the QOM way. >=20 > What do you mean the Qom way? Don't we just add fields to the class and > class_info and class_init copies them across ? The QOM way refers to accessing fields directly: k->foo, not x->bar.foo. Your patch adds unnecessary indirection here when all you're actually changing is the assignment of three values. This was discussed between Anthony, mst and others in lengths. >> > + EHCIPCIClass *template =3D data; >> > + >> > + k->pci.init =3D usb_ehci_initfn; >> > + k->pci.vendor_id =3D template->pci.vendor_id; >> > + k->pci.device_id =3D template->pci.device_id; /* ich4 */ >> > + k->pci.revision =3D template->pci.revision; >> > + k->pci.class_id =3D PCI_CLASS_SERIAL_USB; >> > dc->vmsd =3D &vmstate_ehci; >> > dc->props =3D ehci_properties; >> > } >> > >> > -static TypeInfo ich9_ehci_info =3D { >> > - .name =3D "ich9-usb-ehci1", >> > - .parent =3D TYPE_PCI_DEVICE, >> > - .instance_size =3D sizeof(EHCIState), >> > - .class_init =3D ich9_ehci_class_init, >> > +static TypeInfo ehci_info[] =3D { >> >> Can this still be made const despite the embedded class_data? >> >> > + { >> > + .name =3D "usb-ehci", >> > + .parent =3D TYPE_PCI_DEVICE, >> > + .instance_size =3D sizeof(EHCIState), >> > + .class_init =3D ehci_class_init, >> > + .class_size =3D sizeof(EHCIPCIClass), >> > + .class_data =3D (EHCIPCIClass[]) {{ >> > + .pci.vendor_id =3D PCI_VENDOR_ID_INTEL, >> > + .pci.device_id =3D PCI_DEVICE_ID_INTEL_82801D, >> > + .pci.revision =3D 0x10, >> > + } } >> > + }, { >> > + .name =3D "ich9-usb-ehci1", >> >> Do we have a suitable header to introduce TYPE_* constants for these >> while at it? That would benefit q35. >> >=20 > No because there are no TYPE_ constants. The classes are private to > hcd-ehci.c. Note that I wrote "introduce TYPE_* constants". What about hw/usb.h at least for instantiatable types? Question was equally addressed to Gerd. No doubt this is v1.3 material but we shouldn't rush pulling something in just because of the Soft Freeze. ;) Adding constants can be done as follow-up, but the QOM issues are easier fixed from the start. Regards, Andreas >> > + .parent =3D TYPE_PCI_DEVICE, >> > + .instance_size =3D sizeof(EHCIState), >> > + .class_init =3D ehci_class_init, >> > + .class_size =3D sizeof(EHCIPCIClass), >> > + .class_data =3D (EHCIPCIClass[]) {{ >> > + .pci.vendor_id =3D PCI_VENDOR_ID_INTEL, >> > + .pci.device_id =3D PCI_DEVICE_ID_INTEL_82801I_EHCI1, >> > + .pci.revision =3D 0x03, >> > + } } >> > + }, >> > }; >> > >> > static int usb_ehci_initfn(PCIDevice *dev) >> > @@ -2769,8 +2772,11 @@ static int usb_ehci_initfn(PCIDevice *dev) >> > >> > static void ehci_register_types(void) >> > { >> > - type_register_static(&ehci_info); >> > - type_register_static(&ich9_ehci_info); >> > + int i; >> > + >> > + for (i =3D 0; i < ARRAY_SIZE(ehci_info); i++) { >> > + type_register_static(&ehci_info[i]); >> > + } >> > } >> > >> > type_init(ehci_register_types) --=20 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrnbe= rg