[PATCH v3 0/2] fpga: dfl: optional VSEC for start of dfl

[PATCH v3 0/2] fpga: dfl: optional VSEC for start of dfl

[matthew.gerlach]

From: Matthew Gerlach <matthew.gerlach@linux.intel.com>

The start of a Device Feature List (DFL) is currently assumed to be at
Bar0/Offset 0 on the PCIe bus by drivers/fpga/dfl-pci.c.  This patchset
adds support for the start one or more DFLs to be specified in a
Vendor-Specific Capability (VSEC) structure in PCIe config space.  If no
such VSEC structure exists, then the start is assumed to be
Bar0/Offset 0 for backward compatibility.

Matthew Gerlach (2):
  fpga: dfl: refactor cci_enumerate_feature_devs()
  fpga: dfl: look for vendor specific capability

 Documentation/fpga/dfl.rst |  25 ++++++
 drivers/fpga/dfl-pci.c     | 169 +++++++++++++++++++++++++++++--------
 2 files changed, 159 insertions(+), 35 deletions(-)

-- 
2.25.2

[PATCH v3 1/2] fpga: dfl: refactor cci_enumerate_feature_devs()

[matthew.gerlach]

From: Matthew Gerlach <matthew.gerlach@linux.intel.com>

In preparation of looking for dfls based on a vendor
specific pcie capability, move code that assumes
Bar0/offset0 as start of DFL to its own function.

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
v3: no change

v2: remove spurious blank lines
    rename find_dfl_in_bar0 to find_dfls_by_default
---
 drivers/fpga/dfl-pci.c | 84 +++++++++++++++++++++++-------------------
 1 file changed, 47 insertions(+), 37 deletions(-)

diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
index a2203d03c9e2..b27fae045536 100644
--- a/drivers/fpga/dfl-pci.c
+++ b/drivers/fpga/dfl-pci.c
@@ -119,49 +119,20 @@ static int *cci_pci_create_irq_table(struct pci_dev *pcidev, unsigned int nvec)
 	return table;
 }
 
-/* enumerate feature devices under pci device */
-static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
+static int find_dfls_by_default(struct pci_dev *pcidev,
+				struct dfl_fpga_enum_info *info)
 {
-	struct cci_drvdata *drvdata = pci_get_drvdata(pcidev);
-	int port_num, bar, i, nvec, ret = 0;
-	struct dfl_fpga_enum_info *info;
-	struct dfl_fpga_cdev *cdev;
 	resource_size_t start, len;
+	int port_num, bar, i;
 	void __iomem *base;
-	int *irq_table;
+	int ret = 0;
 	u32 offset;
 	u64 v;
 
-	/* allocate enumeration info via pci_dev */
-	info = dfl_fpga_enum_info_alloc(&pcidev->dev);
-	if (!info)
-		return -ENOMEM;
-
-	/* add irq info for enumeration if the device support irq */
-	nvec = cci_pci_alloc_irq(pcidev);
-	if (nvec < 0) {
-		dev_err(&pcidev->dev, "Fail to alloc irq %d.\n", nvec);
-		ret = nvec;
-		goto enum_info_free_exit;
-	} else if (nvec) {
-		irq_table = cci_pci_create_irq_table(pcidev, nvec);
-		if (!irq_table) {
-			ret = -ENOMEM;
-			goto irq_free_exit;
-		}
-
-		ret = dfl_fpga_enum_info_add_irq(info, nvec, irq_table);
-		kfree(irq_table);
-		if (ret)
-			goto irq_free_exit;
-	}
-
-	/* start to find Device Feature List in Bar 0 */
+	/* start to find Device Feature List from Bar 0 */
 	base = cci_pci_ioremap_bar0(pcidev);
-	if (!base) {
-		ret = -ENOMEM;
-		goto irq_free_exit;
-	}
+	if (!base)
+		return -ENOMEM;
 
 	/*
 	 * PF device has FME and Ports/AFUs, and VF device only has one
@@ -208,12 +179,51 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
 		dfl_fpga_enum_info_add_dfl(info, start, len);
 	} else {
 		ret = -ENODEV;
-		goto irq_free_exit;
 	}
 
 	/* release I/O mappings for next step enumeration */
 	pcim_iounmap_regions(pcidev, BIT(0));
 
+	return ret;
+}
+
+/* enumerate feature devices under pci device */
+static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
+{
+	struct cci_drvdata *drvdata = pci_get_drvdata(pcidev);
+	struct dfl_fpga_enum_info *info;
+	struct dfl_fpga_cdev *cdev;
+	int nvec, ret = 0;
+	int *irq_table;
+
+	/* allocate enumeration info via pci_dev */
+	info = dfl_fpga_enum_info_alloc(&pcidev->dev);
+	if (!info)
+		return -ENOMEM;
+
+	/* add irq info for enumeration if the device support irq */
+	nvec = cci_pci_alloc_irq(pcidev);
+	if (nvec < 0) {
+		dev_err(&pcidev->dev, "Fail to alloc irq %d.\n", nvec);
+		ret = nvec;
+		goto enum_info_free_exit;
+	} else if (nvec) {
+		irq_table = cci_pci_create_irq_table(pcidev, nvec);
+		if (!irq_table) {
+			ret = -ENOMEM;
+			goto irq_free_exit;
+		}
+
+		ret = dfl_fpga_enum_info_add_irq(info, nvec, irq_table);
+		kfree(irq_table);
+		if (ret)
+			goto irq_free_exit;
+	}
+
+	ret = find_dfls_by_default(pcidev, info);
+	if (ret)
+		goto irq_free_exit;
+
 	/* start enumeration with prepared enumeration information */
 	cdev = dfl_fpga_feature_devs_enumerate(info);
 	if (IS_ERR(cdev)) {
-- 
2.25.2

RE: [PATCH v3 1/2] fpga: dfl: refactor cci_enumerate_feature_devs()

[matthew.gerlach]

On Sat, 28 Nov 2020, Wu, Hao wrote:

>> Subject: [PATCH v3 1/2] fpga: dfl: refactor cci_enumerate_feature_devs()
>>
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> In preparation of looking for dfls based on a vendor
>> specific pcie capability, move code that assumes
>> Bar0/offset0 as start of DFL to its own function.
>
> as the default method to locate the first dfl.
>
How about the following?

In preparation of looking for dfls based on a vendor specific pci 
capability, move the code for the default method of finding the first dfl 
at offset 0 of Bar 0 to its own function.

>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>> v3: no change
>>
>> v2: remove spurious blank lines
>>     rename find_dfl_in_bar0 to find_dfls_by_default
>> ---
>>  drivers/fpga/dfl-pci.c | 84 +++++++++++++++++++++++-------------------
>>  1 file changed, 47 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
>> index a2203d03c9e2..b27fae045536 100644
>> --- a/drivers/fpga/dfl-pci.c
>> +++ b/drivers/fpga/dfl-pci.c
>> @@ -119,49 +119,20 @@ static int *cci_pci_create_irq_table(struct pci_dev
>> *pcidev, unsigned int nvec)
>>  	return table;
>>  }
>>
>> -/* enumerate feature devices under pci device */
>> -static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
>> +static int find_dfls_by_default(struct pci_dev *pcidev,
>> +				struct dfl_fpga_enum_info *info)
>
> Please add one line comment to describe this is the default method
> is finding it in bar 0 and offset 0.
>
>>  {
>> -	struct cci_drvdata *drvdata = pci_get_drvdata(pcidev);
>> -	int port_num, bar, i, nvec, ret = 0;
>> -	struct dfl_fpga_enum_info *info;
>> -	struct dfl_fpga_cdev *cdev;
>>  	resource_size_t start, len;
>> +	int port_num, bar, i;
>>  	void __iomem *base;
>> -	int *irq_table;
>> +	int ret = 0;
>
> can be put into one line as previous code.

Ok, I can fix that.

>
> Other places look good to me.
>
> with above fixings,
> Acked-by: Wu Hao <hao.wu@intel.com>
>
> Thanks
> Hao
>
>>  	u32 offset;
>>  	u64 v;
>>
>> -	/* allocate enumeration info via pci_dev */
>> -	info = dfl_fpga_enum_info_alloc(&pcidev->dev);
>> -	if (!info)
>> -		return -ENOMEM;
>> -
>> -	/* add irq info for enumeration if the device support irq */
>> -	nvec = cci_pci_alloc_irq(pcidev);
>> -	if (nvec < 0) {
>> -		dev_err(&pcidev->dev, "Fail to alloc irq %d.\n", nvec);
>> -		ret = nvec;
>> -		goto enum_info_free_exit;
>> -	} else if (nvec) {
>> -		irq_table = cci_pci_create_irq_table(pcidev, nvec);
>> -		if (!irq_table) {
>> -			ret = -ENOMEM;
>> -			goto irq_free_exit;
>> -		}
>> -
>> -		ret = dfl_fpga_enum_info_add_irq(info, nvec, irq_table);
>> -		kfree(irq_table);
>> -		if (ret)
>> -			goto irq_free_exit;
>> -	}
>> -
>> -	/* start to find Device Feature List in Bar 0 */
>> +	/* start to find Device Feature List from Bar 0 */
>>  	base = cci_pci_ioremap_bar0(pcidev);
>> -	if (!base) {
>> -		ret = -ENOMEM;
>> -		goto irq_free_exit;
>> -	}
>> +	if (!base)
>> +		return -ENOMEM;
>>
>>  	/*
>>  	 * PF device has FME and Ports/AFUs, and VF device only has one
>> @@ -208,12 +179,51 @@ static int cci_enumerate_feature_devs(struct
>> pci_dev *pcidev)
>>  		dfl_fpga_enum_info_add_dfl(info, start, len);
>>  	} else {
>>  		ret = -ENODEV;
>> -		goto irq_free_exit;
>>  	}
>>
>>  	/* release I/O mappings for next step enumeration */
>>  	pcim_iounmap_regions(pcidev, BIT(0));
>>
>> +	return ret;
>> +}
>> +
>> +/* enumerate feature devices under pci device */
>> +static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
>> +{
>> +	struct cci_drvdata *drvdata = pci_get_drvdata(pcidev);
>> +	struct dfl_fpga_enum_info *info;
>> +	struct dfl_fpga_cdev *cdev;
>> +	int nvec, ret = 0;
>> +	int *irq_table;
>> +
>> +	/* allocate enumeration info via pci_dev */
>> +	info = dfl_fpga_enum_info_alloc(&pcidev->dev);
>> +	if (!info)
>> +		return -ENOMEM;
>> +
>> +	/* add irq info for enumeration if the device support irq */
>> +	nvec = cci_pci_alloc_irq(pcidev);
>> +	if (nvec < 0) {
>> +		dev_err(&pcidev->dev, "Fail to alloc irq %d.\n", nvec);
>> +		ret = nvec;
>> +		goto enum_info_free_exit;
>> +	} else if (nvec) {
>> +		irq_table = cci_pci_create_irq_table(pcidev, nvec);
>> +		if (!irq_table) {
>> +			ret = -ENOMEM;
>> +			goto irq_free_exit;
>> +		}
>> +
>> +		ret = dfl_fpga_enum_info_add_irq(info, nvec, irq_table);
>> +		kfree(irq_table);
>> +		if (ret)
>> +			goto irq_free_exit;
>> +	}
>> +
>> +	ret = find_dfls_by_default(pcidev, info);
>> +	if (ret)
>> +		goto irq_free_exit;
>> +
>>  	/* start enumeration with prepared enumeration information */
>>  	cdev = dfl_fpga_feature_devs_enumerate(info);
>>  	if (IS_ERR(cdev)) {
>> --
>> 2.25.2
>
>

[PATCH v3 2/2] fpga: dfl: look for vendor specific capability

[matthew.gerlach]

From: Matthew Gerlach <matthew.gerlach@linux.intel.com>

A DFL may not begin at offset 0 of BAR 0.  A PCIe vendor
specific capability can be used to specify the start of a
number of DFLs.

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
---
v3: Add text and ascii art to documentation.
    Ensure not to exceed PCIe config space in loop.

v2: Update documentation for clarity.
    Clean up  macro names.
    Use GENMASK.
    Removed spurious blank lines.
    Changed some calls from dev_info to dev_dbg.
    Specifically check for VSEC not found, -ENODEV.
    Ensure correct pci vendor id.
    Remove check for page alignment.
    Rename find_dfl_in_cfg to find_dfls_by_vsec.
    Initialize target memory of pci_read_config_dword to invalid values before use.
---
 Documentation/fpga/dfl.rst | 25 +++++++++++
 drivers/fpga/dfl-pci.c     | 91 +++++++++++++++++++++++++++++++++++++-
 2 files changed, 115 insertions(+), 1 deletion(-)

diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
index 0404fe6ffc74..fa0da884a818 100644
--- a/Documentation/fpga/dfl.rst
+++ b/Documentation/fpga/dfl.rst
@@ -501,6 +501,31 @@ Developer only needs to provide a sub feature driver with matched feature id.
 FME Partial Reconfiguration Sub Feature driver (see drivers/fpga/dfl-fme-pr.c)
 could be a reference.
 
+Location of DFLs on a PCI Device
+===========================
+There are two ways of locating DFLs on a PCI Device.  The original
+method assumed the start of the first DFL to offset 0 of bar 0.
+If the first node of the DFL is an FME, then further DFLs
+in the port(s) are specified in FME header registers.
+Alternatively, a vendor specific capability structure can be used to
+specify the location of all the DFLs on the device, providing flexibility
+for the type of starting node in the DFL.  Intel has reserved the
+VSEC ID of 0x43 for this purpose.  The vendor specific
+data begins with a 4 byte vendor specific register for the number of DFLs followed 4 byte
+Offset/BIR vendor specific registers for each DFL. Bits 2:0 of Offset/BIR register
+indicates the BAR, and bits 31:3 form the 8 byte aligned offset where bits 2:0 are
+zero.
+
+        +----------------------------+
+        |31     Number of DFLS      0|
+        +----------------------------+
+        |31     Offset     3|2 BIR  0|
+        +----------------------------+
+                      . . .
+        +----------------------------+
+        |31     Offset     3|2 BIR  0|
+        +----------------------------+
+
 
 Open discussion
 ===============
diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
index b27fae045536..a58bf4299d6b 100644
--- a/drivers/fpga/dfl-pci.c
+++ b/drivers/fpga/dfl-pci.c
@@ -27,6 +27,14 @@
 #define DRV_VERSION	"0.8"
 #define DRV_NAME	"dfl-pci"
 
+#define PCI_VSEC_ID_INTEL_DFLS 0x43
+
+#define PCI_VNDR_DFLS_CNT 8
+#define PCI_VNDR_DFLS_RES 0x0c
+
+#define PCI_VNDR_DFLS_RES_BAR_MASK GENMASK(2, 0)
+#define PCI_VNDR_DFLS_RES_OFF_MASK GENMASK(31, 3)
+
 struct cci_drvdata {
 	struct dfl_fpga_cdev *cdev;	/* container device */
 };
@@ -119,6 +127,84 @@ static int *cci_pci_create_irq_table(struct pci_dev *pcidev, unsigned int nvec)
 	return table;
 }
 
+static int find_dfls_by_vsec(struct pci_dev *pcidev, struct dfl_fpga_enum_info *info)
+{
+	u32 bar, offset, vndr_hdr, dfl_cnt, dfl_res;
+	int dfl_res_off, i, voff = 0;
+	resource_size_t start, len;
+
+	if (pcidev->vendor != PCI_VENDOR_ID_INTEL)
+		return -ENODEV;
+
+	while ((voff = pci_find_next_ext_capability(pcidev, voff, PCI_EXT_CAP_ID_VNDR))) {
+		vndr_hdr = 0;
+		pci_read_config_dword(pcidev, voff + PCI_VNDR_HEADER, &vndr_hdr);
+
+		dev_dbg(&pcidev->dev,
+			"vendor-specific capability id 0x%x, rev 0x%x len 0x%x\n",
+			PCI_VNDR_HEADER_ID(vndr_hdr),
+			PCI_VNDR_HEADER_REV(vndr_hdr),
+			PCI_VNDR_HEADER_LEN(vndr_hdr));
+
+		if (PCI_VNDR_HEADER_ID(vndr_hdr) == PCI_VSEC_ID_INTEL_DFLS)
+			break;
+	}
+
+	if (!voff) {
+		dev_dbg(&pcidev->dev, "%s no VSEC found\n", __func__);
+		return -ENODEV;
+	}
+
+	dfl_cnt = 0;
+	pci_read_config_dword(pcidev, voff + PCI_VNDR_DFLS_CNT, &dfl_cnt);
+	dev_dbg(&pcidev->dev, "dfl_cnt %d\n", dfl_cnt);
+	for (i = 0; i < dfl_cnt; i++) {
+		dfl_res_off = voff + PCI_VNDR_DFLS_RES +
+				      (i * sizeof(dfl_res));
+		if (dfl_res_off >= PCI_CFG_SPACE_EXP_SIZE) {
+			dev_err(&pcidev->dev, "%s offset too big for PCIe config space\n",
+				__func__);
+			return -EINVAL;
+		}
+
+		dfl_res = GENMASK(31, 0);
+		pci_read_config_dword(pcidev, dfl_res_off, &dfl_res);
+
+		dev_dbg(&pcidev->dev, "dfl_res 0x%x\n", dfl_res);
+
+		bar = dfl_res & PCI_VNDR_DFLS_RES_BAR_MASK;
+		if (bar >= PCI_STD_NUM_BARS) {
+			dev_err(&pcidev->dev, "%s bad bar number %d\n",
+				__func__, bar);
+			return -EINVAL;
+		}
+
+		len = pci_resource_len(pcidev, bar);
+		if (len == 0) {
+			dev_err(&pcidev->dev, "%s unmapped bar number %d\n",
+				__func__, bar);
+			return -EINVAL;
+		}
+
+		offset = dfl_res & PCI_VNDR_DFLS_RES_OFF_MASK;
+		if (offset >= len) {
+			dev_err(&pcidev->dev, "%s bad offset %u >= %pa\n",
+				__func__, offset, &len);
+			return -EINVAL;
+		}
+
+		dev_dbg(&pcidev->dev, "%s BAR %d offset 0x%x\n", __func__, bar, offset);
+
+		len -= offset;
+
+		start = pci_resource_start(pcidev, bar) + offset;
+
+		dfl_fpga_enum_info_add_dfl(info, start, len);
+	}
+
+	return 0;
+}
+
 static int find_dfls_by_default(struct pci_dev *pcidev,
 				struct dfl_fpga_enum_info *info)
 {
@@ -220,7 +306,10 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
 			goto irq_free_exit;
 	}
 
-	ret = find_dfls_by_default(pcidev, info);
+	ret = find_dfls_by_vsec(pcidev, info);
+	if (ret == -ENODEV)
+		ret = find_dfls_by_default(pcidev, info);
+
 	if (ret)
 		goto irq_free_exit;
 
-- 
2.25.2

RE: [PATCH v3 2/2] fpga: dfl: look for vendor specific capability

[matthew.gerlach]

On Sat, 28 Nov 2020, Wu, Hao wrote:

>> Subject: [PATCH v3 2/2] fpga: dfl: look for vendor specific capability
>
> Maybe we can change the title a little bit, what about
> fpga: dfl-pci: locate DFLs by PCIe vendor specific capability
>
>>
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> A DFL may not begin at offset 0 of BAR 0.  A PCIe vendor
>> specific capability can be used to specify the start of a
>> number of DFLs.
>
> A PCIe vendor specific extended capability is introduced by Intel to
> specify the start of a number of DFLs.

Your suggestion is more precise.
>
>
>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> ---
>> v3: Add text and ascii art to documentation.
>>     Ensure not to exceed PCIe config space in loop.
>>
>> v2: Update documentation for clarity.
>>     Clean up  macro names.
>>     Use GENMASK.
>>     Removed spurious blank lines.
>>     Changed some calls from dev_info to dev_dbg.
>>     Specifically check for VSEC not found, -ENODEV.
>>     Ensure correct pci vendor id.
>>     Remove check for page alignment.
>>     Rename find_dfl_in_cfg to find_dfls_by_vsec.
>>     Initialize target memory of pci_read_config_dword to invalid values before
>> use.
>> ---
>>  Documentation/fpga/dfl.rst | 25 +++++++++++
>>  drivers/fpga/dfl-pci.c     | 91 +++++++++++++++++++++++++++++++++++++-
>>  2 files changed, 115 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
>> index 0404fe6ffc74..fa0da884a818 100644
>> --- a/Documentation/fpga/dfl.rst
>> +++ b/Documentation/fpga/dfl.rst
>> @@ -501,6 +501,31 @@ Developer only needs to provide a sub feature
>> driver with matched feature id.
>>  FME Partial Reconfiguration Sub Feature driver (see drivers/fpga/dfl-fme-
>> pr.c)
>>  could be a reference.
>>
>> +Location of DFLs on a PCI Device
>> +===========================
>> +There are two ways of locating DFLs on a PCI Device.  The original
>
> I found this new VSEC is only for PCIe device, correct? If so, let's make
> sure descriptions are accurate. E.g. default method for all devices
> and a new method for PCIe device.

Yes, the default method can be used with PCI and PCIe device, and the VSEC 
approach is PCIe, only.  Documentation can be made more precise.

>
>> +method assumed the start of the first DFL to offset 0 of bar 0.
>> +If the first node of the DFL is an FME, then further DFLs
>> +in the port(s) are specified in FME header registers.
>> +Alternatively, a vendor specific capability structure can be used to
>> +specify the location of all the DFLs on the device, providing flexibility
>> +for the type of starting node in the DFL.  Intel has reserved the
>> +VSEC ID of 0x43 for this purpose.  The vendor specific
>> +data begins with a 4 byte vendor specific register for the number of DFLs
>> followed 4 byte
>> +Offset/BIR vendor specific registers for each DFL. Bits 2:0 of Offset/BIR
>> register
>
> Do we have a defined register name here? or it's named as Offset/BIR register?
> Sounds a little wired, and I see you defined it as DFLS_RES?

The Offset/BIR terminology is also used in the MSI-X capability structure.

>
>> +indicates the BAR, and bits 31:3 form the 8 byte aligned offset where bits
>> 2:0 are
>> +zero.
>> +
>> +        +----------------------------+
>> +        |31     Number of DFLS      0|
>> +        +----------------------------+
>> +        |31     Offset     3|2 BIR  0|
>> +        +----------------------------+
>> +                      . . .
>> +        +----------------------------+
>> +        |31     Offset     3|2 BIR  0|
>> +        +----------------------------+
>> +
>
> Maybe it's better to have register name and offset in above table.
> BTW: if there is some public link to related spec, that helps too.

I'll consider adding a register name and offset, but I am not sure it adds 
much information.

>
>>
>>  Open discussion
>>  ===============
>> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
>> index b27fae045536..a58bf4299d6b 100644
>> --- a/drivers/fpga/dfl-pci.c
>> +++ b/drivers/fpga/dfl-pci.c
>> @@ -27,6 +27,14 @@
>>  #define DRV_VERSION	"0.8"
>>  #define DRV_NAME	"dfl-pci"
>>
>> +#define PCI_VSEC_ID_INTEL_DFLS 0x43
>> +
>> +#define PCI_VNDR_DFLS_CNT 8
>> +#define PCI_VNDR_DFLS_RES 0x0c
>
> They are both register offset? it's better to use the same style.
>
> 0x8
> 0xc
>
> Something like this.

Agrreed.
>
>> +
>> +#define PCI_VNDR_DFLS_RES_BAR_MASK GENMASK(2, 0)
>> +#define PCI_VNDR_DFLS_RES_OFF_MASK GENMASK(31, 3)
>> +
>>  struct cci_drvdata {
>>  	struct dfl_fpga_cdev *cdev;	/* container device */
>>  };
>> @@ -119,6 +127,84 @@ static int *cci_pci_create_irq_table(struct pci_dev
>> *pcidev, unsigned int nvec)
>>  	return table;
>>  }
>>
>> +static int find_dfls_by_vsec(struct pci_dev *pcidev, struct
>> dfl_fpga_enum_info *info)
>> +{
>> +	u32 bar, offset, vndr_hdr, dfl_cnt, dfl_res;
>> +	int dfl_res_off, i, voff = 0;
>> +	resource_size_t start, len;
>> +
>> +	if (pcidev->vendor != PCI_VENDOR_ID_INTEL)
>> +		return -ENODEV;
>
> Check it later then other vendor can add their ones easily?
>
>> +
>> +	while ((voff = pci_find_next_ext_capability(pcidev, voff,
>> PCI_EXT_CAP_ID_VNDR))) {
>> +		vndr_hdr = 0;
>
> It seems it doesn't need this.

Initializing vndr_hdr = 0 ensures a failed pci_read_config_dword() failure 
is handled properly.  I will remove the call and the debug information 
anyway.

  >
>> +		pci_read_config_dword(pcidev, voff + PCI_VNDR_HEADER,
>> &vndr_hdr);
>> +
>> +		dev_dbg(&pcidev->dev,
>> +			"vendor-specific capability id 0x%x, rev 0x%x len
>> 0x%x\n",
>> +			PCI_VNDR_HEADER_ID(vndr_hdr),
>> +			PCI_VNDR_HEADER_REV(vndr_hdr),
>> +			PCI_VNDR_HEADER_LEN(vndr_hdr));
>
> Suggest remove this debug information.
>
>> +
>> +		if (PCI_VNDR_HEADER_ID(vndr_hdr) ==
>> PCI_VSEC_ID_INTEL_DFLS)
>
> How about
> 		if (vendor == intel && header_id == INTEL_DFLS)
> 			break;
Seems reasonable.
>
>> +			break;
>> +	}
>> +
>> +	if (!voff) {
>> +		dev_dbg(&pcidev->dev, "%s no VSEC found\n", __func__);
>
> No DFL VSEC found
>
> There could be many different VSECs but no DFL ones.

Agreed.
>
>> +		return -ENODEV;
>> +	}
>> +
>> +	dfl_cnt = 0;
>
> Can be merged into the line which defines dfl_cnt? Or we can just
> remove this line.

This initialization ensures that a failure to the pci_read_config_dword 
function below is handled properly.  It can be merged into the definition 
line.

>
>> +	pci_read_config_dword(pcidev, voff + PCI_VNDR_DFLS_CNT,
>> &dfl_cnt);
>> +	dev_dbg(&pcidev->dev, "dfl_cnt %d\n", dfl_cnt);
>> +	for (i = 0; i < dfl_cnt; i++) {
>> +		dfl_res_off = voff + PCI_VNDR_DFLS_RES +
>> +				      (i * sizeof(dfl_res));
>
> Above two line can be put into one line, it's < 80 length.

Agreed.

>
>> +		if (dfl_res_off >= PCI_CFG_SPACE_EXP_SIZE) {
>> +			dev_err(&pcidev->dev, "%s offset too big for PCIe
>> config space\n",
>> +				__func__);
>> +			return -EINVAL;
>> +		}
>> +
>> +		dfl_res = GENMASK(31, 0);
>
> do we really need this?

Again, the assignment is ensuring that a failure to the 
pci_read_config_dword function is handled properly.

>
>> +		pci_read_config_dword(pcidev, dfl_res_off, &dfl_res);
>> +
>> +		dev_dbg(&pcidev->dev, "dfl_res 0x%x\n", dfl_res);
>
> Can be read by lspci even without driver, so we may not really need this
> debug information here.


I suppose the call to dev_dbg can be removed.

>
>> +
>> +		bar = dfl_res & PCI_VNDR_DFLS_RES_BAR_MASK;
>> +		if (bar >= PCI_STD_NUM_BARS) {
>> +			dev_err(&pcidev->dev, "%s bad bar number %d\n",
>> +				__func__, bar);
>> +			return -EINVAL;
>> +		}
>> +
>> +		len = pci_resource_len(pcidev, bar);
>> +		if (len == 0) {
>> +			dev_err(&pcidev->dev, "%s unmapped bar
>> number %d\n",
>> +				__func__, bar);
>> +			return -EINVAL;
>
> can be covered by below case, as mentioned in previous patch.
Agreed, I forgot to remove it.
>
>> +		}
>> +
>> +		offset = dfl_res & PCI_VNDR_DFLS_RES_OFF_MASK;
>> +		if (offset >= len) {
>> +			dev_err(&pcidev->dev, "%s bad offset %u >= %pa\n",
>> +				__func__, offset, &len);
>> +			return -EINVAL;
>> +		}
>> +
>> +		dev_dbg(&pcidev->dev, "%s BAR %d offset 0x%x\n",
>> __func__, bar, offset);
>> +
>> +		len -= offset;
>> +
>> +		start = pci_resource_start(pcidev, bar) + offset;
>> +
>> +		dfl_fpga_enum_info_add_dfl(info, start, len);
>
> That means everytime, we pass [start, endofbar] region to dfl core
> for enumeration, if there are multiple DFLs in one bar, then each range
> ends at the same endofbar, it seems fine as enumeration can be done
> one by one, but ideally the best case is that this capability can provide
> end address or size too, right? It is possible that information can be
> added to the capability as well? then we don't have such limitation.
>
> Hao

I am not sure having more than one DFL in a bar serves any purpose over a 
single DFL.  Regardless, I think the consistency of just having Offset/BIR 
in the VSEC is better than adding more infomation that has little or no added value.

>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static int find_dfls_by_default(struct pci_dev *pcidev,
>>  				struct dfl_fpga_enum_info *info)
>>  {
>> @@ -220,7 +306,10 @@ static int cci_enumerate_feature_devs(struct
>> pci_dev *pcidev)
>>  			goto irq_free_exit;
>>  	}
>>
>> -	ret = find_dfls_by_default(pcidev, info);
>> +	ret = find_dfls_by_vsec(pcidev, info);
>> +	if (ret == -ENODEV)
>> +		ret = find_dfls_by_default(pcidev, info);
>> +
>>  	if (ret)
>>  		goto irq_free_exit;
>>
>> --
>> 2.25.2
>
>

Re: [PATCH v3 2/2] fpga: dfl: look for vendor specific capability

[Moritz Fischer]

Hi Matthew,

On Mon, Nov 30, 2020 at 04:45:20PM -0800, matthew.gerlach@linux.intel.com wrote:
> 
> 
> On Sat, 28 Nov 2020, Wu, Hao wrote:
> 
> > > Subject: [PATCH v3 2/2] fpga: dfl: look for vendor specific capability
> > 
> > Maybe we can change the title a little bit, what about
> > fpga: dfl-pci: locate DFLs by PCIe vendor specific capability
> > 
> > > 
> > > From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > 
> > > A DFL may not begin at offset 0 of BAR 0.  A PCIe vendor
> > > specific capability can be used to specify the start of a
> > > number of DFLs.
> > 
> > A PCIe vendor specific extended capability is introduced by Intel to
> > specify the start of a number of DFLs.
> 
> Your suggestion is more precise.
> > 
> > 
> > > 
> > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > ---
> > > v3: Add text and ascii art to documentation.
> > >     Ensure not to exceed PCIe config space in loop.
> > > 
> > > v2: Update documentation for clarity.
> > >     Clean up  macro names.
> > >     Use GENMASK.
> > >     Removed spurious blank lines.
> > >     Changed some calls from dev_info to dev_dbg.
> > >     Specifically check for VSEC not found, -ENODEV.
> > >     Ensure correct pci vendor id.
> > >     Remove check for page alignment.
> > >     Rename find_dfl_in_cfg to find_dfls_by_vsec.
> > >     Initialize target memory of pci_read_config_dword to invalid values before
> > > use.
> > > ---
> > >  Documentation/fpga/dfl.rst | 25 +++++++++++
> > >  drivers/fpga/dfl-pci.c     | 91 +++++++++++++++++++++++++++++++++++++-
> > >  2 files changed, 115 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
> > > index 0404fe6ffc74..fa0da884a818 100644
> > > --- a/Documentation/fpga/dfl.rst
> > > +++ b/Documentation/fpga/dfl.rst
> > > @@ -501,6 +501,31 @@ Developer only needs to provide a sub feature
> > > driver with matched feature id.
> > >  FME Partial Reconfiguration Sub Feature driver (see drivers/fpga/dfl-fme-
> > > pr.c)
> > >  could be a reference.
> > > 
> > > +Location of DFLs on a PCI Device
> > > +===========================
> > > +There are two ways of locating DFLs on a PCI Device.  The original
> > 
> > I found this new VSEC is only for PCIe device, correct? If so, let's make
> > sure descriptions are accurate. E.g. default method for all devices
> > and a new method for PCIe device.
> 
> Yes, the default method can be used with PCI and PCIe device, and the VSEC
> approach is PCIe, only.  Documentation can be made more precise.
> 
> > 
> > > +method assumed the start of the first DFL to offset 0 of bar 0.
> > > +If the first node of the DFL is an FME, then further DFLs
> > > +in the port(s) are specified in FME header registers.
> > > +Alternatively, a vendor specific capability structure can be used to
Maybe: a vendor specific extended capability (VSEC) ...
> > > +specify the location of all the DFLs on the device, providing flexibility
> > > +for the type of starting node in the DFL.  Intel has reserved the
> > > +VSEC ID of 0x43 for this purpose.  The vendor specific
> > > +data begins with a 4 byte vendor specific register for the number of DFLs
> > > followed 4 byte
> > > +Offset/BIR vendor specific registers for each DFL. Bits 2:0 of Offset/BIR
> > > register
> > 
> > Do we have a defined register name here? or it's named as Offset/BIR register?
> > Sounds a little wired, and I see you defined it as DFLS_RES?
> 
> The Offset/BIR terminology is also used in the MSI-X capability structure.

Yeah, this intuitively made sense to me having worked with PCIe :)
> 
> > 
> > > +indicates the BAR, and bits 31:3 form the 8 byte aligned offset where bits
> > > 2:0 are
> > > +zero.
> > > +
> > > +        +----------------------------+
> > > +        |31     Number of DFLS      0|
> > > +        +----------------------------+
> > > +        |31     Offset     3|2 BIR  0|
> > > +        +----------------------------+
> > > +                      . . .
> > > +        +----------------------------+
> > > +        |31     Offset     3|2 BIR  0|
> > > +        +----------------------------+
> > > +
> > 
> > Maybe it's better to have register name and offset in above table.
> > BTW: if there is some public link to related spec, that helps too.
> 
> I'll consider adding a register name and offset, but I am not sure it adds
> much information.

I think this is fine together with textual description you have above.
> 
> > 
> > > 
> > >  Open discussion
> > >  ===============
> > > diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
> > > index b27fae045536..a58bf4299d6b 100644
> > > --- a/drivers/fpga/dfl-pci.c
> > > +++ b/drivers/fpga/dfl-pci.c
> > > @@ -27,6 +27,14 @@
> > >  #define DRV_VERSION	"0.8"
> > >  #define DRV_NAME	"dfl-pci"
> > > 
> > > +#define PCI_VSEC_ID_INTEL_DFLS 0x43
> > > +
> > > +#define PCI_VNDR_DFLS_CNT 8
> > > +#define PCI_VNDR_DFLS_RES 0x0c
> > 
> > They are both register offset? it's better to use the same style.
> > 
> > 0x8
> > 0xc
> > 
> > Something like this.
> 
> Agrreed.
> > 
> > > +
> > > +#define PCI_VNDR_DFLS_RES_BAR_MASK GENMASK(2, 0)
> > > +#define PCI_VNDR_DFLS_RES_OFF_MASK GENMASK(31, 3)
> > > +
> > >  struct cci_drvdata {
> > >  	struct dfl_fpga_cdev *cdev;	/* container device */
> > >  };
> > > @@ -119,6 +127,84 @@ static int *cci_pci_create_irq_table(struct pci_dev
> > > *pcidev, unsigned int nvec)
> > >  	return table;
> > >  }
> > > 
> > > +static int find_dfls_by_vsec(struct pci_dev *pcidev, struct
> > > dfl_fpga_enum_info *info)
> > > +{
> > > +	u32 bar, offset, vndr_hdr, dfl_cnt, dfl_res;
> > > +	int dfl_res_off, i, voff = 0;
> > > +	resource_size_t start, len;
> > > +
> > > +	if (pcidev->vendor != PCI_VENDOR_ID_INTEL)
> > > +		return -ENODEV;
> > 
> > Check it later then other vendor can add their ones easily?
> > 
> > > +
> > > +	while ((voff = pci_find_next_ext_capability(pcidev, voff,
> > > PCI_EXT_CAP_ID_VNDR))) {
> > > +		vndr_hdr = 0;
> > 
> > It seems it doesn't need this.
> 
> Initializing vndr_hdr = 0 ensures a failed pci_read_config_dword() failure
> is handled properly.  I will remove the call and the debug information
> anyway.
> 
>  >
> > > +		pci_read_config_dword(pcidev, voff + PCI_VNDR_HEADER,
> > > &vndr_hdr);
> > > +
> > > +		dev_dbg(&pcidev->dev,
> > > +			"vendor-specific capability id 0x%x, rev 0x%x len
> > > 0x%x\n",
> > > +			PCI_VNDR_HEADER_ID(vndr_hdr),
> > > +			PCI_VNDR_HEADER_REV(vndr_hdr),
> > > +			PCI_VNDR_HEADER_LEN(vndr_hdr));
> > 
> > Suggest remove this debug information.
> > 
> > > +
> > > +		if (PCI_VNDR_HEADER_ID(vndr_hdr) ==
> > > PCI_VSEC_ID_INTEL_DFLS)
> > 
> > How about
> > 		if (vendor == intel && header_id == INTEL_DFLS)
> > 			break;
> Seems reasonable.
> > 
> > > +			break;
> > > +	}
> > > +
> > > +	if (!voff) {
> > > +		dev_dbg(&pcidev->dev, "%s no VSEC found\n", __func__);
> > 
> > No DFL VSEC found
> > 
> > There could be many different VSECs but no DFL ones.
> 
> Agreed.
> > 
> > > +		return -ENODEV;
> > > +	}
> > > +
> > > +	dfl_cnt = 0;
> > 
> > Can be merged into the line which defines dfl_cnt? Or we can just
> > remove this line.
> 
> This initialization ensures that a failure to the pci_read_config_dword
> function below is handled properly.  It can be merged into the definition
> line.
> 
> > 
> > > +	pci_read_config_dword(pcidev, voff + PCI_VNDR_DFLS_CNT,
> > > &dfl_cnt);
> > > +	dev_dbg(&pcidev->dev, "dfl_cnt %d\n", dfl_cnt);
> > > +	for (i = 0; i < dfl_cnt; i++) {
> > > +		dfl_res_off = voff + PCI_VNDR_DFLS_RES +
> > > +				      (i * sizeof(dfl_res));
> > 
> > Above two line can be put into one line, it's < 80 length.
> 
> Agreed.
> 
> > 
> > > +		if (dfl_res_off >= PCI_CFG_SPACE_EXP_SIZE) {
> > > +			dev_err(&pcidev->dev, "%s offset too big for PCIe
> > > config space\n",
> > > +				__func__);
> > > +			return -EINVAL;
> > > +		}
> > > +
> > > +		dfl_res = GENMASK(31, 0);
> > 
> > do we really need this?
> 
> Again, the assignment is ensuring that a failure to the
> pci_read_config_dword function is handled properly.
> 
> > 
> > > +		pci_read_config_dword(pcidev, dfl_res_off, &dfl_res);
> > > +
> > > +		dev_dbg(&pcidev->dev, "dfl_res 0x%x\n", dfl_res);
> > 
> > Can be read by lspci even without driver, so we may not really need this
> > debug information here.
> 
> 
> I suppose the call to dev_dbg can be removed.
> 
> > 
> > > +
> > > +		bar = dfl_res & PCI_VNDR_DFLS_RES_BAR_MASK;
> > > +		if (bar >= PCI_STD_NUM_BARS) {
> > > +			dev_err(&pcidev->dev, "%s bad bar number %d\n",
> > > +				__func__, bar);
> > > +			return -EINVAL;
> > > +		}
> > > +
> > > +		len = pci_resource_len(pcidev, bar);
> > > +		if (len == 0) {
> > > +			dev_err(&pcidev->dev, "%s unmapped bar
> > > number %d\n",
> > > +				__func__, bar);
> > > +			return -EINVAL;
> > 
> > can be covered by below case, as mentioned in previous patch.
> Agreed, I forgot to remove it.
> > 
> > > +		}
> > > +
> > > +		offset = dfl_res & PCI_VNDR_DFLS_RES_OFF_MASK;
> > > +		if (offset >= len) {
> > > +			dev_err(&pcidev->dev, "%s bad offset %u >= %pa\n",
> > > +				__func__, offset, &len);
> > > +			return -EINVAL;
> > > +		}
> > > +
> > > +		dev_dbg(&pcidev->dev, "%s BAR %d offset 0x%x\n",
> > > __func__, bar, offset);
> > > +
> > > +		len -= offset;
> > > +
> > > +		start = pci_resource_start(pcidev, bar) + offset;
> > > +
> > > +		dfl_fpga_enum_info_add_dfl(info, start, len);
> > 
> > That means everytime, we pass [start, endofbar] region to dfl core
> > for enumeration, if there are multiple DFLs in one bar, then each range
> > ends at the same endofbar, it seems fine as enumeration can be done
> > one by one, but ideally the best case is that this capability can provide
> > end address or size too, right? It is possible that information can be
> > added to the capability as well? then we don't have such limitation.
> > 
> > Hao
> 
> I am not sure having more than one DFL in a bar serves any purpose over a
> single DFL.  Regardless, I think the consistency of just having Offset/BIR
> in the VSEC is better than adding more infomation that has little or no
> added value.

Agreed. Can't you just link the DFLs in that case?
> 
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static int find_dfls_by_default(struct pci_dev *pcidev,
> > >  				struct dfl_fpga_enum_info *info)
> > >  {
> > > @@ -220,7 +306,10 @@ static int cci_enumerate_feature_devs(struct
> > > pci_dev *pcidev)
> > >  			goto irq_free_exit;
> > >  	}
> > > 
> > > -	ret = find_dfls_by_default(pcidev, info);
> > > +	ret = find_dfls_by_vsec(pcidev, info);
> > > +	if (ret == -ENODEV)
> > > +		ret = find_dfls_by_default(pcidev, info);
> > > +
> > >  	if (ret)
> > >  		goto irq_free_exit;
> > > 
> > > --
> > > 2.25.2
> > 
> > 

- Moritz

Re: [PATCH v3 2/2] fpga: dfl: look for vendor specific capability

[Xu Yilun]

> > > 
> > > > +		}
> > > > +
> > > > +		offset = dfl_res & PCI_VNDR_DFLS_RES_OFF_MASK;
> > > > +		if (offset >= len) {
> > > > +			dev_err(&pcidev->dev, "%s bad offset %u >= %pa\n",
> > > > +				__func__, offset, &len);
> > > > +			return -EINVAL;
> > > > +		}
> > > > +
> > > > +		dev_dbg(&pcidev->dev, "%s BAR %d offset 0x%x\n",
> > > > __func__, bar, offset);
> > > > +
> > > > +		len -= offset;
> > > > +
> > > > +		start = pci_resource_start(pcidev, bar) + offset;
> > > > +
> > > > +		dfl_fpga_enum_info_add_dfl(info, start, len);
> > > 
> > > That means everytime, we pass [start, endofbar] region to dfl core
> > > for enumeration, if there are multiple DFLs in one bar, then each range
> > > ends at the same endofbar, it seems fine as enumeration can be done
> > > one by one, but ideally the best case is that this capability can provide
> > > end address or size too, right? It is possible that information can be
> > > added to the capability as well? then we don't have such limitation.
> > > 
> > > Hao
> > 
> > I am not sure having more than one DFL in a bar serves any purpose over a
> > single DFL.  Regardless, I think the consistency of just having Offset/BIR
> > in the VSEC is better than adding more infomation that has little or no
> > added value.
> 
> Agreed. Can't you just link the DFLs in that case?

I didn't see the value of more DFLs in one bar either. So I think we'd better
document it.

Thanks,
Yilun

RE: [PATCH v3 2/2] fpga: dfl: look for vendor specific capability

[Wu, Hao]

> > > >
> > > > > +		}
> > > > > +
> > > > > +		offset = dfl_res & PCI_VNDR_DFLS_RES_OFF_MASK;
> > > > > +		if (offset >= len) {
> > > > > +			dev_err(&pcidev->dev, "%s bad
> offset %u >= %pa\n",
> > > > > +				__func__, offset, &len);
> > > > > +			return -EINVAL;
> > > > > +		}
> > > > > +
> > > > > +		dev_dbg(&pcidev->dev, "%s BAR %d offset 0x%x\n",
> > > > > __func__, bar, offset);
> > > > > +
> > > > > +		len -= offset;
> > > > > +
> > > > > +		start = pci_resource_start(pcidev, bar) + offset;
> > > > > +
> > > > > +		dfl_fpga_enum_info_add_dfl(info, start, len);
> > > >
> > > > That means everytime, we pass [start, endofbar] region to dfl core
> > > > for enumeration, if there are multiple DFLs in one bar, then each range
> > > > ends at the same endofbar, it seems fine as enumeration can be done
> > > > one by one, but ideally the best case is that this capability can provide
> > > > end address or size too, right? It is possible that information can be
> > > > added to the capability as well? then we don't have such limitation.
> > > >
> > > > Hao
> > >
> > > I am not sure having more than one DFL in a bar serves any purpose over
> a
> > > single DFL.  Regardless, I think the consistency of just having Offset/BIR
> > > in the VSEC is better than adding more infomation that has little or no
> > > added value.
> >
> > Agreed. Can't you just link the DFLs in that case?
> 
> I didn't see the value of more DFLs in one bar either. So I think we'd better
> document it.

Yes, it needs to be documented well, otherwise users may have their own
choices, e.g. link 100 queues together by modify DFH registers of the
queues one by one, or just have them done together in the VSEC. I am not
sure which one is the easier way for logic developer, but at least we need to
document what driver can support. 

Thanks
Hao

> 
> Thanks,
> Yilun

RE: [PATCH v3 2/2] fpga: dfl: look for vendor specific capability

[matthew.gerlach]

On Wed, 2 Dec 2020, Wu, Hao wrote:

>>>>>
>>>>>> +		}
>>>>>> +
>>>>>> +		offset = dfl_res & PCI_VNDR_DFLS_RES_OFF_MASK;
>>>>>> +		if (offset >= len) {
>>>>>> +			dev_err(&pcidev->dev, "%s bad
>> offset %u >= %pa\n",
>>>>>> +				__func__, offset, &len);
>>>>>> +			return -EINVAL;
>>>>>> +		}
>>>>>> +
>>>>>> +		dev_dbg(&pcidev->dev, "%s BAR %d offset 0x%x\n",
>>>>>> __func__, bar, offset);
>>>>>> +
>>>>>> +		len -= offset;
>>>>>> +
>>>>>> +		start = pci_resource_start(pcidev, bar) + offset;
>>>>>> +
>>>>>> +		dfl_fpga_enum_info_add_dfl(info, start, len);
>>>>>
>>>>> That means everytime, we pass [start, endofbar] region to dfl core
>>>>> for enumeration, if there are multiple DFLs in one bar, then each range
>>>>> ends at the same endofbar, it seems fine as enumeration can be done
>>>>> one by one, but ideally the best case is that this capability can provide
>>>>> end address or size too, right? It is possible that information can be
>>>>> added to the capability as well? then we don't have such limitation.
>>>>>
>>>>> Hao
>>>>
>>>> I am not sure having more than one DFL in a bar serves any purpose over
>> a
>>>> single DFL.  Regardless, I think the consistency of just having Offset/BIR
>>>> in the VSEC is better than adding more infomation that has little or no
>>>> added value.
>>>
>>> Agreed. Can't you just link the DFLs in that case?
>>
>> I didn't see the value of more DFLs in one bar either. So I think we'd better
>> document it.
>
> Yes, it needs to be documented well, otherwise users may have their own
> choices, e.g. link 100 queues together by modify DFH registers of the
> queues one by one, or just have them done together in the VSEC. I am not
> sure which one is the easier way for logic developer, but at least we need to
> document what driver can support.
>
> Thanks
> Hao
>

I will update the documentation to clarify the basis of the rule of one 
DFL per bar.  Since we have a clearly stated rule, we can enforce the rule 
in the code and perform more error checking.

>>
>> Thanks,
>> Yilun
>

RE: [PATCH v3 2/2] fpga: dfl: look for vendor specific capability

[Wu, Hao]

> Subject: Re: [PATCH v3 2/2] fpga: dfl: look for vendor specific capability
> 
> Hi Matthew,
> 
> On Mon, Nov 30, 2020 at 04:45:20PM -0800,
> matthew.gerlach@linux.intel.com wrote:
> >
> >
> > On Sat, 28 Nov 2020, Wu, Hao wrote:
> >
> > > > Subject: [PATCH v3 2/2] fpga: dfl: look for vendor specific capability
> > >
> > > Maybe we can change the title a little bit, what about
> > > fpga: dfl-pci: locate DFLs by PCIe vendor specific capability
> > >
> > > >
> > > > From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > >
> > > > A DFL may not begin at offset 0 of BAR 0.  A PCIe vendor
> > > > specific capability can be used to specify the start of a
> > > > number of DFLs.
> > >
> > > A PCIe vendor specific extended capability is introduced by Intel to
> > > specify the start of a number of DFLs.
> >
> > Your suggestion is more precise.
> > >
> > >
> > > >
> > > > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > > > ---
> > > > v3: Add text and ascii art to documentation.
> > > >     Ensure not to exceed PCIe config space in loop.
> > > >
> > > > v2: Update documentation for clarity.
> > > >     Clean up  macro names.
> > > >     Use GENMASK.
> > > >     Removed spurious blank lines.
> > > >     Changed some calls from dev_info to dev_dbg.
> > > >     Specifically check for VSEC not found, -ENODEV.
> > > >     Ensure correct pci vendor id.
> > > >     Remove check for page alignment.
> > > >     Rename find_dfl_in_cfg to find_dfls_by_vsec.
> > > >     Initialize target memory of pci_read_config_dword to invalid values
> before
> > > > use.
> > > > ---
> > > >  Documentation/fpga/dfl.rst | 25 +++++++++++
> > > >  drivers/fpga/dfl-pci.c     | 91
> +++++++++++++++++++++++++++++++++++++-
> > > >  2 files changed, 115 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
> > > > index 0404fe6ffc74..fa0da884a818 100644
> > > > --- a/Documentation/fpga/dfl.rst
> > > > +++ b/Documentation/fpga/dfl.rst
> > > > @@ -501,6 +501,31 @@ Developer only needs to provide a sub feature
> > > > driver with matched feature id.
> > > >  FME Partial Reconfiguration Sub Feature driver (see drivers/fpga/dfl-
> fme-
> > > > pr.c)
> > > >  could be a reference.
> > > >
> > > > +Location of DFLs on a PCI Device
> > > > +===========================
> > > > +There are two ways of locating DFLs on a PCI Device.  The original
> > >
> > > I found this new VSEC is only for PCIe device, correct? If so, let's make
> > > sure descriptions are accurate. E.g. default method for all devices
> > > and a new method for PCIe device.
> >
> > Yes, the default method can be used with PCI and PCIe device, and the
> VSEC
> > approach is PCIe, only.  Documentation can be made more precise.
> >
> > >
> > > > +method assumed the start of the first DFL to offset 0 of bar 0.
> > > > +If the first node of the DFL is an FME, then further DFLs
> > > > +in the port(s) are specified in FME header registers.
> > > > +Alternatively, a vendor specific capability structure can be used to
> Maybe: a vendor specific extended capability (VSEC) ...
> > > > +specify the location of all the DFLs on the device, providing flexibility
> > > > +for the type of starting node in the DFL.  Intel has reserved the
> > > > +VSEC ID of 0x43 for this purpose.  The vendor specific
> > > > +data begins with a 4 byte vendor specific register for the number of
> DFLs
> > > > followed 4 byte
> > > > +Offset/BIR vendor specific registers for each DFL. Bits 2:0 of Offset/BIR
> > > > register
> > >
> > > Do we have a defined register name here? or it's named as Offset/BIR
> register?
> > > Sounds a little wired, and I see you defined it as DFLS_RES?
> >
> > The Offset/BIR terminology is also used in the MSI-X capability structure.
> 
> Yeah, this intuitively made sense to me having worked with PCIe :)

I just feel that it's better to use the same register name defined in the code
below. So people can find matched information in both code and doc easily. : )

Thanks
Hao

RE: [PATCH v3 2/2] fpga: dfl: look for vendor specific capability

[matthew.gerlach]

On Wed, 2 Dec 2020, Wu, Hao wrote:

>> Subject: Re: [PATCH v3 2/2] fpga: dfl: look for vendor specific capability
>>
>> Hi Matthew,
>>
>> On Mon, Nov 30, 2020 at 04:45:20PM -0800,
>> matthew.gerlach@linux.intel.com wrote:
>>>
>>>
>>> On Sat, 28 Nov 2020, Wu, Hao wrote:
>>>
>>>>> Subject: [PATCH v3 2/2] fpga: dfl: look for vendor specific capability
>>>>
>>>> Maybe we can change the title a little bit, what about
>>>> fpga: dfl-pci: locate DFLs by PCIe vendor specific capability
>>>>
>>>>>
>>>>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>>
>>>>> A DFL may not begin at offset 0 of BAR 0.  A PCIe vendor
>>>>> specific capability can be used to specify the start of a
>>>>> number of DFLs.
>>>>
>>>> A PCIe vendor specific extended capability is introduced by Intel to
>>>> specify the start of a number of DFLs.
>>>
>>> Your suggestion is more precise.
>>>>
>>>>
>>>>>
>>>>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>>>> ---
>>>>> v3: Add text and ascii art to documentation.
>>>>>     Ensure not to exceed PCIe config space in loop.
>>>>>
>>>>> v2: Update documentation for clarity.
>>>>>     Clean up  macro names.
>>>>>     Use GENMASK.
>>>>>     Removed spurious blank lines.
>>>>>     Changed some calls from dev_info to dev_dbg.
>>>>>     Specifically check for VSEC not found, -ENODEV.
>>>>>     Ensure correct pci vendor id.
>>>>>     Remove check for page alignment.
>>>>>     Rename find_dfl_in_cfg to find_dfls_by_vsec.
>>>>>     Initialize target memory of pci_read_config_dword to invalid values
>> before
>>>>> use.
>>>>> ---
>>>>>  Documentation/fpga/dfl.rst | 25 +++++++++++
>>>>>  drivers/fpga/dfl-pci.c     | 91
>> +++++++++++++++++++++++++++++++++++++-
>>>>>  2 files changed, 115 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
>>>>> index 0404fe6ffc74..fa0da884a818 100644
>>>>> --- a/Documentation/fpga/dfl.rst
>>>>> +++ b/Documentation/fpga/dfl.rst
>>>>> @@ -501,6 +501,31 @@ Developer only needs to provide a sub feature
>>>>> driver with matched feature id.
>>>>>  FME Partial Reconfiguration Sub Feature driver (see drivers/fpga/dfl-
>> fme-
>>>>> pr.c)
>>>>>  could be a reference.
>>>>>
>>>>> +Location of DFLs on a PCI Device
>>>>> +===========================
>>>>> +There are two ways of locating DFLs on a PCI Device.  The original
>>>>
>>>> I found this new VSEC is only for PCIe device, correct? If so, let's make
>>>> sure descriptions are accurate. E.g. default method for all devices
>>>> and a new method for PCIe device.
>>>
>>> Yes, the default method can be used with PCI and PCIe device, and the
>> VSEC
>>> approach is PCIe, only.  Documentation can be made more precise.
>>>
>>>>
>>>>> +method assumed the start of the first DFL to offset 0 of bar 0.
>>>>> +If the first node of the DFL is an FME, then further DFLs
>>>>> +in the port(s) are specified in FME header registers.
>>>>> +Alternatively, a vendor specific capability structure can be used to
>> Maybe: a vendor specific extended capability (VSEC) ...
>>>>> +specify the location of all the DFLs on the device, providing flexibility
>>>>> +for the type of starting node in the DFL.  Intel has reserved the
>>>>> +VSEC ID of 0x43 for this purpose.  The vendor specific
>>>>> +data begins with a 4 byte vendor specific register for the number of
>> DFLs
>>>>> followed 4 byte
>>>>> +Offset/BIR vendor specific registers for each DFL. Bits 2:0 of Offset/BIR
>>>>> register
>>>>
>>>> Do we have a defined register name here? or it's named as Offset/BIR
>> register?
>>>> Sounds a little wired, and I see you defined it as DFLS_RES?
>>>
>>> The Offset/BIR terminology is also used in the MSI-X capability structure.
>>
>> Yeah, this intuitively made sense to me having worked with PCIe :)
>
> I just feel that it's better to use the same register name defined in the code
> below. So people can find matched information in both code and doc easily. : )

I think this makes sense.  I can change the name of the variable to bir to 
match the documentation.

>
> Thanks
> Hao
>