Patchwork [v1,5/8] xilinx_zynq: add USB controllers

login
register
mail settings
Submitter Peter Crosthwaite
Date Oct. 25, 2012, 9:47 a.m.
Message ID <e47518863bca237dd485619a6812d5df31ce16db.1351157627.git.peter.crosthwaite@xilinx.com>
Download mbox | patch
Permalink /patch/194105/
State New
Headers show

Comments

Peter Crosthwaite - Oct. 25, 2012, 9:47 a.m.
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(-)
Gerd Hoffmann - Oct. 25, 2012, 12:12 p.m.
> +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
Peter Maydell - Oct. 25, 2012, 12:16 p.m.
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
Peter Crosthwaite - Oct. 25, 2012, 12:56 p.m.
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
>
Gerd Hoffmann - Oct. 25, 2012, 1:14 p.m.
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
Peter Crosthwaite - Oct. 25, 2012, 1:24 p.m.
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
>
>
Gerd Hoffmann - Oct. 25, 2012, 1:49 p.m.
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

Patch

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]);