Re: [Qemu-devel] [Qemu-ppc] [PATCH v2 2/3] prep: Add Raven PCI host SysBus device

["Andreas Färber" <andreas.faerber@web.de>]

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