| Message ID | 1491524180-12644-4-git-send-email-gwshan@linux.vnet.ibm.com |
|---|---|
| State | Rejected |
| Headers | show |
On Fri, 2017-04-07 at 10:16 +1000, Gavin Shan wrote: > This uses the cached vendor/device/class IDs when populating the > device node for the particular PCI device, no need to read from > the hardware in order to save some CPU cycles. I don't like the class change. The class is 3 bytes, don't make it be class | revid, it's pointless, we cache it already as class, so that doesn't need changing. > Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> > --- > core/pci.c | 54 +++++++++++++++++++++++------------------------------- > include/pci.h | 3 +++ > 2 files changed, 26 insertions(+), 31 deletions(-) > > diff --git a/core/pci.c b/core/pci.c > index a93239b..7742454 100644 > --- a/core/pci.c > +++ b/core/pci.c > @@ -251,7 +251,6 @@ static struct pci_device *pci_scan_one(struct phb *phb, struct pci_device *paren > pd->vdid = vdid; > pci_cfg_read32(phb, bdfn, PCI_CFG_SUBSYS_VENDOR_ID, &pd->sub_vdid); > pci_cfg_read32(phb, bdfn, PCI_CFG_REV_ID, &pd->class); > - pd->class >>= 8; > > pd->parent = parent; > list_head_init(&pd->pcrf); > @@ -268,6 +267,14 @@ static struct pci_device *pci_scan_one(struct phb *phb, struct pci_device *paren > > pci_init_capabilities(phb, pd); > > + /* > + * Quirk for IBM bridge bogus class on PCIe root complex. > + * Without it, the PCI DN won't be created for its downstream > + * devices in Linux. > + */ > + if (!parent && pci_has_cap(pd, PCI_CFG_CAP_ID_EXP, false)) > + pd->class = (0x06040000 | (pd->class & 0xff)); > + > /* If it's a bridge, sanitize the bus numbers to avoid forwarding > * > * This will help when walking down those bridges later on > @@ -1331,16 +1338,12 @@ static void pci_add_loc_code(struct dt_node *np, struct pci_device *pd) > } > > static void pci_print_summary_line(struct phb *phb, struct pci_device *pd, > - struct dt_node *np, u32 rev_class, > - const char *cname) > + struct dt_node *np, const char *cname) > { > const char *label, *dtype, *s; > - u32 vdid; > #define MAX_SLOTSTR 32 > char slotstr[MAX_SLOTSTR + 1] = { 0, }; > > - pci_cfg_read32(phb, pd->bdfn, 0, &vdid); > - > /* If it's a slot, it has a slot-label */ > label = dt_prop_get_def(np, "ibm,slot-label", NULL); > if (label) { > @@ -1379,14 +1382,15 @@ static void pci_print_summary_line(struct phb *phb, struct pci_device *pd, > if (pd->is_bridge) > PCINOTICE(phb, pd->bdfn, > "[%s] %04x %04x R:%02x C:%06x B:%02x..%02x %s\n", > - dtype, vdid & 0xffff, vdid >> 16, > - rev_class & 0xff, rev_class >> 8, pd->secondary_bus, > - pd->subordinate_bus, slotstr); > + dtype, PCI_VENDOR_ID(pd->vdid), PCI_DEVICE_ID(pd->vdid), > + PCI_CLASS_REV(pd->class), PCI_CLASS_CODE(pd->class), > + pd->secondary_bus, pd->subordinate_bus, slotstr); > else > PCINOTICE(phb, pd->bdfn, > "[%s] %04x %04x R:%02x C:%06x (%14s) %s\n", > - dtype, vdid & 0xffff, vdid >> 16, > - rev_class & 0xff, rev_class >> 8, cname, slotstr); > + dtype, PCI_VENDOR_ID(pd->vdid), PCI_DEVICE_ID(pd->vdid), > + PCI_CLASS_REV(pd->class), PCI_CLASS_CODE(pd->class), > + cname, slotstr); > } > > static void pci_add_one_device_node(struct phb *phb, > @@ -1400,7 +1404,6 @@ static void pci_add_one_device_node(struct phb *phb, > #define MAX_NAME 256 > char name[MAX_NAME]; > char compat[MAX_NAME]; > - uint32_t rev_class, vdid; > uint32_t reg[5]; > uint8_t intpin; > const uint32_t ranges_direct[] = { > @@ -1411,19 +1414,8 @@ static void pci_add_one_device_node(struct phb *phb, > 0x02000000, 0x0, 0x0, > 0xf0000000, 0x0}; > > - pci_cfg_read32(phb, pd->bdfn, 0, &vdid); > - pci_cfg_read32(phb, pd->bdfn, PCI_CFG_REV_ID, &rev_class); > pci_cfg_read8(phb, pd->bdfn, PCI_CFG_INT_PIN, &intpin); > - > - /* > - * Quirk for IBM bridge bogus class on PCIe root complex. > - * Without it, the PCI DN won't be created for its downstream > - * devices in Linux. > - */ > - if (pci_has_cap(pd, PCI_CFG_CAP_ID_EXP, false) && > - parent_node == phb->dt_node) > - rev_class = (rev_class & 0xff) | 0x6040000; > - cname = pci_class_name(rev_class >> 8); > + cname = pci_class_name(PCI_CLASS_CODE(pd->class)); > > if (pd->bdfn & 0x7) > snprintf(name, MAX_NAME - 1, "%s@%x,%x", > @@ -1436,17 +1428,17 @@ static void pci_add_one_device_node(struct phb *phb, > /* XXX FIXME: make proper "compatible" properties */ > if (pci_has_cap(pd, PCI_CFG_CAP_ID_EXP, false)) { > snprintf(compat, MAX_NAME, "pciex%x,%x", > - vdid & 0xffff, vdid >> 16); > + PCI_VENDOR_ID(pd->vdid), PCI_DEVICE_ID(pd->vdid)); > dt_add_property_cells(np, "ibm,pci-config-space-type", 1); > } else { > snprintf(compat, MAX_NAME, "pci%x,%x", > - vdid & 0xffff, vdid >> 16); > + PCI_VENDOR_ID(pd->vdid), PCI_DEVICE_ID(pd->vdid)); > dt_add_property_cells(np, "ibm,pci-config-space-type", 0); > } > - dt_add_property_cells(np, "class-code", rev_class >> 8); > - dt_add_property_cells(np, "revision-id", rev_class & 0xff); > - dt_add_property_cells(np, "vendor-id", vdid & 0xffff); > - dt_add_property_cells(np, "device-id", vdid >> 16); > + dt_add_property_cells(np, "class-code", PCI_CLASS_CODE(pd->class)); > + dt_add_property_cells(np, "revision-id", PCI_CLASS_REV(pd->class)); > + dt_add_property_cells(np, "vendor-id", PCI_VENDOR_ID(pd->vdid)); > + dt_add_property_cells(np, "device-id", PCI_DEVICE_ID(pd->vdid)); > if (intpin) > dt_add_property_cells(np, "interrupts", intpin); > > @@ -1478,7 +1470,7 @@ static void pci_add_one_device_node(struct phb *phb, > dt_add_property(np, "reg", reg, sizeof(reg)); > > /* Print summary info about the device */ > - pci_print_summary_line(phb, pd, np, rev_class, cname); > + pci_print_summary_line(phb, pd, np, cname); > if (!pd->is_bridge) > return; > > diff --git a/include/pci.h b/include/pci.h > index 296fb8d..1c13545 100644 > --- a/include/pci.h > +++ b/include/pci.h > @@ -75,6 +75,9 @@ struct pci_device { > #define PCI_VENDOR_ID(x) ((x) & 0xFFFF) > #define PCI_DEVICE_ID(x) ((x) >> 8) > uint32_t class; > +#define PCI_CLASS_REV(x) ((x) & 0xFF) > +#define PCI_CLASS_CODE(x) ((x) >> 8) > +#define PCI_CLASS_ID(x) ((x) >> 16) > uint64_t cap_list; > struct { > uint32_t pos;
On Thu, Jun 15, 2017 at 03:32:51PM +1000, Benjamin Herrenschmidt wrote: >On Fri, 2017-04-07 at 10:16 +1000, Gavin Shan wrote: >> This uses the cached vendor/device/class IDs when populating the >> device node for the particular PCI device, no need to read from >> the hardware in order to save some CPU cycles. > >I don't like the class change. > >The class is 3 bytes, don't make it be class | revid, it's pointless, >we cache it already as class, so that doesn't need changing. > Ok. I'll drop the part changing (class | revid), and keep the code using the cached vdid. Cheers, Gavin >> Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> >> --- >> core/pci.c | 54 +++++++++++++++++++++++------------------------------- >> include/pci.h | 3 +++ >> 2 files changed, 26 insertions(+), 31 deletions(-) >> >> diff --git a/core/pci.c b/core/pci.c >> index a93239b..7742454 100644 >> --- a/core/pci.c >> +++ b/core/pci.c >> @@ -251,7 +251,6 @@ static struct pci_device *pci_scan_one(struct phb *phb, struct pci_device *paren >> pd->vdid = vdid; >> pci_cfg_read32(phb, bdfn, PCI_CFG_SUBSYS_VENDOR_ID, &pd->sub_vdid); >> pci_cfg_read32(phb, bdfn, PCI_CFG_REV_ID, &pd->class); >> - pd->class >>= 8; >> >> pd->parent = parent; >> list_head_init(&pd->pcrf); >> @@ -268,6 +267,14 @@ static struct pci_device *pci_scan_one(struct phb *phb, struct pci_device *paren >> >> pci_init_capabilities(phb, pd); >> >> + /* >> + * Quirk for IBM bridge bogus class on PCIe root complex. >> + * Without it, the PCI DN won't be created for its downstream >> + * devices in Linux. >> + */ >> + if (!parent && pci_has_cap(pd, PCI_CFG_CAP_ID_EXP, false)) >> + pd->class = (0x06040000 | (pd->class & 0xff)); >> + >> /* If it's a bridge, sanitize the bus numbers to avoid forwarding >> * >> * This will help when walking down those bridges later on >> @@ -1331,16 +1338,12 @@ static void pci_add_loc_code(struct dt_node *np, struct pci_device *pd) >> } >> >> static void pci_print_summary_line(struct phb *phb, struct pci_device *pd, >> - struct dt_node *np, u32 rev_class, >> - const char *cname) >> + struct dt_node *np, const char *cname) >> { >> const char *label, *dtype, *s; >> - u32 vdid; >> #define MAX_SLOTSTR 32 >> char slotstr[MAX_SLOTSTR + 1] = { 0, }; >> >> - pci_cfg_read32(phb, pd->bdfn, 0, &vdid); >> - >> /* If it's a slot, it has a slot-label */ >> label = dt_prop_get_def(np, "ibm,slot-label", NULL); >> if (label) { >> @@ -1379,14 +1382,15 @@ static void pci_print_summary_line(struct phb *phb, struct pci_device *pd, >> if (pd->is_bridge) >> PCINOTICE(phb, pd->bdfn, >> "[%s] %04x %04x R:%02x C:%06x B:%02x..%02x %s\n", >> - dtype, vdid & 0xffff, vdid >> 16, >> - rev_class & 0xff, rev_class >> 8, pd->secondary_bus, >> - pd->subordinate_bus, slotstr); >> + dtype, PCI_VENDOR_ID(pd->vdid), PCI_DEVICE_ID(pd->vdid), >> + PCI_CLASS_REV(pd->class), PCI_CLASS_CODE(pd->class), >> + pd->secondary_bus, pd->subordinate_bus, slotstr); >> else >> PCINOTICE(phb, pd->bdfn, >> "[%s] %04x %04x R:%02x C:%06x (%14s) %s\n", >> - dtype, vdid & 0xffff, vdid >> 16, >> - rev_class & 0xff, rev_class >> 8, cname, slotstr); >> + dtype, PCI_VENDOR_ID(pd->vdid), PCI_DEVICE_ID(pd->vdid), >> + PCI_CLASS_REV(pd->class), PCI_CLASS_CODE(pd->class), >> + cname, slotstr); >> } >> >> static void pci_add_one_device_node(struct phb *phb, >> @@ -1400,7 +1404,6 @@ static void pci_add_one_device_node(struct phb *phb, >> #define MAX_NAME 256 >> char name[MAX_NAME]; >> char compat[MAX_NAME]; >> - uint32_t rev_class, vdid; >> uint32_t reg[5]; >> uint8_t intpin; >> const uint32_t ranges_direct[] = { >> @@ -1411,19 +1414,8 @@ static void pci_add_one_device_node(struct phb *phb, >> 0x02000000, 0x0, 0x0, >> 0xf0000000, 0x0}; >> >> - pci_cfg_read32(phb, pd->bdfn, 0, &vdid); >> - pci_cfg_read32(phb, pd->bdfn, PCI_CFG_REV_ID, &rev_class); >> pci_cfg_read8(phb, pd->bdfn, PCI_CFG_INT_PIN, &intpin); >> - >> - /* >> - * Quirk for IBM bridge bogus class on PCIe root complex. >> - * Without it, the PCI DN won't be created for its downstream >> - * devices in Linux. >> - */ >> - if (pci_has_cap(pd, PCI_CFG_CAP_ID_EXP, false) && >> - parent_node == phb->dt_node) >> - rev_class = (rev_class & 0xff) | 0x6040000; >> - cname = pci_class_name(rev_class >> 8); >> + cname = pci_class_name(PCI_CLASS_CODE(pd->class)); >> >> if (pd->bdfn & 0x7) >> snprintf(name, MAX_NAME - 1, "%s@%x,%x", >> @@ -1436,17 +1428,17 @@ static void pci_add_one_device_node(struct phb *phb, >> /* XXX FIXME: make proper "compatible" properties */ >> if (pci_has_cap(pd, PCI_CFG_CAP_ID_EXP, false)) { >> snprintf(compat, MAX_NAME, "pciex%x,%x", >> - vdid & 0xffff, vdid >> 16); >> + PCI_VENDOR_ID(pd->vdid), PCI_DEVICE_ID(pd->vdid)); >> dt_add_property_cells(np, "ibm,pci-config-space-type", 1); >> } else { >> snprintf(compat, MAX_NAME, "pci%x,%x", >> - vdid & 0xffff, vdid >> 16); >> + PCI_VENDOR_ID(pd->vdid), PCI_DEVICE_ID(pd->vdid)); >> dt_add_property_cells(np, "ibm,pci-config-space-type", 0); >> } >> - dt_add_property_cells(np, "class-code", rev_class >> 8); >> - dt_add_property_cells(np, "revision-id", rev_class & 0xff); >> - dt_add_property_cells(np, "vendor-id", vdid & 0xffff); >> - dt_add_property_cells(np, "device-id", vdid >> 16); >> + dt_add_property_cells(np, "class-code", PCI_CLASS_CODE(pd->class)); >> + dt_add_property_cells(np, "revision-id", PCI_CLASS_REV(pd->class)); >> + dt_add_property_cells(np, "vendor-id", PCI_VENDOR_ID(pd->vdid)); >> + dt_add_property_cells(np, "device-id", PCI_DEVICE_ID(pd->vdid)); >> if (intpin) >> dt_add_property_cells(np, "interrupts", intpin); >> >> @@ -1478,7 +1470,7 @@ static void pci_add_one_device_node(struct phb *phb, >> dt_add_property(np, "reg", reg, sizeof(reg)); >> >> /* Print summary info about the device */ >> - pci_print_summary_line(phb, pd, np, rev_class, cname); >> + pci_print_summary_line(phb, pd, np, cname); >> if (!pd->is_bridge) >> return; >> >> diff --git a/include/pci.h b/include/pci.h >> index 296fb8d..1c13545 100644 >> --- a/include/pci.h >> +++ b/include/pci.h >> @@ -75,6 +75,9 @@ struct pci_device { >> #define PCI_VENDOR_ID(x) ((x) & 0xFFFF) >> #define PCI_DEVICE_ID(x) ((x) >> 8) >> uint32_t class; >> +#define PCI_CLASS_REV(x) ((x) & 0xFF) >> +#define PCI_CLASS_CODE(x) ((x) >> 8) >> +#define PCI_CLASS_ID(x) ((x) >> 16) >> uint64_t cap_list; >> struct { >> uint32_t pos; >
diff --git a/core/pci.c b/core/pci.c index a93239b..7742454 100644 --- a/core/pci.c +++ b/core/pci.c @@ -251,7 +251,6 @@ static struct pci_device *pci_scan_one(struct phb *phb, struct pci_device *paren pd->vdid = vdid; pci_cfg_read32(phb, bdfn, PCI_CFG_SUBSYS_VENDOR_ID, &pd->sub_vdid); pci_cfg_read32(phb, bdfn, PCI_CFG_REV_ID, &pd->class); - pd->class >>= 8; pd->parent = parent; list_head_init(&pd->pcrf); @@ -268,6 +267,14 @@ static struct pci_device *pci_scan_one(struct phb *phb, struct pci_device *paren pci_init_capabilities(phb, pd); + /* + * Quirk for IBM bridge bogus class on PCIe root complex. + * Without it, the PCI DN won't be created for its downstream + * devices in Linux. + */ + if (!parent && pci_has_cap(pd, PCI_CFG_CAP_ID_EXP, false)) + pd->class = (0x06040000 | (pd->class & 0xff)); + /* If it's a bridge, sanitize the bus numbers to avoid forwarding * * This will help when walking down those bridges later on @@ -1331,16 +1338,12 @@ static void pci_add_loc_code(struct dt_node *np, struct pci_device *pd) } static void pci_print_summary_line(struct phb *phb, struct pci_device *pd, - struct dt_node *np, u32 rev_class, - const char *cname) + struct dt_node *np, const char *cname) { const char *label, *dtype, *s; - u32 vdid; #define MAX_SLOTSTR 32 char slotstr[MAX_SLOTSTR + 1] = { 0, }; - pci_cfg_read32(phb, pd->bdfn, 0, &vdid); - /* If it's a slot, it has a slot-label */ label = dt_prop_get_def(np, "ibm,slot-label", NULL); if (label) { @@ -1379,14 +1382,15 @@ static void pci_print_summary_line(struct phb *phb, struct pci_device *pd, if (pd->is_bridge) PCINOTICE(phb, pd->bdfn, "[%s] %04x %04x R:%02x C:%06x B:%02x..%02x %s\n", - dtype, vdid & 0xffff, vdid >> 16, - rev_class & 0xff, rev_class >> 8, pd->secondary_bus, - pd->subordinate_bus, slotstr); + dtype, PCI_VENDOR_ID(pd->vdid), PCI_DEVICE_ID(pd->vdid), + PCI_CLASS_REV(pd->class), PCI_CLASS_CODE(pd->class), + pd->secondary_bus, pd->subordinate_bus, slotstr); else PCINOTICE(phb, pd->bdfn, "[%s] %04x %04x R:%02x C:%06x (%14s) %s\n", - dtype, vdid & 0xffff, vdid >> 16, - rev_class & 0xff, rev_class >> 8, cname, slotstr); + dtype, PCI_VENDOR_ID(pd->vdid), PCI_DEVICE_ID(pd->vdid), + PCI_CLASS_REV(pd->class), PCI_CLASS_CODE(pd->class), + cname, slotstr); } static void pci_add_one_device_node(struct phb *phb, @@ -1400,7 +1404,6 @@ static void pci_add_one_device_node(struct phb *phb, #define MAX_NAME 256 char name[MAX_NAME]; char compat[MAX_NAME]; - uint32_t rev_class, vdid; uint32_t reg[5]; uint8_t intpin; const uint32_t ranges_direct[] = { @@ -1411,19 +1414,8 @@ static void pci_add_one_device_node(struct phb *phb, 0x02000000, 0x0, 0x0, 0xf0000000, 0x0}; - pci_cfg_read32(phb, pd->bdfn, 0, &vdid); - pci_cfg_read32(phb, pd->bdfn, PCI_CFG_REV_ID, &rev_class); pci_cfg_read8(phb, pd->bdfn, PCI_CFG_INT_PIN, &intpin); - - /* - * Quirk for IBM bridge bogus class on PCIe root complex. - * Without it, the PCI DN won't be created for its downstream - * devices in Linux. - */ - if (pci_has_cap(pd, PCI_CFG_CAP_ID_EXP, false) && - parent_node == phb->dt_node) - rev_class = (rev_class & 0xff) | 0x6040000; - cname = pci_class_name(rev_class >> 8); + cname = pci_class_name(PCI_CLASS_CODE(pd->class)); if (pd->bdfn & 0x7) snprintf(name, MAX_NAME - 1, "%s@%x,%x", @@ -1436,17 +1428,17 @@ static void pci_add_one_device_node(struct phb *phb, /* XXX FIXME: make proper "compatible" properties */ if (pci_has_cap(pd, PCI_CFG_CAP_ID_EXP, false)) { snprintf(compat, MAX_NAME, "pciex%x,%x", - vdid & 0xffff, vdid >> 16); + PCI_VENDOR_ID(pd->vdid), PCI_DEVICE_ID(pd->vdid)); dt_add_property_cells(np, "ibm,pci-config-space-type", 1); } else { snprintf(compat, MAX_NAME, "pci%x,%x", - vdid & 0xffff, vdid >> 16); + PCI_VENDOR_ID(pd->vdid), PCI_DEVICE_ID(pd->vdid)); dt_add_property_cells(np, "ibm,pci-config-space-type", 0); } - dt_add_property_cells(np, "class-code", rev_class >> 8); - dt_add_property_cells(np, "revision-id", rev_class & 0xff); - dt_add_property_cells(np, "vendor-id", vdid & 0xffff); - dt_add_property_cells(np, "device-id", vdid >> 16); + dt_add_property_cells(np, "class-code", PCI_CLASS_CODE(pd->class)); + dt_add_property_cells(np, "revision-id", PCI_CLASS_REV(pd->class)); + dt_add_property_cells(np, "vendor-id", PCI_VENDOR_ID(pd->vdid)); + dt_add_property_cells(np, "device-id", PCI_DEVICE_ID(pd->vdid)); if (intpin) dt_add_property_cells(np, "interrupts", intpin); @@ -1478,7 +1470,7 @@ static void pci_add_one_device_node(struct phb *phb, dt_add_property(np, "reg", reg, sizeof(reg)); /* Print summary info about the device */ - pci_print_summary_line(phb, pd, np, rev_class, cname); + pci_print_summary_line(phb, pd, np, cname); if (!pd->is_bridge) return; diff --git a/include/pci.h b/include/pci.h index 296fb8d..1c13545 100644 --- a/include/pci.h +++ b/include/pci.h @@ -75,6 +75,9 @@ struct pci_device { #define PCI_VENDOR_ID(x) ((x) & 0xFFFF) #define PCI_DEVICE_ID(x) ((x) >> 8) uint32_t class; +#define PCI_CLASS_REV(x) ((x) & 0xFF) +#define PCI_CLASS_CODE(x) ((x) >> 8) +#define PCI_CLASS_ID(x) ((x) >> 16) uint64_t cap_list; struct { uint32_t pos;
This uses the cached vendor/device/class IDs when populating the device node for the particular PCI device, no need to read from the hardware in order to save some CPU cycles. Signed-off-by: Gavin Shan <gwshan@linux.vnet.ibm.com> --- core/pci.c | 54 +++++++++++++++++++++++------------------------------- include/pci.h | 3 +++ 2 files changed, 26 insertions(+), 31 deletions(-)