RE: [RFC 2/2] pci-expender-bus:Add pcie-root-port to pxb-pcie under arm.

[miaoyubo]

> -----Original Message-----
> From: Michael S. Tsirkin [mailto:address@hidden]
> Sent: Thursday, February 13, 2020 6:17 PM
> To: miaoyubo <address@hidden>
> Cc: address@hidden; address@hidden; Xiexiangyou
> <address@hidden>; address@hidden; qemu-
> address@hidden
> Subject: Re: [RFC 2/2] pci-expender-bus:Add pcie-root-port to pxb-pcie
> under arm.
> 
> On Thu, Feb 13, 2020 at 03:49:52PM +0800, Yubo Miao wrote:
> > From: miaoyubo <address@hidden>
> >
> > Since devices could not directly plugged into pxb-pcie,
> 
> Hmm is this different from the root port? intergrated devices do exist for
> that actually.
> 

Thanks for replying
The pxb-pcie is like a host bridge,
 you have to plug pcie-root-port or pci-bridge so devices could be plugged

> > under arm,
> 
> how is arm special?
> 

Cureently, if u define a pxb-pcie device, one pcie-root-port or pci-bridge or 
something 
else have to be defined also, The patch just auto generate pcie-root-port for 
arm to 
avoid affect other architecture

> > one
> > pcie-root port is plugged into pxb-pcie. Due to the bus for each
> > pxb-pcie is defined as 2 in acpi dsdt tables(one for pxb-pcie, one for
> > pcie-root-port), only one device could be plugged into one pxb-pcie.
> 
> So why can't we have users specify any number of root ports using -device?
> then make acpi tables match the # of ports created?
> 
> 

Users could specify multiply pxb-devices, it is supported.
But only one device could be plugged for each pxb-pcie,  it is the same with 
pxb-pci for piix.

> >
> > Signed-off-by: miaoyubo <address@hidden>
> > ---
> >  hw/pci-bridge/pci_expander_bridge.c | 9 +++++++++
> >  include/hw/pci/pcie_port.h          | 1 +
> >  2 files changed, 10 insertions(+)
> >
> > diff --git a/hw/pci-bridge/pci_expander_bridge.c
> > b/hw/pci-bridge/pci_expander_bridge.c
> > index 47aaaf8fd1..3d896dd452 100644
> > --- a/hw/pci-bridge/pci_expander_bridge.c
> > +++ b/hw/pci-bridge/pci_expander_bridge.c
> > @@ -15,6 +15,7 @@
> >  #include "hw/pci/pci.h"
> >  #include "hw/pci/pci_bus.h"
> >  #include "hw/pci/pci_host.h"
> > +#include "hw/pci/pcie_port.h"
> >  #include "hw/qdev-properties.h"
> >  #include "hw/pci/pci_bridge.h"
> >  #include "qemu/range.h"
> > @@ -233,7 +234,15 @@ static void pxb_dev_realize_common(PCIDevice
> > *dev, bool pcie, Error **errp)
> >
> >      ds = qdev_create(NULL, TYPE_PXB_HOST);
> >      if (pcie) {
> > +#ifdef __aarch64__
> > +        bus = pci_root_bus_new(ds, "pxb-pcie-internal",
> > +                               NULL, NULL, 0, TYPE_PXB_PCIE_BUS);
> > +        bds = qdev_create(BUS(bus), "pcie-root-port");
> > +        bds->id = dev_name;
> > +        qdev_prop_set_uint8(bds, PCIE_ROOT_PORT_PROP_CHASSIS,
> > +pxb->bus_nr); #else
> >          bus = pci_root_bus_new(ds, dev_name, NULL, NULL, 0,
> > TYPE_PXB_PCIE_BUS);
> > +#endif
> 
> What does all this have to do with building on aarch64?
> 

Based on the comments, this patch would be abandoned in patch V2, 
PXB-PCIE would also be useful but pcie-root-port or pci-bridge have to Be 
defined by user.

> >      } else {
> >          bus = pci_root_bus_new(ds, "pxb-internal", NULL, NULL, 0,
> TYPE_PXB_BUS);
> >          bds = qdev_create(BUS(bus), "pci-bridge"); diff --git
> > a/include/hw/pci/pcie_port.h b/include/hw/pci/pcie_port.h index
> > 4b3d254b08..b41d473220 100644
> > --- a/include/hw/pci/pcie_port.h
> > +++ b/include/hw/pci/pcie_port.h
> > @@ -64,6 +64,7 @@ int pcie_chassis_add_slot(struct PCIESlot *slot);
> > void pcie_chassis_del_slot(PCIESlot *s);
> >
> >  #define TYPE_PCIE_ROOT_PORT         "pcie-root-port-base"
> > +#define PCIE_ROOT_PORT_PROP_CHASSIS "chassis"
> 
> If you are going to do this, replace other instances of "chassis"
> with the macro.
> 

Thanks for your replay, this patch would be abandoned.

> >  #define PCIE_ROOT_PORT_CLASS(klass) \
> >       OBJECT_CLASS_CHECK(PCIERootPortClass, (klass),
> > TYPE_PCIE_ROOT_PORT)  #define PCIE_ROOT_PORT_GET_CLASS(obj) \
> > --
> > 2.19.1
> >

Regards,
Miao