Re: [Qemu-devel] [RFC PATCH 09/12] ppc440: Add emulation of plb-pcix controller found in some 440 SoCs

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 4510 bytes --]

On Sun, Aug 13, 2017 at 07:04:38PM +0200, BALATON Zoltan wrote:

You know I'm going to say it, right: needs a commit message.

What's a "plb-pcix", and what's an example of a 440 SoCs which has it.

This is basically a new device, so I'm pretty willing to merge for
2.11 with minimal review once rebased with the rest of the series.

Couple of comments below

[snip]
> +static void ppc440_pcix_reset(DeviceState *dev)
> +{
> +    struct PPC440PCIXState *s = PPC440_PCIX_HOST_BRIDGE(dev);
> +    int i;
> +
> +    memset(s->pom, 0, sizeof(s->pom));
> +    memset(s->pim, 0, sizeof(s->pim));

Is it safe to just memset() the memory region objects within the
pim/pom arrays without cleaning them up?  I'm guessing not..

> +    for (i = 0; i < PPC440_PCIX_NR_PIMS; i++) {
> +        s->pim[i].sa = 0xffffffff00000000ULL;
> +    }
> +    s->sts = 0;
> +}
> +
> +/* All pins from each slot are tied to a single board IRQ.
> + * This may need further refactoring for other boards. */
> +static int ppc440_pcix_map_irq(PCIDevice *pci_dev, int irq_num)
> +{
> +    int slot = pci_dev->devfn >> 3;
> +
> +    DPRINTF("%s: devfn %x irq %d -> %d\n", __func__,
> +            pci_dev->devfn, irq_num, slot);
> +
> +    return slot - 1;
> +}
> +
> +static void ppc440_pcix_set_irq(void *opaque, int irq_num, int level)
> +{
> +    qemu_irq *pci_irqs = opaque;
> +
> +    DPRINTF("%s: PCI irq %d\n", __func__, irq_num);
> +    if (irq_num < 0) {
> +        fprintf(stderr, "%s: PCI irq %d\n", __func__, irq_num);

Use report_error() please.

> +        return;
> +    }
> +    qemu_set_irq(pci_irqs[irq_num], level);
> +}
> +
> +static AddressSpace *ppc440_pcix_set_iommu(PCIBus *b, void *opaque, int devfn)
> +{
> +    PPC440PCIXState *s = opaque;
> +
> +    return &s->bm_as;
> +}
> +
> +static int ppc440_pcix_initfn(SysBusDevice *dev)
> +{
> +    PPC440PCIXState *s;
> +    PCIHostState *h;
> +    int i;
> +
> +    h = PCI_HOST_BRIDGE(dev);
> +    s = PPC440_PCIX_HOST_BRIDGE(dev);
> +
> +    for (i = 0; i < ARRAY_SIZE(s->irq); i++) {
> +        sysbus_init_irq(dev, &s->irq[i]);
> +    }
> +
> +    memory_region_init(&s->busmem, OBJECT(dev), "pci bus memory", UINT64_MAX);
> +    h->bus = pci_register_bus(DEVICE(dev), NULL, ppc440_pcix_set_irq,
> +                         ppc440_pcix_map_irq, s->irq, &s->busmem,
> +                         get_system_io(), PCI_DEVFN(0, 0), 4, TYPE_PCI_BUS);
> +
> +    s->dev = pci_create_simple(h->bus, PCI_DEVFN(0, 0), "ppc4xx-host-bridge");
> +
> +    memory_region_init(&s->bm, OBJECT(s), "bm-ppc440-pcix", UINT64_MAX);
> +    memory_region_add_subregion(&s->bm, 0x0, &s->busmem);
> +    address_space_init(&s->bm_as, &s->bm, "pci-bm");
> +    pci_setup_iommu(h->bus, ppc440_pcix_set_iommu, s);
> +
> +    memory_region_init(&s->container, OBJECT(s), "pci-container", PCI_ALL_SIZE);
> +    memory_region_init_io(&h->conf_mem, OBJECT(s), &pci_host_conf_le_ops, h,
> +                          "pci-conf-idx", 4);
> +    memory_region_init_io(&h->data_mem, OBJECT(s), &pci_host_data_le_ops, h,
> +                          "pci-conf-data", 4);
> +    memory_region_init_io(&s->iomem, OBJECT(s), &pci_reg_ops, s,
> +                          "pci.reg", PPC440_REG_SIZE);
> +    memory_region_add_subregion(&s->container, PCIC0_CFGADDR, &h->conf_mem);
> +    memory_region_add_subregion(&s->container, PCIC0_CFGDATA, &h->data_mem);
> +    memory_region_add_subregion(&s->container, PPC440_REG_BASE, &s->iomem);
> +    sysbus_init_mmio(dev, &s->container);
> +
> +    return 0;
> +}
> +
> +static void ppc440_pcix_class_init(ObjectClass *klass, void *data)
> +{
> +    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> +    DeviceClass *dc = DEVICE_CLASS(klass);
> +
> +    k->init = ppc440_pcix_initfn;
> +    dc->reset = ppc440_pcix_reset;
> +}
> +
> +static const TypeInfo ppc440_pcix_info = {
> +    .name          = TYPE_PPC440_PCIX_HOST_BRIDGE,
> +    .parent        = TYPE_PCI_HOST_BRIDGE,
> +    .instance_size = sizeof(PPC440PCIXState),
> +    .class_init    = ppc440_pcix_class_init,
> +};
> +
> +static void ppc440_pcix_register_types(void)
> +{
> +    type_register_static(&ppc440_pcix_info);
> +}
> +
> +type_init(ppc440_pcix_register_types)

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]