From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38200) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z5pNy-0002wQ-BG for qemu-devel@nongnu.org; Fri, 19 Jun 2015 02:02:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z5pNv-000497-5J for qemu-devel@nongnu.org; Fri, 19 Jun 2015 02:02:42 -0400 Date: Fri, 19 Jun 2015 11:48:40 +1000 From: David Gibson Message-ID: <20150619014840.GJ13352@voom.redhat.com> References: <1434020549-26362-1-git-send-email-nikunj@linux.vnet.ibm.com> <1434020549-26362-4-git-send-email-nikunj@linux.vnet.ibm.com> <20150617064936.GS13352@voom.redhat.com> <87k2v2enfd.fsf@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="AZuoSAvZwvV/ife4" Content-Disposition: inline In-Reply-To: <87k2v2enfd.fsf@linux.vnet.ibm.com> Subject: Re: [Qemu-devel] [PATCH v7 3/6] spapr_pci: enumerate and add PCI device tree List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Nikunj A Dadhania Cc: agraf@suse.de, thuth@redhat.com, aik@ozlabs.ru, mdroth@linux.vnet.ibm.com, qemu-devel@nongnu.org, qemu-ppc@nongnu.org --AZuoSAvZwvV/ife4 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Jun 17, 2015 at 02:12:14PM +0530, Nikunj A Dadhania wrote: > David Gibson writes: >=20 > > 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. > >>=20 > >> Signed-off-by: Nikunj A Dadhania > >> [ Squashed Michael's drc_index changes ] > >> Signed-off-by: Michael Roth > >> Signed-off-by: Nikunj A Dadhania > >> --- > >> hw/ppc/spapr_pci.c | 167 ++++++++++++++++++++++++++++++++++++++++++++= +-------- > >> 1 file changed, 142 insertions(+), 25 deletions(-) > >>=20 > >> 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" > >> =20 > >> +#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(dr= c_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_in= dex)); > >> + } > >> =20 > >> _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; > >> } > >> =20 > >> +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. >=20 > I added this structure with pci_for_each_device() in mind, which has > following prototype. >=20 > void pci_for_each_device(PCIBus *bus, int bus_num, > void (*fn)(PCIBus *bus, PCIDevice *d, void *= opaque), > void *opaque); >=20 > 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(). >=20 > 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. --=20 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 --AZuoSAvZwvV/ife4 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJVg3T4AAoJEGw4ysog2bOSCHMP/3vKsh544SuxoUlX12+Q0DDW KNgoTl9IA8yFphCkCueueTrSKwGdfALdZzUo42G2LGE0nCeEtS7GVNuZGFl/Nl83 897XlRhTwCb8k5f6wdHrrfd6+iPKz3gl27rXXrfp4QVUfl3/14aGekHv+DKsFYUF 62TeGJ4H02eGLjfSaA5CwNTeX3JziPkPIYmX9txzf4O2pt+ShuVmc9z24W93zkTr mprWL+KB2QZRwwMiMahr12LIM70cutsDwjjTUeRJrMVonTg9kUn1av4XfcMt1jj7 9cn2kBDsSZnRcTa/cDTJ5iUrH+gEXNWI+DJVYaqVKxd6cJMTctP1M4jJVpMzUcbT 6iclJCwxswryU84EsrufGshCOX4sp/catGp4UiXiWsX1VCBJTBh7kedxvVew9Q/y vOeGiQS+UMLiZG9IY4jWNaIUoYrdWpxFOWlGevfuPu/A5j6Tof2ndZ8XTB54UJat qUz+1r+UtyCeXGPaxMWANcX4/yx+0TNxWUZiyehL5i2MTFAXOSJg9bsGSRAtkJyr uL+6jrrQBT7QrPXW9GZFoHsj2mZ7hOwAccZKGn4tHQ7BhhQWyVep6aq0qKZXeGhx kq4V1YqgPHrXM3maX4I0ki5KHVJG91A7uaPvDR47BPyDHB/mGEy3CLHXz2capkt4 0K31BcRUtTvA4B+xPQjw =5Psx -----END PGP SIGNATURE----- --AZuoSAvZwvV/ife4--