Re: [Qemu-devel] [PATCH 1/1] IBM PowerPC 4xx 32-bit PCI controller emulation

[Anthony Liguori]

Hollis Blanchard wrote:
> This PCI controller can be found on a number of 4xx SoCs, including the 440EP.
>
> Signed-off-by: Hollis Blanchard <address@hidden>
>
> --- /dev/null
> +++ b/hw/ppc4xx_pci.c
> @@ -0,0 +1,371 @@
> +/*
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License, version 2, as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
> + *
> + * Copyright IBM Corp. 2008
> + *
> + * Authors: Hollis Blanchard <address@hidden>
> + */
> +
> +/* This file implements emulation of the 32-bit PCI controller found in some
> + * 440 SoCs, such as the 440EP. */
> +
> +#include "hw.h"
> +
> +typedef target_phys_addr_t pci_addr_t;
>   

Nice :-)

> +#include "pci.h"
> +#include "pci_host.h"
> +#include "bswap.h"
> +
> +#undef DEBUG
> +#ifdef DEBUG
> +#define DPRINTF(fmt, args...) do { printf(fmt, ##args); } while (0)
> +#else
> +#define DPRINTF(fmt, args...)
> +#endif /* DEBUG */
>   

This is a GCC-ism that's deprecated.  The proper syntax (C99) is:

#define DPRINTF(fmt, ...) do { printf(fmt, ## __VA_ARGS__); } while (0)

> +struct pci_master_map {
> +    uint32_t la;
> +    uint32_t ma;
> +    uint32_t pcila;
> +    uint32_t pciha;
> +};
> +
> +struct pci_target_map {
> +    uint32_t ms;
> +    uint32_t la;
> +    uint32_t bar;
> +};
> +
> +#define PPC44x_PCI_NR_PMMS 3
> +#define PPC44x_PCI_NR_PTMS 2
> +
> +struct ppc4xx_pci_t {
> +    struct pci_master_map pmm[PPC44x_PCI_NR_PMMS];
> +    struct pci_target_map ptm[PPC44x_PCI_NR_PTMS];
> +
> +    PCIHostState pci_state;
> +};
> +typedef struct ppc4xx_pci_t ppc4xx_pci_t;
>   

It would be better to use QEMU style type naming.

> +#define PCIC0_CFGADDR       0x0
> +#define PCIC0_CFGDATA       0x4
> +
> +/* PLB Memory Map (PMM) registers specify which PLB addresses are translated to
> + * PCI accesses. */
> +#define PCIL0_PMM0LA        0x0
> +#define PCIL0_PMM0MA        0x4
> +#define PCIL0_PMM0PCILA     0x8
> +#define PCIL0_PMM0PCIHA     0xc
> +#define PCIL0_PMM1LA        0x10
> +#define PCIL0_PMM1MA        0x14
> +#define PCIL0_PMM1PCILA     0x18
> +#define PCIL0_PMM1PCIHA     0x1c
> +#define PCIL0_PMM2LA        0x20
> +#define PCIL0_PMM2MA        0x24
> +#define PCIL0_PMM2PCILA     0x28
> +#define PCIL0_PMM2PCIHA     0x2c
> +
> +/* PCI Target Map (PTM) registers specify which PCI addresses are translated to
> + * PLB accesses. */
> +#define PCIL0_PTM1MS        0x30
> +#define PCIL0_PTM1LA        0x34
> +#define PCIL0_PTM2MS        0x38
> +#define PCIL0_PTM2LA        0x3c
> +#define PCI_REG_SIZE        0x40
> +
> +
> +static uint32_t pci4xx_cfgaddr_readl(void *opaque, target_phys_addr_t addr)
> +{
> +    ppc4xx_pci_t *ppc4xx_pci = opaque;
> +
> +    return ppc4xx_pci->pci_state.config_reg;
> +}
> +
> +static CPUReadMemoryFunc *pci4xx_cfgaddr_read[] = {
> +    &pci4xx_cfgaddr_readl,
> +    &pci4xx_cfgaddr_readl,
> +    &pci4xx_cfgaddr_readl,
> +};
> +
> +static void pci4xx_cfgaddr_writel(void *opaque, target_phys_addr_t addr,
> +                                  uint32_t value)
> +{
> +    ppc4xx_pci_t *ppc4xx_pci = opaque;
> +
> +#ifdef TARGET_WORDS_BIGENDIAN
> +    value = bswap32(value);
> +#endif
>
>   

Is this byte swapping correct?

> +
> +    /* XXX register_savevm() */
>   

Should be easy enough to add a register_savevm() function, no?

Regards,

Anthony Liguori

> +    qemu_register_reset(ppc4xx_pci_reset, controller);
> +
> +    return controller->pci_state.bus;
> +
> +free:
> +    printf("%s error\n", __func__);
> +    qemu_free(controller);
> +    return NULL;
> +}
>