Patchwork [v2,2/3] prep: Add Raven PCI host SysBus device

login
register
mail settings
Submitter Andreas Färber
Date Jan. 7, 2012, 12:06 a.m.
Message ID <1325894809-17322-3-git-send-email-andreas.faerber@web.de>
Download mbox | patch
Permalink /patch/134716/
State New
Headers show

Comments

Andreas Färber - Jan. 7, 2012, 12:06 a.m.
For now, focus on qdev'ification and leave PIC IRQs unchanged.

Signed-off-by: Andreas Färber <andreas.faerber@web.de>
Cc: Hervé Poussineau <hpoussin@reactos.org>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Anthony Liguori <aliguori@us.ibm.com>
---
 hw/prep_pci.c |   41 +++++++++++++++++++++++++++++++----------
 1 files changed, 31 insertions(+), 10 deletions(-)
Anthony Liguori - Jan. 11, 2012, 10:01 p.m.
On 01/06/2012 06:06 PM, Andreas Färber wrote:
> For now, focus on qdev'ification and leave PIC IRQs unchanged.
>
> Signed-off-by: Andreas Färber<andreas.faerber@web.de>
> Cc: Hervé Poussineau<hpoussin@reactos.org>
> Cc: Michael S. Tsirkin<mst@redhat.com>
> Cc: Anthony Liguori<aliguori@us.ibm.com>
> ---
>   hw/prep_pci.c |   41 +++++++++++++++++++++++++++++++----------
>   1 files changed, 31 insertions(+), 10 deletions(-)
>
> diff --git a/hw/prep_pci.c b/hw/prep_pci.c
> index 741b273..2ff6b8c 100644
> --- a/hw/prep_pci.c
> +++ b/hw/prep_pci.c
> @@ -114,31 +114,43 @@ PCIBus *pci_prep_init(qemu_irq *pic,
>                         MemoryRegion *address_space_mem,
>                         MemoryRegion *address_space_io)
>   {
> +    DeviceState *dev;
>       PREPPCIState *s;
>
> -    s = g_malloc0(sizeof(PREPPCIState));
> -    s->bus = pci_register_bus(NULL, "pci",
> +    dev = qdev_create(NULL, "raven-pcihost");
> +    s = FROM_SYSBUS(PREPPCIState, sysbus_from_qdev(dev));
> +    s->address_space = address_space_mem;
> +    s->bus = pci_register_bus(&s->busdev.qdev, "pci",
>                                 prep_set_irq, prep_map_irq, pic,
>                                 address_space_mem,
>                                 address_space_io,
>                                 0, 4);
> +    qdev_init_nofail(dev);
> +    qdev_property_add_child(qdev_get_root(), "raven", dev, NULL);
> +
> +    memory_region_init_io(&s->mmcfg,&PPC_PCIIO_ops, s, "pciio", 0x00400000);
> +    memory_region_add_subregion(address_space_mem, 0x80800000,&s->mmcfg);
> +
> +    pci_create_simple(s->bus, 0, "raven");
> +
> +    return s->bus;
> +}
> +
> +static int raven_pcihost_init(SysBusDevice *dev)
> +{
> +    PREPPCIState *s = FROM_SYSBUS(PREPPCIState, dev);
>
>       memory_region_init_io(&s->conf_mem,&pci_host_conf_be_ops, s,
>                             "pci-conf-idx", 1);
> -    memory_region_add_subregion(address_space_io, 0xcf8,&s->conf_mem);
> +    sysbus_add_io(dev, 0xcf8,&s->conf_mem);
>       sysbus_init_ioports(&s->busdev, 0xcf8, 1);
>
>       memory_region_init_io(&s->data_mem,&pci_host_data_be_ops, s,
>                             "pci-conf-data", 1);
> -    memory_region_add_subregion(address_space_io, 0xcfc,&s->data_mem);
> +    sysbus_add_io(dev, 0xcfc,&s->data_mem);
>       sysbus_init_ioports(&s->busdev, 0xcfc, 1);
>
> -    memory_region_init_io(&s->mmcfg,&PPC_PCIIO_ops, s, "pciio", 0x00400000);
> -    memory_region_add_subregion(address_space_mem, 0x80800000,&s->mmcfg);
> -
> -    pci_create_simple(s->bus, 0, "raven");
> -
> -    return s->bus;
> +    return 0;
>   }
>
>   static int raven_init(PCIDevice *d)
> @@ -177,8 +189,17 @@ static PCIDeviceInfo raven_info = {
>       },
>   };
>
> +static SysBusDeviceInfo raven_pcihost_info = {
> +    .qdev.name = "raven-pcihost",
> +    .qdev.fw_name = "pci",
> +    .qdev.size = sizeof(PREPPCIState),
> +    .qdev.no_user = 1,
> +    .init = raven_pcihost_init,
> +};
> +
>   static void raven_register_devices(void)
>   {
> +    sysbus_register_withprop(&raven_pcihost_info);
>       pci_qdev_register(&raven_info);

I see now :-)  Ignore previous message.

Regards,

Anthony Liguori

>   }
>
Alexander Graf - Jan. 11, 2012, 10:12 p.m.
On 07.01.2012, at 01:06, Andreas Färber wrote:

> For now, focus on qdev'ification and leave PIC IRQs unchanged.
> 
> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
> Cc: Hervé Poussineau <hpoussin@reactos.org>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Anthony Liguori <aliguori@us.ibm.com>
> ---
> hw/prep_pci.c |   41 +++++++++++++++++++++++++++++++----------
> 1 files changed, 31 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/prep_pci.c b/hw/prep_pci.c
> index 741b273..2ff6b8c 100644
> --- a/hw/prep_pci.c
> +++ b/hw/prep_pci.c
> @@ -114,31 +114,43 @@ PCIBus *pci_prep_init(qemu_irq *pic,
>                       MemoryRegion *address_space_mem,
>                       MemoryRegion *address_space_io)
> {

I'm not sure this is the best way to do this. For e500, we just create the host bridge explicitly in the board file:

    /* PCI */
    dev = sysbus_create_varargs("e500-pcihost", MPC8544_PCI_REGS_BASE,
                                mpic[pci_irq_nrs[0]], mpic[pci_irq_nrs[1]],
                                mpic[pci_irq_nrs[2]], mpic[pci_irq_nrs[3]],
                                NULL);
    pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci.0");
    if (!pci_bus)
        printf("couldn't create PCI controller!\n");

and that's all the interaction there is between the pci host code and the board code. No calling into functions. The way you're doing it now, the board still needs to call into prep_pci.c which doesn't sound too appealing to me :).

> +    DeviceState *dev;
>     PREPPCIState *s;
> 
> -    s = g_malloc0(sizeof(PREPPCIState));
> -    s->bus = pci_register_bus(NULL, "pci",
> +    dev = qdev_create(NULL, "raven-pcihost");
> +    s = FROM_SYSBUS(PREPPCIState, sysbus_from_qdev(dev));
> +    s->address_space = address_space_mem;
> +    s->bus = pci_register_bus(&s->busdev.qdev, "pci",
>                               prep_set_irq, prep_map_irq, pic,
>                               address_space_mem,
>                               address_space_io,
>                               0, 4);

This should be happening in the host bridge init code. Take a look at e500_pcihost_initfn() in hw/ppce500_pci.c.


Alex
Andreas Färber - Jan. 11, 2012, 10:24 p.m.
Am 11.01.2012 23:12, schrieb Alexander Graf:
> 
> On 07.01.2012, at 01:06, Andreas Färber wrote:
> 
>> For now, focus on qdev'ification and leave PIC IRQs unchanged.
>>
>> Signed-off-by: Andreas Färber <andreas.faerber@web.de>
>> Cc: Hervé Poussineau <hpoussin@reactos.org>
>> Cc: Michael S. Tsirkin <mst@redhat.com>
>> Cc: Anthony Liguori <aliguori@us.ibm.com>
>> ---
>> hw/prep_pci.c |   41 +++++++++++++++++++++++++++++++----------
>> 1 files changed, 31 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/prep_pci.c b/hw/prep_pci.c
>> index 741b273..2ff6b8c 100644
>> --- a/hw/prep_pci.c
>> +++ b/hw/prep_pci.c
>> @@ -114,31 +114,43 @@ PCIBus *pci_prep_init(qemu_irq *pic,
>>                       MemoryRegion *address_space_mem,
>>                       MemoryRegion *address_space_io)
>> {
> 
> I'm not sure this is the best way to do this. For e500, we just create the host bridge explicitly in the board file:
> 
>     /* PCI */
>     dev = sysbus_create_varargs("e500-pcihost", MPC8544_PCI_REGS_BASE,
>                                 mpic[pci_irq_nrs[0]], mpic[pci_irq_nrs[1]],
>                                 mpic[pci_irq_nrs[2]], mpic[pci_irq_nrs[3]],
>                                 NULL);
>     pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci.0");
>     if (!pci_bus)
>         printf("couldn't create PCI controller!\n");
> 
> and that's all the interaction there is between the pci host code and the board code. No calling into functions. The way you're doing it now, the board still needs to call into prep_pci.c which doesn't sound too appealing to me :).

That's a TODO for a later patch. As you can see, those lines were not
introduced in this series. For the PCI-ISA bridge, we need to get rid of
qemu_irq *pic anyway - that's what the commit message refers to. Should
clarify that, thanks.

>> +    DeviceState *dev;
>>     PREPPCIState *s;
>>
>> -    s = g_malloc0(sizeof(PREPPCIState));
>> -    s->bus = pci_register_bus(NULL, "pci",
>> +    dev = qdev_create(NULL, "raven-pcihost");
>> +    s = FROM_SYSBUS(PREPPCIState, sysbus_from_qdev(dev));
>> +    s->address_space = address_space_mem;
>> +    s->bus = pci_register_bus(&s->busdev.qdev, "pci",
>>                               prep_set_irq, prep_map_irq, pic,
>>                               address_space_mem,
>>                               address_space_io,
>>                               0, 4);
> 
> This should be happening in the host bridge init code. Take a look at e500_pcihost_initfn() in hw/ppce500_pci.c.

I don't see how that could work for PReP: prep_set_irq and prep_map_irq
need the IRQs allocated by the i8259 on the upcoming i82378 PCI-ISA
bridge, which as a PCIDevice needs the PCI host bridge set up already...

To allow for board-specific setup (prep vs. 40p) v2 uses pci_bus_new()
there and uses pci_bus_irqs() on the board. :)

Andreas

Patch

diff --git a/hw/prep_pci.c b/hw/prep_pci.c
index 741b273..2ff6b8c 100644
--- a/hw/prep_pci.c
+++ b/hw/prep_pci.c
@@ -114,31 +114,43 @@  PCIBus *pci_prep_init(qemu_irq *pic,
                       MemoryRegion *address_space_mem,
                       MemoryRegion *address_space_io)
 {
+    DeviceState *dev;
     PREPPCIState *s;
 
-    s = g_malloc0(sizeof(PREPPCIState));
-    s->bus = pci_register_bus(NULL, "pci",
+    dev = qdev_create(NULL, "raven-pcihost");
+    s = FROM_SYSBUS(PREPPCIState, sysbus_from_qdev(dev));
+    s->address_space = address_space_mem;
+    s->bus = pci_register_bus(&s->busdev.qdev, "pci",
                               prep_set_irq, prep_map_irq, pic,
                               address_space_mem,
                               address_space_io,
                               0, 4);
+    qdev_init_nofail(dev);
+    qdev_property_add_child(qdev_get_root(), "raven", dev, NULL);
+
+    memory_region_init_io(&s->mmcfg, &PPC_PCIIO_ops, s, "pciio", 0x00400000);
+    memory_region_add_subregion(address_space_mem, 0x80800000, &s->mmcfg);
+
+    pci_create_simple(s->bus, 0, "raven");
+
+    return s->bus;
+}
+
+static int raven_pcihost_init(SysBusDevice *dev)
+{
+    PREPPCIState *s = FROM_SYSBUS(PREPPCIState, dev);
 
     memory_region_init_io(&s->conf_mem, &pci_host_conf_be_ops, s,
                           "pci-conf-idx", 1);
-    memory_region_add_subregion(address_space_io, 0xcf8, &s->conf_mem);
+    sysbus_add_io(dev, 0xcf8, &s->conf_mem);
     sysbus_init_ioports(&s->busdev, 0xcf8, 1);
 
     memory_region_init_io(&s->data_mem, &pci_host_data_be_ops, s,
                           "pci-conf-data", 1);
-    memory_region_add_subregion(address_space_io, 0xcfc, &s->data_mem);
+    sysbus_add_io(dev, 0xcfc, &s->data_mem);
     sysbus_init_ioports(&s->busdev, 0xcfc, 1);
 
-    memory_region_init_io(&s->mmcfg, &PPC_PCIIO_ops, s, "pciio", 0x00400000);
-    memory_region_add_subregion(address_space_mem, 0x80800000, &s->mmcfg);
-
-    pci_create_simple(s->bus, 0, "raven");
-
-    return s->bus;
+    return 0;
 }
 
 static int raven_init(PCIDevice *d)
@@ -177,8 +189,17 @@  static PCIDeviceInfo raven_info = {
     },
 };
 
+static SysBusDeviceInfo raven_pcihost_info = {
+    .qdev.name = "raven-pcihost",
+    .qdev.fw_name = "pci",
+    .qdev.size = sizeof(PREPPCIState),
+    .qdev.no_user = 1,
+    .init = raven_pcihost_init,
+};
+
 static void raven_register_devices(void)
 {
+    sysbus_register_withprop(&raven_pcihost_info);
     pci_qdev_register(&raven_info);
 }