Re: [Qemu-devel] [PATCH v7 3/6] spapr_pci: enumerate and add PCI device tree

[David Gibson]

On Wed, Jun 17, 2015 at 02:12:14PM +0530, Nikunj A Dadhania wrote:
> David Gibson <address@hidden> 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 <address@hidden>
> >> [ Squashed Michael's drc_index changes ]
> >> Signed-off-by: Michael Roth <address@hidden>
> >> Signed-off-by: Nikunj A Dadhania <address@hidden>
> >> ---
> >>  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

pgpiugA3qlK9j.pgp
Description: PGP signature