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

[Aurelien Jarno]

On Mon, Feb 09, 2009 at 04:33:25PM +0800, Liu Yu-B13201 wrote:
> > -----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.
> 

I see, then it's fine with me.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
address@hidden                 http://www.aurel32.net