[3/4] core/pci: Use cached vendor/device/class IDs on populating device node

Message ID 1491524180-12644-4-git-send-email-gwshan@linux.vnet.ibm.com
State Rejected
Headers show

Commit Message

Gavin Shan April 7, 2017, 12:16 a.m.
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(-)

Comments

Benjamin Herrenschmidt June 15, 2017, 5:32 a.m. | #1
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;
Gavin Shan June 19, 2017, 3:29 a.m. | #2
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;
>

Patch

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;