Patchwork [v3,17/26] Add i21154 bridge chip.

login
register
mail settings
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

Michael S. Tsirkin - Oct. 22, 2012, 4:17 p.m.
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?

--->

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>

---
Jason Baron - Oct. 22, 2012, 6:18 p.m.
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);
>  }
Andreas Färber - Oct. 22, 2012, 6:53 p.m.
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
Blue Swirl - Oct. 27, 2012, 12:42 p.m.
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);
 }