diff mbox

[v2,05/11] usb/ehci: seperate out PCIisms

Message ID 16ccd8856bbc63649b8d5bed1123213ac0a05edc.1351229557.git.peter.crosthwaite@xilinx.com
State New
Headers show

Commit Message

Peter Crosthwaite Oct. 26, 2012, 5:47 a.m. UTC 
Seperate the PCI stuff from the EHCI components. Extracted the PCIDevice
out into a new wrapper struct to make EHCIState non-PCI-specific. Seperated
tho non PCI init component out into a seperate "common" init function.

Signed-off-by: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
---
Changed from v1:
renamed VMSD defintions to preserve backwards compatibility (Gerd Review)

 hw/usb/hcd-ehci.c |  136 ++++++++++++++++++++++++++++++++--------------------
 1 files changed, 84 insertions(+), 52 deletions(-)

Comments

Gerd Hoffmann Oct. 26, 2012, 12:24 p.m. UTC | #1 
Hi,

> +typedef struct EHCItfState {
> +    union {
> +        PCIDevice pcidev;
> +    };
> +    struct EHCIState ehci;
> +} EHCIItfState;

I still think we should have EHCIPCIState here, then add a
EHCISysbusState variant for sysbus.  Everybody else does it this way
(ohci, esp, serial, ...) and I'd like ehci follow.  Also makes it easier
to split the source code into core, pci and sysbus pieces, i.e. move all
the pci bits to hcd-ehci-pci.c and compile only for targets with pci
support.

cheers,
  Gerd
Peter Crosthwaite Oct. 27, 2012, 12:32 a.m. UTC | #2 
On Fri, Oct 26, 2012 at 10:24 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>   Hi,
>
>> +typedef struct EHCItfState {
>> +    union {
>> +        PCIDevice pcidev;
>> +    };
>> +    struct EHCIState ehci;
>> +} EHCIItfState;
>
> I still think we should have EHCIPCIState here, then add a
> EHCISysbusState variant for sysbus.  Everybody else does it this way
> (ohci, esp, serial, ...) and I'd like ehci follow.

There still has to be a way to share the Property[] array (currently
contains maxframes). Duplicating the properties array to all
definitions is verbose and fragile. If I want to add a new properties
to EHCI i need to put it in the props array of every subclass. serial
has this problem, with the "chardev" prop appearing in both isa and
pci variants (and the device variant out of tree that Anthony has). If
we decide to add a new prop to serial we have to DEFINE_PROP_FOO it 3
times.

Whats the real answer here? Can we get the shared init function to add
to properties explicify? Blow away the dc->properties = foo and
replace with code that parses to prop array?

I just think its a false economy to have to replicate your code for
the sake of the anit-union movement,

Regards,
Peter

Also makes it easier
> to split the source code into core, pci and sysbus pieces, i.e. move all
> the pci bits to hcd-ehci-pci.c and compile only for targets with pci
> support.
>
> cheers,
>   Gerd
>
>
Peter Crosthwaite Oct. 29, 2012, 1:04 a.m. UTC | #3 
On Sat, Oct 27, 2012 at 10:32 AM, Peter Crosthwaite
<peter.crosthwaite@xilinx.com> wrote:
> On Fri, Oct 26, 2012 at 10:24 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
>>   Hi,
>>
>>> +typedef struct EHCItfState {
>>> +    union {
>>> +        PCIDevice pcidev;
>>> +    };
>>> +    struct EHCIState ehci;
>>> +} EHCIItfState;
>>
>> I still think we should have EHCIPCIState here, then add a
>> EHCISysbusState variant for sysbus.  Everybody else does it this way
>> (ohci, esp, serial, ...) and I'd like ehci follow.
>

In the interest of hopefully getting something merged before freeze I
had remade it this way (With EHCIPCIState). Can revisit this code
replication issue another day. v3 will be up shortly.

Regards,
Peter

> There still has to be a way to share the Property[] array (currently
> contains maxframes). Duplicating the properties array to all
> definitions is verbose and fragile. If I want to add a new properties
> to EHCI i need to put it in the props array of every subclass. serial
> has this problem, with the "chardev" prop appearing in both isa and
> pci variants (and the device variant out of tree that Anthony has). If
> we decide to add a new prop to serial we have to DEFINE_PROP_FOO it 3
> times.
>
> Whats the real answer here? Can we get the shared init function to add
> to properties explicify? Blow away the dc->properties = foo and
> replace with code that parses to prop array?
>
> I just think its a false economy to have to replicate your code for
> the sake of the anit-union movement,
>
> Regards,
> Peter
>
> Also makes it easier
>> to split the source code into core, pci and sysbus pieces, i.e. move all
>> the pci bits to hcd-ehci-pci.c and compile only for targets with pci
>> support.
>>
>> cheers,
>>   Gerd
>>
>>
Gerd Hoffmann Oct. 29, 2012, 7:43 a.m. UTC | #4 
Hi,

> There still has to be a way to share the Property[] array (currently
> contains maxframes). Duplicating the properties array to all
> definitions is verbose and fragile. If I want to add a new properties
> to EHCI i need to put it in the props array of every subclass. serial
> has this problem, with the "chardev" prop appearing in both isa and
> pci variants (and the device variant out of tree that Anthony has). If
> we decide to add a new prop to serial we have to DEFINE_PROP_FOO it 3
> times.

> Whats the real answer here? Can we get the shared init function to add
> to properties explicify? Blow away the dc->properties = foo and
> replace with code that parses to prop array?

Existing practice is to use a #define for that, see
DEFINE_NIC_PROPERTIES in net.h for example.

Maybe QOM allows us to do something more elegant here.

cheers,
  Gerd
diff mbox

Patch

diff --git a/hw/usb/hcd-ehci.c b/hw/usb/hcd-ehci.c
index 28bd7e9..1c5e5ed 100644
--- a/hw/usb/hcd-ehci.c
+++ b/hw/usb/hcd-ehci.c
@@ -386,7 +386,6 @@  struct EHCIQueue {
 typedef QTAILQ_HEAD(EHCIQueueHead, EHCIQueue) EHCIQueueHead;
 
 struct EHCIState {
-    PCIDevice dev;
     USBBus bus;
     qemu_irq irq;
     MemoryRegion mem;
@@ -446,6 +445,13 @@  struct EHCIState {
     uint32_t async_stepdown;
 };
 
+typedef struct EHCItfState {
+    union {
+        PCIDevice pcidev;
+    };
+    struct EHCIState ehci;
+} EHCIItfState;
+
 #define SET_LAST_RUN_CLOCK(s) \
     (s)->last_run_ns = qemu_get_clock_ns(vm_clock);
 
@@ -2539,7 +2545,7 @@  static const MemoryRegionOps ehci_mmio_port_ops = {
     .endianness = DEVICE_LITTLE_ENDIAN,
 };
 
-static int usb_ehci_initfn(PCIDevice *dev);
+static int usb_ehci_pci_initfn(PCIDevice *dev);
 
 static USBPortOps ehci_port_ops = {
     .attach = ehci_attach,
@@ -2600,12 +2606,11 @@  static void usb_ehci_vm_state_change(void *opaque, int running, RunState state)
 }
 
 static const VMStateDescription vmstate_ehci = {
-    .name        = "ehci",
+    .name        = "ehci-core",
     .version_id  = 2,
     .minimum_version_id  = 1,
     .post_load   = usb_ehci_post_load,
     .fields      = (VMStateField[]) {
-        VMSTATE_PCI_DEVICE(dev, EHCIState),
         /* mmio registers */
         VMSTATE_UINT32(usbcmd, EHCIState),
         VMSTATE_UINT32(usbsts, EHCIState),
@@ -2636,8 +2641,19 @@  static const VMStateDescription vmstate_ehci = {
     }
 };
 
+static const VMStateDescription vmstate_ehci_pci = {
+    .name        = "ehci",
+    .version_id  = 2,
+    .minimum_version_id  = 1,
+    .post_load   = usb_ehci_post_load,
+    .fields      = (VMStateField[]) {
+        VMSTATE_PCI_DEVICE(pcidev, EHCIItfState),
+        VMSTATE_STRUCT(ehci, EHCIItfState, 2, vmstate_ehci, EHCIState),
+    }
+};
+
 static Property ehci_properties[] = {
-    DEFINE_PROP_UINT32("maxframes", EHCIState, maxframes, 128),
+    DEFINE_PROP_UINT32("maxframes", EHCIItfState, ehci.maxframes, 128),
     DEFINE_PROP_END_OF_LIST(),
 };
 
@@ -2657,23 +2673,33 @@  static void ehci_class_init(ObjectClass *klass, void *data)
     EHCIClass *k = (EHCIClass *)klass;
     EHCIClass *template = data;
 
-    k->pci.init = usb_ehci_initfn;
-    k->pci.vendor_id = template->pci.vendor_id;
-    k->pci.device_id = template->pci.device_id; /* ich4 */
-    k->pci.revision = template->pci.revision;
-    k->pci.class_id = PCI_CLASS_SERIAL_USB;
     k->opregbase = template->opregbase;
     k->capabase = template->capabase;
     dc->vmsd = &vmstate_ehci;
     dc->props = ehci_properties;
 }
 
+static void ehci_pci_class_init(ObjectClass *klass, void *data)
+{
+    /* FIXME: how do you do object check for a dynamic class? */
+    PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
+    EHCIClass *template = data;
+
+    k->init = usb_ehci_pci_initfn;
+    k->vendor_id = template->pci.vendor_id;
+    k->device_id = template->pci.device_id; /* ich4 */
+    k->revision = template->pci.revision;
+    k->class_id = PCI_CLASS_SERIAL_USB;
+
+    ehci_class_init(klass, data);
+}
+
 static TypeInfo ehci_info[] = {
     {
         .name          = "usb-ehci",
         .parent        = TYPE_PCI_DEVICE,
-        .instance_size = sizeof(EHCIState),
-        .class_init    = ehci_class_init,
+        .instance_size = sizeof(EHCIItfState),
+        .class_init    = ehci_pci_class_init,
         .class_size    = sizeof(EHCIClass),
         .class_data    = (EHCIClass[]) {{{
                 .pci.vendor_id = PCI_VENDOR_ID_INTEL,
@@ -2686,8 +2712,8 @@  static TypeInfo ehci_info[] = {
     }, {
         .name          = "ich9-usb-ehci1",
         .parent        = TYPE_PCI_DEVICE,
-        .instance_size = sizeof(EHCIState),
-        .class_init    = ehci_class_init,
+        .instance_size = sizeof(EHCIItfState),
+        .class_init    = ehci_pci_class_init,
         .class_size    = sizeof(EHCIClass),
         .class_data    = (EHCIClass[]) {{{
                 .pci.vendor_id = PCI_VENDOR_ID_INTEL,
@@ -2700,42 +2726,12 @@  static TypeInfo ehci_info[] = {
     }, { .name = NULL }
 };
 
-static int usb_ehci_initfn(PCIDevice *dev)
+
+static void usb_ehci_initfn(EHCIState *s, DeviceState *dev)
 {
-    EHCIState *s = DO_UPCAST(EHCIState, dev, dev);
     EHCIClass *c = (EHCIClass *)object_get_class(OBJECT(dev));
-    uint8_t *pci_conf = s->dev.config;
     int i;
 
-    pci_set_byte(&pci_conf[PCI_CLASS_PROG], 0x20);
-
-    /* capabilities pointer */
-    pci_set_byte(&pci_conf[PCI_CAPABILITY_LIST], 0x00);
-    //pci_set_byte(&pci_conf[PCI_CAPABILITY_LIST], 0x50);
-
-    pci_set_byte(&pci_conf[PCI_INTERRUPT_PIN], 4); /* interrupt pin D */
-    pci_set_byte(&pci_conf[PCI_MIN_GNT], 0);
-    pci_set_byte(&pci_conf[PCI_MAX_LAT], 0);
-
-    // pci_conf[0x50] = 0x01; // power management caps
-
-    pci_set_byte(&pci_conf[USB_SBRN], USB_RELEASE_2); // release number (2.1.4)
-    pci_set_byte(&pci_conf[0x61], 0x20);  // frame length adjustment (2.1.5)
-    pci_set_word(&pci_conf[0x62], 0x00);  // port wake up capability (2.1.6)
-
-    pci_conf[0x64] = 0x00;
-    pci_conf[0x65] = 0x00;
-    pci_conf[0x66] = 0x00;
-    pci_conf[0x67] = 0x00;
-    pci_conf[0x68] = 0x01;
-    pci_conf[0x69] = 0x00;
-    pci_conf[0x6a] = 0x00;
-    pci_conf[0x6b] = 0x00;  // USBLEGSUP
-    pci_conf[0x6c] = 0x00;
-    pci_conf[0x6d] = 0x00;
-    pci_conf[0x6e] = 0x00;
-    pci_conf[0x6f] = 0xc0;  // USBLEFCTLSTS
-
     s->opregbase = c->opregbase;
 
     /* 2.2 host controller interface version */
@@ -2752,11 +2748,7 @@  static int usb_ehci_initfn(PCIDevice *dev)
     s->caps[0x0a] = 0x00;
     s->caps[0x0b] = 0x00;
 
-    s->irq = s->dev.irq[3];
-
-    s->dma = pci_dma_context(dev);
-
-    usb_bus_new(&s->bus, &ehci_bus_ops, &s->dev.qdev);
+    usb_bus_new(&s->bus, &ehci_bus_ops, dev);
     for(i = 0; i < NB_PORTS; i++) {
         usb_register_port(&s->bus, &s->ports[i], s, i, &ehci_port_ops,
                           USB_SPEED_MASK_HIGH);
@@ -2784,8 +2776,48 @@  static int usb_ehci_initfn(PCIDevice *dev)
     memory_region_add_subregion(&s->mem, s->opregbase, &s->mem_opreg);
     memory_region_add_subregion(&s->mem, s->opregbase + PORTSC_BEGIN,
                                 &s->mem_ports);
+}
+
+static int usb_ehci_pci_initfn(PCIDevice *dev)
+{
+    EHCIItfState *i = DO_UPCAST(EHCIItfState, pcidev, dev);
+    EHCIState *s = &i->ehci;
+    uint8_t *pci_conf = dev->config;
+
+    pci_set_byte(&pci_conf[PCI_CLASS_PROG], 0x20);
+
+    /* capabilities pointer */
+    pci_set_byte(&pci_conf[PCI_CAPABILITY_LIST], 0x00);
+    /* pci_set_byte(&pci_conf[PCI_CAPABILITY_LIST], 0x50); */
+
+    pci_set_byte(&pci_conf[PCI_INTERRUPT_PIN], 4); /* interrupt pin D */
+    pci_set_byte(&pci_conf[PCI_MIN_GNT], 0);
+    pci_set_byte(&pci_conf[PCI_MAX_LAT], 0);
+
+    /* pci_conf[0x50] = 0x01; *//* power management caps */
+
+    pci_set_byte(&pci_conf[USB_SBRN], USB_RELEASE_2); /* release # (2.1.4) */
+    pci_set_byte(&pci_conf[0x61], 0x20);  /* frame length adjustment (2.1.5) */
+    pci_set_word(&pci_conf[0x62], 0x00);  /* port wake up capability (2.1.6) */
+
+    pci_conf[0x64] = 0x00;
+    pci_conf[0x65] = 0x00;
+    pci_conf[0x66] = 0x00;
+    pci_conf[0x67] = 0x00;
+    pci_conf[0x68] = 0x01;
+    pci_conf[0x69] = 0x00;
+    pci_conf[0x6a] = 0x00;
+    pci_conf[0x6b] = 0x00;  /* USBLEGSUP */
+    pci_conf[0x6c] = 0x00;
+    pci_conf[0x6d] = 0x00;
+    pci_conf[0x6e] = 0x00;
+    pci_conf[0x6f] = 0xc0;  /* USBLEFCTLSTS */
+
+    s->irq = dev->irq[3];
+    s->dma = pci_dma_context(dev);
 
-    pci_register_bar(&s->dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->mem);
+    usb_ehci_initfn(s, DEVICE(dev));
+    pci_register_bar(dev, 0, PCI_BASE_ADDRESS_SPACE_MEMORY, &s->mem);
 
     return 0;
 }