Re: [Qemu-devel] [PATCH 1/2] usb/ehci: Refactor ehci-sysbus to add exynos4210 ehci support

["Andreas Färber" <afaerber@suse.de>]

Am 01.12.2012 16:27, schrieb Liming Wang:
> Refactor ehci-sysbus to accept different capsbase and opregbase.
> And then add exynos4210 ehci support to ehci-sysbus.
> 
> Signed-off-by: Liming Wang <walimisdev@gmail.com>
> ---
>  hw/usb/hcd-ehci-sysbus.c |   48 ++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 38 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/usb/hcd-ehci-sysbus.c b/hw/usb/hcd-ehci-sysbus.c
> index 803df92..f4f3ffe 100644
> --- a/hw/usb/hcd-ehci-sysbus.c
> +++ b/hw/usb/hcd-ehci-sysbus.c
> @@ -23,6 +23,11 @@ typedef struct EHCISysBusState {
>      EHCIState ehci;
>  } EHCISysBusState;
>  
> +typedef struct EHCISysBusClass {
> +    SysBusDeviceClass sdc;
> +    EHCIState ehci;
> +} EHCISysBusClass;

This is still not the right way to do this. And please keep me CC'ed.

FWIW sdc should be named parent_class and separated from normal fields.

> +
>  static const VMStateDescription vmstate_ehci_sysbus = {
>      .name        = "ehci-sysbus",
>      .version_id  = 2,
> @@ -41,10 +46,11 @@ static Property ehci_sysbus_properties[] = {
>  static int usb_ehci_sysbus_initfn(SysBusDevice *dev)
>  {
>      EHCISysBusState *i = FROM_SYSBUS(EHCISysBusState, dev);
> +    EHCISysBusClass *c = (EHCISysBusClass *)object_get_class(OBJECT(dev));

If you add a class, you also need to add a matching cast macro (and in
this case TypeInfo) and use that. I'm not aware of any other example
that does it like this, otherwise please point it out so that it can be
fixed.

>      EHCIState *s = &i->ehci;
>  
> -    s->capsbase = 0x100;
> -    s->opregbase = 0x140;
> +    s->capsbase = c->ehci.capsbase;
> +    s->opregbase = c->ehci.opregbase;
>      s->dma = &dma_context_memory;
>  
>      usb_ehci_initfn(s, DEVICE(dev));
> @@ -56,23 +62,45 @@ static int usb_ehci_sysbus_initfn(SysBusDevice *dev)
>  static void ehci_sysbus_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
> -    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +    EHCISysBusClass *k = (EHCISysBusClass *)klass;
> +    EHCISysBusClass *c = (EHCISysBusClass *)data;
>  
> -    k->init = usb_ehci_sysbus_initfn;
> +    k->sdc.init = usb_ehci_sysbus_initfn;

Never access the parent field directly, use a cast macro.

> +    k->ehci = c->ehci;
>      dc->vmsd = &vmstate_ehci_sysbus;
>      dc->props = ehci_sysbus_properties;
>  }
>  
> -TypeInfo ehci_xlnx_type_info = {
> -    .name          = "xlnx,ps7-usb",
> -    .parent        = TYPE_SYS_BUS_DEVICE,
> -    .instance_size = sizeof(EHCISysBusState),
> -    .class_init    = ehci_sysbus_class_init,
> +static TypeInfo ehci_sysbus_type_info[] = {
> +    {
> +        .name          = "xlnx,ps7-usb",
> +        .parent        = TYPE_SYS_BUS_DEVICE,
> +        .instance_size = sizeof(EHCISysBusState),
> +        .class_init    = ehci_sysbus_class_init,
> +        .class_size    = sizeof(EHCISysBusClass),
> +        .class_data    = (EHCISysBusClass[]) {{
> +	    .ehci.capsbase  = 0x100,
> +	    .ehci.opregbase = 0x140,
> +        } }

Neither is this. As discussed before, this is misusing QOM concepts. The
instance state has no business being stored in the class, and the class
shouldn't be duplicated to initialize itself, even less so when it's
only about two integer values.

> +    }, {
> +        .name          = "exynos4210.usb",
> +        .parent        = TYPE_SYS_BUS_DEVICE,
> +        .instance_size = sizeof(EHCISysBusState),
> +        .class_init    = ehci_sysbus_class_init,
> +        .class_size    = sizeof(EHCISysBusClass),
> +        .class_data    = (EHCISysBusClass[]) {{
> +            .ehci.capsbase  = 0x0,
> +            .ehci.opregbase = 0x40,
> +        } }
> +    },
>  };
>  
>  static void ehci_sysbus_register_types(void)
>  {
> -    type_register_static(&ehci_xlnx_type_info);
> +    int i;
> +    for (i = 0; i < ARRAY_SIZE(ehci_sysbus_type_info); i++) {
> +        type_register_static(&ehci_sysbus_type_info[i]);

If a loop becomes necessary, this is indicating that something is in the
odds, complicating things. As indicated to Gerd at KVM Forum, I didn't
get around to sending out a cleanup yet (and have been busy since):

We need an abstract intermediate type whose class specifies only the
needed fields (capsbase, opregbase), has cast macros to access them and
stores the majority of TypeInfo cruft that you are duplicating here. The
specific types (xlnx, exynos, tegra) would then add their own class_init
on top to set/override those two values, and the base class'
instance_init would copy them over. I'll try to find some spare minutes
tomorrow.

Regards,
Andreas

> +    }
>  }
>  
>  type_init(ehci_sysbus_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