[02/10] versatile_pci: Expose PCI I/O region on Versatile PB

Comments in the QEMU source code claim that the version of the PCI
controller on the VersatilePB board doesn't support the PCI I/O
region, but this is incorrect; expose that region, map it in the
correct location, and drop the misleading comments.

This change removes the only currently implemented difference
between the realview-pci and versatile-pci models; however there
are other differences in not-yet-implemented functionality, so we
retain the distinction between the two device types.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/arm/versatilepb.c |    3 +--
 hw/versatile_pci.c   |    8 +++-----
 2 files changed, 4 insertions(+), 7 deletions(-)

Hi Peter,

On Sun, Mar 24, 2013 at 9:32 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> Comments in the QEMU source code claim that the version of the PCI
> controller on the VersatilePB board doesn't support the PCI I/O
> region, but this is incorrect; expose that region, map it in the
> correct location, and drop the misleading comments.
>
> This change removes the only currently implemented difference
> between the realview-pci and versatile-pci models; however there
> are other differences in not-yet-implemented functionality, so we
> retain the distinction between the two device types.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  hw/arm/versatilepb.c |    3 +--
>  hw/versatile_pci.c   |    8 +++-----
>  2 files changed, 4 insertions(+), 7 deletions(-)
>
> diff --git a/hw/arm/versatilepb.c b/hw/arm/versatilepb.c
> index baaa265..0d08d15 100644
> --- a/hw/arm/versatilepb.c
> +++ b/hw/arm/versatilepb.c
> @@ -226,14 +226,13 @@ static void versatile_init(QEMUMachineInitArgs *args, int board_id)
>      qdev_init_nofail(dev);
>      sysbus_mmio_map(busdev, 0, 0x41000000); /* PCI self-config */
>      sysbus_mmio_map(busdev, 1, 0x42000000); /* PCI config */
> +    sysbus_mmio_map(busdev, 2, 0x43000000); /* PCI I/O */
>      sysbus_connect_irq(busdev, 0, sic[27]);
>      sysbus_connect_irq(busdev, 1, sic[28]);
>      sysbus_connect_irq(busdev, 2, sic[29]);
>      sysbus_connect_irq(busdev, 3, sic[30]);
>      pci_bus = (PCIBus *)qdev_get_child_bus(dev, "pci");
>
> -    /* The Versatile PCI bridge does not provide access to PCI IO space,
> -       so many of the qemu PCI devices are not useable.  */
>      for(n = 0; n < nb_nics; n++) {
>          nd = &nd_table[n];
>
> diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
> index 16ce5d1..1312f46 100644
> --- a/hw/versatile_pci.c
> +++ b/hw/versatile_pci.c
> @@ -77,7 +77,7 @@ static int pci_vpb_init(SysBusDevice *dev)
>      /* Our memory regions are:
>       * 0 : PCI self config window
>       * 1 : PCI config window
> -     * 2 : PCI IO window (realview_pci only)
> +     * 2 : PCI IO window
>       */
>      memory_region_init_io(&s->mem_config, &pci_vpb_config_ops, bus,
>                            "pci-vpb-selfconfig", 0x1000000);
> @@ -85,10 +85,8 @@ static int pci_vpb_init(SysBusDevice *dev)
>      memory_region_init_io(&s->mem_config2, &pci_vpb_config_ops, bus,
>                            "pci-vpb-config", 0x1000000);
>      sysbus_init_mmio(dev, &s->mem_config2);
> -    if (s->realview) {

This is the one and only usage of ->realview. I wonder if this
argument is flawed - in real hardware is there any functional
difference between realview and versatile PCI requiring the level of
heirachy defined here? Could we take the oppurtunity to knock out this
realview flag and the "realview_pci" class altogether and save on all
the QOM boilerplate for the now functionally identical subclass?

Regards,
Peter

> -        isa_mmio_setup(&s->isa, 0x0100000);
> -        sysbus_init_mmio(dev, &s->isa);
> -    }
> +    isa_mmio_setup(&s->isa, 0x0100000);
> +    sysbus_init_mmio(dev, &s->isa);
>
>      pci_create_simple(bus, -1, "versatile_pci_host");
>      return 0;
> --
> 1.7.9.5
>
>

On 25 March 2013 01:01, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
> On Sun, Mar 24, 2013 at 9:32 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> Comments in the QEMU source code claim that the version of the PCI
>> controller on the VersatilePB board doesn't support the PCI I/O
>> region, but this is incorrect; expose that region, map it in the
>> correct location, and drop the misleading comments.
>>
>> This change removes the only currently implemented difference
>> between the realview-pci and versatile-pci models; however there
>> are other differences in not-yet-implemented functionality, so we
>> retain the distinction between the two device types.

>> @@ -85,10 +85,8 @@ static int pci_vpb_init(SysBusDevice *dev)
>>      memory_region_init_io(&s->mem_config2, &pci_vpb_config_ops, bus,
>>                            "pci-vpb-config", 0x1000000);
>>      sysbus_init_mmio(dev, &s->mem_config2);
>> -    if (s->realview) {
>
> This is the one and only usage of ->realview. I wonder if this
> argument is flawed - in real hardware is there any functional
> difference between realview and versatile PCI requiring the level of
> heirachy defined here?

Please read the commit message and the later patches in the series :-)

thanks
-- PMM

On 25 March 2013 09:47, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 25 March 2013 01:01, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote:
>> This is the one and only usage of ->realview. I wonder if this
>> argument is flawed - in real hardware is there any functional
>> difference between realview and versatile PCI requiring the level of
>> heirachy defined here?
>
> Please read the commit message and the later patches in the series :-)

Er, and to be slightly more helpful, the following things differ
between versatilepb and realview:
 * behaviour of IMAP registers
 * memory window sizes
 * IRQ mapping

and some things we don't yet implement:
 * SMAP register behaviour
 * PCI_FLAGS register bits seem to be a little different

-- PMM