On Tue, Jan 12, 2010 at 05:52:58PM +0900, Isaku Yamahata wrote:
> Move out pci address decoding logic from pci_data_{write, read}()
> by making pci_data_{write, read}() get PCIConfigAddress.
>
> Cc: Blue Swirl <blauwir...@gmail.com>
> Cc: Aurelien Jarno <aurel...@aurel32.net>
> Cc: Paul Brook <p...@codesourcery.com>
> Signed-off-by: Isaku Yamahata <yamah...@valinux.co.jp>
> ---
> hw/apb_pci.c | 12 ++++++++----
> hw/gt64xxx.c | 20 ++++++++++++--------
> hw/pci_host.c | 25 ++++++++++++++-----------
> hw/pci_host.h | 5 +++--
> hw/pci_host_template.h | 15 +++++++--------
> hw/prep_pci.c | 28 ++++++++++++++++++++++------
> hw/sh_pci.c | 24 ++++++++++++++++++++----
> hw/versatile_pci.c | 30 ++++++++++++++++++++++++------
> 8 files changed, 110 insertions(+), 49 deletions(-)
>
> diff --git a/hw/apb_pci.c b/hw/apb_pci.c
> index c14e1c0..3c098d2 100644
> --- a/hw/apb_pci.c
> +++ b/hw/apb_pci.c
> @@ -107,18 +107,22 @@ static CPUReadMemoryFunc * const apb_config_read[] = {
> static void apb_pci_config_write(APBState *s, target_phys_addr_t addr,
> uint32_t val, int size)
> {
> + PCIConfigAddress conf_addr;
> APB_DPRINTF("%s: addr " TARGET_FMT_lx " val %x\n", __func__, addr, val);
> - pci_data_write(s->host_state.bus, (addr & 0x00ffffff) | (1u << 31), val,
> - size);
> + pci_host_decode_config_addr_valid(&s->host_state, (addr & 0x00ffffff),
> + &conf_addr);
> + pci_data_write(&conf_addr, 0, val, size);
> }
>
> static uint32_t apb_pci_config_read(APBState *s, target_phys_addr_t addr,
> int size)
> {
> + PCIConfigAddress conf_addr;
> uint32_t ret;
>
> - ret = pci_data_read(s->host_state.bus, (addr & 0x00ffffff) | (1u << 31),
> - size);
> + pci_host_decode_config_addr_valid(&s->host_state, (addr & 0x00ffffff),
> + &conf_addr);
> + ret = pci_data_read(&conf_addr, 0, size);
> APB_DPRINTF("%s: addr " TARGET_FMT_lx " -> %x\n", __func__, addr, ret);
> return ret;
> }
> diff --git a/hw/gt64xxx.c b/hw/gt64xxx.c
> index c8034e2..2e135dc 100644
> --- a/hw/gt64xxx.c
> +++ b/hw/gt64xxx.c
> @@ -527,12 +527,15 @@ static void gt64120_writel (void *opaque,
> target_phys_addr_t addr,
> case GT_PCI0_CFGADDR:
> s->pci->config_reg = val & 0x80fffffc;
> break;
> - case GT_PCI0_CFGDATA:
> + case GT_PCI0_CFGDATA: {
> + PCIConfigAddress conf_addr;
> if (!(s->regs[GT_PCI0_CMD] & 1) && (s->pci->config_reg & 0x00fff800))
> val = bswap32(val);
> - if (s->pci->config_reg & (1u << 31))
> - pci_data_write(s->pci->bus, s->pci->config_reg, val, 4);
> + pci_host_decode_config_addr_cfge(s->pci, s->pci->config_reg,
> + &conf_addr);
> + pci_data_write(&conf_addr, 0, val, 4);
> break;
> + }
>
> /* Interrupts */
> case GT_INTRCAUSE:
> @@ -767,14 +770,15 @@ static uint32_t gt64120_readl (void *opaque,
> case GT_PCI0_CFGADDR:
> val = s->pci->config_reg;
> break;
> - case GT_PCI0_CFGDATA:
> - if (!(s->pci->config_reg & (1 << 31)))
> - val = 0xffffffff;
> - else
> - val = pci_data_read(s->pci->bus, s->pci->config_reg, 4);
> + case GT_PCI0_CFGDATA: {
> + PCIConfigAddress conf_addr;
> + pci_host_decode_config_addr_cfge(s->pci, s->pci->config_reg,
> + &conf_addr);
> + val = pci_data_read(&conf_addr, 0, 4);
> if (!(s->regs[GT_PCI0_CMD] & 1) && (s->pci->config_reg & 0x00fff800))
> val = bswap32(val);
> break;
> + }
>
> case GT_PCI0_CMD:
> case GT_PCI0_TOR:
> diff --git a/hw/pci_host.c b/hw/pci_host.c
> index fa194e2..c837110 100644
> --- a/hw/pci_host.c
> +++ b/hw/pci_host.c
> @@ -72,18 +72,21 @@ void pci_host_decode_config_addr_valid(const PCIHostState
> *s,
> }
>
> /* the helper functio to get a PCIDeice* for a given pci address */
> -static inline PCIDevice *pci_dev_find_by_addr(PCIBus *bus, uint32_t addr)
> +static inline PCIDevice *pci_dev_find_by_addr(
> + const PCIConfigAddress *conf_addr)
> {
> - uint8_t bus_num = addr >> 16;
> - uint8_t devfn = addr >> 8;
> -
> - return pci_find_device(bus, bus_num, PCI_SLOT(devfn), PCI_FUNC(devfn));
> + const PCIAddress *addr = &conf_addr->addr;
> + if (!conf_addr->valid) {
> + return NULL;
> + }
> + return pci_find_device(addr->domain, addr->bus, addr->slot, addr->fn);
> }
>
> -void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
> +void pci_data_write(PCIConfigAddress *conf_addr,
> + uint32_t addr, uint32_t val, int len)
> {
> - PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
> - uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
> + PCIDevice *pci_dev = pci_dev_find_by_addr(conf_addr);
> + uint32_t config_addr = conf_addr->offset | (conf_addr->addr_mask & addr);
>
> if (!pci_dev)
> return;
> @@ -93,10 +96,10 @@ void pci_data_write(PCIBus *s, uint32_t addr, uint32_t
> val, int len)
> pci_dev->config_write(pci_dev, config_addr, val, len);
> }
>
> -uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len)
> +uint32_t pci_data_read(PCIConfigAddress *conf_addr, uint32_t addr, int len)
> {
> - PCIDevice *pci_dev = pci_dev_find_by_addr(s, addr);
> - uint32_t config_addr = addr & (PCI_CONFIG_SPACE_SIZE - 1);
> + PCIDevice *pci_dev = pci_dev_find_by_addr(conf_addr);
> + uint32_t config_addr = conf_addr->offset | (conf_addr->addr_mask & addr);
> uint32_t val;
>
> assert(len == 1 || len == 2 || len == 4);
> diff --git a/hw/pci_host.h b/hw/pci_host.h
> index ebc95f2..d2d558d 100644
> --- a/hw/pci_host.h
> +++ b/hw/pci_host.h
> @@ -52,8 +52,9 @@ void pci_host_decode_config_addr_valid(const PCIHostState
> *s,
> uint32_t config_reg,
> PCIConfigAddress *decoded);
>
> -void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len);
> -uint32_t pci_data_read(PCIBus *s, uint32_t addr, int len);
> +void pci_data_write(PCIConfigAddress *conf_addr,
> + uint32_t addr, uint32_t val, int len);
> +uint32_t pci_data_read(PCIConfigAddress *conf_addr, uint32_t addr, int len);
>
> /* for mmio */
> int pci_host_conf_register_mmio(PCIHostState *s);
> diff --git a/hw/pci_host_template.h b/hw/pci_host_template.h
> index 2c508bf..02f0ff1 100644
> --- a/hw/pci_host_template.h
> +++ b/hw/pci_host_template.h
> @@ -29,28 +29,27 @@ static void glue(glue(pci_host_data_write, PCI_HOST_BWL),
> PCI_HOST_SUFFIX)(
> void* opaque, PCI_ADDR_T addr, uint32_t val)
> {
> PCIHostState *s = opaque;
> + PCIConfigAddress conf_addr;
>
> #if defined(TARGET_WORDS_BIGENDIAN) && (PCI_HOST_BITS > 8)
> val = glue(bswap, PCI_HOST_BITS)(val);
> #endif
> PCI_DPRINTF("%s: addr " TARGET_FMT_plx " val %x\n",
> __func__, (target_phys_addr_t)addr, val);
> - if (s->config_reg & (1u << 31))
> - pci_data_write(s->bus, s->config_reg | (addr & 3), val,
> - sizeof(glue(glue(uint, PCI_HOST_BITS), _t)));
> + pci_host_decode_config_addr_cfge(s, s->config_reg, &conf_addr);
> + pci_data_write(&conf_addr, addr, val,
> + sizeof(glue(glue(uint, PCI_HOST_BITS), _t)));
> }
>
> static uint32_t glue(glue(pci_host_data_read, PCI_HOST_BWL),
> PCI_HOST_SUFFIX)(
> void* opaque, PCI_ADDR_T addr)
> {
> PCIHostState *s = opaque;
> + PCIConfigAddress conf_addr;
> uint32_t val;
>
> - if (!(s->config_reg & (1 << 31))) {
> - return ( 1ULL << PCI_HOST_BITS ) - 1;
> - }
> -
> - val = pci_data_read(s->bus, s->config_reg | (addr & 3),
> + pci_host_decode_config_addr_cfge(s, s->config_reg, &conf_addr);
> + val = pci_data_read(&conf_addr, addr,
> sizeof(glue(glue(uint, PCI_HOST_BITS), _t)));
> PCI_DPRINTF("%s addr " TARGET_FMT_plx " val %x\n",
> __func__, (target_phys_addr_t)addr, val);
> diff --git a/hw/prep_pci.c b/hw/prep_pci.c
> index 19f028c..53862d7 100644
> --- a/hw/prep_pci.c
> +++ b/hw/prep_pci.c
> @@ -40,10 +40,26 @@ static inline uint32_t
> PPC_PCIIO_config(target_phys_addr_t addr)
> return (addr & 0x7ff) | (i << 11);
> }
>
> +static void PPC_pci_data_write(PREPPCIState *s, target_phys_addr_t addr,
> + uint32_t val, int len)
> +{
> + PCIConfigAddress conf_addr;
> + pci_host_decode_config_addr_valid(s, PPC_PCIIO_config(addr), &conf_addr);
> + pci_data_write(&conf_addr, 0, val, len);
> +}
> +
> +static uint32_t PPC_pci_data_read(PREPPCIState *s, target_phys_addr_t addr,
> + int len)
> +{
> + PCIConfigAddress conf_addr;
> + pci_host_decode_config_addr_valid(s, PPC_PCIIO_config(addr), &conf_addr);
> + return pci_data_read(&conf_addr, 0, len);
> +}
> +
> static void PPC_PCIIO_writeb (void *opaque, target_phys_addr_t addr,
> uint32_t val)
> {
> PREPPCIState *s = opaque;
> - pci_data_write(s->bus, PPC_PCIIO_config(addr), val, 1);
> + PPC_pci_data_write(s, addr, val, 1);
> }
>
> static void PPC_PCIIO_writew (void *opaque, target_phys_addr_t addr,
> uint32_t val)
> @@ -52,7 +68,7 @@ static void PPC_PCIIO_writew (void *opaque,
> target_phys_addr_t addr, uint32_t va
> #ifdef TARGET_WORDS_BIGENDIAN
> val = bswap16(val);
> #endif
> - pci_data_write(s->bus, PPC_PCIIO_config(addr), val, 2);
> + PPC_pci_data_write(s, addr, val, 2);
> }
>
> static void PPC_PCIIO_writel (void *opaque, target_phys_addr_t addr,
> uint32_t val)
> @@ -61,14 +77,14 @@ static void PPC_PCIIO_writel (void *opaque,
> target_phys_addr_t addr, uint32_t va
> #ifdef TARGET_WORDS_BIGENDIAN
> val = bswap32(val);
> #endif
> - pci_data_write(s->bus, PPC_PCIIO_config(addr), val, 4);
> + PPC_pci_data_write(s, addr, val, 4);
> }
>
> static uint32_t PPC_PCIIO_readb (void *opaque, target_phys_addr_t addr)
> {
> PREPPCIState *s = opaque;
> uint32_t val;
> - val = pci_data_read(s->bus, PPC_PCIIO_config(addr), 1);
> + val = PPC_pci_data_read(s, addr, 1);
> return val;
> }
>
> @@ -76,7 +92,7 @@ static uint32_t PPC_PCIIO_readw (void *opaque,
> target_phys_addr_t addr)
> {
> PREPPCIState *s = opaque;
> uint32_t val;
> - val = pci_data_read(s->bus, PPC_PCIIO_config(addr), 2);
> + val = PPC_pci_data_read(s, addr, 2);
> #ifdef TARGET_WORDS_BIGENDIAN
> val = bswap16(val);
> #endif
> @@ -87,7 +103,7 @@ static uint32_t PPC_PCIIO_readl (void *opaque,
> target_phys_addr_t addr)
> {
> PREPPCIState *s = opaque;
> uint32_t val;
> - val = pci_data_read(s->bus, PPC_PCIIO_config(addr), 4);
> + val = PPC_pci_data_read(s, addr, 4);
> #ifdef TARGET_WORDS_BIGENDIAN
> val = bswap32(val);
> #endif
> diff --git a/hw/sh_pci.c b/hw/sh_pci.c
> index 046db2e..2a8f132 100644
> --- a/hw/sh_pci.c
> +++ b/hw/sh_pci.c
> @@ -36,6 +36,22 @@ typedef struct {
> uint32_t iobr;
> } SHPCIC;
>
> +static void sh_pci_data_write(SHPCIC *pcic, target_phys_addr_t addr,
> + uint32_t val, int size)
> +{
> + PCIConfigAddress conf_addr;
> + pci_host_decode_config_addr_valid(&pcic->s, addr, &conf_addr);
> + pci_data_write(&conf_addr, 0, val, size);
> +}
> +
> +static uint32_t sh_pci_data_read(SHPCIC *pcic,
> + target_phys_addr_t addr, int size)
> +{
> + PCIConfigAddress conf_addr;
> + pci_host_decode_config_addr_valid(&pcic->s, addr, &conf_addr);
> + return pci_data_read(&conf_addr, 0, size);
> +}
> +
> static void sh_pci_reg_write (void *p, target_phys_addr_t addr, uint32_t val)
> {
> SHPCIC *pcic = p;
> @@ -53,7 +69,7 @@ static void sh_pci_reg_write (void *p, target_phys_addr_t
> addr, uint32_t val)
> pcic->iobr = val;
> break;
> case 0x220:
> - pci_data_write(pcic->s.bus, pcic->par, val, 4);
> + sh_pci_data_write(pcic, pcic->par, val, 4);
> break;
> }
> }
> @@ -67,7 +83,7 @@ static uint32_t sh_pci_reg_read (void *p,
> target_phys_addr_t addr)
> case 0x1c0:
> return pcic->par;
> case 0x220:
> - return pci_data_read(pcic->s.bus, pcic->par, 4);
> + return sh_pci_data_read(pcic, pcic->par, 4);
> }
> return 0;
> }
> @@ -75,13 +91,13 @@ static uint32_t sh_pci_reg_read (void *p,
> target_phys_addr_t addr)
> static void sh_pci_mem_write (SHPCIC *pcic, target_phys_addr_t addr,
> uint32_t val, int size)
> {
> - pci_data_write(pcic->s.bus, addr + pcic->mbr, val, size);
> + sh_pci_data_write(pcic, addr + pcic->mbr, val, size);
> }
>
> static uint32_t sh_pci_mem_read (SHPCIC *pcic, target_phys_addr_t addr,
> int size)
> {
> - return pci_data_read(pcic->s.bus, addr + pcic->mbr, size);
> + return sh_pci_data_read(pcic, addr + pcic->mbr, size);
> }
>
> static void sh_pci_writeb (void *p, target_phys_addr_t addr, uint32_t val)
> diff --git a/hw/versatile_pci.c b/hw/versatile_pci.c
> index d99b7fa..14a728a 100644
> --- a/hw/versatile_pci.c
> +++ b/hw/versatile_pci.c
> @@ -23,11 +23,29 @@ static inline uint32_t
> vpb_pci_config_addr(target_phys_addr_t addr)
> return addr & 0xffffff;
> }
>
> +static void vpb_pci_data_write(PCIHostState *s, target_phys_addr_t addr,
> + uint32_t val, int len)
> +{
> + PCIConfigAddress conf_addr;
> + pci_host_decode_config_addr_valid(s, vpb_pci_config_addr(addr),
> + &conf_addr);
> + pci_data_write(&conf_addr, 0, val, len);
> +}
> +
> +static uint32_t vpb_pci_data_read(PCIHostState *s, target_phys_addr_t addr,
> + int len)
> +{
> + PCIConfigAddress conf_addr;
> + pci_host_decode_config_addr_valid(s, vpb_pci_config_addr(addr),
> + &conf_addr);
> + return pci_data_read(&conf_addr, 0, len);
> +}
> +
I thought we will get rid of vpb_pci_config_addr, and fill in
fields in PCIConfigAddress directly. If we don't, and still
recode into PC format, this is not making code any prettier
so I don't really see what this buys us.
> static void pci_vpb_config_writeb (void *opaque, target_phys_addr_t addr,
> uint32_t val)
> {
> PCIHostState *s = opaque;
> - pci_data_write(s->bus, vpb_pci_config_addr (addr), val, 1);
> + vpb_pci_data_write(s, addr, val, 1);
> }
>
> static void pci_vpb_config_writew (void *opaque, target_phys_addr_t addr,
> @@ -37,7 +55,7 @@ static void pci_vpb_config_writew (void *opaque,
> target_phys_addr_t addr,
> #ifdef TARGET_WORDS_BIGENDIAN
> val = bswap16(val);
> #endif
> - pci_data_write(s->bus, vpb_pci_config_addr (addr), val, 2);
> + vpb_pci_data_write(s, addr, val, 2);
> }
>
> static void pci_vpb_config_writel (void *opaque, target_phys_addr_t addr,
> @@ -47,14 +65,14 @@ static void pci_vpb_config_writel (void *opaque,
> target_phys_addr_t addr,
> #ifdef TARGET_WORDS_BIGENDIAN
> val = bswap32(val);
> #endif
> - pci_data_write(s->bus, vpb_pci_config_addr (addr), val, 4);
> + vpb_pci_data_write(s, addr, val, 4);
> }
>
> static uint32_t pci_vpb_config_readb (void *opaque, target_phys_addr_t addr)
> {
> PCIHostState *s = opaque;
> uint32_t val;
> - val = pci_data_read(s->bus, vpb_pci_config_addr (addr), 1);
> + val = vpb_pci_data_read(s, addr, 1);
> return val;
> }
>
> @@ -62,7 +80,7 @@ static uint32_t pci_vpb_config_readw (void *opaque,
> target_phys_addr_t addr)
> {
> PCIHostState *s = opaque;
> uint32_t val;
> - val = pci_data_read(s->bus, vpb_pci_config_addr (addr), 2);
> + val = vpb_pci_data_read(s, addr, 2);
> #ifdef TARGET_WORDS_BIGENDIAN
> val = bswap16(val);
> #endif
> @@ -73,7 +91,7 @@ static uint32_t pci_vpb_config_readl (void *opaque,
> target_phys_addr_t addr)
> {
> PCIHostState *s = opaque;
> uint32_t val;
> - val = pci_data_read(s->bus, vpb_pci_config_addr (addr), 4);
> + val = vpb_pci_data_read(s, addr, 4);
> #ifdef TARGET_WORDS_BIGENDIAN
> val = bswap32(val);
> #endif
> --
> 1.6.5.4
