Re: [Qemu-devel] [PATCH 2/2] usb-ehci: add Faraday FUSBH200 support

[Kuo-Jung Su]

2013/1/29 Andreas Färber <address@hidden>

Am 29.01.2013 06:43, schrieb Kuo-Jung Su:

> From: Kuo-Jung Su <address@hidden>
>
> Add Faraday FUSBH200 support, which is slightly different from EHCI spec.
> (Or maybe simply a bad/wrong implementation...)
>
> Signed-off-by: Kuo-Jung Su <address@hidden>
> Cc: Gerd Hoffmann <address@hidden>
> Cc: Andreas <address@hidden>
> Cc: Peter Crosthwaite <address@hidden>
> ---
>  hw/usb/hcd-ehci-sysbus.c |   66 ++++++++++++++++++++++++++++++++++++++++++++++
>  hw/usb/hcd-ehci.h        |    5 ++++
>  2 files changed, 71 insertions(+)
>
> diff --git a/hw/usb/hcd-ehci-sysbus.c b/hw/usb/hcd-ehci-sysbus.c
> index ae2db1a..404a227 100644
> --- a/hw/usb/hcd-ehci-sysbus.c
> +++ b/hw/usb/hcd-ehci-sysbus.c
> @@ -17,6 +17,49 @@
>
>  #include "hw/usb/hcd-ehci.h"
>
> +/*
> + * Faraday FUSBH200 USB 2.0 EHCI
> + */
> +
> +static uint64_t
> +ehci_fusbh200_read(void *ptr, hwaddr addr, unsigned size)
> +{
> +    hwaddr off = 0x34 + addr;
> +
> +    switch (off) {
> +    case 0x34:  /* fusbh200: EOF/Async. Sleep Timer Register */
> +        return 0x00000041;
> +    case 0x40:  /* fusbh200: Bus Monitor Control/Status Register */
> +        /* High-Speed, VBUS valid, interrupt level-high active */
> +        return (2 << 9) | (1 << 8) | (1 << 3);
> +    }
> +
> +    return 0;
> +}
> +
> +static void
> +ehci_fusbh200_write(void *ptr, hwaddr addr, uint64_t val, unsigned size)
> +{
> +}
> +
> +static const MemoryRegionOps ehci_mmio_fusbh200_ops = {
> +    .read = ehci_fusbh200_read,
> +    .write = ehci_fusbh200_write,
> +    .valid.min_access_size = 4,
> +    .valid.max_access_size = 4,
> +    .endianness = DEVICE_LITTLE_ENDIAN,
> +};

This part looks okay now, except that I'd suggest to name the ops
ehci_fusbh200_mmio_ops (ehci_fusbh200_ being a consistent prefix).

Sure, consider it done.

 

> +
> +static void
> +usb_ehci_fusbh200_initfn(EHCIState *s, DeviceState *dev)

Bad naming, the inconsistent use of "initfn" is what my patch tried to
clean up for you.
And this signature is only necessary in common EHCI code because it
needs to access EHCIState from both PCIDevice and SysBusDevice. This
change is in SysBus-only code though, so you can access EHCIState
through the device state.

My bad, because I didn't know what's the difference between realizefn and initfn,

so I did it in a wrong way.

 

> +{
> +    memory_region_init_io(&s->mem_vendor, &ehci_mmio_fusbh200_ops, s,
> +                          "fusbh200", 0x4c);

This has no dependencies on other fields, so it should go into a
void ehci_fusbh200_initfn(Object *obj) hooked to your .instance_init.

> +    memory_region_add_subregion(&s->mem,
> +                                s->opregbase + s->portscbase + 4 * s->portnr,
> +                                &s->mem_vendor);
> +}
> +
>  static const VMStateDescription vmstate_ehci_sysbus = {
>      .name        = "ehci-sysbus",
>      .version_id  = 2,
> @@ -46,6 +89,9 @@ static void usb_ehci_sysbus_realizefn(DeviceState *dev, Error **errp)
>      s->dma = &dma_context_memory;
>
>      usb_ehci_initfn(s, dev);

FWIW I notice that I should've renamed this in my patch, too (outside
the scope of your patch).

> +    if (sec->vendor_init) {
> +        sec->vendor_init(s, DEVICE(dev));
> +    }
>      sysbus_init_irq(d, &s->irq);
>      sysbus_init_mmio(d, &s->mem);
>  }

This is not what I had in mind. For one thing your initialization does
not need to go before sysbus_init_{irq,mmio}.

What I am not too sure about without digging out SysBus and PCI EHCI
sources is at which point the fields may get modified. In 1/2 your new
fields are only ever initialized in class_init and then transferred to
instance state in the realizefn.

There's two better options:

1) Change 1/2 to set the default values s->... in an instance_init
initfn directly, then your instance_init can execute "on top" (with QOM
infrastructure taking care of calling the parent's initfn before yours)
and overwrite its values.
You won't need sec->portscbase and sec->portnr then and we avoid
duplicating "sec->portscbase = 0x44; sec->portnr = NB_PORTS;".

2) Keep 1/2 as is and override the realizefn in 2/2 via dc->realize =
your_realizefn. This involves saving the parent's realizefn in a new
FUSHBH200SysBusEHCIClass::parent_realize and explicitly invoking that
parent_realize function before adding your MemoryRegion as subregion.

The big underlying question is whether other future devices may want to
override the values via qdev static properties. Then the assignment
needs to remain in realizefn, otherwise initfn makes our life easier.

No problem,  consider it done.

> @@ -76,6 +122,7 @@ static void ehci_xlnx_class_init(ObjectClass *oc, void *data)
>      sec->opregbase = 0x140;
>      sec->portscbase = 0x44;
>      sec->portnr = NB_PORTS;
> +    sec->vendor_init = NULL;
>  }
>
>  static const TypeInfo ehci_xlnx_type_info = {
> @@ -92,6 +139,7 @@ static void ehci_exynos4210_class_init(ObjectClass *oc, void *data)
>      sec->opregbase = 0x10;
>      sec->portscbase = 0x44;
>      sec->portnr = NB_PORTS;
> +    sec->vendor_init = NULL;
>  }
>
>  static const TypeInfo ehci_exynos4210_type_info = {

You don't need to zero-initialize fields in class_init or initfn.

got you, thanks

 

> @@ -100,11 +148,29 @@ static const TypeInfo ehci_exynos4210_type_info = {
>      .class_init    = ehci_exynos4210_class_init,
>  };
>
> +static void ehci_fusbh200_class_init(ObjectClass *oc, void *data)
> +{
> +    SysBusEHCIClass *sec = SYS_BUS_EHCI_CLASS(oc);
> +
> +    sec->capsbase = 0x0;
> +    sec->opregbase = 0x10;
> +    sec->portscbase = 0x20;
> +    sec->portnr = 1;
> +    sec->vendor_init = usb_ehci_fusbh200_initfn;
> +}
> +
> +static const TypeInfo ehci_fusbh200_type_info = {
> +    .name          = TYPE_FUSBH200_EHCI,
> +    .parent        = TYPE_SYS_BUS_EHCI,
> +    .class_init    = ehci_fusbh200_class_init,
> +};
> +
>  static void ehci_sysbus_register_types(void)
>  {
>      type_register_static(&ehci_type_info);
>      type_register_static(&ehci_xlnx_type_info);
>      type_register_static(&ehci_exynos4210_type_info);
> +    type_register_static(&ehci_fusbh200_type_info);
>  }
>
>  type_init(ehci_sysbus_register_types)
> diff --git a/hw/usb/hcd-ehci.h b/hw/usb/hcd-ehci.h
> index e587b67..3ca9c8f 100644
> --- a/hw/usb/hcd-ehci.h
> +++ b/hw/usb/hcd-ehci.h
> @@ -261,6 +261,7 @@ struct EHCIState {
>      MemoryRegion mem_caps;
>      MemoryRegion mem_opreg;
>      MemoryRegion mem_ports;
> +    MemoryRegion mem_vendor;
>      int companion_count;
>      uint16_t capsbase;
>      uint16_t opregbase;

Similar question here: Do we expect other models to expose a single
vendor MemoryRegion as well?

I would prefer to place this a) in the SysBus-specific state or if only
for this device b) in a new FUSBH200-specific state derived from it.

ok, no problem

 

> @@ -336,6 +337,7 @@ typedef struct EHCIPCIState {
>
>  #define TYPE_SYS_BUS_EHCI "sysbus-ehci-usb"
>  #define TYPE_EXYNOS4210_EHCI "exynos4210-ehci-usb"
> +#define TYPE_FUSBH200_EHCI "fusbh200-ehci-usb"
>
>  #define SYS_BUS_EHCI(obj) \
>      OBJECT_CHECK(EHCISysBusState, (obj), TYPE_SYS_BUS_EHCI)
> @@ -361,6 +363,9 @@ typedef struct SysBusEHCIClass {
>      uint16_t opregbase;
>      uint16_t portscbase;
>      uint16_t portnr;
> +
> +    /* vendor specific init function */
> +    void (*vendor_init)(EHCIState *s, DeviceState *dev);
>  } SysBusEHCIClass;
>
>  #endif

Gerd, what are your thoughts? If Kuo-Jung doesn't mind, I would offer to
send a v2 implementing the changes I suggested in the way you prefer.

Don't worry about me, I'm O.K to any changes. It does not bother me at all,

what's really killing me is only the patch rules....

 

Regards,
Andreas

--
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

--
Best wishes,
Kuo-Jung Su