[PATCH v8 0/4] Enhance definition of DFH and use enhancements for UART driver

[PATCH v8 0/4] Enhance definition of DFH and use enhancements for UART driver

[matthew.gerlach]

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

This patchset enhances the definition of the Device Feature Header (DFH) used by
the Device Feature List (DFL) bus and then uses the new enhancements in a UART
driver.

The enhancements to the DFH includes the introduction of parameter blocks.
Like PCI capabilities, the DFH parameter blocks further describe
the hardware to software. In the case of the UART, the parameter blocks
provide information for the interrupt, clock frequency, and register layout.

Duplication of code parsing of the parameter blocks in multiple DFL drivers
is a concern. Using swnodes was considered to help minimize parsing code 
duplication, but their use did not help the problem. Furthermore the highly
changeable nature of FPGAs employing the DFL bus makes the use of swnodes
inappropriate. 

Patch 1 updates the DFL documentation to describe the added functionality to DFH.

Patch 2 adds the definitions for DFHv1.

Patch 3 adds basic support for DFHv1. It adds functionality to parse parameter blocks
and adds the functionality to parse the explicit location of a feature's register set.

Patch 4 adds a DFL UART driver that makes use of the new features of DFHv1.

Basheer Ahmed Muddebihal (1):
  fpga: dfl: Add DFHv1 Register Definitions

Matthew Gerlach (3):
  Documentation: fpga: dfl: Add documentation for DFHv1
  fpga: dfl: add basic support for DFHv1
  tty: serial: 8250: add DFL bus driver for Altera 16550.

 Documentation/fpga/dfl.rst         | 112 ++++++++++++
 drivers/fpga/dfl.c                 | 265 +++++++++++++++++++++++------
 drivers/fpga/dfl.h                 |  41 +++++
 drivers/tty/serial/8250/8250_dfl.c | 150 ++++++++++++++++
 drivers/tty/serial/8250/Kconfig    |  12 ++
 drivers/tty/serial/8250/Makefile   |   1 +
 include/linux/dfl.h                |   6 +
 7 files changed, 536 insertions(+), 51 deletions(-)
 create mode 100644 drivers/tty/serial/8250/8250_dfl.c

-- 
2.25.1

[PATCH v8 1/4] Documentation: fpga: dfl: Add documentation for DFHv1

[matthew.gerlach]

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

Add documentation describing the extensions provided by Version
1 of the Device Feature Header (DFHv1).

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
v8: fix section titles

v7: shorten long lines and wording suggestions by bagasdotme@gmail.com

v6: no change

v5: use nested list for field descriptions
    clean up prose
    add reviewed-by and comments from Ilpo Järvinen

v4: Remove marketing speak and separate v0 and v1 descriptions.
    Fix errors reported by "make htmldocs".

v3: no change

v2: s/GUILD/GUID/
    add picture
---
 Documentation/fpga/dfl.rst | 112 +++++++++++++++++++++++++++++++++++++
 1 file changed, 112 insertions(+)

diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
index 15b670926084..264b476fc6ac 100644
--- a/Documentation/fpga/dfl.rst
+++ b/Documentation/fpga/dfl.rst
@@ -561,6 +561,118 @@ new DFL feature via UIO direct access, its feature id should be added to the
 driver's id_table.
 
 
+Device Feature Header - Version 0
+=================================
+Version 0 (DFHv0) is the original version of the Device Feature Header.
+The format of DFHv0 is shown below::
+
+    +-----------------------------------------------------------------------+
+    |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0| 0x00
+    +-----------------------------------------------------------------------+
+    |63                                 GUID_L                             0| 0x08
+    +-----------------------------------------------------------------------+
+    |63                                 GUID_H                             0| 0x10
+    +-----------------------------------------------------------------------+
+
+- Offset 0x00
+
+  * Type - The type of DFH (e.g. FME, AFU, or private feature).
+  * DFH VER - The version of the DFH.
+  * Rsvd - Currently unused.
+  * EOL - Set if the DFH is the end of the Device Feature List (DFL).
+  * Next - The offset of the next DFH in the DFL from the DFH start.
+    If EOL is set, Next is the size of MMIO of the last feature in the list.
+  * ID - The feature ID if Type is private feature.
+
+- Offset 0x08
+
+  * GUID_L - Least significant 64 bits of a 128-bit Globally Unique Identifier
+    (present only if Type is FME or AFU).
+
+- Offset 0x10
+
+  * GUID_H - Most significant 64 bits of a 128-bit Globally Unique Identifier
+    (present only if Type is FME or AFU).
+
+
+Device Feature Header - Version 1
+=================================
+Version 1 (DFHv1) of the Device Feature Header adds the following functionality:
+
+* Provides a standardized mechanism for features to describe
+  parameters/capabilities to software.
+* Standardize the use of a GUID for all DFHv1 types.
+* Decouples the DFH location from the register space of the feature itself.
+
+The format of Version 1 of the Device Feature Header (DFH) is shown below::
+
+    +-----------------------------------------------------------------------+
+    |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0| 0x00
+    +-----------------------------------------------------------------------+
+    |63                                 GUID_L                             0| 0x08
+    +-----------------------------------------------------------------------+
+    |63                                 GUID_H                             0| 0x10
+    +-----------------------------------------------------------------------+
+    |63                   Reg Address/Offset                      1|  Rel  0| 0x18
+    +-----------------------------------------------------------------------+
+    |63        Reg Size       32|Params 31|30 Group    16|15 Instance      0| 0x20
+    +-----------------------------------------------------------------------+
+    |63 Next    35|34RSV33|EOP32|31 Param Version 16|15 Param ID           0| 0x28
+    +-----------------------------------------------------------------------+
+    |63                 Parameter Data                                     0| 0x30
+    +-----------------------------------------------------------------------+
+
+                                  ...
+
+    +-----------------------------------------------------------------------+
+    |63 Next    35|34RSV33|EOP32|31 Param Version 16|15 Param ID           0|
+    +-----------------------------------------------------------------------+
+    |63                 Parameter Data                                     0|
+    +-----------------------------------------------------------------------+
+
+- Offset 0x00
+
+  * Type - The type of DFH (e.g. FME, AFU, or private feature).
+  * DFH VER - The version of the DFH.
+  * Rsvd - Currently unused.
+  * EOL - Set if the DFH is the end of the Device Feature List (DFL).
+  * Next - The offset of the next DFH in the DFL from the DFH start.
+    If EOL is set, Next is the size of MMIO of the last feature in the list.
+  * ID - The feature ID if Type is private feature.
+
+- Offset 0x08
+
+  * GUID_L - Least significant 64 bits of a 128-bit Globally Unique Identifier.
+
+- Offset 0x10
+
+  * GUID_H - Most significant 64 bits of a 128-bit Globally Unique Identifier.
+
+- Offset 0x18
+
+  * Reg Address/Offset - If Rel bit is set, then the value is the high 63 bits
+    of a 16-bit aligned absolute address of the feature's registers. Otherwise
+    the value is the offset from the start of the DFH of the feature's registers.
+
+- Offset 0x20
+
+  * Reg Size - Size of feature's register set in bytes.
+  * Params - Set if DFH has a list of parameter blocks.
+  * Group - Id of group if feature is part of a group.
+  * Instance - Id of feature instance within a group.
+
+- Offset 0x28 if feature has parameters
+
+  * Next - Offset to the next parameter block in 8 byte words. If EOP set,
+    size in 8 byte words of last parameter.
+  * Param Version - Version of Param ID.
+  * Param ID - ID of parameter.
+
+- Offset 0x30
+
+  * Parameter Data - Parameter data whose size and format is defined by
+    version and ID of the parameter.
+
 Open discussion
 ===============
 FME driver exports one ioctl (DFL_FPGA_FME_PORT_PR) for partial reconfiguration
-- 
2.25.1

[PATCH v8 2/4] fpga: dfl: Add DFHv1 Register Definitions

[matthew.gerlach]

From: Basheer Ahmed Muddebihal <basheer.ahmed.muddebihal@linux.intel.com>

This patch adds the definitions for DFHv1 header and related register
bitfields.

Signed-off-by: Basheer Ahmed Muddebihal <basheer.ahmed.muddebihal@linux.intel.com>
Co-developed-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
v8: add Rb Andy Shevchenko

v7: no change

v6: remove parameter definitions from include/linux/dfl.h

v5: consistently use fields for parameter data
    s/EOL/EOP/ to match doc
    remove unneeded mask
    added Co-developed-by

v4: s/MSIX/MSI_X/g
    move kerneldoc to implementation
    don't change copyright date

v3:
    keep DFHv1 definitions "hidden" in drivers/fpga/dfl.h

v2: clean up white space and one line comments
    remove extra space in commit
    use uniform number of digits in constants
    don't change copyright date because of removed content
---
 drivers/fpga/dfl.h | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index 06cfcd5e84bb..fc59f33367ee 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -74,11 +74,43 @@
 #define DFH_REVISION		GENMASK_ULL(15, 12)	/* Feature revision */
 #define DFH_NEXT_HDR_OFST	GENMASK_ULL(39, 16)	/* Offset to next DFH */
 #define DFH_EOL			BIT_ULL(40)		/* End of list */
+#define DFH_VERSION		GENMASK_ULL(59, 52)	/* DFH version */
 #define DFH_TYPE		GENMASK_ULL(63, 60)	/* Feature type */
 #define DFH_TYPE_AFU		1
 #define DFH_TYPE_PRIVATE	3
 #define DFH_TYPE_FIU		4
 
+/*
+ * DFHv1 Register Offset definitons
+ * In DHFv1, DFH + GUID + CSR_START + CSR_SIZE_GROUP + PARAM_HDR + PARAM_DATA
+ * as common header registers
+ */
+#define DFHv1_CSR_ADDR		0x18  /* CSR Register start address */
+#define DFHv1_CSR_SIZE_GRP	0x20  /* Size of Reg Block and Group/tag */
+#define DFHv1_PARAM_HDR		0x28  /* Optional First Param header */
+
+/*
+ * CSR Rel Bit, 1'b0 = relative (offset from feature DFH start),
+ * 1'b1 = absolute (ARM or other non-PCIe use)
+ */
+#define DFHv1_CSR_ADDR_REL	BIT_ULL(0)
+
+/* CSR Header Register Bit Definitions */
+#define DFHv1_CSR_ADDR_MASK       GENMASK_ULL(63, 1)  /* 63:1 of CSR address */
+
+/* CSR SIZE Goup Register Bit Definitions */
+#define DFHv1_CSR_SIZE_GRP_INSTANCE_ID	GENMASK_ULL(15, 0)	/* Enumeration instantiated IP */
+#define DFHv1_CSR_SIZE_GRP_GROUPING_ID	GENMASK_ULL(30, 16)	/* Group Features/interfaces */
+#define DFHv1_CSR_SIZE_GRP_HAS_PARAMS	BIT_ULL(31)		/* Presence of Parameters */
+#define DFHv1_CSR_SIZE_GRP_SIZE		GENMASK_ULL(63, 32)	/* Size of CSR Block in bytes */
+
+/* PARAM Header Register Bit Definitions */
+#define DFHv1_PARAM_HDR_ID		GENMASK_ULL(15, 0) /* Id of this Param  */
+#define DFHv1_PARAM_HDR_VER		GENMASK_ULL(31, 16) /* Version Param */
+#define DFHv1_PARAM_HDR_NEXT_OFFSET	GENMASK_ULL(63, 35) /* Offset of next Param */
+#define DFHv1_PARAM_HDR_NEXT_EOP	BIT_ULL(32)
+#define DFHv1_PARAM_DATA		0x08  /* Offset of Param data from Param header */
+
 /* Next AFU Register Bitfield */
 #define NEXT_AFU_NEXT_DFH_OFST	GENMASK_ULL(23, 0)	/* Offset to next AFU */
 
-- 
2.25.1

[PATCH v8 3/4] fpga: dfl: add basic support for DFHv1

[matthew.gerlach]

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

Version 1 of the Device Feature Header (DFH) definition adds
functionality to the DFL bus.

A DFHv1 header may have one or more parameter blocks that
further describes the HW to SW.  Add support to the DFL bus
to parse the MSI-X parameter.

The location of a feature's register set is explicitly
described in DFHv1 and can be relative to the base of the DFHv1
or an absolute address.  Parse the location and pass the information
to DFL driver.

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
v8: use struct_size() from overflow.h
    add dfh_get_u64_param_vals()

v7: no change

v6: move MSI_X parameter definitions to drivers/fpga/dfl.h

v5: update field names
    fix find_param/dfh_get_psize
    clean up mmio_res assignments
    use u64* instead of void*
    use FIELD_GET instead of masking

v4: s/MSIX/MSI_X
    move kernel doc to implementation
    use structure assignment
    fix decode of absolute address
    clean up comment in parse_feature_irqs
    remove use of csr_res

v3: remove unneeded blank line
    use clearer variable name
    pass finfo into parse_feature_irqs()
    refactor code for better indentation
    use switch statement for irq parsing
    squash in code parsing register location

v2: fix kernel doc
    clarify use of DFH_VERSION field
---
 drivers/fpga/dfl.c  | 265 +++++++++++++++++++++++++++++++++++---------
 drivers/fpga/dfl.h  |   9 ++
 include/linux/dfl.h |   6 +
 3 files changed, 229 insertions(+), 51 deletions(-)

diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index b9aae85ba930..403fa425c1fe 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -13,6 +13,7 @@
 #include <linux/dfl.h>
 #include <linux/fpga-dfl.h>
 #include <linux/module.h>
+#include <linux/overflow.h>
 #include <linux/uaccess.h>
 
 #include "dfl.h"
@@ -342,6 +343,8 @@ static void release_dfl_dev(struct device *dev)
 	if (ddev->mmio_res.parent)
 		release_resource(&ddev->mmio_res);
 
+	kfree(ddev->params);
+
 	ida_free(&dfl_device_ida, ddev->id);
 	kfree(ddev->irqs);
 	kfree(ddev);
@@ -380,7 +383,16 @@ dfl_dev_add(struct dfl_feature_platform_data *pdata,
 	ddev->type = feature_dev_id_type(pdev);
 	ddev->feature_id = feature->id;
 	ddev->revision = feature->revision;
+	ddev->dfh_version = feature->dfh_version;
 	ddev->cdev = pdata->dfl_cdev;
+	if (feature->param_size) {
+		ddev->params = kmemdup(feature->params, feature->param_size, GFP_KERNEL);
+		if (!ddev->params) {
+			ret = -ENOMEM;
+			goto put_dev;
+		}
+		ddev->param_size = feature->param_size;
+	}
 
 	/* add mmio resource */
 	parent_res = &pdev->resource[feature->resource_index];
@@ -708,20 +720,27 @@ struct build_feature_devs_info {
  * struct dfl_feature_info - sub feature info collected during feature dev build
  *
  * @fid: id of this sub feature.
+ * @revision: revision of this sub feature
+ * @dfh_version: version of Device Feature Header (DFH)
  * @mmio_res: mmio resource of this sub feature.
  * @ioaddr: mapped base address of mmio resource.
  * @node: node in sub_features linked list.
  * @irq_base: start of irq index in this sub feature.
  * @nr_irqs: number of irqs of this sub feature.
+ * @param_size: size DFH parameters.
+ * @params: DFH parameter data.
  */
 struct dfl_feature_info {
 	u16 fid;
 	u8 revision;
+	u8 dfh_version;
 	struct resource mmio_res;
 	void __iomem *ioaddr;
 	struct list_head node;
 	unsigned int irq_base;
 	unsigned int nr_irqs;
+	unsigned int param_size;
+	u64 params[];
 };
 
 static void dfl_fpga_cdev_add_port_dev(struct dfl_fpga_cdev *cdev,
@@ -797,7 +816,17 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
 		feature->dev = fdev;
 		feature->id = finfo->fid;
 		feature->revision = finfo->revision;
+		feature->dfh_version = finfo->dfh_version;
 
+		if (finfo->param_size) {
+			feature->params = devm_kmemdup(binfo->dev,
+						       finfo->params, finfo->param_size,
+						       GFP_KERNEL);
+			if (!feature->params)
+				return -ENOMEM;
+
+			feature->param_size = finfo->param_size;
+		}
 		/*
 		 * the FIU header feature has some fundamental functions (sriov
 		 * set, port enable/disable) needed for the dfl bus device and
@@ -934,56 +963,135 @@ static u16 feature_id(u64 value)
 	return 0;
 }
 
+static u64 *find_param(u64 *params, resource_size_t max, int param_id)
+{
+	u64 *end = params + max / sizeof(u64);
+	u64 v, next;
+
+	while (params < end) {
+		v = *params;
+		if (param_id == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
+			return params;
+
+		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
+		params += next;
+		if (FIELD_GET(DFHv1_PARAM_HDR_NEXT_EOP, v))
+			break;
+	}
+
+	return NULL;
+}
+
+/**
+ * dfh_find_param() - find parameter block for the given parameter id
+ * @dfl_dev: dfl device
+ * @param: id of dfl parameter
+ *
+ * Return: pointer to start of parameter block, NULL otherwise.
+ */
+u64 *dfh_find_param(struct dfl_device *dfl_dev, int param_id)
+{
+	return find_param(dfl_dev->params, dfl_dev->param_size, param_id);
+}
+EXPORT_SYMBOL_GPL(dfh_find_param);
+
+/**
+ * dfh_get_u64_param_vals() - get array of u64 param values for given parameter id
+ * @dfl_dev: dfl device
+ * @param: id of dfl parameter
+ * @pval: location of parameter data destination
+ * @nvals: number of u64 elements of parameter data
+ *
+ * Return: pointer to start of parameter block, PTR_ERR otherwise
+ */
+u64 *dfh_get_u64_param_vals(struct dfl_device *dfl_dev, int param_id, u64 *pval, int nvals)
+{
+	u64 *param = find_param(dfl_dev->params, dfl_dev->param_size, param_id);
+	u64 next;
+	int i;
+
+	if (!param)
+		return ERR_PTR(-ENOENT);
+
+	next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, *param);
+
+	if (nvals >= next)
+		return ERR_PTR(-ENOMEM);
+
+	for (i = 0; i < nvals; i++)
+		*pval++ = param[i + 1];
+
+	return param;
+}
+EXPORT_SYMBOL_GPL(dfh_get_u64_param_vals);
+
 static int parse_feature_irqs(struct build_feature_devs_info *binfo,
-			      resource_size_t ofst, u16 fid,
-			      unsigned int *irq_base, unsigned int *nr_irqs)
+			      resource_size_t ofst, struct dfl_feature_info *finfo)
 {
 	void __iomem *base = binfo->ioaddr + ofst;
 	unsigned int i, ibase, inr = 0;
+	void *params = finfo->params;
 	enum dfl_id_type type;
+	u16 fid = finfo->fid;
 	int virq;
+	u64 *p;
 	u64 v;
 
-	type = feature_dev_id_type(binfo->feature_dev);
+	switch (finfo->dfh_version) {
+	case 0:
+		/*
+		 * DFHv0 only provides MMIO resource information for each feature
+		 * in the DFL header.  There is no generic interrupt information.
+		 * Instead, features with interrupt functionality provide
+		 * the information in feature specific registers.
+		 */
+		type = feature_dev_id_type(binfo->feature_dev);
+		if (type == PORT_ID) {
+			switch (fid) {
+			case PORT_FEATURE_ID_UINT:
+				v = readq(base + PORT_UINT_CAP);
+				ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
+				inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
+				break;
+			case PORT_FEATURE_ID_ERROR:
+				v = readq(base + PORT_ERROR_CAP);
+				ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
+				inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
+				break;
+			}
+		} else if (type == FME_ID) {
+			switch (fid) {
+			case FME_FEATURE_ID_GLOBAL_ERR:
+				v = readq(base + FME_ERROR_CAP);
+				ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
+				inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
+				break;
+			}
+		}
+		break;
 
-	/*
-	 * Ideally DFL framework should only read info from DFL header, but
-	 * current version DFL only provides mmio resources information for
-	 * each feature in DFL Header, no field for interrupt resources.
-	 * Interrupt resource information is provided by specific mmio
-	 * registers of each private feature which supports interrupt. So in
-	 * order to parse and assign irq resources, DFL framework has to look
-	 * into specific capability registers of these private features.
-	 *
-	 * Once future DFL version supports generic interrupt resource
-	 * information in common DFL headers, the generic interrupt parsing
-	 * code will be added. But in order to be compatible to old version
-	 * DFL, the driver may still fall back to these quirks.
-	 */
-	if (type == PORT_ID) {
-		switch (fid) {
-		case PORT_FEATURE_ID_UINT:
-			v = readq(base + PORT_UINT_CAP);
-			ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
-			inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
-			break;
-		case PORT_FEATURE_ID_ERROR:
-			v = readq(base + PORT_ERROR_CAP);
-			ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
-			inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
+	case 1:
+		/*
+		 * DFHv1 provides interrupt resource information in DFHv1
+		 * parameter blocks.
+		 */
+		p = find_param(params, finfo->param_size, DFHv1_PARAM_ID_MSI_X);
+		if (!p)
 			break;
-		}
-	} else if (type == FME_ID) {
-		if (fid == FME_FEATURE_ID_GLOBAL_ERR) {
-			v = readq(base + FME_ERROR_CAP);
-			ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
-			inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
-		}
+
+		p++;
+		ibase = FIELD_GET(DFHv1_PARAM_MSI_X_STARTV, *p);
+		inr = FIELD_GET(DFHv1_PARAM_MSI_X_NUMV, *p);
+		break;
+
+	default:
+		dev_warn(binfo->dev, "unexpected DFH version %d\n", finfo->dfh_version);
+		break;
 	}
 
 	if (!inr) {
-		*irq_base = 0;
-		*nr_irqs = 0;
+		finfo->irq_base = 0;
+		finfo->nr_irqs = 0;
 		return 0;
 	}
 
@@ -1006,12 +1114,37 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
 		}
 	}
 
-	*irq_base = ibase;
-	*nr_irqs = inr;
+	finfo->irq_base = ibase;
+	finfo->nr_irqs = inr;
 
 	return 0;
 }
 
+static int dfh_get_psize(void __iomem *dfh_base, resource_size_t max)
+{
+	int size = 0;
+	u64 v, next;
+
+	if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS,
+		       readq(dfh_base + DFHv1_CSR_SIZE_GRP)))
+		return 0;
+
+	while (size + DFHv1_PARAM_HDR < max) {
+		v = readq(dfh_base + DFHv1_PARAM_HDR + size);
+
+		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
+		if (!next)
+			return -EINVAL;
+
+		size += next * sizeof(u64);
+
+		if (FIELD_GET(DFHv1_PARAM_HDR_NEXT_EOP, v))
+			return size;
+	}
+
+	return -ENOENT;
+}
+
 /*
  * when create sub feature instances, for private features, it doesn't need
  * to provide resource size and feature id as they could be read from DFH
@@ -1023,39 +1156,69 @@ static int
 create_feature_instance(struct build_feature_devs_info *binfo,
 			resource_size_t ofst, resource_size_t size, u16 fid)
 {
-	unsigned int irq_base, nr_irqs;
 	struct dfl_feature_info *finfo;
+	resource_size_t start, end;
+	int dfh_psize = 0;
 	u8 revision = 0;
+	u64 v, addr_off;
+	u8 dfh_ver = 0;
 	int ret;
-	u64 v;
 
 	if (fid != FEATURE_ID_AFU) {
 		v = readq(binfo->ioaddr + ofst);
 		revision = FIELD_GET(DFH_REVISION, v);
-
+		dfh_ver = FIELD_GET(DFH_VERSION, v);
 		/* read feature size and id if inputs are invalid */
 		size = size ? size : feature_size(v);
 		fid = fid ? fid : feature_id(v);
+		if (dfh_ver == 1) {
+			dfh_psize = dfh_get_psize(binfo->ioaddr + ofst, size);
+			if (dfh_psize < 0) {
+				dev_err(binfo->dev,
+					"failed to read size of DFHv1 parameters %d\n",
+					dfh_psize);
+				return dfh_psize;
+			}
+			dev_dbg(binfo->dev, "dfhv1_psize %d\n", dfh_psize);
+		}
 	}
 
 	if (binfo->len - ofst < size)
 		return -EINVAL;
 
-	ret = parse_feature_irqs(binfo, ofst, fid, &irq_base, &nr_irqs);
-	if (ret)
-		return ret;
-
-	finfo = kzalloc(sizeof(*finfo), GFP_KERNEL);
+	finfo = kzalloc(struct_size(finfo, params, dfh_psize/sizeof(u64)), GFP_KERNEL);
 	if (!finfo)
 		return -ENOMEM;
 
+	memcpy_fromio(finfo->params, binfo->ioaddr + ofst + DFHv1_PARAM_HDR, dfh_psize);
+	finfo->param_size = dfh_psize;
+
 	finfo->fid = fid;
 	finfo->revision = revision;
-	finfo->mmio_res.start = binfo->start + ofst;
-	finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
+	finfo->dfh_version = dfh_ver;
+	if (dfh_ver == 1) {
+		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
+		addr_off = FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
+		if (FIELD_GET(DFHv1_CSR_ADDR_REL, v))
+			start = addr_off << 1;
+		else
+			start = binfo->start + ofst + addr_off;
+
+		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
+		end = start + FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) - 1;
+	} else {
+		start = binfo->start + ofst;
+		end = start + size - 1;
+	}
 	finfo->mmio_res.flags = IORESOURCE_MEM;
-	finfo->irq_base = irq_base;
-	finfo->nr_irqs = nr_irqs;
+	finfo->mmio_res.start = start;
+	finfo->mmio_res.end = end;
+
+	ret = parse_feature_irqs(binfo, ofst, finfo);
+	if (ret) {
+		kfree(finfo);
+		return ret;
+	}
 
 	list_add_tail(&finfo->node, &binfo->sub_features);
 	binfo->feature_num++;
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index fc59f33367ee..a21065cec04e 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -111,6 +111,10 @@
 #define DFHv1_PARAM_HDR_NEXT_EOP	BIT_ULL(32)
 #define DFHv1_PARAM_DATA		0x08  /* Offset of Param data from Param header */
 
+#define DFHv1_PARAM_ID_MSI_X		0x1
+#define DFHv1_PARAM_MSI_X_NUMV		GENMASK_ULL(63, 32)
+#define DFHv1_PARAM_MSI_X_STARTV	GENMASK_ULL(31, 0)
+
 /* Next AFU Register Bitfield */
 #define NEXT_AFU_NEXT_DFH_OFST	GENMASK_ULL(23, 0)	/* Offset to next AFU */
 
@@ -272,11 +276,14 @@ struct dfl_feature_irq_ctx {
  * @ops: ops of this sub feature.
  * @ddev: ptr to the dfl device of this sub feature.
  * @priv: priv data of this feature.
+ * @param_size: size of dfh parameters
+ * @params: point to memory copy of dfh parameters
  */
 struct dfl_feature {
 	struct platform_device *dev;
 	u16 id;
 	u8 revision;
+	u8 dfh_version;
 	int resource_index;
 	void __iomem *ioaddr;
 	struct dfl_feature_irq_ctx *irq_ctx;
@@ -284,6 +291,8 @@ struct dfl_feature {
 	const struct dfl_feature_ops *ops;
 	struct dfl_device *ddev;
 	void *priv;
+	unsigned int param_size;
+	void *params;
 };
 
 #define FEATURE_DEV_ID_UNUSED	(-1)
diff --git a/include/linux/dfl.h b/include/linux/dfl.h
index 431636a0dc78..3138ea42d02e 100644
--- a/include/linux/dfl.h
+++ b/include/linux/dfl.h
@@ -39,9 +39,12 @@ struct dfl_device {
 	u16 type;
 	u16 feature_id;
 	u8 revision;
+	u8 dfh_version;
 	struct resource mmio_res;
 	int *irqs;
 	unsigned int num_irqs;
+	unsigned int param_size;
+	void *params;
 	struct dfl_fpga_cdev *cdev;
 	const struct dfl_device_id *id_entry;
 };
@@ -84,4 +87,7 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv);
 	module_driver(__dfl_driver, dfl_driver_register, \
 		      dfl_driver_unregister)
 
+u64 *dfh_find_param(struct dfl_device *dfl_dev, int param);
+
+u64 *dfh_get_u64_param_vals(struct dfl_device *dfl_dev, int param_id, u64 *pval, int nvals);
 #endif /* __LINUX_DFL_H */
-- 
2.25.1

[PATCH v8 4/4] tty: serial: 8250: add DFL bus driver for Altera 16550.

[matthew.gerlach]

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

Add a Device Feature List (DFL) bus driver for the Altera
16550 implementation of UART.

Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
v8: use dfh_get_u64_param_vals()

v7: no change

v6: move driver specific parameter definitions to limit scope

v5: removed unneeded blank line
    removed unneeded includes
    included device.h and types.h
    removed unneeded local variable
    remove calls to dev_dbg
    memset -> { }
    remove space after period
    explicitly include used headers
    remove redundant Inc from Copyright
    fix format specifier

v4: use dev_err_probe() everywhere that is appropriate
    clean up noise
    change error messages to use the word, unsupported
    tried again to sort Makefile and KConfig better
    reorder probe function for easier error handling
    use new dfh_find_param API

v3: use passed in location of registers
    use cleaned up functions for parsing parameters

v2: clean up error messages
    alphabetize header files
    fix 'missing prototype' error by making function static
    tried to sort Makefile and Kconfig better
---
 drivers/tty/serial/8250/8250_dfl.c | 150 +++++++++++++++++++++++++++++
 drivers/tty/serial/8250/Kconfig    |  12 +++
 drivers/tty/serial/8250/Makefile   |   1 +
 3 files changed, 163 insertions(+)
 create mode 100644 drivers/tty/serial/8250/8250_dfl.c

diff --git a/drivers/tty/serial/8250/8250_dfl.c b/drivers/tty/serial/8250/8250_dfl.c
new file mode 100644
index 000000000000..0ee78c38a721
--- /dev/null
+++ b/drivers/tty/serial/8250/8250_dfl.c
@@ -0,0 +1,150 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for FPGA UART
+ *
+ * Copyright (C) 2022 Intel Corporation.
+ *
+ * Authors:
+ *   Ananda Ravuri <ananda.ravuri@intel.com>
+ *   Matthew Gerlach <matthew.gerlach@linux.intel.com>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/device.h>
+#include <linux/dfl.h>
+#include <linux/errno.h>
+#include <linux/ioport.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/types.h>
+
+#include <linux/serial.h>
+#include <linux/serial_8250.h>
+
+#define DFHv1_PARAM_ID_CLK_FRQ    0x2
+#define DFHv1_PARAM_ID_FIFO_LEN   0x3
+
+#define DFHv1_PARAM_ID_REG_LAYOUT	0x4
+#define DFHv1_PARAM_REG_LAYOUT_WIDTH	GENMASK_ULL(63, 32)
+#define DFHv1_PARAM_REG_LAYOUT_SHIFT	GENMASK_ULL(31, 0)
+
+struct dfl_uart {
+	int line;
+};
+
+static int dfl_uart_get_params(struct dfl_device *dfl_dev, struct uart_8250_port *uart)
+{
+	struct device *dev = &dfl_dev->dev;
+	u64 fifo_len, clk_freq, reg_layout;
+	u32 reg_width;
+	u64 *p;
+
+	p = dfh_get_u64_param_vals(dfl_dev, DFHv1_PARAM_ID_CLK_FRQ, &clk_freq, 1);
+	if (IS_ERR(p))
+		return dev_err_probe(dev, PTR_ERR(p), "missing CLK_FRQ param\n");
+
+	uart->port.uartclk = clk_freq;
+
+	p = dfh_get_u64_param_vals(dfl_dev, DFHv1_PARAM_ID_FIFO_LEN, &fifo_len, 1);
+	if (IS_ERR(p))
+		return dev_err_probe(dev, PTR_ERR(p), "missing FIFO_LEN param\n");
+
+	switch (fifo_len) {
+	case 32:
+		uart->port.type = PORT_ALTR_16550_F32;
+		break;
+
+	case 64:
+		uart->port.type = PORT_ALTR_16550_F64;
+		break;
+
+	case 128:
+		uart->port.type = PORT_ALTR_16550_F128;
+		break;
+
+	default:
+		return dev_err_probe(dev, -EINVAL, "unsupported FIFO_LEN %llu\n", fifo_len);
+	}
+
+	p = dfh_get_u64_param_vals(dfl_dev, DFHv1_PARAM_ID_REG_LAYOUT, &reg_layout, 1);
+	if (IS_ERR(p))
+		return dev_err_probe(dev, PTR_ERR(p), "missing REG_LAYOUT param\n");
+
+	uart->port.regshift = FIELD_GET(DFHv1_PARAM_REG_LAYOUT_SHIFT, reg_layout);
+	reg_width = FIELD_GET(DFHv1_PARAM_REG_LAYOUT_WIDTH, reg_layout);
+	switch (reg_width) {
+	case 4:
+		uart->port.iotype = UPIO_MEM32;
+		break;
+
+	case 2:
+		uart->port.iotype = UPIO_MEM16;
+		break;
+
+	default:
+		return dev_err_probe(dev, -EINVAL, "unsupported reg-width %u\n", reg_width);
+
+	}
+
+	return 0;
+}
+
+static int dfl_uart_probe(struct dfl_device *dfl_dev)
+{
+	struct device *dev = &dfl_dev->dev;
+	struct uart_8250_port uart = { };
+	struct dfl_uart *dfluart;
+	int ret;
+
+	uart.port.flags = UPF_IOREMAP;
+	uart.port.mapbase = dfl_dev->mmio_res.start;
+	uart.port.mapsize = resource_size(&dfl_dev->mmio_res);
+
+	ret = dfl_uart_get_params(dfl_dev, &uart);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "failed uart feature walk\n");
+
+	if (dfl_dev->num_irqs == 1)
+		uart.port.irq = dfl_dev->irqs[0];
+
+	dfluart = devm_kzalloc(dev, sizeof(*dfluart), GFP_KERNEL);
+	if (!dfluart)
+		return -ENOMEM;
+
+	dfluart->line = serial8250_register_8250_port(&uart);
+	if (dfluart->line < 0)
+		return dev_err_probe(dev, dfluart->line, "unable to register 8250 port.\n");
+
+	dev_set_drvdata(dev, dfluart);
+
+	return 0;
+}
+
+static void dfl_uart_remove(struct dfl_device *dfl_dev)
+{
+	struct dfl_uart *dfluart = dev_get_drvdata(&dfl_dev->dev);
+
+	serial8250_unregister_port(dfluart->line);
+}
+
+#define FME_FEATURE_ID_UART 0x24
+
+static const struct dfl_device_id dfl_uart_ids[] = {
+	{ FME_ID, FME_FEATURE_ID_UART },
+	{ }
+};
+MODULE_DEVICE_TABLE(dfl, dfl_uart_ids);
+
+static struct dfl_driver dfl_uart_driver = {
+	.drv = {
+		.name = "dfl-uart",
+	},
+	.id_table = dfl_uart_ids,
+	.probe = dfl_uart_probe,
+	.remove = dfl_uart_remove,
+};
+module_dfl_driver(dfl_uart_driver);
+
+MODULE_DESCRIPTION("DFL Intel UART driver");
+MODULE_AUTHOR("Intel Corporation");
+MODULE_LICENSE("GPL");
diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
index b0f62345bc84..08af2acd4645 100644
--- a/drivers/tty/serial/8250/Kconfig
+++ b/drivers/tty/serial/8250/Kconfig
@@ -370,6 +370,18 @@ config SERIAL_8250_FSL
 	  erratum for Freescale 16550 UARTs in the 8250 driver. It also
 	  enables support for ACPI enumeration.
 
+config SERIAL_8250_DFL
+	tristate "DFL bus driver for Altera 16550 UART"
+	depends on SERIAL_8250 && FPGA_DFL
+	help
+	  This option enables support for a Device Feature List (DFL) bus
+	  driver for the Altera 16650 UART. One or more Altera 16650 UARTs
+	  can be instantiated in a FPGA and then be discovered during
+	  enumeration of the DFL bus.
+
+	  To compile this driver as a module, chose M here: the
+	  module will be called 8250_dfl.
+
 config SERIAL_8250_DW
 	tristate "Support for Synopsys DesignWare 8250 quirks"
 	depends on SERIAL_8250
diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
index 1615bfdde2a0..4e1a32812683 100644
--- a/drivers/tty/serial/8250/Makefile
+++ b/drivers/tty/serial/8250/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_SERIAL_8250_EXAR_ST16C554)	+= 8250_exar_st16c554.o
 obj-$(CONFIG_SERIAL_8250_HUB6)		+= 8250_hub6.o
 obj-$(CONFIG_SERIAL_8250_FSL)		+= 8250_fsl.o
 obj-$(CONFIG_SERIAL_8250_MEN_MCB)	+= 8250_men_mcb.o
+obj-$(CONFIG_SERIAL_8250_DFL)		+= 8250_dfl.o
 obj-$(CONFIG_SERIAL_8250_DW)		+= 8250_dw.o
 obj-$(CONFIG_SERIAL_8250_EM)		+= 8250_em.o
 obj-$(CONFIG_SERIAL_8250_IOC3)		+= 8250_ioc3.o
-- 
2.25.1

Re: [PATCH v8 3/4] fpga: dfl: add basic support for DFHv1

[Andy Shevchenko]

On Wed, Dec 28, 2022 at 10:16:23AM -0800, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Version 1 of the Device Feature Header (DFH) definition adds
> functionality to the DFL bus.
> 
> A DFHv1 header may have one or more parameter blocks that
> further describes the HW to SW.  Add support to the DFL bus

Single space is enough.

> to parse the MSI-X parameter.
> 
> The location of a feature's register set is explicitly
> described in DFHv1 and can be relative to the base of the DFHv1
> or an absolute address.  Parse the location and pass the information
> to DFL driver.

I'm wondering what DFL states for.

...

> +/**
> + * dfh_get_u64_param_vals() - get array of u64 param values for given parameter id
> + * @dfl_dev: dfl device
> + * @param: id of dfl parameter
> + * @pval: location of parameter data destination
> + * @nvals: number of u64 elements of parameter data
> + *
> + * Return: pointer to start of parameter block, PTR_ERR otherwise
> + */
> +u64 *dfh_get_u64_param_vals(struct dfl_device *dfl_dev, int param_id, u64 *pval, int nvals)
> +{
> +	u64 *param = find_param(dfl_dev->params, dfl_dev->param_size, param_id);
> +	u64 next;
> +	int i;
> +
> +	if (!param)
> +		return ERR_PTR(-ENOENT);
> +
> +	next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, *param);
> +
> +	if (nvals >= next)
> +		return ERR_PTR(-ENOMEM);

ENODATA ?

> +	for (i = 0; i < nvals; i++)
> +		*pval++ = param[i + 1];

memcpy() ?

> +	return param;
> +}

...

> +	finfo = kzalloc(struct_size(finfo, params, dfh_psize/sizeof(u64)), GFP_KERNEL);

' / ' (mind the spaces)

Also, perhaps better to use sizeof(*params) or what is the member of that
structure. So it will be more robust against possible changes.

>  	if (!finfo)
>  		return -ENOMEM;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 4/4] tty: serial: 8250: add DFL bus driver for Altera 16550.

[Andy Shevchenko]

On Wed, Dec 28, 2022 at 10:16:24AM -0800, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Add a Device Feature List (DFL) bus driver for the Altera
> 16550 implementation of UART.

With that helper looks better,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
> v8: use dfh_get_u64_param_vals()
> 
> v7: no change
> 
> v6: move driver specific parameter definitions to limit scope
> 
> v5: removed unneeded blank line
>     removed unneeded includes
>     included device.h and types.h
>     removed unneeded local variable
>     remove calls to dev_dbg
>     memset -> { }
>     remove space after period
>     explicitly include used headers
>     remove redundant Inc from Copyright
>     fix format specifier
> 
> v4: use dev_err_probe() everywhere that is appropriate
>     clean up noise
>     change error messages to use the word, unsupported
>     tried again to sort Makefile and KConfig better
>     reorder probe function for easier error handling
>     use new dfh_find_param API
> 
> v3: use passed in location of registers
>     use cleaned up functions for parsing parameters
> 
> v2: clean up error messages
>     alphabetize header files
>     fix 'missing prototype' error by making function static
>     tried to sort Makefile and Kconfig better
> ---
>  drivers/tty/serial/8250/8250_dfl.c | 150 +++++++++++++++++++++++++++++
>  drivers/tty/serial/8250/Kconfig    |  12 +++
>  drivers/tty/serial/8250/Makefile   |   1 +
>  3 files changed, 163 insertions(+)
>  create mode 100644 drivers/tty/serial/8250/8250_dfl.c
> 
> diff --git a/drivers/tty/serial/8250/8250_dfl.c b/drivers/tty/serial/8250/8250_dfl.c
> new file mode 100644
> index 000000000000..0ee78c38a721
> --- /dev/null
> +++ b/drivers/tty/serial/8250/8250_dfl.c
> @@ -0,0 +1,150 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for FPGA UART
> + *
> + * Copyright (C) 2022 Intel Corporation.
> + *
> + * Authors:
> + *   Ananda Ravuri <ananda.ravuri@intel.com>
> + *   Matthew Gerlach <matthew.gerlach@linux.intel.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/device.h>
> +#include <linux/dfl.h>
> +#include <linux/errno.h>
> +#include <linux/ioport.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/types.h>
> +
> +#include <linux/serial.h>
> +#include <linux/serial_8250.h>
> +
> +#define DFHv1_PARAM_ID_CLK_FRQ    0x2
> +#define DFHv1_PARAM_ID_FIFO_LEN   0x3
> +
> +#define DFHv1_PARAM_ID_REG_LAYOUT	0x4
> +#define DFHv1_PARAM_REG_LAYOUT_WIDTH	GENMASK_ULL(63, 32)
> +#define DFHv1_PARAM_REG_LAYOUT_SHIFT	GENMASK_ULL(31, 0)
> +
> +struct dfl_uart {
> +	int line;
> +};
> +
> +static int dfl_uart_get_params(struct dfl_device *dfl_dev, struct uart_8250_port *uart)
> +{
> +	struct device *dev = &dfl_dev->dev;
> +	u64 fifo_len, clk_freq, reg_layout;
> +	u32 reg_width;
> +	u64 *p;
> +
> +	p = dfh_get_u64_param_vals(dfl_dev, DFHv1_PARAM_ID_CLK_FRQ, &clk_freq, 1);
> +	if (IS_ERR(p))
> +		return dev_err_probe(dev, PTR_ERR(p), "missing CLK_FRQ param\n");
> +
> +	uart->port.uartclk = clk_freq;
> +
> +	p = dfh_get_u64_param_vals(dfl_dev, DFHv1_PARAM_ID_FIFO_LEN, &fifo_len, 1);
> +	if (IS_ERR(p))
> +		return dev_err_probe(dev, PTR_ERR(p), "missing FIFO_LEN param\n");
> +
> +	switch (fifo_len) {
> +	case 32:
> +		uart->port.type = PORT_ALTR_16550_F32;
> +		break;
> +
> +	case 64:
> +		uart->port.type = PORT_ALTR_16550_F64;
> +		break;
> +
> +	case 128:
> +		uart->port.type = PORT_ALTR_16550_F128;
> +		break;
> +
> +	default:
> +		return dev_err_probe(dev, -EINVAL, "unsupported FIFO_LEN %llu\n", fifo_len);
> +	}
> +
> +	p = dfh_get_u64_param_vals(dfl_dev, DFHv1_PARAM_ID_REG_LAYOUT, &reg_layout, 1);
> +	if (IS_ERR(p))
> +		return dev_err_probe(dev, PTR_ERR(p), "missing REG_LAYOUT param\n");
> +
> +	uart->port.regshift = FIELD_GET(DFHv1_PARAM_REG_LAYOUT_SHIFT, reg_layout);
> +	reg_width = FIELD_GET(DFHv1_PARAM_REG_LAYOUT_WIDTH, reg_layout);
> +	switch (reg_width) {
> +	case 4:
> +		uart->port.iotype = UPIO_MEM32;
> +		break;
> +
> +	case 2:
> +		uart->port.iotype = UPIO_MEM16;
> +		break;
> +
> +	default:
> +		return dev_err_probe(dev, -EINVAL, "unsupported reg-width %u\n", reg_width);
> +
> +	}
> +
> +	return 0;
> +}
> +
> +static int dfl_uart_probe(struct dfl_device *dfl_dev)
> +{
> +	struct device *dev = &dfl_dev->dev;
> +	struct uart_8250_port uart = { };
> +	struct dfl_uart *dfluart;
> +	int ret;
> +
> +	uart.port.flags = UPF_IOREMAP;
> +	uart.port.mapbase = dfl_dev->mmio_res.start;
> +	uart.port.mapsize = resource_size(&dfl_dev->mmio_res);
> +
> +	ret = dfl_uart_get_params(dfl_dev, &uart);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "failed uart feature walk\n");
> +
> +	if (dfl_dev->num_irqs == 1)
> +		uart.port.irq = dfl_dev->irqs[0];
> +
> +	dfluart = devm_kzalloc(dev, sizeof(*dfluart), GFP_KERNEL);
> +	if (!dfluart)
> +		return -ENOMEM;
> +
> +	dfluart->line = serial8250_register_8250_port(&uart);
> +	if (dfluart->line < 0)
> +		return dev_err_probe(dev, dfluart->line, "unable to register 8250 port.\n");
> +
> +	dev_set_drvdata(dev, dfluart);
> +
> +	return 0;
> +}
> +
> +static void dfl_uart_remove(struct dfl_device *dfl_dev)
> +{
> +	struct dfl_uart *dfluart = dev_get_drvdata(&dfl_dev->dev);
> +
> +	serial8250_unregister_port(dfluart->line);
> +}
> +
> +#define FME_FEATURE_ID_UART 0x24
> +
> +static const struct dfl_device_id dfl_uart_ids[] = {
> +	{ FME_ID, FME_FEATURE_ID_UART },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(dfl, dfl_uart_ids);
> +
> +static struct dfl_driver dfl_uart_driver = {
> +	.drv = {
> +		.name = "dfl-uart",
> +	},
> +	.id_table = dfl_uart_ids,
> +	.probe = dfl_uart_probe,
> +	.remove = dfl_uart_remove,
> +};
> +module_dfl_driver(dfl_uart_driver);
> +
> +MODULE_DESCRIPTION("DFL Intel UART driver");
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
> index b0f62345bc84..08af2acd4645 100644
> --- a/drivers/tty/serial/8250/Kconfig
> +++ b/drivers/tty/serial/8250/Kconfig
> @@ -370,6 +370,18 @@ config SERIAL_8250_FSL
>  	  erratum for Freescale 16550 UARTs in the 8250 driver. It also
>  	  enables support for ACPI enumeration.
>  
> +config SERIAL_8250_DFL
> +	tristate "DFL bus driver for Altera 16550 UART"
> +	depends on SERIAL_8250 && FPGA_DFL
> +	help
> +	  This option enables support for a Device Feature List (DFL) bus
> +	  driver for the Altera 16650 UART. One or more Altera 16650 UARTs
> +	  can be instantiated in a FPGA and then be discovered during
> +	  enumeration of the DFL bus.
> +
> +	  To compile this driver as a module, chose M here: the
> +	  module will be called 8250_dfl.
> +
>  config SERIAL_8250_DW
>  	tristate "Support for Synopsys DesignWare 8250 quirks"
>  	depends on SERIAL_8250
> diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
> index 1615bfdde2a0..4e1a32812683 100644
> --- a/drivers/tty/serial/8250/Makefile
> +++ b/drivers/tty/serial/8250/Makefile
> @@ -28,6 +28,7 @@ obj-$(CONFIG_SERIAL_8250_EXAR_ST16C554)	+= 8250_exar_st16c554.o
>  obj-$(CONFIG_SERIAL_8250_HUB6)		+= 8250_hub6.o
>  obj-$(CONFIG_SERIAL_8250_FSL)		+= 8250_fsl.o
>  obj-$(CONFIG_SERIAL_8250_MEN_MCB)	+= 8250_men_mcb.o
> +obj-$(CONFIG_SERIAL_8250_DFL)		+= 8250_dfl.o
>  obj-$(CONFIG_SERIAL_8250_DW)		+= 8250_dw.o
>  obj-$(CONFIG_SERIAL_8250_EM)		+= 8250_em.o
>  obj-$(CONFIG_SERIAL_8250_IOC3)		+= 8250_ioc3.o
> -- 
> 2.25.1
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 1/4] Documentation: fpga: dfl: Add documentation for DFHv1

[Tom Rix]

On 12/28/22 10:16 AM, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>
> Add documentation describing the extensions provided by Version
> 1 of the Device Feature Header (DFHv1).
>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
> v8: fix section titles
>
> v7: shorten long lines and wording suggestions by bagasdotme@gmail.com
>
> v6: no change
>
> v5: use nested list for field descriptions
>      clean up prose
>      add reviewed-by and comments from Ilpo Järvinen
>
> v4: Remove marketing speak and separate v0 and v1 descriptions.
>      Fix errors reported by "make htmldocs".
>
> v3: no change
>
> v2: s/GUILD/GUID/
>      add picture
> ---
>   Documentation/fpga/dfl.rst | 112 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 112 insertions(+)
>
> diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
> index 15b670926084..264b476fc6ac 100644
> --- a/Documentation/fpga/dfl.rst
> +++ b/Documentation/fpga/dfl.rst
> @@ -561,6 +561,118 @@ new DFL feature via UIO direct access, its feature id should be added to the
>   driver's id_table.
>   
>   

I think a better location for this section would be in the beginning 
after the

Device Feature List (DFL) Overview

The reader will be looking for the details of the Header once they have 
read the overview.

It would be handy if they were next

> +Device Feature Header - Version 0
> +=================================
> +Version 0 (DFHv0) is the original version of the Device Feature Header.
> +The format of DFHv0 is shown below::
> +
> +    +-----------------------------------------------------------------------+
> +    |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0| 0x00
> +    +-----------------------------------------------------------------------+
> +    |63                                 GUID_L                             0| 0x08
> +    +-----------------------------------------------------------------------+
> +    |63                                 GUID_H                             0| 0x10
> +    +-----------------------------------------------------------------------+
> +
> +- Offset 0x00
> +
> +  * Type - The type of DFH (e.g. FME, AFU, or private feature).
> +  * DFH VER - The version of the DFH.
> +  * Rsvd - Currently unused.
> +  * EOL - Set if the DFH is the end of the Device Feature List (DFL).
> +  * Next - The offset of the next DFH in the DFL from the DFH start.
> +    If EOL is set, Next is the size of MMIO of the last feature in the list.

Missed describing feature revision bits 12-15

There as two VER's, it would be clearer if they were different. maybe 
REV for bits 12-15

Similar below.

> +  * ID - The feature ID if Type is private feature.
> +
> +- Offset 0x08
> +
> +  * GUID_L - Least significant 64 bits of a 128-bit Globally Unique Identifier
> +    (present only if Type is FME or AFU).
> +
> +- Offset 0x10
> +
> +  * GUID_H - Most significant 64 bits of a 128-bit Globally Unique Identifier
> +    (present only if Type is FME or AFU).
> +
> +
> +Device Feature Header - Version 1
> +=================================
> +Version 1 (DFHv1) of the Device Feature Header adds the following functionality:
> +
> +* Provides a standardized mechanism for features to describe
> +  parameters/capabilities to software.
> +* Standardize the use of a GUID for all DFHv1 types.
> +* Decouples the DFH location from the register space of the feature itself.
> +
> +The format of Version 1 of the Device Feature Header (DFH) is shown below::
> +
> +    +-----------------------------------------------------------------------+
> +    |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 0| 0x00
> +    +-----------------------------------------------------------------------+
> +    |63                                 GUID_L                             0| 0x08
> +    +-----------------------------------------------------------------------+
> +    |63                                 GUID_H                             0| 0x10
> +    +-----------------------------------------------------------------------+
> +    |63                   Reg Address/Offset                      1|  Rel  0| 0x18
> +    +-----------------------------------------------------------------------+
> +    |63        Reg Size       32|Params 31|30 Group    16|15 Instance      0| 0x20
> +    +-----------------------------------------------------------------------+
> +    |63 Next    35|34RSV33|EOP32|31 Param Version 16|15 Param ID           0| 0x28
> +    +-----------------------------------------------------------------------+
> +    |63                 Parameter Data                                     0| 0x30
> +    +-----------------------------------------------------------------------+
> +
> +                                  ...
> +
> +    +-----------------------------------------------------------------------+
> +    |63 Next    35|34RSV33|EOP32|31 Param Version 16|15 Param ID           0|
> +    +-----------------------------------------------------------------------+
> +    |63                 Parameter Data                                     0|
> +    +-----------------------------------------------------------------------+
> +
> +- Offset 0x00
> +
> +  * Type - The type of DFH (e.g. FME, AFU, or private feature).
> +  * DFH VER - The version of the DFH.
> +  * Rsvd - Currently unused.
> +  * EOL - Set if the DFH is the end of the Device Feature List (DFL).
> +  * Next - The offset of the next DFH in the DFL from the DFH start.
> +    If EOL is set, Next is the size of MMIO of the last feature in the list.

Units of size ?

Tom

> +  * ID - The feature ID if Type is private feature.
> +
> +- Offset 0x08
> +
> +  * GUID_L - Least significant 64 bits of a 128-bit Globally Unique Identifier.
> +
> +- Offset 0x10
> +
> +  * GUID_H - Most significant 64 bits of a 128-bit Globally Unique Identifier.
> +
> +- Offset 0x18
> +
> +  * Reg Address/Offset - If Rel bit is set, then the value is the high 63 bits
> +    of a 16-bit aligned absolute address of the feature's registers. Otherwise
> +    the value is the offset from the start of the DFH of the feature's registers.
> +
> +- Offset 0x20
> +
> +  * Reg Size - Size of feature's register set in bytes.
> +  * Params - Set if DFH has a list of parameter blocks.
> +  * Group - Id of group if feature is part of a group.
> +  * Instance - Id of feature instance within a group.
> +
> +- Offset 0x28 if feature has parameters
> +
> +  * Next - Offset to the next parameter block in 8 byte words. If EOP set,
> +    size in 8 byte words of last parameter.
> +  * Param Version - Version of Param ID.
> +  * Param ID - ID of parameter.
> +
> +- Offset 0x30
> +
> +  * Parameter Data - Parameter data whose size and format is defined by
> +    version and ID of the parameter.
> +
>   Open discussion
>   ===============
>   FME driver exports one ioctl (DFL_FPGA_FME_PORT_PR) for partial reconfiguration

Re: [PATCH v8 3/4] fpga: dfl: add basic support for DFHv1

[Tom Rix]

On 12/28/22 10:16 AM, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>
> Version 1 of the Device Feature Header (DFH) definition adds
> functionality to the DFL bus.
>
> A DFHv1 header may have one or more parameter blocks that
> further describes the HW to SW.  Add support to the DFL bus
> to parse the MSI-X parameter.
>
> The location of a feature's register set is explicitly
> described in DFHv1 and can be relative to the base of the DFHv1
> or an absolute address.  Parse the location and pass the information
> to DFL driver.
>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
> v8: use struct_size() from overflow.h
>      add dfh_get_u64_param_vals()
>
> v7: no change
>
> v6: move MSI_X parameter definitions to drivers/fpga/dfl.h
>
> v5: update field names
>      fix find_param/dfh_get_psize
>      clean up mmio_res assignments
>      use u64* instead of void*
>      use FIELD_GET instead of masking
>
> v4: s/MSIX/MSI_X
>      move kernel doc to implementation
>      use structure assignment
>      fix decode of absolute address
>      clean up comment in parse_feature_irqs
>      remove use of csr_res
>
> v3: remove unneeded blank line
>      use clearer variable name
>      pass finfo into parse_feature_irqs()
>      refactor code for better indentation
>      use switch statement for irq parsing
>      squash in code parsing register location
>
> v2: fix kernel doc
>      clarify use of DFH_VERSION field
> ---
>   drivers/fpga/dfl.c  | 265 +++++++++++++++++++++++++++++++++++---------
>   drivers/fpga/dfl.h  |   9 ++
>   include/linux/dfl.h |   6 +
>   3 files changed, 229 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index b9aae85ba930..403fa425c1fe 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -13,6 +13,7 @@
>   #include <linux/dfl.h>
>   #include <linux/fpga-dfl.h>
>   #include <linux/module.h>
> +#include <linux/overflow.h>
>   #include <linux/uaccess.h>
>   
>   #include "dfl.h"
> @@ -342,6 +343,8 @@ static void release_dfl_dev(struct device *dev)
>   	if (ddev->mmio_res.parent)
>   		release_resource(&ddev->mmio_res);
>   
> +	kfree(ddev->params);
> +
>   	ida_free(&dfl_device_ida, ddev->id);
>   	kfree(ddev->irqs);
>   	kfree(ddev);
> @@ -380,7 +383,16 @@ dfl_dev_add(struct dfl_feature_platform_data *pdata,
>   	ddev->type = feature_dev_id_type(pdev);
>   	ddev->feature_id = feature->id;
>   	ddev->revision = feature->revision;
> +	ddev->dfh_version = feature->dfh_version;
>   	ddev->cdev = pdata->dfl_cdev;
> +	if (feature->param_size) {
> +		ddev->params = kmemdup(feature->params, feature->param_size, GFP_KERNEL);
> +		if (!ddev->params) {
> +			ret = -ENOMEM;
> +			goto put_dev;
> +		}
> +		ddev->param_size = feature->param_size;
> +	}
>   
>   	/* add mmio resource */
>   	parent_res = &pdev->resource[feature->resource_index];
> @@ -708,20 +720,27 @@ struct build_feature_devs_info {
>    * struct dfl_feature_info - sub feature info collected during feature dev build
>    *
>    * @fid: id of this sub feature.
> + * @revision: revision of this sub feature
> + * @dfh_version: version of Device Feature Header (DFH)
>    * @mmio_res: mmio resource of this sub feature.
>    * @ioaddr: mapped base address of mmio resource.
>    * @node: node in sub_features linked list.
>    * @irq_base: start of irq index in this sub feature.
>    * @nr_irqs: number of irqs of this sub feature.
> + * @param_size: size DFH parameters.
> + * @params: DFH parameter data.
>    */
>   struct dfl_feature_info {
>   	u16 fid;
>   	u8 revision;
> +	u8 dfh_version;
>   	struct resource mmio_res;
>   	void __iomem *ioaddr;
>   	struct list_head node;
>   	unsigned int irq_base;
>   	unsigned int nr_irqs;
> +	unsigned int param_size;
> +	u64 params[];
u64 *params
>   };
>   
>   static void dfl_fpga_cdev_add_port_dev(struct dfl_fpga_cdev *cdev,
> @@ -797,7 +816,17 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
>   		feature->dev = fdev;
>   		feature->id = finfo->fid;
>   		feature->revision = finfo->revision;
> +		feature->dfh_version = finfo->dfh_version;
>   
> +		if (finfo->param_size) {
> +			feature->params = devm_kmemdup(binfo->dev,
> +						       finfo->params, finfo->param_size,
> +						       GFP_KERNEL);
> +			if (!feature->params)
> +				return -ENOMEM;
> +
> +			feature->param_size = finfo->param_size;
> +		}
>   		/*
>   		 * the FIU header feature has some fundamental functions (sriov
>   		 * set, port enable/disable) needed for the dfl bus device and
> @@ -934,56 +963,135 @@ static u16 feature_id(u64 value)
>   	return 0;
>   }
>   
> +static u64 *find_param(u64 *params, resource_size_t max, int param_id)
> +{
> +	u64 *end = params + max / sizeof(u64);
> +	u64 v, next;
> +
> +	while (params < end) {
> +		v = *params;
> +		if (param_id == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
> +			return params;
> +
> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
> +		params += next;
> +		if (FIELD_GET(DFHv1_PARAM_HDR_NEXT_EOP, v))
> +			break;
Move before next =, and save some work calculating unsed next
> +	}
> +
> +	return NULL;
> +}
> +
> +/**
> + * dfh_find_param() - find parameter block for the given parameter id
> + * @dfl_dev: dfl device
> + * @param: id of dfl parameter
> + *
> + * Return: pointer to start of parameter block, NULL otherwise.
> + */
> +u64 *dfh_find_param(struct dfl_device *dfl_dev, int param_id)
> +{
> +	return find_param(dfl_dev->params, dfl_dev->param_size, param_id);
> +}
> +EXPORT_SYMBOL_GPL(dfh_find_param);

This and similar should be noop's for dfh v0

Generally there should be better checking if code is running a v1 vs  v0 
header.

> +
> +/**
> + * dfh_get_u64_param_vals() - get array of u64 param values for given parameter id
> + * @dfl_dev: dfl device
> + * @param: id of dfl parameter
> + * @pval: location of parameter data destination
> + * @nvals: number of u64 elements of parameter data
> + *
> + * Return: pointer to start of parameter block, PTR_ERR otherwise
> + */
> +u64 *dfh_get_u64_param_vals(struct dfl_device *dfl_dev, int param_id, u64 *pval, int nvals)
nvals should be unsigned
> +{
> +	u64 *param = find_param(dfl_dev->params, dfl_dev->param_size, param_id);
> +	u64 next;
> +	int i;
> +
> +	if (!param)
> +		return ERR_PTR(-ENOENT);
> +
> +	next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, *param);
> +
> +	if (nvals >= next)
> +		return ERR_PTR(-ENOMEM);
> +
> +	for (i = 0; i < nvals; i++)
> +		*pval++ = param[i + 1];
memcpy ?
> +
> +	return param;
> +}
> +EXPORT_SYMBOL_GPL(dfh_get_u64_param_vals);
> +
>   static int parse_feature_irqs(struct build_feature_devs_info *binfo,
> -			      resource_size_t ofst, u16 fid,
> -			      unsigned int *irq_base, unsigned int *nr_irqs)
> +			      resource_size_t ofst, struct dfl_feature_info *finfo)
>   {
>   	void __iomem *base = binfo->ioaddr + ofst;
>   	unsigned int i, ibase, inr = 0;
> +	void *params = finfo->params;
>   	enum dfl_id_type type;
> +	u16 fid = finfo->fid;
>   	int virq;
> +	u64 *p;
>   	u64 v;
>   
> -	type = feature_dev_id_type(binfo->feature_dev);
> +	switch (finfo->dfh_version) {
> +	case 0:

A switch makes this code complicated to read.

Have you thought about a function pointer table to separate out similar 
v1 vs v0 code blocks?

> +		/*
> +		 * DFHv0 only provides MMIO resource information for each feature
> +		 * in the DFL header.  There is no generic interrupt information.
> +		 * Instead, features with interrupt functionality provide
> +		 * the information in feature specific registers.
> +		 */
> +		type = feature_dev_id_type(binfo->feature_dev);
> +		if (type == PORT_ID) {
> +			switch (fid) {
> +			case PORT_FEATURE_ID_UINT:
> +				v = readq(base + PORT_UINT_CAP);
> +				ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
> +				inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
> +				break;
> +			case PORT_FEATURE_ID_ERROR:
> +				v = readq(base + PORT_ERROR_CAP);
> +				ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
> +				inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
> +				break;
> +			}
> +		} else if (type == FME_ID) {
> +			switch (fid) {
> +			case FME_FEATURE_ID_GLOBAL_ERR:
> +				v = readq(base + FME_ERROR_CAP);
> +				ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
> +				inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
> +				break;
> +			}
> +		}
> +		break;
>   
> -	/*
> -	 * Ideally DFL framework should only read info from DFL header, but
> -	 * current version DFL only provides mmio resources information for
> -	 * each feature in DFL Header, no field for interrupt resources.
> -	 * Interrupt resource information is provided by specific mmio
> -	 * registers of each private feature which supports interrupt. So in
> -	 * order to parse and assign irq resources, DFL framework has to look
> -	 * into specific capability registers of these private features.
> -	 *
> -	 * Once future DFL version supports generic interrupt resource
> -	 * information in common DFL headers, the generic interrupt parsing
> -	 * code will be added. But in order to be compatible to old version
> -	 * DFL, the driver may still fall back to these quirks.
> -	 */
> -	if (type == PORT_ID) {
> -		switch (fid) {
> -		case PORT_FEATURE_ID_UINT:
> -			v = readq(base + PORT_UINT_CAP);
> -			ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
> -			inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
> -			break;
> -		case PORT_FEATURE_ID_ERROR:
> -			v = readq(base + PORT_ERROR_CAP);
> -			ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
> -			inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
> +	case 1:
> +		/*
> +		 * DFHv1 provides interrupt resource information in DFHv1
> +		 * parameter blocks.
> +		 */
> +		p = find_param(params, finfo->param_size, DFHv1_PARAM_ID_MSI_X);
> +		if (!p)
>   			break;
> -		}
> -	} else if (type == FME_ID) {
> -		if (fid == FME_FEATURE_ID_GLOBAL_ERR) {
> -			v = readq(base + FME_ERROR_CAP);
> -			ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
> -			inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
> -		}
> +
> +		p++;
> +		ibase = FIELD_GET(DFHv1_PARAM_MSI_X_STARTV, *p);
> +		inr = FIELD_GET(DFHv1_PARAM_MSI_X_NUMV, *p);
> +		break;
> +
> +	default:
> +		dev_warn(binfo->dev, "unexpected DFH version %d\n", finfo->dfh_version);
> +		break;
>   	}
>   
>   	if (!inr) {
> -		*irq_base = 0;
> -		*nr_irqs = 0;
> +		finfo->irq_base = 0;
> +		finfo->nr_irqs = 0;
>   		return 0;
>   	}
>   
> @@ -1006,12 +1114,37 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
>   		}
>   	}
>   
> -	*irq_base = ibase;
> -	*nr_irqs = inr;
> +	finfo->irq_base = ibase;
> +	finfo->nr_irqs = inr;
>   
>   	return 0;
>   }
>   
> +static int dfh_get_psize(void __iomem *dfh_base, resource_size_t max)
for clarity, psize should be expanded to param_size
> +{
> +	int size = 0;
> +	u64 v, next;
> +
> +	if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS,
> +		       readq(dfh_base + DFHv1_CSR_SIZE_GRP)))
> +		return 0;
> +
> +	while (size + DFHv1_PARAM_HDR < max) {
> +		v = readq(dfh_base + DFHv1_PARAM_HDR + size);
> +
> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
> +		if (!next)
> +			return -EINVAL;
> +
> +		size += next * sizeof(u64);
> +
> +		if (FIELD_GET(DFHv1_PARAM_HDR_NEXT_EOP, v))
> +			return size;
> +	}
> +
> +	return -ENOENT;
> +}
> +
>   /*
>    * when create sub feature instances, for private features, it doesn't need
>    * to provide resource size and feature id as they could be read from DFH
> @@ -1023,39 +1156,69 @@ static int
>   create_feature_instance(struct build_feature_devs_info *binfo,
>   			resource_size_t ofst, resource_size_t size, u16 fid)
>   {
> -	unsigned int irq_base, nr_irqs;
>   	struct dfl_feature_info *finfo;
> +	resource_size_t start, end;
> +	int dfh_psize = 0;
>   	u8 revision = 0;
> +	u64 v, addr_off;
> +	u8 dfh_ver = 0;
>   	int ret;
> -	u64 v;
>   
>   	if (fid != FEATURE_ID_AFU) {
>   		v = readq(binfo->ioaddr + ofst);
>   		revision = FIELD_GET(DFH_REVISION, v);
> -
> +		dfh_ver = FIELD_GET(DFH_VERSION, v);
>   		/* read feature size and id if inputs are invalid */
>   		size = size ? size : feature_size(v);
>   		fid = fid ? fid : feature_id(v);
> +		if (dfh_ver == 1) {
> +			dfh_psize = dfh_get_psize(binfo->ioaddr + ofst, size);
> +			if (dfh_psize < 0) {
> +				dev_err(binfo->dev,
> +					"failed to read size of DFHv1 parameters %d\n",
> +					dfh_psize);
> +				return dfh_psize;
> +			}
> +			dev_dbg(binfo->dev, "dfhv1_psize %d\n", dfh_psize);
> +		}
>   	}
>   
>   	if (binfo->len - ofst < size)
>   		return -EINVAL;
>   
> -	ret = parse_feature_irqs(binfo, ofst, fid, &irq_base, &nr_irqs);
> -	if (ret)
> -		return ret;
> -
> -	finfo = kzalloc(sizeof(*finfo), GFP_KERNEL);
> +	finfo = kzalloc(struct_size(finfo, params, dfh_psize/sizeof(u64)), GFP_KERNEL);
>   	if (!finfo)
>   		return -ENOMEM;
>   
> +	memcpy_fromio(finfo->params, binfo->ioaddr + ofst + DFHv1_PARAM_HDR, dfh_psize);
> +	finfo->param_size = dfh_psize;
> +
>   	finfo->fid = fid;
>   	finfo->revision = revision;
> -	finfo->mmio_res.start = binfo->start + ofst;
> -	finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
> +	finfo->dfh_version = dfh_ver;
> +	if (dfh_ver == 1) {
> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
> +		addr_off = FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
> +		if (FIELD_GET(DFHv1_CSR_ADDR_REL, v))
> +			start = addr_off << 1;
> +		else
> +			start = binfo->start + ofst + addr_off;
> +
> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
> +		end = start + FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) - 1;
> +	} else {
> +		start = binfo->start + ofst;
> +		end = start + size - 1;
> +	}
>   	finfo->mmio_res.flags = IORESOURCE_MEM;
> -	finfo->irq_base = irq_base;
> -	finfo->nr_irqs = nr_irqs;
> +	finfo->mmio_res.start = start;
> +	finfo->mmio_res.end = end;
> +
> +	ret = parse_feature_irqs(binfo, ofst, finfo);
> +	if (ret) {
> +		kfree(finfo);
> +		return ret;
> +	}
>   
>   	list_add_tail(&finfo->node, &binfo->sub_features);
>   	binfo->feature_num++;
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index fc59f33367ee..a21065cec04e 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -111,6 +111,10 @@
>   #define DFHv1_PARAM_HDR_NEXT_EOP	BIT_ULL(32)
>   #define DFHv1_PARAM_DATA		0x08  /* Offset of Param data from Param header */
>   
> +#define DFHv1_PARAM_ID_MSI_X		0x1
> +#define DFHv1_PARAM_MSI_X_NUMV		GENMASK_ULL(63, 32)
> +#define DFHv1_PARAM_MSI_X_STARTV	GENMASK_ULL(31, 0)
> +
>   /* Next AFU Register Bitfield */
>   #define NEXT_AFU_NEXT_DFH_OFST	GENMASK_ULL(23, 0)	/* Offset to next AFU */
>   
> @@ -272,11 +276,14 @@ struct dfl_feature_irq_ctx {
>    * @ops: ops of this sub feature.
>    * @ddev: ptr to the dfl device of this sub feature.
>    * @priv: priv data of this feature.
> + * @param_size: size of dfh parameters
> + * @params: point to memory copy of dfh parameters
>    */
>   struct dfl_feature {
>   	struct platform_device *dev;
>   	u16 id;
>   	u8 revision;
> +	u8 dfh_version;

Needs a description in the comments above.

I am concerned generally about backward compatibility and oot drivers 
that are dfh v0.

Adding an element in the middle will break things.

These structures could be more explicit about what is common, and what 
is v0 and v1 specific.

Tom

>   	int resource_index;
>   	void __iomem *ioaddr;
>   	struct dfl_feature_irq_ctx *irq_ctx;
> @@ -284,6 +291,8 @@ struct dfl_feature {
>   	const struct dfl_feature_ops *ops;
>   	struct dfl_device *ddev;
>   	void *priv;
> +	unsigned int param_size;
> +	void *params;
>   };
>   
>   #define FEATURE_DEV_ID_UNUSED	(-1)
> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
> index 431636a0dc78..3138ea42d02e 100644
> --- a/include/linux/dfl.h
> +++ b/include/linux/dfl.h
> @@ -39,9 +39,12 @@ struct dfl_device {
>   	u16 type;
>   	u16 feature_id;
>   	u8 revision;
> +	u8 dfh_version;
>   	struct resource mmio_res;
>   	int *irqs;
>   	unsigned int num_irqs;
> +	unsigned int param_size;
> +	void *params;
>   	struct dfl_fpga_cdev *cdev;
>   	const struct dfl_device_id *id_entry;
>   };
> @@ -84,4 +87,7 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv);
>   	module_driver(__dfl_driver, dfl_driver_register, \
>   		      dfl_driver_unregister)
>   
> +u64 *dfh_find_param(struct dfl_device *dfl_dev, int param);
> +
> +u64 *dfh_get_u64_param_vals(struct dfl_device *dfl_dev, int param_id, u64 *pval, int nvals);
>   #endif /* __LINUX_DFL_H */

Re: [PATCH v8 3/4] fpga: dfl: add basic support for DFHv1

[matthew.gerlach]

On Thu, 29 Dec 2022, Andy Shevchenko wrote:

> On Wed, Dec 28, 2022 at 10:16:23AM -0800, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Version 1 of the Device Feature Header (DFH) definition adds
>> functionality to the DFL bus.
>>
>> A DFHv1 header may have one or more parameter blocks that
>> further describes the HW to SW.  Add support to the DFL bus
>
> Single space is enough.

Yes, single space is enough. Two spaces after a period is hard habit for 
me break. I will update in v9.

>
>> to parse the MSI-X parameter.
>>
>> The location of a feature's register set is explicitly
>> described in DFHv1 and can be relative to the base of the DFHv1
>> or an absolute address.  Parse the location and pass the information
>> to DFL driver.
>
> I'm wondering what DFL states for.

I will define DFL in the commit message like DFH in the next revision.

>
> ...
>
>> +/**
>> + * dfh_get_u64_param_vals() - get array of u64 param values for given parameter id
>> + * @dfl_dev: dfl device
>> + * @param: id of dfl parameter
>> + * @pval: location of parameter data destination
>> + * @nvals: number of u64 elements of parameter data
>> + *
>> + * Return: pointer to start of parameter block, PTR_ERR otherwise
>> + */
>> +u64 *dfh_get_u64_param_vals(struct dfl_device *dfl_dev, int param_id, u64 *pval, int nvals)
>> +{
>> +	u64 *param = find_param(dfl_dev->params, dfl_dev->param_size, param_id);
>> +	u64 next;
>> +	int i;
>> +
>> +	if (!param)
>> +		return ERR_PTR(-ENOENT);
>> +
>> +	next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, *param);
>> +
>> +	if (nvals >= next)
>> +		return ERR_PTR(-ENOMEM);
>
> ENODATA ?

ENODATA does seem to be more accurate than ENOMEM in this case.

>
>> +	for (i = 0; i < nvals; i++)
>> +		*pval++ = param[i + 1];
>
> memcpy() ?

Using memcpy() will make code cleaner.

>
>> +	return param;
>> +}
>
> ...
>
>> +	finfo = kzalloc(struct_size(finfo, params, dfh_psize/sizeof(u64)), GFP_KERNEL);
>
> ' / ' (mind the spaces)

Yes, I will mind the spaces.

>
> Also, perhaps better to use sizeof(*params) or what is the member of that
> structure. So it will be more robust against possible changes.

params is the name of the structure member that is the trailing array.

>
>>  	if (!finfo)
>>  		return -ENOMEM;
>
> -- 
> With Best Regards,
> Andy Shevchenko

Thanks for the feedback,
Matthew Gerlach

>
>
>

Re: [PATCH v8 4/4] tty: serial: 8250: add DFL bus driver for Altera 16550.

[matthew.gerlach]

[-- Attachment #1: Type: text/plain, Size: 8051 bytes --]



On Thu, 29 Dec 2022, Andy Shevchenko wrote:

> On Wed, Dec 28, 2022 at 10:16:24AM -0800, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Add a Device Feature List (DFL) bus driver for the Altera
>> 16550 implementation of UART.
>
> With that helper looks better,
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

I agree the helper function makes the code look better and is safer.

Thanks for the review,
Matthew Gerlach

>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>> ---
>> v8: use dfh_get_u64_param_vals()
>>
>> v7: no change
>>
>> v6: move driver specific parameter definitions to limit scope
>>
>> v5: removed unneeded blank line
>>     removed unneeded includes
>>     included device.h and types.h
>>     removed unneeded local variable
>>     remove calls to dev_dbg
>>     memset -> { }
>>     remove space after period
>>     explicitly include used headers
>>     remove redundant Inc from Copyright
>>     fix format specifier
>>
>> v4: use dev_err_probe() everywhere that is appropriate
>>     clean up noise
>>     change error messages to use the word, unsupported
>>     tried again to sort Makefile and KConfig better
>>     reorder probe function for easier error handling
>>     use new dfh_find_param API
>>
>> v3: use passed in location of registers
>>     use cleaned up functions for parsing parameters
>>
>> v2: clean up error messages
>>     alphabetize header files
>>     fix 'missing prototype' error by making function static
>>     tried to sort Makefile and Kconfig better
>> ---
>>  drivers/tty/serial/8250/8250_dfl.c | 150 +++++++++++++++++++++++++++++
>>  drivers/tty/serial/8250/Kconfig    |  12 +++
>>  drivers/tty/serial/8250/Makefile   |   1 +
>>  3 files changed, 163 insertions(+)
>>  create mode 100644 drivers/tty/serial/8250/8250_dfl.c
>>
>> diff --git a/drivers/tty/serial/8250/8250_dfl.c b/drivers/tty/serial/8250/8250_dfl.c
>> new file mode 100644
>> index 000000000000..0ee78c38a721
>> --- /dev/null
>> +++ b/drivers/tty/serial/8250/8250_dfl.c
>> @@ -0,0 +1,150 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Driver for FPGA UART
>> + *
>> + * Copyright (C) 2022 Intel Corporation.
>> + *
>> + * Authors:
>> + *   Ananda Ravuri <ananda.ravuri@intel.com>
>> + *   Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> + */
>> +
>> +#include <linux/bitfield.h>
>> +#include <linux/device.h>
>> +#include <linux/dfl.h>
>> +#include <linux/errno.h>
>> +#include <linux/ioport.h>
>> +#include <linux/module.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/types.h>
>> +
>> +#include <linux/serial.h>
>> +#include <linux/serial_8250.h>
>> +
>> +#define DFHv1_PARAM_ID_CLK_FRQ    0x2
>> +#define DFHv1_PARAM_ID_FIFO_LEN   0x3
>> +
>> +#define DFHv1_PARAM_ID_REG_LAYOUT	0x4
>> +#define DFHv1_PARAM_REG_LAYOUT_WIDTH	GENMASK_ULL(63, 32)
>> +#define DFHv1_PARAM_REG_LAYOUT_SHIFT	GENMASK_ULL(31, 0)
>> +
>> +struct dfl_uart {
>> +	int line;
>> +};
>> +
>> +static int dfl_uart_get_params(struct dfl_device *dfl_dev, struct uart_8250_port *uart)
>> +{
>> +	struct device *dev = &dfl_dev->dev;
>> +	u64 fifo_len, clk_freq, reg_layout;
>> +	u32 reg_width;
>> +	u64 *p;
>> +
>> +	p = dfh_get_u64_param_vals(dfl_dev, DFHv1_PARAM_ID_CLK_FRQ, &clk_freq, 1);
>> +	if (IS_ERR(p))
>> +		return dev_err_probe(dev, PTR_ERR(p), "missing CLK_FRQ param\n");
>> +
>> +	uart->port.uartclk = clk_freq;
>> +
>> +	p = dfh_get_u64_param_vals(dfl_dev, DFHv1_PARAM_ID_FIFO_LEN, &fifo_len, 1);
>> +	if (IS_ERR(p))
>> +		return dev_err_probe(dev, PTR_ERR(p), "missing FIFO_LEN param\n");
>> +
>> +	switch (fifo_len) {
>> +	case 32:
>> +		uart->port.type = PORT_ALTR_16550_F32;
>> +		break;
>> +
>> +	case 64:
>> +		uart->port.type = PORT_ALTR_16550_F64;
>> +		break;
>> +
>> +	case 128:
>> +		uart->port.type = PORT_ALTR_16550_F128;
>> +		break;
>> +
>> +	default:
>> +		return dev_err_probe(dev, -EINVAL, "unsupported FIFO_LEN %llu\n", fifo_len);
>> +	}
>> +
>> +	p = dfh_get_u64_param_vals(dfl_dev, DFHv1_PARAM_ID_REG_LAYOUT, &reg_layout, 1);
>> +	if (IS_ERR(p))
>> +		return dev_err_probe(dev, PTR_ERR(p), "missing REG_LAYOUT param\n");
>> +
>> +	uart->port.regshift = FIELD_GET(DFHv1_PARAM_REG_LAYOUT_SHIFT, reg_layout);
>> +	reg_width = FIELD_GET(DFHv1_PARAM_REG_LAYOUT_WIDTH, reg_layout);
>> +	switch (reg_width) {
>> +	case 4:
>> +		uart->port.iotype = UPIO_MEM32;
>> +		break;
>> +
>> +	case 2:
>> +		uart->port.iotype = UPIO_MEM16;
>> +		break;
>> +
>> +	default:
>> +		return dev_err_probe(dev, -EINVAL, "unsupported reg-width %u\n", reg_width);
>> +
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int dfl_uart_probe(struct dfl_device *dfl_dev)
>> +{
>> +	struct device *dev = &dfl_dev->dev;
>> +	struct uart_8250_port uart = { };
>> +	struct dfl_uart *dfluart;
>> +	int ret;
>> +
>> +	uart.port.flags = UPF_IOREMAP;
>> +	uart.port.mapbase = dfl_dev->mmio_res.start;
>> +	uart.port.mapsize = resource_size(&dfl_dev->mmio_res);
>> +
>> +	ret = dfl_uart_get_params(dfl_dev, &uart);
>> +	if (ret < 0)
>> +		return dev_err_probe(dev, ret, "failed uart feature walk\n");
>> +
>> +	if (dfl_dev->num_irqs == 1)
>> +		uart.port.irq = dfl_dev->irqs[0];
>> +
>> +	dfluart = devm_kzalloc(dev, sizeof(*dfluart), GFP_KERNEL);
>> +	if (!dfluart)
>> +		return -ENOMEM;
>> +
>> +	dfluart->line = serial8250_register_8250_port(&uart);
>> +	if (dfluart->line < 0)
>> +		return dev_err_probe(dev, dfluart->line, "unable to register 8250 port.\n");
>> +
>> +	dev_set_drvdata(dev, dfluart);
>> +
>> +	return 0;
>> +}
>> +
>> +static void dfl_uart_remove(struct dfl_device *dfl_dev)
>> +{
>> +	struct dfl_uart *dfluart = dev_get_drvdata(&dfl_dev->dev);
>> +
>> +	serial8250_unregister_port(dfluart->line);
>> +}
>> +
>> +#define FME_FEATURE_ID_UART 0x24
>> +
>> +static const struct dfl_device_id dfl_uart_ids[] = {
>> +	{ FME_ID, FME_FEATURE_ID_UART },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(dfl, dfl_uart_ids);
>> +
>> +static struct dfl_driver dfl_uart_driver = {
>> +	.drv = {
>> +		.name = "dfl-uart",
>> +	},
>> +	.id_table = dfl_uart_ids,
>> +	.probe = dfl_uart_probe,
>> +	.remove = dfl_uart_remove,
>> +};
>> +module_dfl_driver(dfl_uart_driver);
>> +
>> +MODULE_DESCRIPTION("DFL Intel UART driver");
>> +MODULE_AUTHOR("Intel Corporation");
>> +MODULE_LICENSE("GPL");
>> diff --git a/drivers/tty/serial/8250/Kconfig b/drivers/tty/serial/8250/Kconfig
>> index b0f62345bc84..08af2acd4645 100644
>> --- a/drivers/tty/serial/8250/Kconfig
>> +++ b/drivers/tty/serial/8250/Kconfig
>> @@ -370,6 +370,18 @@ config SERIAL_8250_FSL
>>  	  erratum for Freescale 16550 UARTs in the 8250 driver. It also
>>  	  enables support for ACPI enumeration.
>>
>> +config SERIAL_8250_DFL
>> +	tristate "DFL bus driver for Altera 16550 UART"
>> +	depends on SERIAL_8250 && FPGA_DFL
>> +	help
>> +	  This option enables support for a Device Feature List (DFL) bus
>> +	  driver for the Altera 16650 UART. One or more Altera 16650 UARTs
>> +	  can be instantiated in a FPGA and then be discovered during
>> +	  enumeration of the DFL bus.
>> +
>> +	  To compile this driver as a module, chose M here: the
>> +	  module will be called 8250_dfl.
>> +
>>  config SERIAL_8250_DW
>>  	tristate "Support for Synopsys DesignWare 8250 quirks"
>>  	depends on SERIAL_8250
>> diff --git a/drivers/tty/serial/8250/Makefile b/drivers/tty/serial/8250/Makefile
>> index 1615bfdde2a0..4e1a32812683 100644
>> --- a/drivers/tty/serial/8250/Makefile
>> +++ b/drivers/tty/serial/8250/Makefile
>> @@ -28,6 +28,7 @@ obj-$(CONFIG_SERIAL_8250_EXAR_ST16C554)	+= 8250_exar_st16c554.o
>>  obj-$(CONFIG_SERIAL_8250_HUB6)		+= 8250_hub6.o
>>  obj-$(CONFIG_SERIAL_8250_FSL)		+= 8250_fsl.o
>>  obj-$(CONFIG_SERIAL_8250_MEN_MCB)	+= 8250_men_mcb.o
>> +obj-$(CONFIG_SERIAL_8250_DFL)		+= 8250_dfl.o
>>  obj-$(CONFIG_SERIAL_8250_DW)		+= 8250_dw.o
>>  obj-$(CONFIG_SERIAL_8250_EM)		+= 8250_em.o
>>  obj-$(CONFIG_SERIAL_8250_IOC3)		+= 8250_ioc3.o
>> --
>> 2.25.1
>>
>
> -- 
> With Best Regards,
> Andy Shevchenko
>
>
>

Re: [PATCH v8 3/4] fpga: dfl: add basic support for DFHv1

[matthew.gerlach]

[-- Attachment #1: Type: text/plain, Size: 12943 bytes --]



On Thu, 29 Dec 2022, Tom Rix wrote:

> On 12/28/22 10:16 AM, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> 
>> +
>> +	while (params < end) {
>> +		v = *params;
>> +		if (param_id == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
>> +			return params;
>> +
>> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
>> +		params += next;
>> +		if (FIELD_GET(DFHv1_PARAM_HDR_NEXT_EOP, v))
>> +			break;
> Move before next =, and save some work calculating unsed next

Thanks for the suggestion.  Yes, the change will save some work, and the 
code will look better.

>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +/**
>> + * dfh_find_param() - find parameter block for the given parameter id
>> + * @dfl_dev: dfl device
>> + * @param: id of dfl parameter
>> + *
>> + * Return: pointer to start of parameter block, NULL otherwise.
>> + */
>> +u64 *dfh_find_param(struct dfl_device *dfl_dev, int param_id)
>> +{
>> +	return find_param(dfl_dev->params, dfl_dev->param_size, param_id);
>> +}
>> +EXPORT_SYMBOL_GPL(dfh_find_param);
>
> This and similar should be noop's for dfh v0
>
> Generally there should be better checking if code is running a v1 vs  v0 
> header.

With v0, dfl_dev->params will be NULL and dfl_dev->param_size will be 
zero. Earlier feedback suggested to use the internal NULL/size checking 
instead of adding redundant checks.

>
>> +
>> +/**
>> + * dfh_get_u64_param_vals() - get array of u64 param values for given 
>> parameter id
>> + * @dfl_dev: dfl device
>> + * @param: id of dfl parameter
>> + * @pval: location of parameter data destination
>> + * @nvals: number of u64 elements of parameter data
>> + *
>> + * Return: pointer to start of parameter block, PTR_ERR otherwise
>> + */
>> +u64 *dfh_get_u64_param_vals(struct dfl_device *dfl_dev, int param_id, u64 
>> *pval, int nvals)
> nvals should be unsigned
>> +{
>> +	u64 *param = find_param(dfl_dev->params, dfl_dev->param_size, 
>> param_id);
>> +	u64 next;
>> +	int i;
>> +
>> +	if (!param)
>> +		return ERR_PTR(-ENOENT);
>> +
>> +	next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, *param);
>> +
>> +	if (nvals >= next)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	for (i = 0; i < nvals; i++)
>> +		*pval++ = param[i + 1];
> memcpy ?

Yes, memcpy would be better.

>> +
>> +	return param;
>> +}
>> +EXPORT_SYMBOL_GPL(dfh_get_u64_param_vals);
>> +
>>   static int parse_feature_irqs(struct build_feature_devs_info *binfo,
>> -			      resource_size_t ofst, u16 fid,
>> -			      unsigned int *irq_base, unsigned int *nr_irqs)
>> +			      resource_size_t ofst, struct dfl_feature_info 
>> *finfo)
>>   {
>>   	void __iomem *base = binfo->ioaddr + ofst;
>>   	unsigned int i, ibase, inr = 0;
>> +	void *params = finfo->params;
>>   	enum dfl_id_type type;
>> +	u16 fid = finfo->fid;
>>   	int virq;
>> +	u64 *p;
>>   	u64 v;
>>   -	type = feature_dev_id_type(binfo->feature_dev);
>> +	switch (finfo->dfh_version) {
>> +	case 0:
>
> A switch makes this code complicated to read.

Previous feedback suggested a switch statement better than if/else.

>
> Have you thought about a function pointer table to separate out similar v1 vs 
> v0 code blocks?

I haven't though of a function pointer table, and I'm not sure how much 
clearer it would be, but I'll consider it.

>
>> +		/*
>> +		 * DFHv0 only provides MMIO resource information for each 
>> feature
>> +		 * in the DFL header.  There is no generic interrupt 
>> information.
>> +		 * Instead, features with interrupt functionality provide
>> +		 * the information in feature specific registers.
>> +		 */
>> +		type = feature_dev_id_type(binfo->feature_dev);
>> +		if (type == PORT_ID) {
>> +			switch (fid) {
>> +			case PORT_FEATURE_ID_UINT:
>> +				v = readq(base + PORT_UINT_CAP);
>> +				ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
>> +				inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
>> +				break;
>> +			case PORT_FEATURE_ID_ERROR:
>> +				v = readq(base + PORT_ERROR_CAP);
>> +				ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, 
>> v);
>> +				inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
>> +				break;
>> +			}
>> +		} else if (type == FME_ID) {
>> +			switch (fid) {
>> +			case FME_FEATURE_ID_GLOBAL_ERR:
>> +				v = readq(base + FME_ERROR_CAP);
>> +				ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
>> +				inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
>> +				break;
>> +			}
>> +		}
>> +		break;
>>   -	/*
>> -	 * Ideally DFL framework should only read info from DFL header, but
>> -	 * current version DFL only provides mmio resources information for
>> -	 * each feature in DFL Header, no field for interrupt resources.
>> -	 * Interrupt resource information is provided by specific mmio
>> -	 * registers of each private feature which supports interrupt. So in
>> -	 * order to parse and assign irq resources, DFL framework has to look
>> -	 * into specific capability registers of these private features.
>> -	 *
>> -	 * Once future DFL version supports generic interrupt resource
>> -	 * information in common DFL headers, the generic interrupt parsing
>> -	 * code will be added. But in order to be compatible to old version
>> -	 * DFL, the driver may still fall back to these quirks.
>> -	 */
>> -	if (type == PORT_ID) {
>> -		switch (fid) {
>> -		case PORT_FEATURE_ID_UINT:
>> -			v = readq(base + PORT_UINT_CAP);
>> -			ibase = FIELD_GET(PORT_UINT_CAP_FST_VECT, v);
>> -			inr = FIELD_GET(PORT_UINT_CAP_INT_NUM, v);
>> -			break;
>> -		case PORT_FEATURE_ID_ERROR:
>> -			v = readq(base + PORT_ERROR_CAP);
>> -			ibase = FIELD_GET(PORT_ERROR_CAP_INT_VECT, v);
>> -			inr = FIELD_GET(PORT_ERROR_CAP_SUPP_INT, v);
>> +	case 1:
>> +		/*
>> +		 * DFHv1 provides interrupt resource information in DFHv1
>> +		 * parameter blocks.
>> +		 */
>> +		p = find_param(params, finfo->param_size, 
>> DFHv1_PARAM_ID_MSI_X);
>> +		if (!p)
>>   			break;
>> -		}
>> -	} else if (type == FME_ID) {
>> -		if (fid == FME_FEATURE_ID_GLOBAL_ERR) {
>> -			v = readq(base + FME_ERROR_CAP);
>> -			ibase = FIELD_GET(FME_ERROR_CAP_INT_VECT, v);
>> -			inr = FIELD_GET(FME_ERROR_CAP_SUPP_INT, v);
>> -		}
>> +
>> +		p++;
>> +		ibase = FIELD_GET(DFHv1_PARAM_MSI_X_STARTV, *p);
>> +		inr = FIELD_GET(DFHv1_PARAM_MSI_X_NUMV, *p);
>> +		break;
>> +
>> +	default:
>> +		dev_warn(binfo->dev, "unexpected DFH version %d\n", 
>> finfo->dfh_version);
>> +		break;
>>   	}
>>     	if (!inr) {
>> -		*irq_base = 0;
>> -		*nr_irqs = 0;
>> +		finfo->irq_base = 0;
>> +		finfo->nr_irqs = 0;
>>   		return 0;
>>   	}
>>   @@ -1006,12 +1114,37 @@ static int parse_feature_irqs(struct 
>> build_feature_devs_info *binfo,
>>   		}
>>   	}
>>   -	*irq_base = ibase;
>> -	*nr_irqs = inr;
>> +	finfo->irq_base = ibase;
>> +	finfo->nr_irqs = inr;
>>     	return 0;
>>   }
>>   +static int dfh_get_psize(void __iomem *dfh_base, resource_size_t max)
> for clarity, psize should be expanded to param_size

s/psize/param_size would be clearer and more consistent.

>> +{
>> +	int size = 0;
>> +	u64 v, next;
>> +
>> +	if (!FIELD_GET(DFHv1_CSR_SIZE_GRP_HAS_PARAMS,
>> +		       readq(dfh_base + DFHv1_CSR_SIZE_GRP)))
>> +		return 0;
>> +
>> +	while (size + DFHv1_PARAM_HDR < max) {
>> +		v = readq(dfh_base + DFHv1_PARAM_HDR + size);
>> +
>> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
>> +		if (!next)
>> +			return -EINVAL;
>> +
>> +		size += next * sizeof(u64);
>> +
>> +		if (FIELD_GET(DFHv1_PARAM_HDR_NEXT_EOP, v))
>> +			return size;
>> +	}
>> +
>> +	return -ENOENT;
>> +}
>> +
>>   /*
>>    * when create sub feature instances, for private features, it doesn't 
>> need
>>    * to provide resource size and feature id as they could be read from DFH
>> @@ -1023,39 +1156,69 @@ static int
>>   create_feature_instance(struct build_feature_devs_info *binfo,
>>   			resource_size_t ofst, resource_size_t size, u16 fid)
>>   {
>> -	unsigned int irq_base, nr_irqs;
>>   	struct dfl_feature_info *finfo;
>> +	resource_size_t start, end;
>> +	int dfh_psize = 0;
>>   	u8 revision = 0;
>> +	u64 v, addr_off;
>> +	u8 dfh_ver = 0;
>>   	int ret;
>> -	u64 v;
>>     	if (fid != FEATURE_ID_AFU) {
>>   		v = readq(binfo->ioaddr + ofst);
>>   		revision = FIELD_GET(DFH_REVISION, v);
>> -
>> +		dfh_ver = FIELD_GET(DFH_VERSION, v);
>>   		/* read feature size and id if inputs are invalid */
>>   		size = size ? size : feature_size(v);
>>   		fid = fid ? fid : feature_id(v);
>> +		if (dfh_ver == 1) {
>> +			dfh_psize = dfh_get_psize(binfo->ioaddr + ofst, 
>> size);
>> +			if (dfh_psize < 0) {
>> +				dev_err(binfo->dev,
>> +					"failed to read size of DFHv1 
>> parameters %d\n",
>> +					dfh_psize);
>> +				return dfh_psize;
>> +			}
>> +			dev_dbg(binfo->dev, "dfhv1_psize %d\n", dfh_psize);
>> +		}
>>   	}
>>     	if (binfo->len - ofst < size)
>>   		return -EINVAL;
>>   -	ret = parse_feature_irqs(binfo, ofst, fid, &irq_base, &nr_irqs);
>> -	if (ret)
>> -		return ret;
>> -
>> -	finfo = kzalloc(sizeof(*finfo), GFP_KERNEL);
>> +	finfo = kzalloc(struct_size(finfo, params, dfh_psize/sizeof(u64)), 
>> GFP_KERNEL);
>>   	if (!finfo)
>>   		return -ENOMEM;
>>   +	memcpy_fromio(finfo->params, binfo->ioaddr + ofst + DFHv1_PARAM_HDR, 
>> dfh_psize);
>> +	finfo->param_size = dfh_psize;
>> +
>>   	finfo->fid = fid;
>>   	finfo->revision = revision;
>> -	finfo->mmio_res.start = binfo->start + ofst;
>> -	finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
>> +	finfo->dfh_version = dfh_ver;
>> +	if (dfh_ver == 1) {
>> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_ADDR);
>> +		addr_off = FIELD_GET(DFHv1_CSR_ADDR_MASK, v);
>> +		if (FIELD_GET(DFHv1_CSR_ADDR_REL, v))
>> +			start = addr_off << 1;
>> +		else
>> +			start = binfo->start + ofst + addr_off;
>> +
>> +		v = readq(binfo->ioaddr + ofst + DFHv1_CSR_SIZE_GRP);
>> +		end = start + FIELD_GET(DFHv1_CSR_SIZE_GRP_SIZE, v) - 1;
>> +	} else {
>> +		start = binfo->start + ofst;
>> +		end = start + size - 1;
>> +	}
>>   	finfo->mmio_res.flags = IORESOURCE_MEM;
>> -	finfo->irq_base = irq_base;
>> -	finfo->nr_irqs = nr_irqs;
>> +	finfo->mmio_res.start = start;
>> +	finfo->mmio_res.end = end;
>> +
>> +	ret = parse_feature_irqs(binfo, ofst, finfo);
>> +	if (ret) {
>> +		kfree(finfo);
>> +		return ret;
>> +	}
>>     	list_add_tail(&finfo->node, &binfo->sub_features);
>>   	binfo->feature_num++;
>> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
>> index fc59f33367ee..a21065cec04e 100644
>> --- a/drivers/fpga/dfl.h
>> +++ b/drivers/fpga/dfl.h
>> @@ -111,6 +111,10 @@
>>   #define DFHv1_PARAM_HDR_NEXT_EOP	BIT_ULL(32)
>>   #define DFHv1_PARAM_DATA		0x08  /* Offset of Param data from 
>> Param header */
>>   +#define DFHv1_PARAM_ID_MSI_X		0x1
>> +#define DFHv1_PARAM_MSI_X_NUMV		GENMASK_ULL(63, 32)
>> +#define DFHv1_PARAM_MSI_X_STARTV	GENMASK_ULL(31, 0)
>> +
>>   /* Next AFU Register Bitfield */
>>   #define NEXT_AFU_NEXT_DFH_OFST	GENMASK_ULL(23, 0)	/* Offset to 
>> next AFU */
>>   @@ -272,11 +276,14 @@ struct dfl_feature_irq_ctx {
>>    * @ops: ops of this sub feature.
>>    * @ddev: ptr to the dfl device of this sub feature.
>>    * @priv: priv data of this feature.
>> + * @param_size: size of dfh parameters
>> + * @params: point to memory copy of dfh parameters
>>    */
>>   struct dfl_feature {
>>   	struct platform_device *dev;
>>   	u16 id;
>>   	u8 revision;
>> +	u8 dfh_version;
>
> Needs a description in the comments above.

Thanks for catching that.

>
> I am concerned generally about backward compatibility and oot drivers that 
> are dfh v0.

I agree it would be much safter to add the new fields to the end of the 
structure for compatibility reasons. I will do this for struct dfl_device 
as well.

Thanks for the review,
Matthew Gerlach


>
> Adding an element in the middle will break things.
>
> These structures could be more explicit about what is common, and what is v0 
> and v1 specific.
>
> Tom
>
>>   	int resource_index;
>>   	void __iomem *ioaddr;
>>   	struct dfl_feature_irq_ctx *irq_ctx;
>> @@ -284,6 +291,8 @@ struct dfl_feature {
>>   	const struct dfl_feature_ops *ops;
>>   	struct dfl_device *ddev;
>>   	void *priv;
>> +	unsigned int param_size;
>> +	void *params;
>>   };
>>     #define FEATURE_DEV_ID_UNUSED	(-1)
>> diff --git a/include/linux/dfl.h b/include/linux/dfl.h
>> index 431636a0dc78..3138ea42d02e 100644
>> --- a/include/linux/dfl.h
>> +++ b/include/linux/dfl.h
>> @@ -39,9 +39,12 @@ struct dfl_device {
>>   	u16 type;
>>   	u16 feature_id;
>>   	u8 revision;
>> +	u8 dfh_version;
>>   	struct resource mmio_res;
>>   	int *irqs;
>>   	unsigned int num_irqs;
>> +	unsigned int param_size;
>> +	void *params;
>>   	struct dfl_fpga_cdev *cdev;
>>   	const struct dfl_device_id *id_entry;
>>   };
>> @@ -84,4 +87,7 @@ void dfl_driver_unregister(struct dfl_driver *dfl_drv);
>>   	module_driver(__dfl_driver, dfl_driver_register, \
>>   		      dfl_driver_unregister)
>>   +u64 *dfh_find_param(struct dfl_device *dfl_dev, int param);
>> +
>> +u64 *dfh_get_u64_param_vals(struct dfl_device *dfl_dev, int param_id, u64 
>> *pval, int nvals);
>>   #endif /* __LINUX_DFL_H */
>
>

Re: [PATCH v8 1/4] Documentation: fpga: dfl: Add documentation for DFHv1

[matthew.gerlach]

[-- Attachment #1: Type: text/plain, Size: 7686 bytes --]



On Thu, 29 Dec 2022, Tom Rix wrote:

>
> On 12/28/22 10:16 AM, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> 
>> Add documentation describing the extensions provided by Version
>> 1 of the Device Feature Header (DFHv1).
>> 
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>> ---
>> v8: fix section titles
>> 
>> v7: shorten long lines and wording suggestions by bagasdotme@gmail.com
>> 
>> v6: no change
>> 
>> v5: use nested list for field descriptions
>>      clean up prose
>>      add reviewed-by and comments from Ilpo Järvinen
>> 
>> v4: Remove marketing speak and separate v0 and v1 descriptions.
>>      Fix errors reported by "make htmldocs".
>> 
>> v3: no change
>> 
>> v2: s/GUILD/GUID/
>>      add picture
>> ---
>>   Documentation/fpga/dfl.rst | 112 +++++++++++++++++++++++++++++++++++++
>>   1 file changed, 112 insertions(+)
>> 
>> diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
>> index 15b670926084..264b476fc6ac 100644
>> --- a/Documentation/fpga/dfl.rst
>> +++ b/Documentation/fpga/dfl.rst
>> @@ -561,6 +561,118 @@ new DFL feature via UIO direct access, its feature id 
>> should be added to the
>>   driver's id_table.
>> 
>
> I think a better location for this section would be in the beginning after 
> the
>
> Device Feature List (DFL) Overview
>
> The reader will be looking for the details of the Header once they have read 
> the overview.
>
> It would be handy if they were next

I agree that moving the new Documentation immediately after the Overview 
would be better.

>
>> +Device Feature Header - Version 0
>> +=================================
>> +Version 0 (DFHv0) is the original version of the Device Feature Header.
>> +The format of DFHv0 is shown below::
>> +
>> + 
>> +-----------------------------------------------------------------------+
>> +    |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 
>> 0| 0x00
>> + 
>> +-----------------------------------------------------------------------+
>> +    |63                                 GUID_L 
>> 0| 0x08
>> + 
>> +-----------------------------------------------------------------------+
>> +    |63                                 GUID_H 
>> 0| 0x10
>> + 
>> +-----------------------------------------------------------------------+
>> +
>> +- Offset 0x00
>> +
>> +  * Type - The type of DFH (e.g. FME, AFU, or private feature).
>> +  * DFH VER - The version of the DFH.
>> +  * Rsvd - Currently unused.
>> +  * EOL - Set if the DFH is the end of the Device Feature List (DFL).
>> +  * Next - The offset of the next DFH in the DFL from the DFH start.
>> +    If EOL is set, Next is the size of MMIO of the last feature in the 
>> list.
>
> Missed describing feature revision bits 12-15
>
> There as two VER's, it would be clearer if they were different. maybe REV for 
> bits 12-15
>
> Similar below.

Thanks for the feedback.  REV is a good suggestion and more accurate.

>
>> +  * ID - The feature ID if Type is private feature.
>> +
>> +- Offset 0x08
>> +
>> +  * GUID_L - Least significant 64 bits of a 128-bit Globally Unique 
>> Identifier
>> +    (present only if Type is FME or AFU).
>> +
>> +- Offset 0x10
>> +
>> +  * GUID_H - Most significant 64 bits of a 128-bit Globally Unique 
>> Identifier
>> +    (present only if Type is FME or AFU).
>> +
>> +
>> +Device Feature Header - Version 1
>> +=================================
>> +Version 1 (DFHv1) of the Device Feature Header adds the following 
>> functionality:
>> +
>> +* Provides a standardized mechanism for features to describe
>> +  parameters/capabilities to software.
>> +* Standardize the use of a GUID for all DFHv1 types.
>> +* Decouples the DFH location from the register space of the feature 
>> itself.
>> +
>> +The format of Version 1 of the Device Feature Header (DFH) is shown 
>> below::
>> +
>> + 
>> +-----------------------------------------------------------------------+
>> +    |63 Type 60|59 DFH VER 52|51 Rsvd 41|40 EOL|39 Next 16|15 VER 12|11 ID 
>> 0| 0x00
>> + 
>> +-----------------------------------------------------------------------+
>> +    |63                                 GUID_L 
>> 0| 0x08
>> + 
>> +-----------------------------------------------------------------------+
>> +    |63                                 GUID_H 
>> 0| 0x10
>> + 
>> +-----------------------------------------------------------------------+
>> +    |63                   Reg Address/Offset                      1|  Rel 
>> 0| 0x18
>> + 
>> +-----------------------------------------------------------------------+
>> +    |63        Reg Size       32|Params 31|30 Group    16|15 Instance 
>> 0| 0x20
>> + 
>> +-----------------------------------------------------------------------+
>> +    |63 Next    35|34RSV33|EOP32|31 Param Version 16|15 Param ID 
>> 0| 0x28
>> + 
>> +-----------------------------------------------------------------------+
>> +    |63                 Parameter Data 
>> 0| 0x30
>> + 
>> +-----------------------------------------------------------------------+
>> +
>> +                                  ...
>> +
>> + 
>> +-----------------------------------------------------------------------+
>> +    |63 Next    35|34RSV33|EOP32|31 Param Version 16|15 Param ID 
>> 0|
>> + 
>> +-----------------------------------------------------------------------+
>> +    |63                 Parameter Data 
>> 0|
>> + 
>> +-----------------------------------------------------------------------+
>> +
>> +- Offset 0x00
>> +
>> +  * Type - The type of DFH (e.g. FME, AFU, or private feature).
>> +  * DFH VER - The version of the DFH.
>> +  * Rsvd - Currently unused.
>> +  * EOL - Set if the DFH is the end of the Device Feature List (DFL).
>> +  * Next - The offset of the next DFH in the DFL from the DFH start.
>> +    If EOL is set, Next is the size of MMIO of the last feature in the 
>> list.
>
> Units of size ?
>
> Tom

Historically, the units of the size been in bytes. It might make sense to 
redefine this field to bits 39-19 and change the unit to 64 bit words. This
would also be consistent with DFHv1 parameter blocks and enforce 64 bit 
alignment of the DFH.

Thanks for the feedback,
Matthew Gerlach

>
>> +  * ID - The feature ID if Type is private feature.
>> +
>> +- Offset 0x08
>> +
>> +  * GUID_L - Least significant 64 bits of a 128-bit Globally Unique 
>> Identifier.
>> +
>> +- Offset 0x10
>> +
>> +  * GUID_H - Most significant 64 bits of a 128-bit Globally Unique 
>> Identifier.
>> +
>> +- Offset 0x18
>> +
>> +  * Reg Address/Offset - If Rel bit is set, then the value is the high 63 
>> bits
>> +    of a 16-bit aligned absolute address of the feature's registers. 
>> Otherwise
>> +    the value is the offset from the start of the DFH of the feature's 
>> registers.
>> +
>> +- Offset 0x20
>> +
>> +  * Reg Size - Size of feature's register set in bytes.
>> +  * Params - Set if DFH has a list of parameter blocks.
>> +  * Group - Id of group if feature is part of a group.
>> +  * Instance - Id of feature instance within a group.
>> +
>> +- Offset 0x28 if feature has parameters
>> +
>> +  * Next - Offset to the next parameter block in 8 byte words. If EOP set,
>> +    size in 8 byte words of last parameter.
>> +  * Param Version - Version of Param ID.
>> +  * Param ID - ID of parameter.
>> +
>> +- Offset 0x30
>> +
>> +  * Parameter Data - Parameter data whose size and format is defined by
>> +    version and ID of the parameter.
>> +
>>   Open discussion
>>   ===============
>>   FME driver exports one ioctl (DFL_FPGA_FME_PORT_PR) for partial 
>> reconfiguration
>
>

Re: [PATCH v8 3/4] fpga: dfl: add basic support for DFHv1

[Andy Shevchenko]

On Thu, Dec 29, 2022 at 08:18:03AM -0800, Tom Rix wrote:
> On 12/28/22 10:16 AM, matthew.gerlach@linux.intel.com wrote:

...

> >   struct dfl_feature_info {
> >   	u16 fid;
> >   	u8 revision;
> > +	u8 dfh_version;
> >   	struct resource mmio_res;
> >   	void __iomem *ioaddr;
> >   	struct list_head node;
> >   	unsigned int irq_base;
> >   	unsigned int nr_irqs;
> > +	unsigned int param_size;
> > +	u64 params[];
> u64 *params

This will break the overflow.h macros, no?
Besides that it will break the code for sure as it's not an equivalent.

> >   };


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 3/4] fpga: dfl: add basic support for DFHv1

[Xu Yilun]

On 2022-12-28 at 10:16:23 -0800, matthew.gerlach@linux.intel.com wrote:
> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> 
> Version 1 of the Device Feature Header (DFH) definition adds
> functionality to the DFL bus.
> 
> A DFHv1 header may have one or more parameter blocks that
> further describes the HW to SW.  Add support to the DFL bus
> to parse the MSI-X parameter.
> 
> The location of a feature's register set is explicitly
> described in DFHv1 and can be relative to the base of the DFHv1
> or an absolute address.  Parse the location and pass the information
> to DFL driver.
> 
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
> v8: use struct_size() from overflow.h
>     add dfh_get_u64_param_vals()

Could you help check my comments?
https://lore.kernel.org/linux-fpga/alpine.DEB.2.22.394.2212211421210.570436@rhweight-WRK1/T/#md86e3836130ebacd3c088f5c512ba741aac8a4d1

[...]

> 
> +static u64 *find_param(u64 *params, resource_size_t max, int param_id)
> +{
> +	u64 *end = params + max / sizeof(u64);
> +	u64 v, next;
> +
> +	while (params < end) {
> +		v = *params;
> +		if (param_id == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
> +			return params;
> +
> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
> +		params += next;
> +		if (FIELD_GET(DFHv1_PARAM_HDR_NEXT_EOP, v))
> +			break;
> +	}
> +
> +	return NULL;
> +}
> +
> +/**
> + * dfh_find_param() - find parameter block for the given parameter id
> + * @dfl_dev: dfl device
> + * @param: id of dfl parameter
> + *
> + * Return: pointer to start of parameter block, NULL otherwise.
> + */
> +u64 *dfh_find_param(struct dfl_device *dfl_dev, int param_id)
> +{
> +	return find_param(dfl_dev->params, dfl_dev->param_size, param_id);
> +}
> +EXPORT_SYMBOL_GPL(dfh_find_param);

Didn't find where to use it?

> +
> +/**
> + * dfh_get_u64_param_vals() - get array of u64 param values for given parameter id

There is no rule to say one u64 for each property in the parameter block.
So I don't see the reason for DFL core to provide u64 array for the API,
And the size of the parameter block is decided by HW, why make the user
input the value?

As we discussed before, dfl core doesn't try to look into the parameter
block. So please just provide the const void *data & data_size for drivers.
This is the most common way to represent a data block.

Thanks,
Yilun

> + * @dfl_dev: dfl device
> + * @param: id of dfl parameter
> + * @pval: location of parameter data destination
> + * @nvals: number of u64 elements of parameter data
> + *
> + * Return: pointer to start of parameter block, PTR_ERR otherwise
> + */
> +u64 *dfh_get_u64_param_vals(struct dfl_device *dfl_dev, int param_id, u64 *pval, int nvals)
> +{
> +	u64 *param = find_param(dfl_dev->params, dfl_dev->param_size, param_id);
> +	u64 next;
> +	int i;
> +
> +	if (!param)
> +		return ERR_PTR(-ENOENT);
> +
> +	next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, *param);
> +
> +	if (nvals >= next)
> +		return ERR_PTR(-ENOMEM);
> +
> +	for (i = 0; i < nvals; i++)
> +		*pval++ = param[i + 1];
> +
> +	return param;
> +}
> +EXPORT_SYMBOL_GPL(dfh_get_u64_param_vals);
> +

Re: [PATCH v8 3/4] fpga: dfl: add basic support for DFHv1

[matthew.gerlach]

On Thu, 29 Dec 2022, Andy Shevchenko wrote:

> On Thu, Dec 29, 2022 at 08:18:03AM -0800, Tom Rix wrote:
>> On 12/28/22 10:16 AM, matthew.gerlach@linux.intel.com wrote:
>
> ...
>
>>>   struct dfl_feature_info {
>>>   	u16 fid;
>>>   	u8 revision;
>>> +	u8 dfh_version;
>>>   	struct resource mmio_res;
>>>   	void __iomem *ioaddr;
>>>   	struct list_head node;
>>>   	unsigned int irq_base;
>>>   	unsigned int nr_irqs;
>>> +	unsigned int param_size;
>>> +	u64 params[];
>> u64 *params
>
> This will break the overflow.h macros, no?
> Besides that it will break the code for sure as it's not an equivalent.
>
>>>   };

I don't understand how this will break the overflow.h macros. The 
definition of struct dfl_feature_info and all of its uses are in a single 
file, dfl.c.

Matthew Gerlach


>
>
> -- 
> With Best Regards,
> Andy Shevchenko
>
>
>

Re: [PATCH v8 3/4] fpga: dfl: add basic support for DFHv1

[Andy Shevchenko]

On Mon, Jan 02, 2023 at 08:54:48AM -0800, matthew.gerlach@linux.intel.com wrote:
> On Thu, 29 Dec 2022, Andy Shevchenko wrote:
> > On Thu, Dec 29, 2022 at 08:18:03AM -0800, Tom Rix wrote:
> > > On 12/28/22 10:16 AM, matthew.gerlach@linux.intel.com wrote:

...

> > > > +	u64 params[];
> > > u64 *params

> > This will break the overflow.h macros, no?
> > Besides that it will break the code for sure as it's not an equivalent.

> I don't understand how this will break the overflow.h macros. The definition
> of struct dfl_feature_info and all of its uses are in a single file, dfl.c.

Hint: __must_be_array()

As I said, the proposed change is not acceptable since it's not an equivalent.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 3/4] fpga: dfl: add basic support for DFHv1

[Andy Shevchenko]

On Mon, Jan 02, 2023 at 07:27:06PM +0200, Andy Shevchenko wrote:
> On Mon, Jan 02, 2023 at 08:54:48AM -0800, matthew.gerlach@linux.intel.com wrote:
> > On Thu, 29 Dec 2022, Andy Shevchenko wrote:
> > > On Thu, Dec 29, 2022 at 08:18:03AM -0800, Tom Rix wrote:
> > > > On 12/28/22 10:16 AM, matthew.gerlach@linux.intel.com wrote:

...

> > > > > +	u64 params[];
> > > > u64 *params
> 
> > > This will break the overflow.h macros, no?
> > > Besides that it will break the code for sure as it's not an equivalent.
> 
> > I don't understand how this will break the overflow.h macros. The definition
> > of struct dfl_feature_info and all of its uses are in a single file, dfl.c.
> 
> Hint: __must_be_array()
> 
> As I said, the proposed change is not acceptable since it's not an equivalent.

Ah, you meant that there is no use of macros from overflow in the dfl.c?
IIRC we discussed that some of the code may make use of them, or am I
mistaken?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v8 3/4] fpga: dfl: add basic support for DFHv1

[matthew.gerlach]

On Mon, 2 Jan 2023, Andy Shevchenko wrote:

> On Mon, Jan 02, 2023 at 07:27:06PM +0200, Andy Shevchenko wrote:
>> On Mon, Jan 02, 2023 at 08:54:48AM -0800, matthew.gerlach@linux.intel.com wrote:
>>> On Thu, 29 Dec 2022, Andy Shevchenko wrote:
>>>> On Thu, Dec 29, 2022 at 08:18:03AM -0800, Tom Rix wrote:
>>>>> On 12/28/22 10:16 AM, matthew.gerlach@linux.intel.com wrote:
>
> ...
>
>>>>>> +	u64 params[];
>>>>> u64 *params
>>
>>>> This will break the overflow.h macros, no?
>>>> Besides that it will break the code for sure as it's not an equivalent.
>>
>>> I don't understand how this will break the overflow.h macros. The definition
>>> of struct dfl_feature_info and all of its uses are in a single file, dfl.c.
>>
>> Hint: __must_be_array()
>>
>> As I said, the proposed change is not acceptable since it's not an equivalent.
>
> Ah, you meant that there is no use of macros from overflow in the dfl.c?
> IIRC we discussed that some of the code may make use of them, or am I
> mistaken?

There currently is one usage of struct_size() from overflow.h in dfl.c, 
and my patch adds another usage of struct_size(). struct dfl_feature_info 
ends with a trailing array of u64.

I think the confusion is with struct dfl_feature and/or struct struct 
dfl_device. Those structs don't end with a trailing array, and those 
structs are not used with macros from overview.h.

Thanks for the feedback,
Matthew Gerlach

>
> -- 
> With Best Regards,
> Andy Shevchenko
>
>
>

Re: [PATCH v8 3/4] fpga: dfl: add basic support for DFHv1

[matthew.gerlach]

[-- Attachment #1: Type: text/plain, Size: 3623 bytes --]



On Fri, 30 Dec 2022, Xu Yilun wrote:

> On 2022-12-28 at 10:16:23 -0800, matthew.gerlach@linux.intel.com wrote:
>> From: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>>
>> Version 1 of the Device Feature Header (DFH) definition adds
>> functionality to the DFL bus.
>>
>> A DFHv1 header may have one or more parameter blocks that
>> further describes the HW to SW.  Add support to the DFL bus
>> to parse the MSI-X parameter.
>>
>> The location of a feature's register set is explicitly
>> described in DFHv1 and can be relative to the base of the DFHv1
>> or an absolute address.  Parse the location and pass the information
>> to DFL driver.
>>
>> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
>> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>> ---
>> v8: use struct_size() from overflow.h
>>     add dfh_get_u64_param_vals()
>
> Could you help check my comments?
> https://lore.kernel.org/linux-fpga/alpine.DEB.2.22.394.2212211421210.570436@rhweight-WRK1/T/#md86e3836130ebacd3c088f5c512ba741aac8a4d1


Sorry I missed your earlier comments. I have since responded to them 
specificially.


>
> [...]
>
>>
>> +static u64 *find_param(u64 *params, resource_size_t max, int param_id)
>> +{
>> +	u64 *end = params + max / sizeof(u64);
>> +	u64 v, next;
>> +
>> +	while (params < end) {
>> +		v = *params;
>> +		if (param_id == FIELD_GET(DFHv1_PARAM_HDR_ID, v))
>> +			return params;
>> +
>> +		next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, v);
>> +		params += next;
>> +		if (FIELD_GET(DFHv1_PARAM_HDR_NEXT_EOP, v))
>> +			break;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +/**
>> + * dfh_find_param() - find parameter block for the given parameter id
>> + * @dfl_dev: dfl device
>> + * @param: id of dfl parameter
>> + *
>> + * Return: pointer to start of parameter block, NULL otherwise.
>> + */
>> +u64 *dfh_find_param(struct dfl_device *dfl_dev, int param_id)
>> +{
>> +	return find_param(dfl_dev->params, dfl_dev->param_size, param_id);
>> +}
>> +EXPORT_SYMBOL_GPL(dfh_find_param);
>
> Didn't find where to use it?

I understand. Don't export a function unless there is a comsumer for it.

>
>> +
>> +/**
>> + * dfh_get_u64_param_vals() - get array of u64 param values for given parameter id
>
> There is no rule to say one u64 for each property in the parameter block.
> So I don't see the reason for DFL core to provide u64 array for the API,
> And the size of the parameter block is decided by HW, why make the user
> input the value?
>
> As we discussed before, dfl core doesn't try to look into the parameter
> block. So please just provide the const void *data & data_size for drivers.
> This is the most common way to represent a data block.

I will move the parameter parsing helper function to the driver itself.

Thanks for the feedback,
Matthew Gerlach

>
> Thanks,
> Yilun
>
>> + * @dfl_dev: dfl device
>> + * @param: id of dfl parameter
>> + * @pval: location of parameter data destination
>> + * @nvals: number of u64 elements of parameter data
>> + *
>> + * Return: pointer to start of parameter block, PTR_ERR otherwise
>> + */
>> +u64 *dfh_get_u64_param_vals(struct dfl_device *dfl_dev, int param_id, u64 *pval, int nvals)
>> +{
>> +	u64 *param = find_param(dfl_dev->params, dfl_dev->param_size, param_id);
>> +	u64 next;
>> +	int i;
>> +
>> +	if (!param)
>> +		return ERR_PTR(-ENOENT);
>> +
>> +	next = FIELD_GET(DFHv1_PARAM_HDR_NEXT_OFFSET, *param);
>> +
>> +	if (nvals >= next)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	for (i = 0; i < nvals; i++)
>> +		*pval++ = param[i + 1];
>> +
>> +	return param;
>> +}
>> +EXPORT_SYMBOL_GPL(dfh_get_u64_param_vals);
>> +
>