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

[Gerd Hoffmann]

> 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.

The offsets must be initialized before calling usb_ehci_initfn as they
are used to setup the memory regions.

Adding the FUSBH200 specific memory region must be done after calling
usb_ehci_initfn because that creates the parent memory region where you
hook it in.

> 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.

Given the ordering constrains outlined above I think (2) will work
better.  You know QOM better than me though ...

> The big underlying question is whether other future devices may want to
> override the values via qdev static properties.

I don't think so.  The values are hardware-specific and should not be
user-configurable.

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

Unlikely (or at least uncommon).

cheers,
  Gerd