Re: [Qemu-devel] [PATCH 1/2] ppc-e500: some pci related cleanup

[address@hidden]

> -----Original Message-----
> From: Alexander Graf [mailto:address@hidden
> Sent: Thursday, December 19, 2013 3:18 AM
> To: Bhushan Bharat-R65777
> Cc: Wood Scott-B07421; QEMU Developers; qemu-ppc; Bhushan Bharat-R65777
> Subject: Re: [PATCH 1/2] ppc-e500: some pci related cleanup
> 
> 
> On 28.11.2013, at 07:35, Bharat Bhushan <address@hidden> wrote:
> 
> > - Use PCI_NUM_PINS rather than hardcoding
> > - use "pin" wherever possible
> 
> I assume you mean the PCI A/B/C/D pin with "pin".

Yes 

> 
> >
> > Signed-off-by: Bharat Bhushan <address@hidden>
> > ---
> > hw/pci-host/ppce500.c |   14 +++++++-------
> > hw/ppc/e500.c         |   12 +++++++-----
> > 2 files changed, 14 insertions(+), 12 deletions(-)
> >
> > diff --git a/hw/pci-host/ppce500.c b/hw/pci-host/ppce500.c index
> > f00793d..49bfcc6 100644
> > --- a/hw/pci-host/ppce500.c
> > +++ b/hw/pci-host/ppce500.c
> > @@ -87,7 +87,7 @@ struct PPCE500PCIState {
> >     struct pci_outbound pob[PPCE500_PCI_NR_POBS];
> >     struct pci_inbound pib[PPCE500_PCI_NR_PIBS];
> >     uint32_t gasket_time;
> > -    qemu_irq irq[4];
> > +    qemu_irq irq[PCI_NUM_PINS];
> >     uint32_t first_slot;
> >     /* mmio maps */
> >     MemoryRegion container;
> > @@ -252,26 +252,26 @@ static const MemoryRegionOps e500_pci_reg_ops = {
> >     .endianness = DEVICE_BIG_ENDIAN,
> > };
> >
> > -static int mpc85xx_pci_map_irq(PCIDevice *pci_dev, int irq_num)
> > +static int mpc85xx_pci_map_irq(PCIDevice *pci_dev, int pin)
> 
> This function converts pin -> irq, so that's fine.
> 
> > {
> >     int devno = pci_dev->devfn >> 3;
> >     int ret;
> >
> > -    ret = ppce500_pci_map_irq_slot(devno, irq_num);
> > +    ret = ppce500_pci_map_irq_slot(devno, pin);
> >
> >     pci_debug("%s: devfn %x irq %d -> %d  devno:%x\n", __func__,
> > -           pci_dev->devfn, irq_num, ret, devno);
> > +           pci_dev->devfn, pin, ret, devno);
> >
> >     return ret;
> > }
> >
> > -static void mpc85xx_pci_set_irq(void *opaque, int irq_num, int level)
> > +static void mpc85xx_pci_set_irq(void *opaque, int pin, int level)
> 
> While this one ...
> 
> > {
> >     qemu_irq *pic = opaque;
> >
> > -    pci_debug("%s: PCI irq %d, level:%d\n", __func__, irq_num, level);
> > +    pci_debug("%s: PCI irq %d, level:%d\n", __func__, pin , level);
> >
> > -    qemu_set_irq(pic[irq_num], level);
> > +    qemu_set_irq(pic[pin], level);
> 
> ... sets an actual irq number on the PIC, so this is not a pin.

pic[] is array of intA/B/C/B pin, no ?

Thanks
-Bharat

> 
> 
> The rest looks good to me :).
> 
> 
> Alex
> 
>