Re: [Qemu-devel] [PATCH 10/15] apb: remove pci_apb_init() and instantiate APB device using qdev

[Mark Cave-Ayland]

On 20/11/17 17:51, Philippe Mathieu-Daudé wrote:

> Hi Mark,
> 
> On 11/17/2017 10:42 AM, Mark Cave-Ayland wrote:
>> By making the special_base and mem_base values qdev properties, we can move
>> the remaining parts of pci_apb_init() into the pbm init() and realize()
>> functions.
>>
>> This finally allows us to instantiate the APB directly using standard qdev
>> create/init functions in sun4u.c.
>>
>> Signed-off-by: Mark Cave-Ayland <address@hidden>
>> ---
>>  hw/pci-host/apb.c         |  123 
>> ++++++++++++++++++++++-----------------------
>>  hw/sparc64/sun4u.c        |    6 ++-
>>  include/hw/pci-host/apb.h |    4 +-
>>  3 files changed, 68 insertions(+), 65 deletions(-)
>>
>> diff --git a/hw/pci-host/apb.c b/hw/pci-host/apb.c
>> index 823661a..6c20285 100644
>> --- a/hw/pci-host/apb.c
>> +++ b/hw/pci-host/apb.c
>> @@ -611,41 +611,56 @@ static void apb_pci_bridge_realize(PCIDevice *dev, 
>> Error **errp)
>>      pci_bridge_update_mappings(PCI_BRIDGE(br));
>>  }
>>  
>> -APBState *pci_apb_init(hwaddr special_base,
>> -                       hwaddr mem_base)
>> +static void pci_pbm_reset(DeviceState *d)
>>  {
>> -    DeviceState *dev;
>> -    SysBusDevice *s;
>> -    PCIHostState *phb;
>> -    APBState *d;
>> -    IOMMUState *is;
>> +    unsigned int i;
>> +    APBState *s = APB_DEVICE(d);
>> +
>> +    for (i = 0; i < 8; i++) {
>> +        s->pci_irq_map[i] &= PBM_PCI_IMR_MASK;
>> +    }
>> +    for (i = 0; i < 32; i++) {
>> +        s->obio_irq_map[i] &= PBM_PCI_IMR_MASK;
>> +    }
>> +
>> +    s->irq_request = NO_IRQ_REQUEST;
>> +    s->pci_irq_in = 0ULL;
>> +
>> +    if (s->nr_resets++ == 0) {
>> +        /* Power on reset */
>> +        s->reset_control = POR;
>> +    }
>> +}
>> +
>> +static const MemoryRegionOps pci_config_ops = {
>> +    .read = apb_pci_config_read,
>> +    .write = apb_pci_config_write,
>> +    .endianness = DEVICE_LITTLE_ENDIAN,
>> +};
>> +
>> +static void pci_pbm_realize(DeviceState *dev, Error **errp)
>> +{
>> +    APBState *s = APB_DEVICE(dev);
>> +    PCIHostState *phb = PCI_HOST_BRIDGE(dev);
>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(s);
>>      PCIDevice *pci_dev;
>> +    IOMMUState *is;
>>  
>> -    /* Ultrasparc PBM main bus */
>> -    dev = qdev_create(NULL, TYPE_APB);
>> -    d = APB_DEVICE(dev);
>> -    phb = PCI_HOST_BRIDGE(dev);
>> -    phb->bus = pci_register_bus(DEVICE(phb), "pci",
>> -                                pci_apb_set_irq, pci_apb_map_irq, d,
>> -                                &d->pci_mmio,
>> -                                &d->pci_ioport,
>> -                                0, 32, TYPE_PCI_BUS);
>> -    qdev_init_nofail(dev);
>> -    s = SYS_BUS_DEVICE(dev);
>>      /* apb_config */
>> -    sysbus_mmio_map(s, 0, special_base);
>> +    sysbus_mmio_map(sbd, 0, s->special_base);
> 
> You might add a safety check than special_base is correctly initialize
> (compare to -1?)

I guess strictly speaking this would be good to have, however since the
same value is also hard-coded in OpenBIOS then you'll get a crash if you
change it to any other value anyhow. So for that reason I think it's
okay for the moment as it is.


ATB,

Mark.