| Message ID | e47518863bca237dd485619a6812d5df31ce16db.1351157627.git.peter.crosthwaite@xilinx.com |
|---|---|
| State | New |
| Headers | show |
> +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. cheers, Gerd
On 25 October 2012 13:12, Gerd Hoffmann <kraxel@redhat.com> 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? [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.) -- PMM
On Thu, Oct 25, 2012 at 10:16 PM, Peter Maydell <peter.maydell@linaro.org> wrote: > On 25 October 2012 13:12, Gerd Hoffmann <kraxel@redhat.com> 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 >
On 10/25/12 14:56, Peter Crosthwaite wrote: > On Thu, Oct 25, 2012 at 10:16 PM, Peter Maydell > <peter.maydell@linaro.org> wrote: >> On 25 October 2012 13:12, Gerd Hoffmann <kraxel@redhat.com> 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. Maybe "zynq" is the wrong abstraction and this should be named by the actual ehci chip implementation (which could be the same for a bunch of sysbus boards). But, yes, different chips should have different QOM definitions. Like we have a bunch of different uhci variants with a QOM definition for each of them. cheers, Gerd
On Thu, Oct 25, 2012 at 11:14 PM, Gerd Hoffmann <kraxel@redhat.com> wrote: > On 10/25/12 14:56, Peter Crosthwaite wrote: >> On Thu, Oct 25, 2012 at 10:16 PM, Peter Maydell >> <peter.maydell@linaro.org> wrote: >>> On 25 October 2012 13:12, Gerd Hoffmann <kraxel@redhat.com> 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. > > Maybe "zynq" is the wrong abstraction and this should be named by the > actual ehci chip implementation (which could be the same for a bunch of > sysbus boards). > > But, yes, different chips should have different QOM definitions. Like > we have a bunch of different uhci variants with a QOM definition for > each of them. > Can we at least take a data driven approach to this? Get the device names and their varying parameters into a big table up top and they just create N device defs from it? hw/m25p80 is one example of a device that does something similar although the variation is selected using a single qdev prop rather than the QOM names. > cheers, > Gerd > >
Hi, >>> 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. >> >> Maybe "zynq" is the wrong abstraction and this should be named by the >> actual ehci chip implementation (which could be the same for a bunch of >> sysbus boards). >> >> But, yes, different chips should have different QOM definitions. Like >> we have a bunch of different uhci variants with a QOM definition for >> each of them. > > Can we at least take a data driven approach to this? Ahem. Well. I'd love to. It even used to be that way, before QOM. uhci had a simple PCIDeviceInfo table, with an entry for each variant. QOM turned that a bunch of class_init functions ... Looking a bit closer (at include/qemu/object.h): seems we can pass additional data to TypeInfo->class_init via TypeInfo->class_data. That should help here. I'll go try that to simplify uhci ... cheers, Gerd
diff --git a/hw/xilinx_zynq.c b/hw/xilinx_zynq.c index c55dafb..ed6934f 100644 --- a/hw/xilinx_zynq.c +++ b/hw/xilinx_zynq.c @@ -77,6 +77,19 @@ static inline void zynq_init_spi_flashes(uint32_t base_addr, qemu_irq irq) } +static inline void zynq_init_usb(uint32_t base_addr, qemu_irq irq) +{ + DeviceState *dev = qdev_create(NULL, "ehci-sysbus"); + SysBusDevice *busdev; + + qdev_prop_set_uint16(dev, "capabase", 0x100); + qdev_prop_set_uint32(dev, "opregbase", 0x140); + qdev_init_nofail(dev); + busdev = sysbus_from_qdev(dev); + sysbus_mmio_map(busdev, 0, base_addr); + sysbus_connect_irq(busdev, 0, irq); +} + static void zynq_init(QEMUMachineInitArgs *args) { ram_addr_t ram_size = args->ram_size; @@ -150,6 +163,9 @@ static void zynq_init(QEMUMachineInitArgs *args) zynq_init_spi_flashes(0xE0006000, pic[58-IRQ_OFFSET]); zynq_init_spi_flashes(0xE0007000, pic[81-IRQ_OFFSET]); + zynq_init_usb(0xE0002000, pic[53-IRQ_OFFSET]); + zynq_init_usb(0xE0003000, pic[75-IRQ_OFFSET]); + sysbus_create_simple("cadence_uart", 0xE0000000, pic[59-IRQ_OFFSET]); sysbus_create_simple("cadence_uart", 0xE0001000, pic[82-IRQ_OFFSET]);
Add the two usb controllers in Zynq. Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com> --- hw/xilinx_zynq.c | 16 ++++++++++++++++ 1 files changed, 16 insertions(+), 0 deletions(-)