| Submitter | Michael S. Tsirkin |
|---|---|
| Date | Oct. 22, 2012, 4:17 p.m. |
| Message ID | <20121022161730.GA23029@redhat.com> |
| Download | mbox | patch |
| Permalink | /patch/193201/ |
| State | New |
| Headers | show |
Comments
On Mon, Oct 22, 2012 at 06:17:30PM +0200, Michael S. Tsirkin wrote: > On Mon, Oct 22, 2012 at 03:26:24PM +0200, Andreas Färber wrote: > > Am 19.10.2012 22:43, schrieb Jason Baron: > > > From: Jason Baron <jbaron@redhat.com> > > > > > > This adds support for the DECchip 21154 PCI bridge. > > > > > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> > > > Signed-off-by: Jason Baron <jbaron@redhat.com> > > > --- > > > hw/Makefile.objs | 2 +- > > > hw/i21154.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > hw/i21154.h | 9 ++++ > > > 3 files changed, 123 insertions(+), 1 deletions(-) > > > create mode 100644 hw/i21154.c > > > create mode 100644 hw/i21154.h > > > > Why is this creating a new file and not reusing dec_pci.c? We shouldn't > > have two parallel implementations of the same chip. > > > > Andreas > > Good point I missed this. There's a minor difference Yes, I missed this too. No reason to carry this patch, I will drop it. Thanks, -Jason > wrt dec-21154-p2p-bridge in a couple of fields, > these could be set by properties. > Also dec_map_irq differs from the spec compliant > map function. I am guessing this is a bug. > Would appreciate testing of the patch below. > > > Are you familiar with dec_pci.c? Looking at it, it seems to > implement a pci host bridge "dec-21154-sysbus" , > a pci to pci bridge "dec-21154-p2p-bridge", > and something called "dec-21154" which sports a comment > "PCI2PCI bridge same values as PearPC - check this" - > and implements an empty init function; > what this last is and why it's useful I am not sure. > > Anyone? Blue Swirl? Anyone can test this doesn't break > things and report? > > ---> > > dec_pci: irq swizzle PCI spec compliance > > Make IRQ mapping for dec PCI PCI 2 PCI Bridge compliant > with the PCI spec. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > diff --git a/hw/dec_pci.c b/hw/dec_pci.c > index c30ade3..a49f0bd 100644 > --- a/hw/dec_pci.c > +++ b/hw/dec_pci.c > @@ -82,7 +82,7 @@ PCIBus *pci_dec_21154_init(PCIBus *parent_bus, int devfn) > dev = pci_create_multifunction(parent_bus, devfn, false, > "dec-21154-p2p-bridge"); > br = DO_UPCAST(PCIBridge, dev, dev); > - pci_bridge_map_irq(br, "DEC 21154 PCI-PCI bridge", dec_map_irq); > + pci_bridge_map_irq(br, "DEC 21154 PCI-PCI bridge", pci_swizzle_map_irq_fn); > qdev_init_nofail(&dev->qdev); > return pci_bridge_get_sec_bus(br); > }
Am 22.10.2012 18:17, schrieb Michael S. Tsirkin: > On Mon, Oct 22, 2012 at 03:26:24PM +0200, Andreas Färber wrote: >> Am 19.10.2012 22:43, schrieb Jason Baron: >>> create mode 100644 hw/i21154.c >>> create mode 100644 hw/i21154.h >> >> Why is this creating a new file and not reusing dec_pci.c? We shouldn't >> have two parallel implementations of the same chip. > > Good point I missed this. There's a minor difference > wrt dec-21154-p2p-bridge in a couple of fields, > these could be set by properties. > Also dec_map_irq differs from the spec compliant > map function. I am guessing this is a bug. > Would appreciate testing of the patch below. > > > Are you familiar with dec_pci.c? Looking at it, it seems to > implement a pci host bridge "dec-21154-sysbus" , > a pci to pci bridge "dec-21154-p2p-bridge", > and something called "dec-21154" which sports a comment > "PCI2PCI bridge same values as PearPC - check this" - > and implements an empty init function; > what this last is and why it's useful I am not sure. Apart from my PHB rework touching it, I had a branch starting to QOM'ify it. From what I remember it was a PCIDevice that interfaces with a "regular" SysBus PHB device and like most PHBs has a PCIDevice on its PCIBus. That would match the number of devices you mention, although the exact names are pretty confusing for all PHBs IMO. ;) > Anyone? Blue Swirl? Anyone can test this doesn't break > things and report? I'll look more closely and test when I'm back from oSC mid of the week. I think it was ppc_newworld that was using this, possibly subject to #if 0 (which I was trying to clean up, too). Thanks, Andreas
On Mon, Oct 22, 2012 at 4:17 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Mon, Oct 22, 2012 at 03:26:24PM +0200, Andreas Färber wrote: >> Am 19.10.2012 22:43, schrieb Jason Baron: >> > From: Jason Baron <jbaron@redhat.com> >> > >> > This adds support for the DECchip 21154 PCI bridge. >> > >> > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp> >> > Signed-off-by: Jason Baron <jbaron@redhat.com> >> > --- >> > hw/Makefile.objs | 2 +- >> > hw/i21154.c | 113 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> > hw/i21154.h | 9 ++++ >> > 3 files changed, 123 insertions(+), 1 deletions(-) >> > create mode 100644 hw/i21154.c >> > create mode 100644 hw/i21154.h >> >> Why is this creating a new file and not reusing dec_pci.c? We shouldn't >> have two parallel implementations of the same chip. >> >> Andreas > > Good point I missed this. There's a minor difference > wrt dec-21154-p2p-bridge in a couple of fields, > these could be set by properties. > Also dec_map_irq differs from the spec compliant > map function. I am guessing this is a bug. > Would appreciate testing of the patch below. > > > Are you familiar with dec_pci.c? Looking at it, it seems to > implement a pci host bridge "dec-21154-sysbus" , > a pci to pci bridge "dec-21154-p2p-bridge", > and something called "dec-21154" which sports a comment > "PCI2PCI bridge same values as PearPC - check this" - > and implements an empty init function; > what this last is and why it's useful I am not sure. > > Anyone? Blue Swirl? Anyone can test this doesn't break > things and report? The device is only linked by PPC but the init function is not invoked. It should be also used by Sparc64 (it's present on real Ultra-5 machine and several devices should be behind the bridge) but it isn't. > > ---> > > dec_pci: irq swizzle PCI spec compliance > > Make IRQ mapping for dec PCI PCI 2 PCI Bridge compliant > with the PCI spec. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > diff --git a/hw/dec_pci.c b/hw/dec_pci.c > index c30ade3..a49f0bd 100644 > --- a/hw/dec_pci.c > +++ b/hw/dec_pci.c > @@ -82,7 +82,7 @@ PCIBus *pci_dec_21154_init(PCIBus *parent_bus, int devfn) > dev = pci_create_multifunction(parent_bus, devfn, false, > "dec-21154-p2p-bridge"); > br = DO_UPCAST(PCIBridge, dev, dev); > - pci_bridge_map_irq(br, "DEC 21154 PCI-PCI bridge", dec_map_irq); dec_map_irq is now unused and should be removed to avoid build breakage. > + pci_bridge_map_irq(br, "DEC 21154 PCI-PCI bridge", pci_swizzle_map_irq_fn); > qdev_init_nofail(&dev->qdev); > return pci_bridge_get_sec_bus(br); > } Otherwise I think the patch is fine.
Patch
diff --git a/hw/dec_pci.c b/hw/dec_pci.c index c30ade3..a49f0bd 100644 --- a/hw/dec_pci.c +++ b/hw/dec_pci.c @@ -82,7 +82,7 @@ PCIBus *pci_dec_21154_init(PCIBus *parent_bus, int devfn) dev = pci_create_multifunction(parent_bus, devfn, false, "dec-21154-p2p-bridge"); br = DO_UPCAST(PCIBridge, dev, dev); - pci_bridge_map_irq(br, "DEC 21154 PCI-PCI bridge", dec_map_irq); + pci_bridge_map_irq(br, "DEC 21154 PCI-PCI bridge", pci_swizzle_map_irq_fn); qdev_init_nofail(&dev->qdev); return pci_bridge_get_sec_bus(br); }