From: David Gibson <david@gibson.dropbear.id.au>
To: Greg Kurz <groug@kaod.org>
Cc: mdroth@linux.ibm.com, mst@redhat.com, qemu-ppc@nongnu.org,
clg@kaod.org, qemu-devel@nongnu.org
Subject: Re: [Qemu-devel] [PATCH 2/8] spapr: Clean up device tree construction for PCI devices
Date: Thu, 30 May 2019 12:07:53 +1000 [thread overview]
Message-ID: <20190530020753.GE2017@umbus.fritz.box> (raw)
In-Reply-To: <20190524173410.2d362c9a@bahia.lan>
[-- Attachment #1: Type: text/plain, Size: 10434 bytes --]
On Fri, May 24, 2019 at 05:34:10PM +0200, Greg Kurz wrote:
65;5603;1c> On Thu, 23 May 2019 15:29:12 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
> > spapr_create_pci_child_dt() is a trivial wrapper around
> > spapr_populate_pci_child_dt(), but is the latter's only caller. So fold
> > them together into spapr_dt_pci_device(), which closer matches our modern
> > naming convention.
> >
> > While there, make a number of cleanups to the function itself. This is
> > mostly using more temporary locals to avoid awkwardly long lines, and in
> > some cases avoiding double reads of PCI config space variables.
> >
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> > hw/ppc/spapr_pci.c | 119 +++++++++++++++++++++------------------------
> > 1 file changed, 55 insertions(+), 64 deletions(-)
> >
> > diff --git a/hw/ppc/spapr_pci.c b/hw/ppc/spapr_pci.c
> > index b2db46ef1d..4075b433fd 100644
> > --- a/hw/ppc/spapr_pci.c
> > +++ b/hw/ppc/spapr_pci.c
> > @@ -1219,57 +1219,75 @@ static const char *dt_name_from_class(uint8_t class, uint8_t subclass,
> > static uint32_t spapr_phb_get_pci_drc_index(SpaprPhbState *phb,
> > PCIDevice *pdev);
> >
> > -static void spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
> > - SpaprPhbState *sphb)
> > +/* create OF node for pci device and required OF DT properties */
> > +static int spapr_dt_pci_device(SpaprPhbState *sphb, PCIDevice *dev,
> > + void *fdt, int parent_offset)
> > {
> > + int offset;
> > + const gchar *basename;
> > + char *nodename;
>
> Being curious... what's the rationale for using gchar or char, if
> any ?
There isn't one really. I've changed this to gchar for consistency.
>
> > + int slot = PCI_SLOT(dev->devfn);
> > + int func = PCI_FUNC(dev->devfn);
> > ResourceProps rp;
> > - bool is_bridge = false;
> > - int pci_status;
> > - char *buf = NULL;
> > uint32_t drc_index = spapr_phb_get_pci_drc_index(sphb, dev);
> > + uint32_t header_type = pci_default_read_config(dev, PCI_HEADER_TYPE, 1);
> > + bool is_bridge = (header_type == PCI_HEADER_TYPE_BRIDGE);
> > + uint32_t vendor_id = pci_default_read_config(dev, PCI_VENDOR_ID, 2);
> > + uint32_t device_id = pci_default_read_config(dev, PCI_DEVICE_ID, 2);
> > + uint32_t revision_id = pci_default_read_config(dev, PCI_REVISION_ID, 1);
> > uint32_t ccode = pci_default_read_config(dev, PCI_CLASS_PROG, 3);
> > - uint32_t max_msi, max_msix;
> > + uint32_t irq_pin = pci_default_read_config(dev, PCI_INTERRUPT_PIN, 1);
> > + uint32_t subsystem_id = pci_default_read_config(dev, PCI_SUBSYSTEM_ID, 2);
> > + uint32_t subsystem_vendor_id =
> > + pci_default_read_config(dev, PCI_SUBSYSTEM_VENDOR_ID, 2);
> > + uint32_t cache_line_size =
> > + pci_default_read_config(dev, PCI_CACHE_LINE_SIZE, 1);
> > + uint32_t pci_status = pci_default_read_config(dev, PCI_STATUS, 2);
> > + gchar *loc_code;
> > +
>
> trailing space ^^
Fixed.
>
> Apart from that, LGTM.
>
> Reviewed-by: Greg Kurz <groug@kaod.org>
>
> > + basename = dt_name_from_class((ccode >> 16) & 0xff, (ccode >> 8) & 0xff,
> > + ccode & 0xff);
> >
> > - if (pci_default_read_config(dev, PCI_HEADER_TYPE, 1) ==
> > - PCI_HEADER_TYPE_BRIDGE) {
> > - is_bridge = true;
> > + if (func != 0) {
> > + nodename = g_strdup_printf("%s@%x,%x", basename, slot, func);
> > + } else {
> > + nodename = g_strdup_printf("%s@%x", basename, slot);
> > }
> >
> > + _FDT(offset = fdt_add_subnode(fdt, parent_offset, nodename));
> > +
> > + g_free(nodename);
> > +
> > /* in accordance with PAPR+ v2.7 13.6.3, Table 181 */
> > - _FDT(fdt_setprop_cell(fdt, offset, "vendor-id",
> > - pci_default_read_config(dev, PCI_VENDOR_ID, 2)));
> > - _FDT(fdt_setprop_cell(fdt, offset, "device-id",
> > - pci_default_read_config(dev, PCI_DEVICE_ID, 2)));
> > - _FDT(fdt_setprop_cell(fdt, offset, "revision-id",
> > - pci_default_read_config(dev, PCI_REVISION_ID, 1)));
> > + _FDT(fdt_setprop_cell(fdt, offset, "vendor-id", vendor_id));
> > + _FDT(fdt_setprop_cell(fdt, offset, "device-id", device_id));
> > + _FDT(fdt_setprop_cell(fdt, offset, "revision-id", revision_id));
> > +
> > _FDT(fdt_setprop_cell(fdt, offset, "class-code", ccode));
> > - if (pci_default_read_config(dev, PCI_INTERRUPT_PIN, 1)) {
> > - _FDT(fdt_setprop_cell(fdt, offset, "interrupts",
> > - pci_default_read_config(dev, PCI_INTERRUPT_PIN, 1)));
> > + if (irq_pin) {
> > + _FDT(fdt_setprop_cell(fdt, offset, "interrupts", irq_pin));
> > }
> >
> > if (!is_bridge) {
> > - _FDT(fdt_setprop_cell(fdt, offset, "min-grant",
> > - pci_default_read_config(dev, PCI_MIN_GNT, 1)));
> > - _FDT(fdt_setprop_cell(fdt, offset, "max-latency",
> > - pci_default_read_config(dev, PCI_MAX_LAT, 1)));
> > + uint32_t min_grant = pci_default_read_config(dev, PCI_MIN_GNT, 1);
> > + uint32_t max_latency = pci_default_read_config(dev, PCI_MAX_LAT, 1);
> > + _FDT(fdt_setprop_cell(fdt, offset, "min-grant", min_grant));
> > + _FDT(fdt_setprop_cell(fdt, offset, "max-latency", max_latency));
> > }
> >
> > - if (pci_default_read_config(dev, PCI_SUBSYSTEM_ID, 2)) {
> > - _FDT(fdt_setprop_cell(fdt, offset, "subsystem-id",
> > - pci_default_read_config(dev, PCI_SUBSYSTEM_ID, 2)));
> > + if (subsystem_id) {
> > + _FDT(fdt_setprop_cell(fdt, offset, "subsystem-id", subsystem_id));
> > }
> >
> > - if (pci_default_read_config(dev, PCI_SUBSYSTEM_VENDOR_ID, 2)) {
> > + if (subsystem_vendor_id) {
> > _FDT(fdt_setprop_cell(fdt, offset, "subsystem-vendor-id",
> > - pci_default_read_config(dev, PCI_SUBSYSTEM_VENDOR_ID, 2)));
> > + subsystem_vendor_id));
> > }
> >
> > - _FDT(fdt_setprop_cell(fdt, offset, "cache-line-size",
> > - pci_default_read_config(dev, PCI_CACHE_LINE_SIZE, 1)));
> > + _FDT(fdt_setprop_cell(fdt, offset, "cache-line-size", cache_line_size));
> > +
> >
> > /* the following fdt cells are masked off the pci status register */
> > - pci_status = pci_default_read_config(dev, PCI_STATUS, 2);
> > _FDT(fdt_setprop_cell(fdt, offset, "devsel-speed",
> > PCI_STATUS_DEVSEL_MASK & pci_status));
> >
> > @@ -1283,9 +1301,9 @@ static void spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
> > _FDT(fdt_setprop(fdt, offset, "udf-supported", NULL, 0));
> > }
> >
> > - buf = spapr_phb_get_loc_code(sphb, dev);
> > - _FDT(fdt_setprop_string(fdt, offset, "ibm,loc-code", buf));
> > - g_free(buf);
> > + loc_code = spapr_phb_get_loc_code(sphb, dev);
> > + _FDT(fdt_setprop_string(fdt, offset, "ibm,loc-code", loc_code));
> > + g_free(loc_code);
> >
> > if (drc_index) {
> > _FDT(fdt_setprop_cell(fdt, offset, "ibm,my-drc-index", drc_index));
> > @@ -1297,13 +1315,13 @@ static void spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
> > RESOURCE_CELLS_SIZE));
> >
> > if (msi_present(dev)) {
> > - max_msi = msi_nr_vectors_allocated(dev);
> > + uint32_t max_msi = msi_nr_vectors_allocated(dev);
> > if (max_msi) {
> > _FDT(fdt_setprop_cell(fdt, offset, "ibm,req#msi", max_msi));
> > }
> > }
> > if (msix_present(dev)) {
> > - max_msix = dev->msix_entries_nr;
> > + uint32_t max_msix = dev->msix_entries_nr;
> > if (max_msix) {
> > _FDT(fdt_setprop_cell(fdt, offset, "ibm,req#msi-x", max_msix));
> > }
> > @@ -1319,33 +1337,6 @@ static void spapr_populate_pci_child_dt(PCIDevice *dev, void *fdt, int offset,
> > }
> >
> > spapr_phb_nvgpu_populate_pcidev_dt(dev, fdt, offset, sphb);
> > -}
> > -
> > -/* create OF node for pci device and required OF DT properties */
> > -static int spapr_create_pci_child_dt(SpaprPhbState *phb, PCIDevice *dev,
> > - void *fdt, int node_offset)
> > -{
> > - int offset;
> > - const gchar *basename;
> > - char *nodename;
> > - int slot = PCI_SLOT(dev->devfn);
> > - int func = PCI_FUNC(dev->devfn);
> > - uint32_t ccode = pci_default_read_config(dev, PCI_CLASS_PROG, 3);
> > -
> > - basename = dt_name_from_class((ccode >> 16) & 0xff, (ccode >> 8) & 0xff,
> > - ccode & 0xff);
> > -
> > - if (func != 0) {
> > - nodename = g_strdup_printf("%s@%x,%x", basename, slot, func);
> > - } else {
> > - nodename = g_strdup_printf("%s@%x", basename, slot);
> > - }
> > -
> > - _FDT(offset = fdt_add_subnode(fdt, node_offset, nodename));
> > -
> > - g_free(nodename);
> > -
> > - spapr_populate_pci_child_dt(dev, fdt, offset, phb);
> >
> > return offset;
> > }
> > @@ -1393,7 +1384,7 @@ int spapr_pci_dt_populate(SpaprDrc *drc, SpaprMachineState *spapr,
> > SpaprPhbState *sphb = SPAPR_PCI_HOST_BRIDGE(plug_handler);
> > PCIDevice *pdev = PCI_DEVICE(drc->dev);
> >
> > - *fdt_start_offset = spapr_create_pci_child_dt(sphb, pdev, fdt, 0);
> > + *fdt_start_offset = spapr_dt_pci_device(sphb, pdev, fdt, 0);
> > return 0;
> > }
> >
> > @@ -2086,7 +2077,7 @@ static void spapr_populate_pci_devices_dt(PCIBus *bus, PCIDevice *pdev,
> > int offset;
> > SpaprFdt s_fdt;
> >
> > - offset = spapr_create_pci_child_dt(p->sphb, pdev, p->fdt, p->node_off);
> > + offset = spapr_dt_pci_device(p->sphb, pdev, p->fdt, p->node_off);
> > if (!offset) {
> > error_report("Failed to create pci child device tree node");
> > return;
>
--
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2019-05-30 4:26 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-05-23 5:29 [Qemu-devel] [PATCH 1/8] spapr: Clean up device node name generation for PCI devices David Gibson
2019-05-23 5:29 ` [Qemu-devel] [PATCH 2/8] spapr: Clean up device tree construction " David Gibson
2019-05-24 15:34 ` Greg Kurz
2019-05-30 2:07 ` David Gibson [this message]
2019-05-23 5:29 ` [Qemu-devel] [PATCH 3/8] spapr: Clean up dt creation for PCI buses David Gibson
2019-05-24 5:31 ` [Qemu-devel] [Qemu-ppc] " Alexey Kardashevskiy
2019-05-30 5:33 ` David Gibson
2019-05-30 5:43 ` Alexey Kardashevskiy
2019-05-31 10:24 ` David Gibson
2019-05-23 5:29 ` [Qemu-devel] [PATCH 4/8] spapr: Clean up spapr_drc_populate_dt() David Gibson
2019-05-24 16:59 ` Greg Kurz
2019-05-23 5:29 ` [Qemu-devel] [PATCH 5/8] spapr: Clean up DRC index construction David Gibson
2019-05-23 5:29 ` [Qemu-devel] [PATCH 6/8] spapr: Don't use bus number for building DRC ids David Gibson
2019-05-23 5:29 ` [Qemu-devel] [PATCH 7/8] spapr: Direct all PCI hotplug to host bridge, rather than P2P bridge David Gibson
2019-05-23 5:29 ` [Qemu-devel] [PATCH 8/8] spapr: Allow hot plug/unplug of PCI bridges and devices under PCI bridges David Gibson
2019-05-24 13:32 ` [Qemu-devel] [PATCH 1/8] spapr: Clean up device node name generation for PCI devices Greg Kurz
2019-05-30 1:50 ` David Gibson
2019-05-29 3:23 ` Michael S. Tsirkin
2019-05-29 3:24 ` Michael S. Tsirkin
2019-05-30 1:51 ` David Gibson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190530020753.GE2017@umbus.fritz.box \
--to=david@gibson.dropbear.id.au \
--cc=clg@kaod.org \
--cc=groug@kaod.org \
--cc=mdroth@linux.ibm.com \
--cc=mst@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=qemu-ppc@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.