[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 2/8] usb/ehci: Use class_data to init PCI var
From: |
Andreas Färber |
Subject: |
Re: [Qemu-devel] [PATCH v3 2/8] usb/ehci: Use class_data to init PCI variations |
Date: |
Mon, 29 Oct 2012 14:21:13 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:16.0) Gecko/20121010 Thunderbird/16.0.1 |
Am 29.10.2012 12:43, schrieb Peter Crosthwaite:
>
> On Oct 29, 2012 7:35 PM, "Andreas Färber" <address@hidden
> <mailto:address@hidden>> 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.
>
> 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 <address@hidden
> <mailto:address@hidden>>
>> > ---
>> > 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[] = {
>> > DEFINE_PROP_END_OF_LIST(),
>> > };
>> >
>> > -static void ehci_class_init(ObjectClass *klass, void *data)
>> > -{
>> > - DeviceClass *dc = DEVICE_CLASS(klass);
>> > - PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>> > -
>> > - k->init = usb_ehci_initfn;
>> > - k->vendor_id = PCI_VENDOR_ID_INTEL;
>> > - k->device_id = PCI_DEVICE_ID_INTEL_82801D; /* ich4 */
>> > - k->revision = 0x10;
>> > - k->class_id = PCI_CLASS_SERIAL_USB;
>> > - dc->vmsd = &vmstate_ehci;
>> > - dc->props = ehci_properties;
>> > -}
>> > +typedef struct EHCIPCIClass {
>> > + PCIDeviceClass pci;
>> > +} EHCIPCIClass;
>> >
>> > -static TypeInfo ehci_info = {
>> > - .name = "usb-ehci",
>> > - .parent = TYPE_PCI_DEVICE,
>> > - .instance_size = sizeof(EHCIState),
>> > - .class_init = ehci_class_init,
>> > -};
>> > -
>> > -static void ich9_ehci_class_init(ObjectClass *klass, void *data)
>> > +static void ehci_class_init(ObjectClass *klass, void *data)
>> > {
>> > DeviceClass *dc = DEVICE_CLASS(klass);
>> > - PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
>> > -
>> > - k->init = usb_ehci_initfn;
>> > - k->vendor_id = PCI_VENDOR_ID_INTEL;
>> > - k->device_id = PCI_DEVICE_ID_INTEL_82801I_EHCI1;
>> > - k->revision = 0x03;
>> > - k->class_id = PCI_CLASS_SERIAL_USB;
>> > + EHCIPCIClass *k = (EHCIPCIClass *)klass;
>>
>> Please use a proper QOM cast macro: EHCI_PCI_CLASS(klass)
>>
>
> 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 additional
>> variables the QOM way.
>
> 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 = data;
>> > +
>> > + k->pci.init = usb_ehci_initfn;
>> > + k->pci.vendor_id = template->pci.vendor_id;
>> > + k->pci.device_id = template->pci.device_id; /* ich4 */
>> > + k->pci.revision = template->pci.revision;
>> > + k->pci.class_id = PCI_CLASS_SERIAL_USB;
>> > dc->vmsd = &vmstate_ehci;
>> > dc->props = ehci_properties;
>> > }
>> >
>> > -static TypeInfo ich9_ehci_info = {
>> > - .name = "ich9-usb-ehci1",
>> > - .parent = TYPE_PCI_DEVICE,
>> > - .instance_size = sizeof(EHCIState),
>> > - .class_init = ich9_ehci_class_init,
>> > +static TypeInfo ehci_info[] = {
>>
>> Can this still be made const despite the embedded class_data?
>>
>> > + {
>> > + .name = "usb-ehci",
>> > + .parent = TYPE_PCI_DEVICE,
>> > + .instance_size = sizeof(EHCIState),
>> > + .class_init = ehci_class_init,
>> > + .class_size = sizeof(EHCIPCIClass),
>> > + .class_data = (EHCIPCIClass[]) {{
>> > + .pci.vendor_id = PCI_VENDOR_ID_INTEL,
>> > + .pci.device_id = PCI_DEVICE_ID_INTEL_82801D,
>> > + .pci.revision = 0x10,
>> > + } }
>> > + }, {
>> > + .name = "ich9-usb-ehci1",
>>
>> Do we have a suitable header to introduce TYPE_* constants for these
>> while at it? That would benefit q35.
>>
>
> 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 = TYPE_PCI_DEVICE,
>> > + .instance_size = sizeof(EHCIState),
>> > + .class_init = ehci_class_init,
>> > + .class_size = sizeof(EHCIPCIClass),
>> > + .class_data = (EHCIPCIClass[]) {{
>> > + .pci.vendor_id = PCI_VENDOR_ID_INTEL,
>> > + .pci.device_id = PCI_DEVICE_ID_INTEL_82801I_EHCI1,
>> > + .pci.revision = 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 = 0; i < ARRAY_SIZE(ehci_info); i++) {
>> > + type_register_static(&ehci_info[i]);
>> > + }
>> > }
>> >
>> > type_init(ehci_register_types)
--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
- [Qemu-devel] [PATCH v3 0/8] Sysbus EHCI + Zynq USB., Peter Crosthwaite, 2012/10/28
- [Qemu-devel] [PATCH v3 3/8] usb/ehci: parameterise the register region offsets, Peter Crosthwaite, 2012/10/28
- [Qemu-devel] [PATCH v3 4/8] usb/ehci: Abstract away PCI DMA API, Peter Crosthwaite, 2012/10/28
- [Qemu-devel] [PATCH v3 5/8] usb/ehci: seperate out PCIisms, Peter Crosthwaite, 2012/10/28
- [Qemu-devel] [PATCH v3 6/8] usb/ehci: Add Sysbus variant and Xilinx Zynq USB, Peter Crosthwaite, 2012/10/28
- [Qemu-devel] [PATCH v3 7/8] xilinx_zynq: add USB controllers, Peter Crosthwaite, 2012/10/28
- [Qemu-devel] [PATCH v3 8/8] usb/ehci: Guard definition of EHCI_DEBUG, Peter Crosthwaite, 2012/10/28
- Re: [Qemu-devel] [PATCH v3 0/8] Sysbus EHCI + Zynq USB., Gerd Hoffmann, 2012/10/29