Re: [Qemu-devel] [PATCH 02/28] openpic: remove dead code to make a PCI device version

["Andreas Färber" <afaerber@suse.de>]

Am 24.01.2012 20:32, schrieb Anthony Liguori:
> bus is always NULL so the code in this if clause is dead (and therefore
> untested).
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> ---
>  hw/openpic.c      |   30 +++---------------------------
>  hw/openpic.h      |    2 +-
>  hw/ppc_newworld.c |    2 +-
>  3 files changed, 5 insertions(+), 29 deletions(-)
> 
> diff --git a/hw/openpic.c b/hw/openpic.c
> index 22fc275..280b7a9 100644
> --- a/hw/openpic.c
> +++ b/hw/openpic.c
> @@ -1181,41 +1181,17 @@ static void openpic_irq_raise(openpic_t *opp, int n_CPU, IRQ_src_t *src)
>      qemu_irq_raise(opp->dst[n_CPU].irqs[OPENPIC_OUTPUT_INT]);
>  }
>  
> -qemu_irq *openpic_init (PCIBus *bus, MemoryRegion **pmem, int nb_cpus,
> +qemu_irq *openpic_init (MemoryRegion **pmem, int nb_cpus,
>                          qemu_irq **irqs, qemu_irq irq_out)

Here a reindentation (space) would've been nice.

>  {
>      openpic_t *opp;
> -    uint8_t *pci_conf;
>      int i, m;
>  
>      /* XXX: for now, only one CPU is supported */
>      if (nb_cpus != 1)
>          return NULL;
> -    if (bus) {
> -        opp = (openpic_t *)pci_register_device(bus, "OpenPIC", sizeof(openpic_t),
> -                                               -1, NULL, NULL);
> -        pci_conf = opp->pci_dev.config;
> -        pci_config_set_vendor_id(pci_conf, PCI_VENDOR_ID_IBM);
> -        pci_config_set_device_id(pci_conf, PCI_DEVICE_ID_IBM_OPENPIC2);
> -        pci_config_set_class(pci_conf, PCI_CLASS_SYSTEM_OTHER); // FIXME?
> -        pci_conf[0x3d] = 0x00; // no interrupt pin

I'm not too fond of losing PCI device infos from dead code, but
registering an unused PCIDeviceInfo doesn't seem right either...

AFAIU this function implements a PCI and a non-PCI version of the
device. The PCI version is unused and is now being ripped out.

> -
> -        memory_region_init_io(&opp->mem, &openpic_ops, opp, "openpic", 0x40000);
> -#if 0 // Don't implement ISU for now
> -        opp_io_memory = cpu_register_io_memory(openpic_src_read,
> -                                               openpic_src_write, NULL
> -                                               DEVICE_NATIVE_ENDIAN);
> -        cpu_register_physical_memory(isu_base, 0x20 * (EXT_IRQ + 2),
> -                                     opp_io_memory);
> -#endif
> -
> -        /* Register I/O spaces */
> -        pci_register_bar(&opp->pci_dev, 0,
> -                         PCI_BASE_ADDRESS_SPACE_MEMORY, &opp->mem);
> -    } else {
> -        opp = g_malloc0(sizeof(openpic_t));
> -        memory_region_init_io(&opp->mem, &openpic_ops, opp, "openpic", 0x40000);
> -    }
> +    opp = g_malloc0(sizeof(openpic_t));

Ugh. But just moved around.
(More non-qdev'ified code lurking in that file.)

> +    memory_region_init_io(&opp->mem, &openpic_ops, opp, "openpic", 0x40000);
>  
>      //    isu_base &= 0xFFFC0000;
>      opp->nb_cpus = nb_cpus;
> diff --git a/hw/openpic.h b/hw/openpic.h
> index 715f084..8556030 100644
> --- a/hw/openpic.h
> +++ b/hw/openpic.h
> @@ -11,7 +11,7 @@ enum {
>      OPENPIC_OUTPUT_NB,
>  };
>  
> -qemu_irq *openpic_init (PCIBus *bus, MemoryRegion **pmem, int nb_cpus,
> +qemu_irq *openpic_init (MemoryRegion **pmem, int nb_cpus,
>                          qemu_irq **irqs, qemu_irq irq_out);

Space.

>  qemu_irq *mpic_init (MemoryRegion *address_space, target_phys_addr_t base,
>                       int nb_cpus, qemu_irq **irqs, qemu_irq irq_out);
> diff --git a/hw/ppc_newworld.c b/hw/ppc_newworld.c
> index a746c9c..506187b 100644
> --- a/hw/ppc_newworld.c
> +++ b/hw/ppc_newworld.c
> @@ -311,7 +311,7 @@ static void ppc_core99_init (ram_addr_t ram_size,
>              exit(1);
>          }
>      }
> -    pic = openpic_init(NULL, &pic_mem, smp_cpus, openpic_irqs, NULL);
> +    pic = openpic_init(&pic_mem, smp_cpus, openpic_irqs, NULL);
>      if (PPC_INPUT(env) == PPC_FLAGS_INPUT_970) {
>          /* 970 gets a U3 bus */
>          pci_bus = pci_pmac_u3_init(pic, get_system_memory(), get_system_io());

Anyway, to keep moving forward,

Reviewed-by: Andreas Färber <afaerber@suse.de>

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg