On Wed, Jun 17, 2015 at 02:12:14PM +0530, Nikunj A Dadhania wrote:
> David Gibson <da...@gibson.dropbear.id.au> writes:
>
> > On Thu, Jun 11, 2015 at 04:32:26PM +0530, Nikunj A Dadhania wrote:
> >> All the PCI enumeration and device node creation was off-loaded to
> >> SLOF. With PCI hotplug support, code needed to be added to add device
> >> node. This creates multiple copy of the code one in SLOF and other in
> >> hotplug code. To unify this, the patch adds the pci device node
> >> creation in Qemu. For backward compatibility, a flag
> >> "qemu,phb-enumerated" is added to the phb, suggesting to SLOF to not
> >> do device node creation.
> >>
> >> Signed-off-by: Nikunj A Dadhania <nik...@linux.vnet.ibm.com>
> >> [ Squashed Michael's drc_index changes ]
> >> Signed-off-by: Michael Roth <mdr...@linux.vnet.ibm.com>
> >> Signed-off-by: Nikunj A Dadhania <nik...@linux.vnet.ibm.com>
> >> ---
> >> hw/ppc/spapr_pci.c | 167
> >> +++++++++++++++++++++++++++++++++++++++++++++--------
> >> 1 file changed, 142 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> >> index 33254b3..6ef7f44 100644
> >> --- a/hw/ppc/spapr_pci.c
> >> +++ b/hw/ppc/spapr_pci.c
> >> @@ -23,6 +23,7 @@
> >> * THE SOFTWARE.
> >> */
> >> #include "hw/hw.h"
> >> +#include "hw/sysbus.h"
> >> #include "hw/pci/pci.h"
> >> #include "hw/pci/msi.h"
> >> #include "hw/pci/msix.h"
> >> @@ -35,6 +36,7 @@
> >> #include "qemu/error-report.h"
> >> #include "qapi/qmp/qerror.h"
> >>
> >> +#include "hw/pci/pci_bridge.h"
> >> #include "hw/pci/pci_bus.h"
> >> #include "hw/ppc/spapr_drc.h"
> >> #include "sysemu/device_tree.h"
> >> @@ -946,8 +948,13 @@ static int spapr_populate_pci_child_dt(PCIDevice
> >> *dev, void *fdt, int offset,
> >> * processed by OF beforehand
> >> */
> >> _FDT(fdt_setprop_string(fdt, offset, "name", "pci"));
> >> - _FDT(fdt_setprop(fdt, offset, "ibm,loc-code", drc_name,
> >> strlen(drc_name)));
> >> - _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
> >> + if (drc_name) {
> >> + _FDT(fdt_setprop(fdt, offset, "ibm,loc-code", drc_name,
> >> + strlen(drc_name)));
> >> + }
> >> + if (drc_index) {
> >> + _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index",
> >> drc_index));
> >> + }
> >>
> >> _FDT(fdt_setprop_cell(fdt, offset, "#address-cells",
> >> RESOURCE_CELLS_ADDRESS));
> >> @@ -964,30 +971,38 @@ static int spapr_populate_pci_child_dt(PCIDevice
> >> *dev, void *fdt, int offset,
> >> return 0;
> >> }
> >>
> >> +typedef struct sPAPRFDT {
> >> + void *fdt;
> >> + int node_off;
> >> + sPAPRPHBState *sphb;
> >> +} sPAPRFDT;
> >
> > I don't really like this structure - it seems a very ad-hoc collection
> > of things. Even though it means there will be a lot of parameters to
> > the function, I'd prefer passing them separately to
> > spapr_create_pci_child_dt() rather than using this structure.
>
> I added this structure with pci_for_each_device() in mind, which has
> following prototype.
>
> void pci_for_each_device(PCIBus *bus, int bus_num,
> void (*fn)(PCIBus *bus, PCIDevice *d, void
> *opaque),
> void *opaque);
>
> So per device we get this structure and populate PCI device tree entry
> and scan and populate bridge recursively if needed. So I had continued
> using this structure in spapr_create_pci_child_dt().
>
> We cannot remove sPAPRFDT completely as we need it for PCI device tree
> creation.
Ah, yes, I see.
> So if needed, I can change spapr_create_pci_child_dt() with more args.
> And structure sPAPRFDT to be used by spapr_populate_pci_devices_dt()
> called by pci_for_each_device().
Ok, I'd still prefer to see this structure localized to just the
callback function. Which see you've done in the next spin, thanks.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
pgp5zyJycnMLz.pgp
Description: PGP signature
