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

[Andreas Färber]

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 <address@hidden>
>> Cc: Hervé Poussineau <address@hidden>
>> Cc: Michael S. Tsirkin <address@hidden>
>> Cc: Anthony Liguori <address@hidden>
>> ---
>> 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