2013/1/29 Andreas Färber <afaer...@suse.de>
> Am 29.01.2013 06:43, schrieb Kuo-Jung Su:
> > From: Kuo-Jung Su <dant...@faraday-tech.com>
> >
> > Add Faraday FUSBH200 support, which is slightly different from EHCI spec.
> > (Or maybe simply a bad/wrong implementation...)
> >
> > Signed-off-by: Kuo-Jung Su <dant...@faraday-tech.com>
> > Cc: Gerd Hoffmann <kra...@redhat.com>
> > Cc: Andreas <afaer...@suse.de>
> > Cc: Peter Crosthwaite <peter.crosthwa...@xilinx.com>
> > ---
> > 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
