RE: [Qemu-devel] [PATCH 2/6] kvm/powerpc: Add freescale pcicontroller's support

[Liu Yu-B13201]

> -----Original Message-----
> From: Aurelien Jarno [mailto:address@hidden 
> Sent: Sunday, January 25, 2009 12:03 AM
> To: Liu Yu-B13201
> Cc: address@hidden; address@hidden; address@hidden
> Subject: Re: [Qemu-devel] [PATCH 2/6] kvm/powerpc: Add 
> freescale pcicontroller's support
> 
> On Thu, Jan 22, 2009 at 06:14:12PM +0800, Liu Yu wrote:
> > This patch add the emulation of freescale's pci controller 
> for MPC85xx platform.
> > 
> > Signed-off-by: Liu Yu <address@hidden>
> 
> I have one general comment for this patch, QEMU (mostly) uses 
> spaces for
> indentation, while this patch uses spaces + tab. Would it be 
> possible to
> convert it to spaces only, using 4 spaces?

Fixed.

> 
> I also have one minor comment inside. Otherwise the patch looks ok.
> 
> >  Makefile.target  |    2 +
> >  hw/ppce500_pci.c |  370 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 372 insertions(+), 0 deletions(-)
> >  create mode 100644 hw/ppce500_pci.c
> > 
> > diff --git a/Makefile.target b/Makefile.target
> > index a091ce9..5cae257 100644
> > --- a/Makefile.target
> > +++ b/Makefile.target
> > @@ -598,6 +598,8 @@ OBJS+= unin_pci.o ppc_chrp.o
> >  # PowerPC 4xx boards
> >  OBJS+= pflash_cfi02.o ppc4xx_devs.o ppc4xx_pci.o 
> ppc405_uc.o ppc405_boards.o
> >  OBJS+= ppc440.o ppc440_bamboo.o
> > +# PowerPC E500 boards
> > +OBJS+= ppce500_pci.o
> >  ifdef FDT_LIBS
> >  OBJS+= device_tree.o
> >  LIBS+= $(FDT_LIBS)
> > diff --git a/hw/ppce500_pci.c b/hw/ppce500_pci.c
> > new file mode 100644
> > index 0000000..6325555
> > --- /dev/null
> > +++ b/hw/ppce500_pci.c
> > @@ -0,0 +1,370 @@
> > +/*
> > + * QEMU PowerPC E500 embedded processors pci controller emulation
> > + *
> > + * Copyright (C) 2009 Freescale Semiconductor, Inc. All 
> rights reserved.
> > + *
> > + * Author: Yu Liu,     <address@hidden>
> > + *
> > + * This file is derived from hw/ppc4xx_pci.c,
> > + * the copyright for that material belongs to the original owners.
> > + *
> > + * This is free software; you can redistribute it and/or modify
> > + * it under the terms of  the GNU General  Public License 
> as published by
> > + * the Free Software Foundation;  either version 2 of the  
> License, or
> > + * (at your option) any later version.
> > + */
> > +
> > +#include "hw.h"
> > +#include "ppc.h"
> > +#include "ppce500.h"
> > +typedef target_phys_addr_t pci_addr_t;
> > +#include "pci.h"
> > +#include "pci_host.h"
> > +#include "bswap.h"
> > +#include "qemu-log.h"
> > +
> > +#ifdef DEBUG_PCI
> > +#define pci_debug(fmt, arg...) fprintf(stderr, fmt, ##arg)
> > +#else
> > +#define pci_debug(fmt, arg...)
> > +#endif
> > +
> > +#define PCIE500_CFGADDR       0x0
> > +#define PCIE500_CFGDATA       0x4
> > +#define PCIE500_REG_BASE      0xC00
> > +#define PCIE500_REG_SIZE      (0x1000 - PCIE500_REG_BASE)
> > +
> > +#define PPCE500_PCI_CONFIG_ADDR            0x0
> > +#define PPCE500_PCI_CONFIG_DATA            0x4
> > +#define PPCE500_PCI_INTACK         0x8
> > +
> > +#define PPCE500_PCI_OW1                    (0xC20 - 
> PCIE500_REG_BASE)
> > +#define PPCE500_PCI_OW2                    (0xC40 - 
> PCIE500_REG_BASE)
> > +#define PPCE500_PCI_OW3                    (0xC60 - 
> PCIE500_REG_BASE)
> > +#define PPCE500_PCI_OW4                    (0xC80 - 
> PCIE500_REG_BASE)
> > +#define PPCE500_PCI_IW3                    (0xDA0 - 
> PCIE500_REG_BASE)
> > +#define PPCE500_PCI_IW2                    (0xDC0 - 
> PCIE500_REG_BASE)
> > +#define PPCE500_PCI_IW1                    (0xDE0 - 
> PCIE500_REG_BASE)
> > +
> > +#define PPCE500_PCI_GASKET_TIMR            (0xE20 - 
> PCIE500_REG_BASE)
> > +
> > +#define PCI_POTAR          0x0
> > +#define PCI_POTEAR         0x4
> > +#define PCI_POWBAR         0x8
> > +#define PCI_POWAR          0x10
> > +
> > +#define PCI_PITAR          0x0
> > +#define PCI_PIWBAR         0x8
> > +#define PCI_PIWBEAR                0xC
> > +#define PCI_PIWAR          0x10
> > +
> > +#define PPCE500_PCI_NR_POBS 5
> > +#define PPCE500_PCI_NR_PIBS 3
> > +
> > +struct  pci_outbound {
> > +    uint32_t potar;
> > +    uint32_t potear;
> > +    uint32_t powbar;
> > +    uint32_t powar;
> > +};
> > +
> > +struct pci_inbound {
> > +    uint32_t pitar;
> > +    uint32_t piwbar;
> > +    uint32_t piwbear;
> > +    uint32_t piwar;
> > +};
> > +
> > +struct PPCE500PCIState {
> > +    struct pci_outbound pob[PPCE500_PCI_NR_POBS];
> > +    struct pci_inbound pib[PPCE500_PCI_NR_PIBS];
> > +    uint32_t gasket_time;
> > +    PCIHostState pci_state;
> > +    PCIDevice *pci_dev;
> > +};
> > +
> > +typedef struct PPCE500PCIState PPCE500PCIState;
> > +
> > +static uint32_t pcie500_cfgaddr_readl(void *opaque, 
> target_phys_addr_t addr)
> > +{
> > +    PPCE500PCIState *pci = opaque;
> > +
> > +    pci_debug("%s: (addr:%Lx) -> value:%x\n", __func__, addr,
> > +               pci->pci_state.config_reg);
> > +    return pci->pci_state.config_reg;
> > +}
> > +
> > +static CPUReadMemoryFunc *pcie500_cfgaddr_read[] = {
> > +    &pcie500_cfgaddr_readl,
> > +    &pcie500_cfgaddr_readl,
> > +    &pcie500_cfgaddr_readl,
> > +};
> > +
> > +static void pcie500_cfgaddr_writel(void *opaque, 
> target_phys_addr_t addr,
> > +                                  uint32_t value)
> > +{
> > +    PPCE500PCIState *controller = opaque;
> > +
> > +    pci_debug("%s: value:%x -> (addr%Lx)\n", __func__, 
> value, addr);
> > +    controller->pci_state.config_reg = value & ~0x3;
> > +}
> > +
> > +static CPUWriteMemoryFunc *pcie500_cfgaddr_write[] = {
> > +    &pcie500_cfgaddr_writel,
> > +    &pcie500_cfgaddr_writel,
> > +    &pcie500_cfgaddr_writel,
> > +};
> > +
> > +static CPUReadMemoryFunc *pcie500_cfgdata_read[] = {
> > +    &pci_host_data_readb,
> > +    &pci_host_data_readw,
> > +    &pci_host_data_readl,
> > +};
> > +
> > +static CPUWriteMemoryFunc *pcie500_cfgdata_write[] = {
> > +    &pci_host_data_writeb,
> > +    &pci_host_data_writew,
> > +    &pci_host_data_writel,
> > +};
> > +
> > +static uint32_t pci_reg_read4(void *opaque, 
> target_phys_addr_t addr)
> > +{
> > +    PPCE500PCIState *pci = opaque;
> > +    unsigned long win;
> > +    uint32_t value = 0;
> > +
> > +    win = addr & 0xfe0;
> > +
> > +    switch (win) {
> > +    case PPCE500_PCI_OW1:
> > +    case PPCE500_PCI_OW2:
> > +    case PPCE500_PCI_OW3:
> > +    case PPCE500_PCI_OW4:
> > +   switch (addr & 0xC) {
> > +   case PCI_POTAR: value = pci->pob[(addr >> 5) & 
> 0x7].potar; break;
> > +   case PCI_POTEAR: value = pci->pob[(addr >> 5) & 
> 0x7].potear; break;
> > +   case PCI_POWBAR: value = pci->pob[(addr >> 5) & 
> 0x7].powbar; break;
> > +   case PCI_POWAR: value = pci->pob[(addr >> 5) & 
> 0x7].powar; break;
> > +   default: break;
> > +   }
> > +   break;
> > +
> > +    case PPCE500_PCI_IW3:
> > +    case PPCE500_PCI_IW2:
> > +    case PPCE500_PCI_IW1:
> > +   switch (addr & 0xC) {
> > +   case PCI_PITAR: value = pci->pib[(addr >> 5) & 
> 0x3].pitar; break;
> > +   case PCI_PIWBAR: value = pci->pib[(addr >> 5) & 
> 0x3].piwbar; break;
> > +   case PCI_PIWBEAR: value = pci->pib[(addr >> 5) & 
> 0x3].piwbear; break;
> > +   case PCI_PIWAR: value = pci->pib[(addr >> 5) & 
> 0x3].piwar; break;
> > +   default: break;
> > +   };
> > +   break;
> > +
> > +    case PPCE500_PCI_GASKET_TIMR:
> > +   value = pci->gasket_time;
> > +   break;
> > +
> > +    default:
> > +   break;
> > +    }
> > +
> > +    pci_debug("%s: win:%lx(addr:%Lx) -> 
> value:%x\n",__func__,win,addr,value);
> > +    return value;
> > +}
> > +
> > +static CPUReadMemoryFunc *e500_pci_reg_read[] = {
> > +    &pci_reg_read4,
> > +    &pci_reg_read4,
> > +    &pci_reg_read4,
> > +};
> > +
> > +static void pci_reg_write4(void *opaque, target_phys_addr_t addr,
> > +                               uint32_t value)
> > +{
> > +    PPCE500PCIState *pci = opaque;
> > +    unsigned long win;
> > +
> > +    win = addr & 0xfe0;
> > +
> > +    pci_debug("%s: value:%x -> 
> win:%lx(addr:%Lx)\n",__func__,value,win,addr);
> > +
> > +    switch (win) {
> > +    case PPCE500_PCI_OW1:
> > +    case PPCE500_PCI_OW2:
> > +    case PPCE500_PCI_OW3:
> > +    case PPCE500_PCI_OW4:
> > +   switch (addr & 0xC) {
> > +   case PCI_POTAR: pci->pob[(addr >> 5) & 0x7].potar = 
> value; break;
> > +   case PCI_POTEAR: pci->pob[(addr >> 5) & 0x7].potear = 
> value; break;
> > +   case PCI_POWBAR: pci->pob[(addr >> 5) & 0x7].powbar = 
> value; break;
> > +   case PCI_POWAR: pci->pob[(addr >> 5) & 0x7].powar = 
> value; break;
> > +   default: break;
> > +   };
> > +   break;
> > +
> > +    case PPCE500_PCI_IW3:
> > +    case PPCE500_PCI_IW2:
> > +    case PPCE500_PCI_IW1:
> > +   switch (addr & 0xC) {
> > +   case PCI_PITAR: pci->pib[(addr >> 5) & 0x3].pitar = 
> value; break;
> > +   case PCI_PIWBAR: pci->pib[(addr >> 5) & 0x3].piwbar = 
> value; break;
> > +   case PCI_PIWBEAR: pci->pib[(addr >> 5) & 0x3].piwbear = 
> value; break;
> > +   case PCI_PIWAR: pci->pib[(addr >> 5) & 0x3].piwar = 
> value; break;
> > +   default: break;
> > +   };
> > +   break;
> > +
> > +    case PPCE500_PCI_GASKET_TIMR:
> > +   pci->gasket_time = value;
> > +   break;
> > +
> > +    default:
> > +   break;
> > +    };
> > +}
> > +
> > +static CPUWriteMemoryFunc *e500_pci_reg_write[] = {
> > +    &pci_reg_write4,
> > +    &pci_reg_write4,
> > +    &pci_reg_write4,
> > +};
> > +
> > +static int mpc85xx_pci_map_irq(PCIDevice *pci_dev, int irq_num)
> > +{
> > +    int devno = pci_dev->devfn >> 3, ret = 0;
> > +
> > +    switch (devno) {
> > +   /* Two PCI slot */
> > +   case 0x11:
> > +   case 0x12:
> > +       ret = (irq_num + devno - 0x10) % 4;
> > +       break;
> > +   default:
> > +       printf("Error:%s:unknow dev number\n", __func__);
> > +    }
> 
> While I understand that the physical board only supports two PCI
> devices, I wonder if we shouldn't actually support more in QEMU. Think
> for example to virtio which can quickly takes two slots.

Certainly it could support more.
Here only support two is because MPC8544.dts says so, kernel will access pci 
device according to dts file.

The dts file in this patchset is copied from MPC8544's dts file, so we must 
keep it consistent here.

If we need more pci devices, I'd prefer to do it in a isolated patch, as we 
need modify both here and dts file.

> 
> The Linux kernel is supporting IRQ sharing, so supporting 
> more IRQ here
> is probably something to do.
>