Re: [Qemu-devel] [PATCH v1 5/8] xilinx_zynq: add USB controllers

[Peter Crosthwaite]

On Thu, Oct 25, 2012 at 10:16 PM, Peter Maydell
<address@hidden> wrote:
> On 25 October 2012 13:12, Gerd Hoffmann <address@hidden> wrote:
>>> +static inline void zynq_init_usb(uint32_t base_addr, qemu_irq irq)
>>> +{
>>> +    DeviceState *dev = qdev_create(NULL, "ehci-sysbus");
>>
>> I'd suggest to have a "ehci-sysbus-zynq" device instead which sets
>> capsbase & opregbase in ->init() ...
>>
>>> +    qdev_prop_set_uint16(dev, "capabase", 0x100);
>>> +    qdev_prop_set_uint32(dev, "opregbase", 0x140);
>>
>> ... then drop these lines.
>
> That sounds weird to me -- properties are exactly the mechanism
> for having a device which is configurable. Why have two differently
> named devices which only differ in the value of a configurable
> property?
>

Yes I agree. Creating a now QOM definition for every variant of a
device is tedious. EHCI provides a nice abstraction which should not
have awareness of its particular implementations.

The way I have done it here also maps to how it is handled in the
linux kernel as well. capabase and opregbase are left as parameters
and EHCI implementations wrap around and set them as needed.
hcd-ehci.c in the kernel has no awareness of zynq and I think the same
hold for hcd-ehci.c in QEMU.

> [I haven't looked at whether these specific properties make
> sense or if there's some other higher-level-of-abstraction
> knob that would be better to expose.)
>

Im interested to see what up with Tegra here. Might have a look
through the kernel drivers to see what kinda diffs there are from one
EHCI variant to the next tmrw.

Regards,
Peter

> -- PMM
>