[Skiboot] [PATCH] nvlink: Set a bit in config space to indicate a real PCI device was bound
Russell Currey
ruscur at russell.cc
Mon Nov 9 15:29:40 AEDT 2015
The version was already set (somewhat obscurely), that has been refactored and the version incremented as per the following patch (which supercedes the previous):
Signed-off-by: Russell Currey <ruscur at russell.cc>
---
doc/nvlink.txt | 15 +++++++++++++--
hw/npu.c | 31 +++++++++++++++++++++++--------
2 files changed, 36 insertions(+), 10 deletions(-)
diff --git a/doc/nvlink.txt b/doc/nvlink.txt
index 5673de4..d871d20 100644
--- a/doc/nvlink.txt
+++ b/doc/nvlink.txt
@@ -60,15 +60,21 @@ Vendor Specific Capabilities
----------------------------
+-----------------+----------------+----------------+----------------+
-| Reserved | Cap Length | Next Cap Ptr | Cap ID (0x09) |
+| Version (0x02) | Cap Length | Next Cap Ptr | Cap ID (0x09) |
+-----------------+----------------+----------------+----------------+
| Procedure Status Register |
+--------------------------------------------------------------------+
| Procedure Control Register |
+---------------------------------------------------+----------------+
-| Reserved | Link Number |
+| Reserved | PCI Dev Flag | Link Number |
+---------------------------------------------------+----------------+
+Version
+
+ This refers to the version of the NPU config space. Used by device
+ drivers to determine which fields of the config space they can
+ expect to be available.
+
Procedure Control Register
Used to start hardware procedures.
@@ -121,6 +127,11 @@ Procedure Status Register
3 - Procedure aborted.
4 - Unsupported procedure.
+PCI Device Flag
+
+ Bit 0 is set only if an actual PCI device was bound to this
+ emulated device.
+
Link Number
Physical link number this emulated PCI device is assoicated
diff --git a/hw/npu.c b/hw/npu.c
index c9bc12b..85edabb 100644
--- a/hw/npu.c
+++ b/hw/npu.c
@@ -111,6 +111,15 @@
static struct npu_dev_cap *npu_dev_find_capability(struct npu_dev *dev,
uint16_t id);
+#define OPAL_NPU_VERSION 0x02
+
+#define PCIE_CAP_START 0x40
+#define PCIE_CAP_END 0x80
+#define VENDOR_CAP_START 0x80
+#define VENDOR_CAP_END 0x90
+
+#define VENDOR_CAP_PCI_DEV_OFFSET 0x0d
+
/* PCI config raw accessors */
#define NPU_DEV_CFG_NORMAL_RD(d, o, s, v) \
npu_dev_cfg_read_raw(d, NPU_DEV_CFG_NORMAL, o, s, v)
@@ -546,6 +555,9 @@ static void npu_dev_bind_pci_dev(struct npu_dev *dev)
dev->pd = pci_walk_dev(phb, __npu_dev_bind_pci_dev, dev);
if (dev->pd) {
dev->phb = phb;
+ /* Found the device, set the bit in config space */
+ NPU_DEV_CFG_INIT_RO(dev, VENDOR_CAP_START +
+ VENDOR_CAP_PCI_DEV_OFFSET, 1, 0x01);
return;
}
}
@@ -1194,11 +1206,12 @@ static void npu_dev_populate_vendor_cap(struct npu_dev_cap *cap)
{
struct npu_dev *dev = cap->dev;
uint32_t offset = cap->start;
- uint32_t val;
+ uint8_t val;
- /* Add version and length information */
- val = (cap->end - cap->start) | 0x1 << 8;
- NPU_DEV_CFG_INIT_RO(dev, offset + 2, 4, val);
+ /* Add length and version information */
+ val = cap->end - cap->start;
+ NPU_DEV_CFG_INIT_RO(dev, offset + 2, 1, val);
+ NPU_DEV_CFG_INIT_RO(dev, offset + 3, 1, OPAL_NPU_VERSION);
offset += 4;
/* Defaults when the trap can't handle the read/write (eg. due
@@ -1212,7 +1225,7 @@ static void npu_dev_populate_vendor_cap(struct npu_dev_cap *cap)
npu_dev_procedure_write);
offset += 8;
- NPU_DEV_CFG_INIT_RO(dev, offset, 4, dev->index);
+ NPU_DEV_CFG_INIT_RO(dev, offset, 1, dev->index);
}
static void npu_dev_populate_pcie_cap(struct npu_dev_cap *cap)
@@ -1229,7 +1242,7 @@ static void npu_dev_populate_pcie_cap(struct npu_dev_cap *cap)
}
/* Sanity check on spanned registers */
- if ((cap->end - cap->start) < 0x40) {
+ if ((cap->end - cap->start) < PCIE_CAP_START) {
prlog(PR_NOTICE, "%s: Invalid reg region [%x, %x] for cap %d\n",
__func__, cap->start, cap->end, cap->id);
return;
@@ -1352,11 +1365,13 @@ static void npu_dev_create_capabilities(struct npu_dev *dev)
/* PCI express capability */
npu_dev_create_capability(dev, npu_dev_populate_pcie_cap,
- PCI_CFG_CAP_ID_EXP, 0x40, 0x80);
+ PCI_CFG_CAP_ID_EXP, PCIE_CAP_START,
+ PCIE_CAP_END);
/* Vendor specific capability */
npu_dev_create_capability(dev, npu_dev_populate_vendor_cap,
- PCI_CFG_CAP_ID_VENDOR, 0x80, 0x90);
+ PCI_CFG_CAP_ID_VENDOR, VENDOR_CAP_START,
+ VENDOR_CAP_END);
}
static void npu_dev_create_cfg(struct npu_dev *dev)
--
2.6.2
> Russell,
>
> A few minor comments below. Mainly we need to add a version field to the
> vendor specific capability so that Nvidia can detect that this bit is meant to
> be present. I would suggest using the upper byte of the vendor specific
> capability like so:
>
> +-----------------+----------------+----------------+----------------+
> | Version (0x1) | Cap Length | Next Cap Ptr | Cap ID (0x09) |
> +-----------------+----------------+----------------+----------------+
>
>
> On Fri, 30 Oct 2015 15:36:30 Russell Currey wrote:
>> Previously, it was difficult to determine from userspace whether a NVLink
>> emulated PCI device was associated with a real PCI device. A bit in
>> the vendor capabilities section of the config space is now set if a
>> device was bound. The documentation has been updated to reflect this.
>>
>> In addition, the config space offsets are refactored into #defines.
>>
>> Signed-off-by: Russell Currey <ruscur at russell.cc>
>> ---
>> doc/nvlink.txt | 7 ++++++-
>> hw/npu.c | 16 ++++++++++++++--
>> 2 files changed, 20 insertions(+), 3 deletions(-)
>>
>> diff --git a/doc/nvlink.txt b/doc/nvlink.txt
>> index 5673de4..d5f96c1 100644
>> --- a/doc/nvlink.txt
>> +++ b/doc/nvlink.txt
>> @@ -66,7 +66,7 @@ Vendor Specific Capabilities
>> +--------------------------------------------------------------------+
>> | Procedure Control Register |
>> +---------------------------------------------------+----------------+
>> -| Reserved | Link Number |
>> +| Reserved | PCI Dev Flag | Link Number |
>> +---------------------------------------------------+----------------+
>>
>> Procedure Control Register
>> @@ -121,6 +121,11 @@ Procedure Status Register
>> 3 - Procedure aborted.
>> 4 - Unsupported procedure.
>>
>> +PCI Device Flag
>> +
>> + This bit is set only if an actual PCI device was bound to this
>> + emulated device.
>
> It would be good if we could call out the actual bit here (ie. bit 0) - we may
> define other PCI device flag bits in future.
>
> Thanks.
>
> - Alistair
>
>> Link Number
>>
>> Physical link number this emulated PCI device is assoicated
>> diff --git a/hw/npu.c b/hw/npu.c
>> index c9bc12b..daf3d1f 100644
>> --- a/hw/npu.c
>> +++ b/hw/npu.c
>> @@ -111,6 +111,13 @@
>> static struct npu_dev_cap *npu_dev_find_capability(struct npu_dev *dev,
>> uint16_t id);
>>
>> +#define PCIE_CAP_START 0x40
>> +#define PCIE_CAP_END 0x80
>> +#define VENDOR_CAP_START 0x80
>> +#define VENDOR_CAP_END 0x90
>> +
>> +#define VENDOR_CAP_PCI_DEV_OFFSET 0x0d
>> +
>> /* PCI config raw accessors */
>> #define NPU_DEV_CFG_NORMAL_RD(d, o, s, v) \
>> npu_dev_cfg_read_raw(d, NPU_DEV_CFG_NORMAL, o, s, v)
>> @@ -546,6 +553,9 @@ static void npu_dev_bind_pci_dev(struct npu_dev *dev)
>> dev->pd = pci_walk_dev(phb, __npu_dev_bind_pci_dev, dev);
>> if (dev->pd) {
>> dev->phb = phb;
>> + /* Found the device, set the bit in config space */
>> + NPU_DEV_CFG_INIT_RO(dev, VENDOR_CAP_START +
>> + VENDOR_CAP_PCI_DEV_OFFSET, 1, 0x01);
>> return;
>> }
>> }
>> @@ -1352,11 +1362,13 @@ static void npu_dev_create_capabilities(struct
> npu_dev *dev)
>>
>> /* PCI express capability */
>> npu_dev_create_capability(dev, npu_dev_populate_pcie_cap,
>> - PCI_CFG_CAP_ID_EXP, 0x40, 0x80);
>> + PCI_CFG_CAP_ID_EXP, PCIE_CAP_START,
>> + PCIE_CAP_END);
>>
>> /* Vendor specific capability */
>> npu_dev_create_capability(dev, npu_dev_populate_vendor_cap,
>> - PCI_CFG_CAP_ID_VENDOR, 0x80, 0x90);
>> + PCI_CFG_CAP_ID_VENDOR, VENDOR_CAP_START,
>> + VENDOR_CAP_END);
>> }
>>
>> static void npu_dev_create_cfg(struct npu_dev *dev)
>>
>
More information about the Skiboot
mailing list