[PATCH 0/2] Modularization of DFL private feature drivers

[PATCH 0/2] Modularization of DFL private feature drivers

[Xu Yilun]

This patchset makes it possible to develop independent driver modules
for DFL private features. It also helps to leverage existing kernel
drivers to enable some IP blocks in DFL.

Xu Yilun (2):
  fpga: dfl: map feature mmio resources in their own feature drivers
  fpga: dfl: create a dfl bus type to support DFL devices

 Documentation/ABI/testing/sysfs-bus-dfl |  15 ++
 drivers/fpga/dfl-pci.c                  |  21 +-
 drivers/fpga/dfl.c                      | 435 +++++++++++++++++++++++++++-----
 drivers/fpga/dfl.h                      |  91 ++++++-
 4 files changed, 492 insertions(+), 70 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl

-- 
2.7.4

[PATCH 1/2] fpga: dfl: map feature mmio resources in their own feature drivers

[Xu Yilun]

This patch makes preparation for modularization of DFL sub feature
drivers.

Currently, if we need to support a new DFL sub feature, an entry should
be added to fme/port_feature_drvs[] in dfl-fme/port-main.c. And we need
to re-compile the whole DFL modules. That make the DFL drivers hard to be
extended.

Another consideration is that DFL may contain some IP blocks which are
already supported by kernel, most of them are supported by platform
device drivers. We could create platform devices for these IP blocks and
get them supported by these drivers.

An important issue is that platform device drivers usually requests mmio
resources on probe. But now dfl mmio is mapped in dfl bus driver (e.g.
dfl-pci) as a whole region. Then platform device drivers for sub features
can't request their own mmio resources again. This is what the patch
trying to resolve.

This patch changes the DFL enumeration. DFL bus driver will unmap mmio
resources after first step enumeration and pass enumeration info to DFL
framework. Then DFL framework will map the mmio resources again, do 2nd
step enumeration, and also unmap the mmio resources. In this way, sub
feature drivers could then request their own mmio resources as needed.

An exception is that mmio resource of FIU headers are still mapped in dfl
bus driver. The FIU headers have some fundamental functions (sriov set,
port enable/disable) needed for dfl bus devices and other sub features.
They should not be unmapped as long as dfl bus device is alive.

Signed-off-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Wu Hao <hao.wu@intel.com>
Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Signed-off-by: Russ Weight <russell.h.weight@intel.com>
---
 drivers/fpga/dfl-pci.c |  21 ++++--
 drivers/fpga/dfl.c     | 187 +++++++++++++++++++++++++++++++++++--------------
 drivers/fpga/dfl.h     |   6 +-
 3 files changed, 152 insertions(+), 62 deletions(-)

diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
index e220bec..22dc025 100644
--- a/drivers/fpga/dfl-pci.c
+++ b/drivers/fpga/dfl-pci.c
@@ -39,6 +39,11 @@ static void __iomem *cci_pci_ioremap_bar(struct pci_dev *pcidev, int bar)
 	return pcim_iomap_table(pcidev)[bar];
 }
 
+static void cci_pci_iounmap_bars(struct pci_dev *pcidev, int mapped_bars)
+{
+	pcim_iounmap_regions(pcidev, mapped_bars);
+}
+
 static int cci_pci_alloc_irq(struct pci_dev *pcidev)
 {
 	int ret, nvec = pci_msix_vec_count(pcidev);
@@ -123,7 +128,7 @@ static int *cci_pci_create_irq_table(struct pci_dev *pcidev, unsigned int nvec)
 static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
 {
 	struct cci_drvdata *drvdata = pci_get_drvdata(pcidev);
-	int port_num, bar, i, nvec, ret = 0;
+	int port_num, bar, i, nvec, mapped_bars, ret = 0;
 	struct dfl_fpga_enum_info *info;
 	struct dfl_fpga_cdev *cdev;
 	resource_size_t start, len;
@@ -163,6 +168,8 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
 		goto irq_free_exit;
 	}
 
+	mapped_bars = BIT(0);
+
 	/*
 	 * PF device has FME and Ports/AFUs, and VF device only has one
 	 * Port/AFU. Check them and add related "Device Feature List" info
@@ -172,7 +179,7 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
 		start = pci_resource_start(pcidev, 0);
 		len = pci_resource_len(pcidev, 0);
 
-		dfl_fpga_enum_info_add_dfl(info, start, len, base);
+		dfl_fpga_enum_info_add_dfl(info, start, len);
 
 		/*
 		 * find more Device Feature Lists (e.g. Ports) per information
@@ -200,22 +207,26 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
 			if (!base)
 				continue;
 
+			mapped_bars |= BIT(bar);
+
 			start = pci_resource_start(pcidev, bar) + offset;
 			len = pci_resource_len(pcidev, bar) - offset;
 
-			dfl_fpga_enum_info_add_dfl(info, start, len,
-						   base + offset);
+			dfl_fpga_enum_info_add_dfl(info, start, len);
 		}
 	} else if (dfl_feature_is_port(base)) {
 		start = pci_resource_start(pcidev, 0);
 		len = pci_resource_len(pcidev, 0);
 
-		dfl_fpga_enum_info_add_dfl(info, start, len, base);
+		dfl_fpga_enum_info_add_dfl(info, start, len);
 	} else {
 		ret = -ENODEV;
 		goto irq_free_exit;
 	}
 
+	/* release I/O mappings for next step enumeration */
+	cci_pci_iounmap_bars(pcidev, mapped_bars);
+
 	/* start enumeration with prepared enumeration information */
 	cdev = dfl_fpga_feature_devs_enumerate(info);
 	if (IS_ERR(cdev)) {
diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index 649958a..7dc6411 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -250,6 +250,11 @@ int dfl_fpga_check_port_id(struct platform_device *pdev, void *pport_id)
 }
 EXPORT_SYMBOL_GPL(dfl_fpga_check_port_id);
 
+static bool is_header_feature(struct dfl_feature *feature)
+{
+	return feature->id == FEATURE_ID_FIU_HEADER;
+}
+
 /**
  * dfl_fpga_dev_feature_uinit - uinit for sub features of dfl feature device
  * @pdev: feature device.
@@ -273,8 +278,20 @@ static int dfl_feature_instance_init(struct platform_device *pdev,
 				     struct dfl_feature *feature,
 				     struct dfl_feature_driver *drv)
 {
+	void __iomem *base;
 	int ret = 0;
 
+	if (!is_header_feature(feature)) {
+		base = devm_platform_ioremap_resource(pdev,
+						      feature->resource_index);
+		if (IS_ERR(base)) {
+			dev_err(&pdev->dev, "fail to get iomem resource!\n");
+			return PTR_ERR(base);
+		}
+
+		feature->ioaddr = base;
+	}
+
 	if (drv->ops->init) {
 		ret = drv->ops->init(pdev, feature);
 		if (ret)
@@ -427,7 +444,9 @@ EXPORT_SYMBOL_GPL(dfl_fpga_dev_ops_unregister);
  * @irq_table: Linux IRQ numbers for all irqs, indexed by local irq index of
  *	       this device.
  * @feature_dev: current feature device.
- * @ioaddr: header register region address of feature device in enumeration.
+ * @ioaddr: header register region address of current FIU in enumeration.
+ * @start: register resource start of current FIU.
+ * @len: max register resource length of current FIU.
  * @sub_features: a sub features linked list for feature device in enumeration.
  * @feature_num: number of sub features for feature device in enumeration.
  */
@@ -439,6 +458,9 @@ struct build_feature_devs_info {
 
 	struct platform_device *feature_dev;
 	void __iomem *ioaddr;
+	resource_size_t start;
+	resource_size_t len;
+
 	struct list_head sub_features;
 	int feature_num;
 };
@@ -484,10 +506,7 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
 	struct dfl_feature_platform_data *pdata;
 	struct dfl_feature_info *finfo, *p;
 	enum dfl_id_type type;
-	int ret, index = 0;
-
-	if (!fdev)
-		return 0;
+	int ret, index = 0, res_idx = 0;
 
 	type = feature_dev_id_type(fdev);
 	if (WARN_ON_ONCE(type >= DFL_ID_MAX))
@@ -530,16 +549,30 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
 
 	/* fill features and resource information for feature dev */
 	list_for_each_entry_safe(finfo, p, &binfo->sub_features, node) {
-		struct dfl_feature *feature = &pdata->features[index];
+		struct dfl_feature *feature = &pdata->features[index++];
 		struct dfl_feature_irq_ctx *ctx;
 		unsigned int i;
 
 		/* save resource information for each feature */
 		feature->dev = fdev;
 		feature->id = finfo->fid;
-		feature->resource_index = index;
-		feature->ioaddr = finfo->ioaddr;
-		fdev->resource[index++] = finfo->mmio_res;
+
+		/*
+		 * map header resource for dfl bus device. Don't add header
+		 * resource to feature devices, or the resource tree will be
+		 * disordered and cause warning on resource release
+		 */
+		if (is_header_feature(feature)) {
+			feature->resource_index = -1;
+			feature->ioaddr =
+				devm_ioremap_resource(binfo->dev,
+						      &finfo->mmio_res);
+			if (IS_ERR(feature->ioaddr))
+				return PTR_ERR(feature->ioaddr);
+		} else {
+			feature->resource_index = res_idx;
+			fdev->resource[res_idx++] = finfo->mmio_res;
+		}
 
 		if (finfo->nr_irqs) {
 			ctx = devm_kcalloc(binfo->dev, finfo->nr_irqs,
@@ -582,19 +615,13 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
 
 static int
 build_info_create_dev(struct build_feature_devs_info *binfo,
-		      enum dfl_id_type type, void __iomem *ioaddr)
+		      enum dfl_id_type type)
 {
 	struct platform_device *fdev;
-	int ret;
 
 	if (type >= DFL_ID_MAX)
 		return -EINVAL;
 
-	/* we will create a new device, commit current device first */
-	ret = build_info_commit_dev(binfo);
-	if (ret)
-		return ret;
-
 	/*
 	 * we use -ENODEV as the initialization indicator which indicates
 	 * whether the id need to be reclaimed
@@ -605,7 +632,7 @@ build_info_create_dev(struct build_feature_devs_info *binfo,
 
 	binfo->feature_dev = fdev;
 	binfo->feature_num = 0;
-	binfo->ioaddr = ioaddr;
+
 	INIT_LIST_HEAD(&binfo->sub_features);
 
 	fdev->id = dfl_id_alloc(type, &fdev->dev);
@@ -747,18 +774,17 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
  */
 static int
 create_feature_instance(struct build_feature_devs_info *binfo,
-			struct dfl_fpga_enum_dfl *dfl, resource_size_t ofst,
-			resource_size_t size, u64 fid)
+			resource_size_t ofst, resource_size_t size, u64 fid)
 {
 	unsigned int irq_base, nr_irqs;
 	struct dfl_feature_info *finfo;
 	int ret;
 
 	/* read feature size and id if inputs are invalid */
-	size = size ? size : feature_size(dfl->ioaddr + ofst);
-	fid = fid ? fid : feature_id(dfl->ioaddr + ofst);
+	size = size ? size : feature_size(binfo->ioaddr + ofst);
+	fid = fid ? fid : feature_id(binfo->ioaddr + ofst);
 
-	if (dfl->len - ofst < size)
+	if (binfo->len - ofst < size)
 		return -EINVAL;
 
 	ret = parse_feature_irqs(binfo, ofst, fid, &irq_base, &nr_irqs);
@@ -770,12 +796,11 @@ create_feature_instance(struct build_feature_devs_info *binfo,
 		return -ENOMEM;
 
 	finfo->fid = fid;
-	finfo->mmio_res.start = dfl->start + ofst;
+	finfo->mmio_res.start = binfo->start + ofst;
 	finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
 	finfo->mmio_res.flags = IORESOURCE_MEM;
 	finfo->irq_base = irq_base;
 	finfo->nr_irqs = nr_irqs;
-	finfo->ioaddr = dfl->ioaddr + ofst;
 
 	list_add_tail(&finfo->node, &binfo->sub_features);
 	binfo->feature_num++;
@@ -784,7 +809,6 @@ create_feature_instance(struct build_feature_devs_info *binfo,
 }
 
 static int parse_feature_port_afu(struct build_feature_devs_info *binfo,
-				  struct dfl_fpga_enum_dfl *dfl,
 				  resource_size_t ofst)
 {
 	u64 v = readq(binfo->ioaddr + PORT_HDR_CAP);
@@ -792,11 +816,10 @@ static int parse_feature_port_afu(struct build_feature_devs_info *binfo,
 
 	WARN_ON(!size);
 
-	return create_feature_instance(binfo, dfl, ofst, size, FEATURE_ID_AFU);
+	return create_feature_instance(binfo, ofst, size, FEATURE_ID_AFU);
 }
 
 static int parse_feature_afu(struct build_feature_devs_info *binfo,
-			     struct dfl_fpga_enum_dfl *dfl,
 			     resource_size_t ofst)
 {
 	if (!binfo->feature_dev) {
@@ -806,7 +829,7 @@ static int parse_feature_afu(struct build_feature_devs_info *binfo,
 
 	switch (feature_dev_id_type(binfo->feature_dev)) {
 	case PORT_ID:
-		return parse_feature_port_afu(binfo, dfl, ofst);
+		return parse_feature_port_afu(binfo, ofst);
 	default:
 		dev_info(binfo->dev, "AFU belonging to FIU %s is not supported yet.\n",
 			 binfo->feature_dev->name);
@@ -815,35 +838,91 @@ static int parse_feature_afu(struct build_feature_devs_info *binfo,
 	return 0;
 }
 
+static bool is_feature_dev_detected(struct build_feature_devs_info *binfo)
+{
+	return !!binfo->feature_dev;
+}
+
+static void dfl_binfo_shift(struct build_feature_devs_info *binfo,
+			    resource_size_t ofst)
+{
+	binfo->start = binfo->start + ofst;
+	binfo->len = binfo->len - ofst;
+}
+
+static int dfl_binfo_prepare(struct build_feature_devs_info *binfo,
+			     resource_size_t start, resource_size_t len)
+{
+	struct device *dev = binfo->dev;
+	void __iomem *ioaddr;
+
+	if (!devm_request_mem_region(dev, start, len, dev_name(dev))) {
+		dev_err(dev, "request region fail, start:%pa, len:%pa\n",
+			&start, &len);
+		return -ENOMEM;
+	}
+
+	ioaddr = devm_ioremap(dev, start, len);
+	if (!ioaddr) {
+		dev_err(dev, "ioremap region fail, start:%pa, len:%pa\n",
+			&start, &len);
+		devm_release_mem_region(dev, start, len);
+		return -EFAULT;
+	}
+
+	binfo->start = start;
+	binfo->len = len;
+	binfo->ioaddr = ioaddr;
+
+	return 0;
+}
+
+static void dfl_binfo_finish(struct build_feature_devs_info *binfo)
+{
+	devm_iounmap(binfo->dev, binfo->ioaddr);
+	devm_release_mem_region(binfo->dev, binfo->start, binfo->len);
+}
+
 static int parse_feature_fiu(struct build_feature_devs_info *binfo,
-			     struct dfl_fpga_enum_dfl *dfl,
 			     resource_size_t ofst)
 {
 	u32 id, offset;
 	u64 v;
 	int ret = 0;
 
-	v = readq(dfl->ioaddr + ofst + DFH);
+	if (is_feature_dev_detected(binfo)) {
+		dfl_binfo_finish(binfo);
+
+		ret = build_info_commit_dev(binfo);
+		if (ret)
+			return ret;
+
+		dfl_binfo_prepare(binfo, binfo->start + ofst,
+				  binfo->len - ofst);
+	} else {
+		dfl_binfo_shift(binfo, ofst);
+	}
+
+	v = readq(binfo->ioaddr + DFH);
 	id = FIELD_GET(DFH_ID, v);
 
 	/* create platform device for dfl feature dev */
-	ret = build_info_create_dev(binfo, dfh_id_to_type(id),
-				    dfl->ioaddr + ofst);
+	ret = build_info_create_dev(binfo, dfh_id_to_type(id));
 	if (ret)
 		return ret;
 
-	ret = create_feature_instance(binfo, dfl, ofst, 0, 0);
+	ret = create_feature_instance(binfo, 0, 0, 0);
 	if (ret)
 		return ret;
 	/*
 	 * find and parse FIU's child AFU via its NEXT_AFU register.
 	 * please note that only Port has valid NEXT_AFU pointer per spec.
 	 */
-	v = readq(dfl->ioaddr + ofst + NEXT_AFU);
+	v = readq(binfo->ioaddr + NEXT_AFU);
 
 	offset = FIELD_GET(NEXT_AFU_NEXT_DFH_OFST, v);
 	if (offset)
-		return parse_feature_afu(binfo, dfl, ofst + offset);
+		return parse_feature_afu(binfo, offset);
 
 	dev_dbg(binfo->dev, "No AFUs detected on FIU %d\n", id);
 
@@ -851,16 +930,15 @@ static int parse_feature_fiu(struct build_feature_devs_info *binfo,
 }
 
 static int parse_feature_private(struct build_feature_devs_info *binfo,
-				 struct dfl_fpga_enum_dfl *dfl,
 				 resource_size_t ofst)
 {
 	if (!binfo->feature_dev) {
 		dev_err(binfo->dev, "the private feature %llx does not belong to any AFU.\n",
-			(unsigned long long)feature_id(dfl->ioaddr + ofst));
+			(unsigned long long)feature_id(binfo->ioaddr + ofst));
 		return -EINVAL;
 	}
 
-	return create_feature_instance(binfo, dfl, ofst, 0, 0);
+	return create_feature_instance(binfo, ofst, 0, 0);
 }
 
 /**
@@ -868,24 +946,24 @@ static int parse_feature_private(struct build_feature_devs_info *binfo,
  *
  * @binfo: build feature devices information.
  * @dfl: device feature list to parse
- * @ofst: offset to feature header on this device feature list
+ * @ofst: offset to current FIU header
  */
 static int parse_feature(struct build_feature_devs_info *binfo,
-			 struct dfl_fpga_enum_dfl *dfl, resource_size_t ofst)
+			 resource_size_t ofst)
 {
 	u64 v;
 	u32 type;
 
-	v = readq(dfl->ioaddr + ofst + DFH);
+	v = readq(binfo->ioaddr + ofst + DFH);
 	type = FIELD_GET(DFH_TYPE, v);
 
 	switch (type) {
 	case DFH_TYPE_AFU:
-		return parse_feature_afu(binfo, dfl, ofst);
+		return parse_feature_afu(binfo, ofst);
 	case DFH_TYPE_PRIVATE:
-		return parse_feature_private(binfo, dfl, ofst);
+		return parse_feature_private(binfo, ofst);
 	case DFH_TYPE_FIU:
-		return parse_feature_fiu(binfo, dfl, ofst);
+		return parse_feature_fiu(binfo, ofst);
 	default:
 		dev_info(binfo->dev,
 			 "Feature Type %x is not supported.\n", type);
@@ -897,12 +975,18 @@ static int parse_feature(struct build_feature_devs_info *binfo,
 static int parse_feature_list(struct build_feature_devs_info *binfo,
 			      struct dfl_fpga_enum_dfl *dfl)
 {
-	void __iomem *start = dfl->ioaddr;
-	void __iomem *end = dfl->ioaddr + dfl->len;
+	resource_size_t start, end;
 	int ret = 0;
 	u32 ofst = 0;
 	u64 v;
 
+	ret = dfl_binfo_prepare(binfo, dfl->start, dfl->len);
+	if (ret)
+		return ret;
+
+	start = dfl->start;
+	end = start + dfl->len;
+
 	/* walk through the device feature list via DFH's next DFH pointer. */
 	for (; start < end; start += ofst) {
 		if (end - start < DFH_SIZE) {
@@ -910,11 +994,11 @@ static int parse_feature_list(struct build_feature_devs_info *binfo,
 			return -EINVAL;
 		}
 
-		ret = parse_feature(binfo, dfl, start - dfl->ioaddr);
+		ret = parse_feature(binfo, start - binfo->start);
 		if (ret)
 			return ret;
 
-		v = readq(start + DFH);
+		v = readq(binfo->ioaddr + start - binfo->start + DFH);
 		ofst = FIELD_GET(DFH_NEXT_HDR_OFST, v);
 
 		/* stop parsing if EOL(End of List) is set or offset is 0 */
@@ -923,6 +1007,8 @@ static int parse_feature_list(struct build_feature_devs_info *binfo,
 	}
 
 	/* commit current feature device when reach the end of list */
+	dfl_binfo_finish(binfo);
+
 	return build_info_commit_dev(binfo);
 }
 
@@ -976,7 +1062,6 @@ EXPORT_SYMBOL_GPL(dfl_fpga_enum_info_free);
  * @info: ptr to dfl_fpga_enum_info
  * @start: mmio resource address of the device feature list.
  * @len: mmio resource length of the device feature list.
- * @ioaddr: mapped mmio resource address of the device feature list.
  *
  * One FPGA device may have one or more Device Feature Lists (DFLs), use this
  * function to add information of each DFL to common data structure for next
@@ -985,8 +1070,7 @@ EXPORT_SYMBOL_GPL(dfl_fpga_enum_info_free);
  * Return: 0 on success, negative error code otherwise.
  */
 int dfl_fpga_enum_info_add_dfl(struct dfl_fpga_enum_info *info,
-			       resource_size_t start, resource_size_t len,
-			       void __iomem *ioaddr)
+			       resource_size_t start, resource_size_t len)
 {
 	struct dfl_fpga_enum_dfl *dfl;
 
@@ -996,7 +1080,6 @@ int dfl_fpga_enum_info_add_dfl(struct dfl_fpga_enum_info *info,
 
 	dfl->start = start;
 	dfl->len = len;
-	dfl->ioaddr = ioaddr;
 
 	list_add_tail(&dfl->node, &info->dfls);
 
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index a32dfba..f605c28 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -441,22 +441,18 @@ struct dfl_fpga_enum_info {
  *
  * @start: base address of this device feature list.
  * @len: size of this device feature list.
- * @ioaddr: mapped base address of this device feature list.
  * @node: node in list of device feature lists.
  */
 struct dfl_fpga_enum_dfl {
 	resource_size_t start;
 	resource_size_t len;
 
-	void __iomem *ioaddr;
-
 	struct list_head node;
 };
 
 struct dfl_fpga_enum_info *dfl_fpga_enum_info_alloc(struct device *dev);
 int dfl_fpga_enum_info_add_dfl(struct dfl_fpga_enum_info *info,
-			       resource_size_t start, resource_size_t len,
-			       void __iomem *ioaddr);
+			       resource_size_t start, resource_size_t len);
 int dfl_fpga_enum_info_add_irq(struct dfl_fpga_enum_info *info,
 			       unsigned int nr_irqs, int *irq_table);
 void dfl_fpga_enum_info_free(struct dfl_fpga_enum_info *info);
-- 
2.7.4

RE: [PATCH 1/2] fpga: dfl: map feature mmio resources in their own feature drivers

[Wu, Hao]

> -----Original Message-----
> From: linux-fpga-owner@vger.kernel.org <linux-fpga-owner@vger.kernel.org>
> On Behalf Of Xu Yilun
> Sent: Wednesday, July 15, 2020 1:38 PM
> To: mdf@kernel.org; linux-fpga@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Cc: trix@redhat.com; lgoncalv@redhat.com; Xu, Yilun <yilun.xu@intel.com>;
> Wu, Hao <hao.wu@intel.com>; Matthew Gerlach
> <matthew.gerlach@linux.intel.com>; Weight, Russell H
> <russell.h.weight@intel.com>
> Subject: [PATCH 1/2] fpga: dfl: map feature mmio resources in their own
> feature drivers
> 
> This patch makes preparation for modularization of DFL sub feature
> drivers.
> 
> Currently, if we need to support a new DFL sub feature, an entry should
> be added to fme/port_feature_drvs[] in dfl-fme/port-main.c. And we need
> to re-compile the whole DFL modules. That make the DFL drivers hard to be
> extended.
> 
> Another consideration is that DFL may contain some IP blocks which are
> already supported by kernel, most of them are supported by platform
> device drivers. We could create platform devices for these IP blocks and
> get them supported by these drivers.
> 
> An important issue is that platform device drivers usually requests mmio
> resources on probe. But now dfl mmio is mapped in dfl bus driver (e.g.
> dfl-pci) as a whole region. Then platform device drivers for sub features
> can't request their own mmio resources again. This is what the patch
> trying to resolve.
> 
> This patch changes the DFL enumeration. DFL bus driver will unmap mmio
> resources after first step enumeration and pass enumeration info to DFL
> framework. Then DFL framework will map the mmio resources again, do 2nd
> step enumeration, and also unmap the mmio resources. In this way, sub
> feature drivers could then request their own mmio resources as needed.
> 
> An exception is that mmio resource of FIU headers are still mapped in dfl
> bus driver. The FIU headers have some fundamental functions (sriov set,
> port enable/disable) needed for dfl bus devices and other sub features.
> They should not be unmapped as long as dfl bus device is alive.
> 
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> ---
>  drivers/fpga/dfl-pci.c |  21 ++++--
>  drivers/fpga/dfl.c     | 187 +++++++++++++++++++++++++++++++++++-----------
> ---
>  drivers/fpga/dfl.h     |   6 +-
>  3 files changed, 152 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
> index e220bec..22dc025 100644
> --- a/drivers/fpga/dfl-pci.c
> +++ b/drivers/fpga/dfl-pci.c
> @@ -39,6 +39,11 @@ static void __iomem *cci_pci_ioremap_bar(struct
> pci_dev *pcidev, int bar)
>  	return pcim_iomap_table(pcidev)[bar];
>  }
> 
> +static void cci_pci_iounmap_bars(struct pci_dev *pcidev, int mapped_bars)
> +{
> +	pcim_iounmap_regions(pcidev, mapped_bars);
> +}
> +
>  static int cci_pci_alloc_irq(struct pci_dev *pcidev)
>  {
>  	int ret, nvec = pci_msix_vec_count(pcidev);
> @@ -123,7 +128,7 @@ static int *cci_pci_create_irq_table(struct pci_dev
> *pcidev, unsigned int nvec)
>  static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
>  {
>  	struct cci_drvdata *drvdata = pci_get_drvdata(pcidev);
> -	int port_num, bar, i, nvec, ret = 0;
> +	int port_num, bar, i, nvec, mapped_bars, ret = 0;
>  	struct dfl_fpga_enum_info *info;
>  	struct dfl_fpga_cdev *cdev;
>  	resource_size_t start, len;
> @@ -163,6 +168,8 @@ static int cci_enumerate_feature_devs(struct pci_dev
> *pcidev)
>  		goto irq_free_exit;
>  	}
> 
> +	mapped_bars = BIT(0);
> +
>  	/*
>  	 * PF device has FME and Ports/AFUs, and VF device only has one
>  	 * Port/AFU. Check them and add related "Device Feature List" info
> @@ -172,7 +179,7 @@ static int cci_enumerate_feature_devs(struct pci_dev
> *pcidev)
>  		start = pci_resource_start(pcidev, 0);
>  		len = pci_resource_len(pcidev, 0);
> 
> -		dfl_fpga_enum_info_add_dfl(info, start, len, base);
> +		dfl_fpga_enum_info_add_dfl(info, start, len);
> 
>  		/*
>  		 * find more Device Feature Lists (e.g. Ports) per information
> @@ -200,22 +207,26 @@ static int cci_enumerate_feature_devs(struct
> pci_dev *pcidev)
>  			if (!base)
>  				continue;
> 
> +			mapped_bars |= BIT(bar);
> +
>  			start = pci_resource_start(pcidev, bar) + offset;
>  			len = pci_resource_len(pcidev, bar) - offset;
> 
> -			dfl_fpga_enum_info_add_dfl(info, start, len,
> -						   base + offset);
> +			dfl_fpga_enum_info_add_dfl(info, start, len);
>  		}
>  	} else if (dfl_feature_is_port(base)) {
>  		start = pci_resource_start(pcidev, 0);
>  		len = pci_resource_len(pcidev, 0);
> 
> -		dfl_fpga_enum_info_add_dfl(info, start, len, base);
> +		dfl_fpga_enum_info_add_dfl(info, start, len);
>  	} else {
>  		ret = -ENODEV;
>  		goto irq_free_exit;
>  	}
> 
> +	/* release I/O mappings for next step enumeration */
> +	cci_pci_iounmap_bars(pcidev, mapped_bars);
> +
>  	/* start enumeration with prepared enumeration information */
>  	cdev = dfl_fpga_feature_devs_enumerate(info);
>  	if (IS_ERR(cdev)) {
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index 649958a..7dc6411 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -250,6 +250,11 @@ int dfl_fpga_check_port_id(struct platform_device
> *pdev, void *pport_id)
>  }
>  EXPORT_SYMBOL_GPL(dfl_fpga_check_port_id);
> 
> +static bool is_header_feature(struct dfl_feature *feature)
> +{
> +	return feature->id == FEATURE_ID_FIU_HEADER;
> +}
> +
>  /**
>   * dfl_fpga_dev_feature_uinit - uinit for sub features of dfl feature device
>   * @pdev: feature device.
> @@ -273,8 +278,20 @@ static int dfl_feature_instance_init(struct
> platform_device *pdev,
>  				     struct dfl_feature *feature,
>  				     struct dfl_feature_driver *drv)
>  {
> +	void __iomem *base;
>  	int ret = 0;
> 
> +	if (!is_header_feature(feature)) {
> +		base = devm_platform_ioremap_resource(pdev,
> +						      feature->resource_index);
> +		if (IS_ERR(base)) {
> +			dev_err(&pdev->dev, "fail to get iomem
> resource!\n");

Maybe you want to show which feature failed with ioremap here?

> +			return PTR_ERR(base);
> +		}
> +
> +		feature->ioaddr = base;
> +	}
> +
>  	if (drv->ops->init) {
>  		ret = drv->ops->init(pdev, feature);
>  		if (ret)
> @@ -427,7 +444,9 @@ EXPORT_SYMBOL_GPL(dfl_fpga_dev_ops_unregister);
>   * @irq_table: Linux IRQ numbers for all irqs, indexed by local irq index of
>   *	       this device.
>   * @feature_dev: current feature device.
> - * @ioaddr: header register region address of feature device in enumeration.
> + * @ioaddr: header register region address of current FIU in enumeration.
> + * @start: register resource start of current FIU.
> + * @len: max register resource length of current FIU.
>   * @sub_features: a sub features linked list for feature device in
> enumeration.
>   * @feature_num: number of sub features for feature device in
> enumeration.
>   */
> @@ -439,6 +458,9 @@ struct build_feature_devs_info {
> 
>  	struct platform_device *feature_dev;
>  	void __iomem *ioaddr;
> +	resource_size_t start;
> +	resource_size_t len;
> +
>  	struct list_head sub_features;
>  	int feature_num;
>  };
> @@ -484,10 +506,7 @@ static int build_info_commit_dev(struct
> build_feature_devs_info *binfo)
>  	struct dfl_feature_platform_data *pdata;
>  	struct dfl_feature_info *finfo, *p;
>  	enum dfl_id_type type;
> -	int ret, index = 0;
> -
> -	if (!fdev)
> -		return 0;

Why you remove this checking?

> +	int ret, index = 0, res_idx = 0;
> 
>  	type = feature_dev_id_type(fdev);
>  	if (WARN_ON_ONCE(type >= DFL_ID_MAX))
> @@ -530,16 +549,30 @@ static int build_info_commit_dev(struct
> build_feature_devs_info *binfo)
> 
>  	/* fill features and resource information for feature dev */
>  	list_for_each_entry_safe(finfo, p, &binfo->sub_features, node) {
> -		struct dfl_feature *feature = &pdata->features[index];
> +		struct dfl_feature *feature = &pdata->features[index++];
>  		struct dfl_feature_irq_ctx *ctx;
>  		unsigned int i;
> 
>  		/* save resource information for each feature */
>  		feature->dev = fdev;
>  		feature->id = finfo->fid;
> -		feature->resource_index = index;
> -		feature->ioaddr = finfo->ioaddr;
> -		fdev->resource[index++] = finfo->mmio_res;
> +
> +		/*
> +		 * map header resource for dfl bus device. Don't add header
> +		 * resource to feature devices, or the resource tree will be
> +		 * disordered and cause warning on resource release
> +		 */
> +		if (is_header_feature(feature)) {
> +			feature->resource_index = -1;
> +			feature->ioaddr =
> +				devm_ioremap_resource(binfo->dev,
> +						      &finfo->mmio_res);
> +			if (IS_ERR(feature->ioaddr))
> +				return PTR_ERR(feature->ioaddr);

For current device, this should work, I am not sure if we still need pass
the resource to header features, but if we consider that some header
features want to mmap resource to userspace, then only passing ioaddr
may not be enough for that case.

> +		} else {
> +			feature->resource_index = res_idx;
> +			fdev->resource[res_idx++] = finfo->mmio_res;
> +		}
> 
>  		if (finfo->nr_irqs) {
>  			ctx = devm_kcalloc(binfo->dev, finfo->nr_irqs,
> @@ -582,19 +615,13 @@ static int build_info_commit_dev(struct
> build_feature_devs_info *binfo)
> 
>  static int
>  build_info_create_dev(struct build_feature_devs_info *binfo,
> -		      enum dfl_id_type type, void __iomem *ioaddr)
> +		      enum dfl_id_type type)
>  {
>  	struct platform_device *fdev;
> -	int ret;
> 
>  	if (type >= DFL_ID_MAX)
>  		return -EINVAL;
> 
> -	/* we will create a new device, commit current device first */
> -	ret = build_info_commit_dev(binfo);
> -	if (ret)
> -		return ret;
> -
>  	/*
>  	 * we use -ENODEV as the initialization indicator which indicates
>  	 * whether the id need to be reclaimed
> @@ -605,7 +632,7 @@ build_info_create_dev(struct
> build_feature_devs_info *binfo,
> 
>  	binfo->feature_dev = fdev;
>  	binfo->feature_num = 0;
> -	binfo->ioaddr = ioaddr;
> +
>  	INIT_LIST_HEAD(&binfo->sub_features);
> 
>  	fdev->id = dfl_id_alloc(type, &fdev->dev);
> @@ -747,18 +774,17 @@ static int parse_feature_irqs(struct
> build_feature_devs_info *binfo,
>   */
>  static int
>  create_feature_instance(struct build_feature_devs_info *binfo,
> -			struct dfl_fpga_enum_dfl *dfl, resource_size_t ofst,
> -			resource_size_t size, u64 fid)
> +			resource_size_t ofst, resource_size_t size, u64 fid)
>  {
>  	unsigned int irq_base, nr_irqs;
>  	struct dfl_feature_info *finfo;
>  	int ret;
> 
>  	/* read feature size and id if inputs are invalid */
> -	size = size ? size : feature_size(dfl->ioaddr + ofst);
> -	fid = fid ? fid : feature_id(dfl->ioaddr + ofst);
> +	size = size ? size : feature_size(binfo->ioaddr + ofst);
> +	fid = fid ? fid : feature_id(binfo->ioaddr + ofst);
> 
> -	if (dfl->len - ofst < size)
> +	if (binfo->len - ofst < size)
>  		return -EINVAL;
> 
>  	ret = parse_feature_irqs(binfo, ofst, fid, &irq_base, &nr_irqs);
> @@ -770,12 +796,11 @@ create_feature_instance(struct
> build_feature_devs_info *binfo,
>  		return -ENOMEM;
> 
>  	finfo->fid = fid;
> -	finfo->mmio_res.start = dfl->start + ofst;
> +	finfo->mmio_res.start = binfo->start + ofst;
>  	finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
>  	finfo->mmio_res.flags = IORESOURCE_MEM;
>  	finfo->irq_base = irq_base;
>  	finfo->nr_irqs = nr_irqs;
> -	finfo->ioaddr = dfl->ioaddr + ofst;
> 
>  	list_add_tail(&finfo->node, &binfo->sub_features);
>  	binfo->feature_num++;
> @@ -784,7 +809,6 @@ create_feature_instance(struct
> build_feature_devs_info *binfo,
>  }
> 
>  static int parse_feature_port_afu(struct build_feature_devs_info *binfo,
> -				  struct dfl_fpga_enum_dfl *dfl,
>  				  resource_size_t ofst)
>  {
>  	u64 v = readq(binfo->ioaddr + PORT_HDR_CAP);
> @@ -792,11 +816,10 @@ static int parse_feature_port_afu(struct
> build_feature_devs_info *binfo,
> 
>  	WARN_ON(!size);
> 
> -	return create_feature_instance(binfo, dfl, ofst, size,
> FEATURE_ID_AFU);
> +	return create_feature_instance(binfo, ofst, size, FEATURE_ID_AFU);
>  }
> 
>  static int parse_feature_afu(struct build_feature_devs_info *binfo,
> -			     struct dfl_fpga_enum_dfl *dfl,
>  			     resource_size_t ofst)
>  {
>  	if (!binfo->feature_dev) {
> @@ -806,7 +829,7 @@ static int parse_feature_afu(struct
> build_feature_devs_info *binfo,
> 
>  	switch (feature_dev_id_type(binfo->feature_dev)) {
>  	case PORT_ID:
> -		return parse_feature_port_afu(binfo, dfl, ofst);
> +		return parse_feature_port_afu(binfo, ofst);
>  	default:
>  		dev_info(binfo->dev, "AFU belonging to FIU %s is not
> supported yet.\n",
>  			 binfo->feature_dev->name);
> @@ -815,35 +838,91 @@ static int parse_feature_afu(struct
> build_feature_devs_info *binfo,
>  	return 0;
>  }
> 
> +static bool is_feature_dev_detected(struct build_feature_devs_info *binfo)
> +{
> +	return !!binfo->feature_dev;
> +}
> +
> +static void dfl_binfo_shift(struct build_feature_devs_info *binfo,
> +			    resource_size_t ofst)
> +{
> +	binfo->start = binfo->start + ofst;
> +	binfo->len = binfo->len - ofst;
> +}
> +
> +static int dfl_binfo_prepare(struct build_feature_devs_info *binfo,
> +			     resource_size_t start, resource_size_t len)
> +{
> +	struct device *dev = binfo->dev;
> +	void __iomem *ioaddr;
> +
> +	if (!devm_request_mem_region(dev, start, len, dev_name(dev))) {
> +		dev_err(dev, "request region fail, start:%pa, len:%pa\n",
> +			&start, &len);
> +		return -ENOMEM;

Why ENOMEM? Or -EBUSY is better?

> +	}
> +
> +	ioaddr = devm_ioremap(dev, start, len);
> +	if (!ioaddr) {
> +		dev_err(dev, "ioremap region fail, start:%pa, len:%pa\n",
> +			&start, &len);
> +		devm_release_mem_region(dev, start, len);
> +		return -EFAULT;

Why EFAULT? Or -ENOMEM?

> +	}
> +
> +	binfo->start = start;
> +	binfo->len = len;
> +	binfo->ioaddr = ioaddr;
> +
> +	return 0;
> +}
> +
> +static void dfl_binfo_finish(struct build_feature_devs_info *binfo)
> +{
> +	devm_iounmap(binfo->dev, binfo->ioaddr);
> +	devm_release_mem_region(binfo->dev, binfo->start, binfo->len);
> +}
> +
>  static int parse_feature_fiu(struct build_feature_devs_info *binfo,
> -			     struct dfl_fpga_enum_dfl *dfl,
>  			     resource_size_t ofst)
>  {
>  	u32 id, offset;
>  	u64 v;
>  	int ret = 0;
> 
> -	v = readq(dfl->ioaddr + ofst + DFH);
> +	if (is_feature_dev_detected(binfo)) {
> +		dfl_binfo_finish(binfo);
> +
> +		ret = build_info_commit_dev(binfo);
> +		if (ret)
> +			return ret;
> +
> +		dfl_binfo_prepare(binfo, binfo->start + ofst,
> +				  binfo->len - ofst);
> +	} else {
> +		dfl_binfo_shift(binfo, ofst);

Any possibility that it can fall into this case? or we can just drop it?

> +	}
> +
> +	v = readq(binfo->ioaddr + DFH);

And if you do shift start and len in binfo, but no shift to ioaddr here?

>  	id = FIELD_GET(DFH_ID, v);
> 
>  	/* create platform device for dfl feature dev */
> -	ret = build_info_create_dev(binfo, dfh_id_to_type(id),
> -				    dfl->ioaddr + ofst);
> +	ret = build_info_create_dev(binfo, dfh_id_to_type(id));
>  	if (ret)
>  		return ret;
> 
> -	ret = create_feature_instance(binfo, dfl, ofst, 0, 0);
> +	ret = create_feature_instance(binfo, 0, 0, 0);
>  	if (ret)
>  		return ret;
>  	/*
>  	 * find and parse FIU's child AFU via its NEXT_AFU register.
>  	 * please note that only Port has valid NEXT_AFU pointer per spec.
>  	 */
> -	v = readq(dfl->ioaddr + ofst + NEXT_AFU);
> +	v = readq(binfo->ioaddr + NEXT_AFU);
> 
>  	offset = FIELD_GET(NEXT_AFU_NEXT_DFH_OFST, v);
>  	if (offset)
> -		return parse_feature_afu(binfo, dfl, ofst + offset);
> +		return parse_feature_afu(binfo, offset);
> 
>  	dev_dbg(binfo->dev, "No AFUs detected on FIU %d\n", id);
> 
> @@ -851,16 +930,15 @@ static int parse_feature_fiu(struct
> build_feature_devs_info *binfo,
>  }
> 
>  static int parse_feature_private(struct build_feature_devs_info *binfo,
> -				 struct dfl_fpga_enum_dfl *dfl,
>  				 resource_size_t ofst)
>  {
>  	if (!binfo->feature_dev) {
>  		dev_err(binfo->dev, "the private feature %llx does not belong
> to any AFU.\n",
> -			(unsigned long long)feature_id(dfl->ioaddr + ofst));
> +			(unsigned long long)feature_id(binfo->ioaddr + ofst));
>  		return -EINVAL;
>  	}
> 
> -	return create_feature_instance(binfo, dfl, ofst, 0, 0);
> +	return create_feature_instance(binfo, ofst, 0, 0);
>  }
> 
>  /**
> @@ -868,24 +946,24 @@ static int parse_feature_private(struct
> build_feature_devs_info *binfo,
>   *
>   * @binfo: build feature devices information.
>   * @dfl: device feature list to parse
> - * @ofst: offset to feature header on this device feature list
> + * @ofst: offset to current FIU header
>   */
>  static int parse_feature(struct build_feature_devs_info *binfo,
> -			 struct dfl_fpga_enum_dfl *dfl, resource_size_t ofst)
> +			 resource_size_t ofst)
>  {
>  	u64 v;
>  	u32 type;
> 
> -	v = readq(dfl->ioaddr + ofst + DFH);
> +	v = readq(binfo->ioaddr + ofst + DFH);
>  	type = FIELD_GET(DFH_TYPE, v);
> 
>  	switch (type) {
>  	case DFH_TYPE_AFU:
> -		return parse_feature_afu(binfo, dfl, ofst);
> +		return parse_feature_afu(binfo, ofst);
>  	case DFH_TYPE_PRIVATE:
> -		return parse_feature_private(binfo, dfl, ofst);
> +		return parse_feature_private(binfo, ofst);
>  	case DFH_TYPE_FIU:
> -		return parse_feature_fiu(binfo, dfl, ofst);
> +		return parse_feature_fiu(binfo, ofst);
>  	default:
>  		dev_info(binfo->dev,
>  			 "Feature Type %x is not supported.\n", type);
> @@ -897,12 +975,18 @@ static int parse_feature(struct
> build_feature_devs_info *binfo,
>  static int parse_feature_list(struct build_feature_devs_info *binfo,
>  			      struct dfl_fpga_enum_dfl *dfl)
>  {
> -	void __iomem *start = dfl->ioaddr;
> -	void __iomem *end = dfl->ioaddr + dfl->len;
> +	resource_size_t start, end;
>  	int ret = 0;
>  	u32 ofst = 0;
>  	u64 v;
> 
> +	ret = dfl_binfo_prepare(binfo, dfl->start, dfl->len);

Hm.. looks like dfl is only used for some initialization work, could we just pass
Start and len from the function parameters? Then all parse_feature_xx functions
don't need to know dfl data structure any more.

> +	if (ret)
> +		return ret;
> +
> +	start = dfl->start;
> +	end = start + dfl->len;

Above lines can be replaced with binfo, right?

> +
>  	/* walk through the device feature list via DFH's next DFH pointer. */
>  	for (; start < end; start += ofst) {
>  		if (end - start < DFH_SIZE) {
> @@ -910,11 +994,11 @@ static int parse_feature_list(struct
> build_feature_devs_info *binfo,
>  			return -EINVAL;
>  		}
> 
> -		ret = parse_feature(binfo, dfl, start - dfl->ioaddr);
> +		ret = parse_feature(binfo, start - binfo->start);
>  		if (ret)
>  			return ret;
> 
> -		v = readq(start + DFH);
> +		v = readq(binfo->ioaddr + start - binfo->start + DFH);
>  		ofst = FIELD_GET(DFH_NEXT_HDR_OFST, v);
> 
>  		/* stop parsing if EOL(End of List) is set or offset is 0 */
> @@ -923,6 +1007,8 @@ static int parse_feature_list(struct
> build_feature_devs_info *binfo,
>  	}
> 
>  	/* commit current feature device when reach the end of list */
> +	dfl_binfo_finish(binfo);

Or complete a better name for this function?

> +
>  	return build_info_commit_dev(binfo);
>  }
> 
> @@ -976,7 +1062,6 @@ EXPORT_SYMBOL_GPL(dfl_fpga_enum_info_free);
>   * @info: ptr to dfl_fpga_enum_info
>   * @start: mmio resource address of the device feature list.
>   * @len: mmio resource length of the device feature list.
> - * @ioaddr: mapped mmio resource address of the device feature list.
>   *
>   * One FPGA device may have one or more Device Feature Lists (DFLs), use
> this
>   * function to add information of each DFL to common data structure for
> next
> @@ -985,8 +1070,7 @@ EXPORT_SYMBOL_GPL(dfl_fpga_enum_info_free);
>   * Return: 0 on success, negative error code otherwise.
>   */
>  int dfl_fpga_enum_info_add_dfl(struct dfl_fpga_enum_info *info,
> -			       resource_size_t start, resource_size_t len,
> -			       void __iomem *ioaddr)
> +			       resource_size_t start, resource_size_t len)
>  {
>  	struct dfl_fpga_enum_dfl *dfl;
> 
> @@ -996,7 +1080,6 @@ int dfl_fpga_enum_info_add_dfl(struct
> dfl_fpga_enum_info *info,
> 
>  	dfl->start = start;
>  	dfl->len = len;
> -	dfl->ioaddr = ioaddr;
> 
>  	list_add_tail(&dfl->node, &info->dfls);
> 
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index a32dfba..f605c28 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -441,22 +441,18 @@ struct dfl_fpga_enum_info {
>   *
>   * @start: base address of this device feature list.
>   * @len: size of this device feature list.
> - * @ioaddr: mapped base address of this device feature list.
>   * @node: node in list of device feature lists.
>   */
>  struct dfl_fpga_enum_dfl {
>  	resource_size_t start;
>  	resource_size_t len;
> 
> -	void __iomem *ioaddr;
> -
>  	struct list_head node;
>  };
> 
>  struct dfl_fpga_enum_info *dfl_fpga_enum_info_alloc(struct device *dev);
>  int dfl_fpga_enum_info_add_dfl(struct dfl_fpga_enum_info *info,
> -			       resource_size_t start, resource_size_t len,
> -			       void __iomem *ioaddr);
> +			       resource_size_t start, resource_size_t len);
>  int dfl_fpga_enum_info_add_irq(struct dfl_fpga_enum_info *info,
>  			       unsigned int nr_irqs, int *irq_table);
>  void dfl_fpga_enum_info_free(struct dfl_fpga_enum_info *info);
> --
> 2.7.4

Re: [PATCH 1/2] fpga: dfl: map feature mmio resources in their own feature drivers

[Xu Yilun]

On Fri, Jul 17, 2020 at 05:48:55PM +0800, Wu, Hao wrote:
> > -----Original Message-----
> > From: linux-fpga-owner@vger.kernel.org <linux-fpga-owner@vger.kernel.org>
> > On Behalf Of Xu Yilun
> > Sent: Wednesday, July 15, 2020 1:38 PM
> > To: mdf@kernel.org; linux-fpga@vger.kernel.org; linux-
> > kernel@vger.kernel.org
> > Cc: trix@redhat.com; lgoncalv@redhat.com; Xu, Yilun <yilun.xu@intel.com>;
> > Wu, Hao <hao.wu@intel.com>; Matthew Gerlach
> > <matthew.gerlach@linux.intel.com>; Weight, Russell H
> > <russell.h.weight@intel.com>
> > Subject: [PATCH 1/2] fpga: dfl: map feature mmio resources in their own
> > feature drivers
> >
> > This patch makes preparation for modularization of DFL sub feature
> > drivers.
> >
> > Currently, if we need to support a new DFL sub feature, an entry should
> > be added to fme/port_feature_drvs[] in dfl-fme/port-main.c. And we need
> > to re-compile the whole DFL modules. That make the DFL drivers hard to be
> > extended.
> >
> > Another consideration is that DFL may contain some IP blocks which are
> > already supported by kernel, most of them are supported by platform
> > device drivers. We could create platform devices for these IP blocks and
> > get them supported by these drivers.
> >
> > An important issue is that platform device drivers usually requests mmio
> > resources on probe. But now dfl mmio is mapped in dfl bus driver (e.g.
> > dfl-pci) as a whole region. Then platform device drivers for sub features
> > can't request their own mmio resources again. This is what the patch
> > trying to resolve.
> >
> > This patch changes the DFL enumeration. DFL bus driver will unmap mmio
> > resources after first step enumeration and pass enumeration info to DFL
> > framework. Then DFL framework will map the mmio resources again, do 2nd
> > step enumeration, and also unmap the mmio resources. In this way, sub
> > feature drivers could then request their own mmio resources as needed.
> >
> > An exception is that mmio resource of FIU headers are still mapped in dfl
> > bus driver. The FIU headers have some fundamental functions (sriov set,
> > port enable/disable) needed for dfl bus devices and other sub features.
> > They should not be unmapped as long as dfl bus device is alive.
> >
> > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > Signed-off-by: Wu Hao <hao.wu@intel.com>
> > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> > ---
> >  drivers/fpga/dfl-pci.c |  21 ++++--
> >  drivers/fpga/dfl.c     | 187 +++++++++++++++++++++++++++++++++++-----------
> > ---
> >  drivers/fpga/dfl.h     |   6 +-
> >  3 files changed, 152 insertions(+), 62 deletions(-)
> >
> > diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
> > index e220bec..22dc025 100644
> > --- a/drivers/fpga/dfl-pci.c
> > +++ b/drivers/fpga/dfl-pci.c
> > @@ -39,6 +39,11 @@ static void __iomem *cci_pci_ioremap_bar(struct
> > pci_dev *pcidev, int bar)
> >  return pcim_iomap_table(pcidev)[bar];
> >  }
> >
> > +static void cci_pci_iounmap_bars(struct pci_dev *pcidev, int mapped_bars)
> > +{
> > +pcim_iounmap_regions(pcidev, mapped_bars);
> > +}
> > +
> >  static int cci_pci_alloc_irq(struct pci_dev *pcidev)
> >  {
> >  int ret, nvec = pci_msix_vec_count(pcidev);
> > @@ -123,7 +128,7 @@ static int *cci_pci_create_irq_table(struct pci_dev
> > *pcidev, unsigned int nvec)
> >  static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
> >  {
> >  struct cci_drvdata *drvdata = pci_get_drvdata(pcidev);
> > -int port_num, bar, i, nvec, ret = 0;
> > +int port_num, bar, i, nvec, mapped_bars, ret = 0;
> >  struct dfl_fpga_enum_info *info;
> >  struct dfl_fpga_cdev *cdev;
> >  resource_size_t start, len;
> > @@ -163,6 +168,8 @@ static int cci_enumerate_feature_devs(struct pci_dev
> > *pcidev)
> >  goto irq_free_exit;
> >  }
> >
> > +mapped_bars = BIT(0);
> > +
> >  /*
> >   * PF device has FME and Ports/AFUs, and VF device only has one
> >   * Port/AFU. Check them and add related "Device Feature List" info
> > @@ -172,7 +179,7 @@ static int cci_enumerate_feature_devs(struct pci_dev
> > *pcidev)
> >  start = pci_resource_start(pcidev, 0);
> >  len = pci_resource_len(pcidev, 0);
> >
> > -dfl_fpga_enum_info_add_dfl(info, start, len, base);
> > +dfl_fpga_enum_info_add_dfl(info, start, len);
> >
> >  /*
> >   * find more Device Feature Lists (e.g. Ports) per information
> > @@ -200,22 +207,26 @@ static int cci_enumerate_feature_devs(struct
> > pci_dev *pcidev)
> >  if (!base)
> >  continue;
> >
> > +mapped_bars |= BIT(bar);
> > +
> >  start = pci_resource_start(pcidev, bar) + offset;
> >  len = pci_resource_len(pcidev, bar) - offset;
> >
> > -dfl_fpga_enum_info_add_dfl(info, start, len,
> > -   base + offset);
> > +dfl_fpga_enum_info_add_dfl(info, start, len);
> >  }
> >  } else if (dfl_feature_is_port(base)) {
> >  start = pci_resource_start(pcidev, 0);
> >  len = pci_resource_len(pcidev, 0);
> >
> > -dfl_fpga_enum_info_add_dfl(info, start, len, base);
> > +dfl_fpga_enum_info_add_dfl(info, start, len);
> >  } else {
> >  ret = -ENODEV;
> >  goto irq_free_exit;
> >  }
> >
> > +/* release I/O mappings for next step enumeration */
> > +cci_pci_iounmap_bars(pcidev, mapped_bars);
> > +
> >  /* start enumeration with prepared enumeration information */
> >  cdev = dfl_fpga_feature_devs_enumerate(info);
> >  if (IS_ERR(cdev)) {
> > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> > index 649958a..7dc6411 100644
> > --- a/drivers/fpga/dfl.c
> > +++ b/drivers/fpga/dfl.c
> > @@ -250,6 +250,11 @@ int dfl_fpga_check_port_id(struct platform_device
> > *pdev, void *pport_id)
> >  }
> >  EXPORT_SYMBOL_GPL(dfl_fpga_check_port_id);
> >
> > +static bool is_header_feature(struct dfl_feature *feature)
> > +{
> > +return feature->id == FEATURE_ID_FIU_HEADER;
> > +}
> > +
> >  /**
> >   * dfl_fpga_dev_feature_uinit - uinit for sub features of dfl feature device
> >   * @pdev: feature device.
> > @@ -273,8 +278,20 @@ static int dfl_feature_instance_init(struct
> > platform_device *pdev,
> >       struct dfl_feature *feature,
> >       struct dfl_feature_driver *drv)
> >  {
> > +void __iomem *base;
> >  int ret = 0;
> >
> > +if (!is_header_feature(feature)) {
> > +base = devm_platform_ioremap_resource(pdev,
> > +      feature->resource_index);
> > +if (IS_ERR(base)) {
> > +dev_err(&pdev->dev, "fail to get iomem
> > resource!\n");
> 
> Maybe you want to show which feature failed with ioremap here?

Yes, I could improve the log.

> 
> > +return PTR_ERR(base);
> > +}
> > +
> > +feature->ioaddr = base;
> > +}
> > +
> >  if (drv->ops->init) {
> >  ret = drv->ops->init(pdev, feature);
> >  if (ret)
> > @@ -427,7 +444,9 @@ EXPORT_SYMBOL_GPL(dfl_fpga_dev_ops_unregister);
> >   * @irq_table: Linux IRQ numbers for all irqs, indexed by local irq index of
> >   *       this device.
> >   * @feature_dev: current feature device.
> > - * @ioaddr: header register region address of feature device in enumeration.
> > + * @ioaddr: header register region address of current FIU in enumeration.
> > + * @start: register resource start of current FIU.
> > + * @len: max register resource length of current FIU.
> >   * @sub_features: a sub features linked list for feature device in
> > enumeration.
> >   * @feature_num: number of sub features for feature device in
> > enumeration.
> >   */
> > @@ -439,6 +458,9 @@ struct build_feature_devs_info {
> >
> >  struct platform_device *feature_dev;
> >  void __iomem *ioaddr;
> > +resource_size_t start;
> > +resource_size_t len;
> > +
> >  struct list_head sub_features;
> >  int feature_num;
> >  };
> > @@ -484,10 +506,7 @@ static int build_info_commit_dev(struct
> > build_feature_devs_info *binfo)
> >  struct dfl_feature_platform_data *pdata;
> >  struct dfl_feature_info *finfo, *p;
> >  enum dfl_id_type type;
> > -int ret, index = 0;
> > -
> > -if (!fdev)
> > -return 0;
> 
> Why you remove this checking?

This check is moved out of this function in parse_feature_fiu(). It is
now an small function is_feature_dev_detected()

However we may still need the check when the whole DFL walk is about to
finish. I think I may add the check in that place.

> 
> > +int ret, index = 0, res_idx = 0;
> >
> >  type = feature_dev_id_type(fdev);
> >  if (WARN_ON_ONCE(type >= DFL_ID_MAX))
> > @@ -530,16 +549,30 @@ static int build_info_commit_dev(struct
> > build_feature_devs_info *binfo)
> >
> >  /* fill features and resource information for feature dev */
> >  list_for_each_entry_safe(finfo, p, &binfo->sub_features, node) {
> > -struct dfl_feature *feature = &pdata->features[index];
> > +struct dfl_feature *feature = &pdata->features[index++];
> >  struct dfl_feature_irq_ctx *ctx;
> >  unsigned int i;
> >
> >  /* save resource information for each feature */
> >  feature->dev = fdev;
> >  feature->id = finfo->fid;
> > -feature->resource_index = index;
> > -feature->ioaddr = finfo->ioaddr;
> > -fdev->resource[index++] = finfo->mmio_res;
> > +
> > +/*
> > + * map header resource for dfl bus device. Don't add header
> > + * resource to feature devices, or the resource tree will be
> > + * disordered and cause warning on resource release
> > + */
> > +if (is_header_feature(feature)) {
> > +feature->resource_index = -1;
> > +feature->ioaddr =
> > +devm_ioremap_resource(binfo->dev,
> > +      &finfo->mmio_res);
> > +if (IS_ERR(feature->ioaddr))
> > +return PTR_ERR(feature->ioaddr);
> 
> For current device, this should work, I am not sure if we still need pass
> the resource to header features, but if we consider that some header
> features want to mmap resource to userspace, then only passing ioaddr
> may not be enough for that case.

The header feature has critical controls to the whole FPGA card, like SRIOV,
port reset that impact other features. So its mmio resource is owned by dfl-pci
device. I don't think we are going to pass it to userspace.

> 
> > +} else {
> > +feature->resource_index = res_idx;
> > +fdev->resource[res_idx++] = finfo->mmio_res;
> > +}
> >
> >  if (finfo->nr_irqs) {
> >  ctx = devm_kcalloc(binfo->dev, finfo->nr_irqs,
> > @@ -582,19 +615,13 @@ static int build_info_commit_dev(struct
> > build_feature_devs_info *binfo)
> >
> >  static int
> >  build_info_create_dev(struct build_feature_devs_info *binfo,
> > -      enum dfl_id_type type, void __iomem *ioaddr)
> > +      enum dfl_id_type type)
> >  {
> >  struct platform_device *fdev;
> > -int ret;
> >
> >  if (type >= DFL_ID_MAX)
> >  return -EINVAL;
> >
> > -/* we will create a new device, commit current device first */
> > -ret = build_info_commit_dev(binfo);
> > -if (ret)
> > -return ret;
> > -
> >  /*
> >   * we use -ENODEV as the initialization indicator which indicates
> >   * whether the id need to be reclaimed
> > @@ -605,7 +632,7 @@ build_info_create_dev(struct
> > build_feature_devs_info *binfo,
> >
> >  binfo->feature_dev = fdev;
> >  binfo->feature_num = 0;
> > -binfo->ioaddr = ioaddr;
> > +
> >  INIT_LIST_HEAD(&binfo->sub_features);
> >
> >  fdev->id = dfl_id_alloc(type, &fdev->dev);
> > @@ -747,18 +774,17 @@ static int parse_feature_irqs(struct
> > build_feature_devs_info *binfo,
> >   */
> >  static int
> >  create_feature_instance(struct build_feature_devs_info *binfo,
> > -struct dfl_fpga_enum_dfl *dfl, resource_size_t ofst,
> > -resource_size_t size, u64 fid)
> > +resource_size_t ofst, resource_size_t size, u64 fid)
> >  {
> >  unsigned int irq_base, nr_irqs;
> >  struct dfl_feature_info *finfo;
> >  int ret;
> >
> >  /* read feature size and id if inputs are invalid */
> > -size = size ? size : feature_size(dfl->ioaddr + ofst);
> > -fid = fid ? fid : feature_id(dfl->ioaddr + ofst);
> > +size = size ? size : feature_size(binfo->ioaddr + ofst);
> > +fid = fid ? fid : feature_id(binfo->ioaddr + ofst);
> >
> > -if (dfl->len - ofst < size)
> > +if (binfo->len - ofst < size)
> >  return -EINVAL;
> >
> >  ret = parse_feature_irqs(binfo, ofst, fid, &irq_base, &nr_irqs);
> > @@ -770,12 +796,11 @@ create_feature_instance(struct
> > build_feature_devs_info *binfo,
> >  return -ENOMEM;
> >
> >  finfo->fid = fid;
> > -finfo->mmio_res.start = dfl->start + ofst;
> > +finfo->mmio_res.start = binfo->start + ofst;
> >  finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
> >  finfo->mmio_res.flags = IORESOURCE_MEM;
> >  finfo->irq_base = irq_base;
> >  finfo->nr_irqs = nr_irqs;
> > -finfo->ioaddr = dfl->ioaddr + ofst;
> >
> >  list_add_tail(&finfo->node, &binfo->sub_features);
> >  binfo->feature_num++;
> > @@ -784,7 +809,6 @@ create_feature_instance(struct
> > build_feature_devs_info *binfo,
> >  }
> >
> >  static int parse_feature_port_afu(struct build_feature_devs_info *binfo,
> > -  struct dfl_fpga_enum_dfl *dfl,
> >    resource_size_t ofst)
> >  {
> >  u64 v = readq(binfo->ioaddr + PORT_HDR_CAP);
> > @@ -792,11 +816,10 @@ static int parse_feature_port_afu(struct
> > build_feature_devs_info *binfo,
> >
> >  WARN_ON(!size);
> >
> > -return create_feature_instance(binfo, dfl, ofst, size,
> > FEATURE_ID_AFU);
> > +return create_feature_instance(binfo, ofst, size, FEATURE_ID_AFU);
> >  }
> >
> >  static int parse_feature_afu(struct build_feature_devs_info *binfo,
> > -     struct dfl_fpga_enum_dfl *dfl,
> >       resource_size_t ofst)
> >  {
> >  if (!binfo->feature_dev) {
> > @@ -806,7 +829,7 @@ static int parse_feature_afu(struct
> > build_feature_devs_info *binfo,
> >
> >  switch (feature_dev_id_type(binfo->feature_dev)) {
> >  case PORT_ID:
> > -return parse_feature_port_afu(binfo, dfl, ofst);
> > +return parse_feature_port_afu(binfo, ofst);
> >  default:
> >  dev_info(binfo->dev, "AFU belonging to FIU %s is not
> > supported yet.\n",
> >   binfo->feature_dev->name);
> > @@ -815,35 +838,91 @@ static int parse_feature_afu(struct
> > build_feature_devs_info *binfo,
> >  return 0;
> >  }
> >
> > +static bool is_feature_dev_detected(struct build_feature_devs_info *binfo)
> > +{
> > +return !!binfo->feature_dev;
> > +}
> > +
> > +static void dfl_binfo_shift(struct build_feature_devs_info *binfo,
> > +    resource_size_t ofst)
> > +{
> > +binfo->start = binfo->start + ofst;
> > +binfo->len = binfo->len - ofst;
> > +}
> > +
> > +static int dfl_binfo_prepare(struct build_feature_devs_info *binfo,
> > +     resource_size_t start, resource_size_t len)
> > +{
> > +struct device *dev = binfo->dev;
> > +void __iomem *ioaddr;
> > +
> > +if (!devm_request_mem_region(dev, start, len, dev_name(dev))) {
> > +dev_err(dev, "request region fail, start:%pa, len:%pa\n",
> > +&start, &len);
> > +return -ENOMEM;
> 
> Why ENOMEM? Or -EBUSY is better?

Yes, I think -EBUSY is better.

> 
> > +}
> > +
> > +ioaddr = devm_ioremap(dev, start, len);
> > +if (!ioaddr) {
> > +dev_err(dev, "ioremap region fail, start:%pa, len:%pa\n",
> > +&start, &len);
> > +devm_release_mem_region(dev, start, len);
> > +return -EFAULT;
> 
> Why EFAULT? Or -ENOMEM?

Yes, I'll also change it to -ENOMEM.

> 
> > +}
> > +
> > +binfo->start = start;
> > +binfo->len = len;
> > +binfo->ioaddr = ioaddr;
> > +
> > +return 0;
> > +}
> > +
> > +static void dfl_binfo_finish(struct build_feature_devs_info *binfo)
> > +{
> > +devm_iounmap(binfo->dev, binfo->ioaddr);
> > +devm_release_mem_region(binfo->dev, binfo->start, binfo->len);
> > +}
> > +
> >  static int parse_feature_fiu(struct build_feature_devs_info *binfo,
> > -     struct dfl_fpga_enum_dfl *dfl,
> >       resource_size_t ofst)
> >  {
> >  u32 id, offset;
> >  u64 v;
> >  int ret = 0;
> >
> > -v = readq(dfl->ioaddr + ofst + DFH);
> > +if (is_feature_dev_detected(binfo)) {
> > +dfl_binfo_finish(binfo);
> > +
> > +ret = build_info_commit_dev(binfo);
> > +if (ret)
> > +return ret;
> > +
> > +dfl_binfo_prepare(binfo, binfo->start + ofst,
> > +  binfo->len - ofst);
> > +} else {
> > +dfl_binfo_shift(binfo, ofst);
> 
> Any possibility that it can fall into this case? or we can just drop it?

I checked and think we can drop the case.

> 
> > +}
> > +
> > +v = readq(binfo->ioaddr + DFH);
> 
> And if you do shift start and len in binfo, but no shift to ioaddr here?

OK. drop the dfl_binfo_shift() and it will be fine.

> 
> >  id = FIELD_GET(DFH_ID, v);
> >
> >  /* create platform device for dfl feature dev */
> > -ret = build_info_create_dev(binfo, dfh_id_to_type(id),
> > -    dfl->ioaddr + ofst);
> > +ret = build_info_create_dev(binfo, dfh_id_to_type(id));
> >  if (ret)
> >  return ret;
> >
> > -ret = create_feature_instance(binfo, dfl, ofst, 0, 0);
> > +ret = create_feature_instance(binfo, 0, 0, 0);
> >  if (ret)
> >  return ret;
> >  /*
> >   * find and parse FIU's child AFU via its NEXT_AFU register.
> >   * please note that only Port has valid NEXT_AFU pointer per spec.
> >   */
> > -v = readq(dfl->ioaddr + ofst + NEXT_AFU);
> > +v = readq(binfo->ioaddr + NEXT_AFU);
> >
> >  offset = FIELD_GET(NEXT_AFU_NEXT_DFH_OFST, v);
> >  if (offset)
> > -return parse_feature_afu(binfo, dfl, ofst + offset);
> > +return parse_feature_afu(binfo, offset);
> >
> >  dev_dbg(binfo->dev, "No AFUs detected on FIU %d\n", id);
> >
> > @@ -851,16 +930,15 @@ static int parse_feature_fiu(struct
> > build_feature_devs_info *binfo,
> >  }
> >
> >  static int parse_feature_private(struct build_feature_devs_info *binfo,
> > - struct dfl_fpga_enum_dfl *dfl,
> >   resource_size_t ofst)
> >  {
> >  if (!binfo->feature_dev) {
> >  dev_err(binfo->dev, "the private feature %llx does not belong
> > to any AFU.\n",
> > -(unsigned long long)feature_id(dfl->ioaddr + ofst));
> > +(unsigned long long)feature_id(binfo->ioaddr + ofst));
> >  return -EINVAL;
> >  }
> >
> > -return create_feature_instance(binfo, dfl, ofst, 0, 0);
> > +return create_feature_instance(binfo, ofst, 0, 0);
> >  }
> >
> >  /**
> > @@ -868,24 +946,24 @@ static int parse_feature_private(struct
> > build_feature_devs_info *binfo,
> >   *
> >   * @binfo: build feature devices information.
> >   * @dfl: device feature list to parse
> > - * @ofst: offset to feature header on this device feature list
> > + * @ofst: offset to current FIU header
> >   */
> >  static int parse_feature(struct build_feature_devs_info *binfo,
> > - struct dfl_fpga_enum_dfl *dfl, resource_size_t ofst)
> > + resource_size_t ofst)
> >  {
> >  u64 v;
> >  u32 type;
> >
> > -v = readq(dfl->ioaddr + ofst + DFH);
> > +v = readq(binfo->ioaddr + ofst + DFH);
> >  type = FIELD_GET(DFH_TYPE, v);
> >
> >  switch (type) {
> >  case DFH_TYPE_AFU:
> > -return parse_feature_afu(binfo, dfl, ofst);
> > +return parse_feature_afu(binfo, ofst);
> >  case DFH_TYPE_PRIVATE:
> > -return parse_feature_private(binfo, dfl, ofst);
> > +return parse_feature_private(binfo, ofst);
> >  case DFH_TYPE_FIU:
> > -return parse_feature_fiu(binfo, dfl, ofst);
> > +return parse_feature_fiu(binfo, ofst);
> >  default:
> >  dev_info(binfo->dev,
> >   "Feature Type %x is not supported.\n", type);
> > @@ -897,12 +975,18 @@ static int parse_feature(struct
> > build_feature_devs_info *binfo,
> >  static int parse_feature_list(struct build_feature_devs_info *binfo,
> >        struct dfl_fpga_enum_dfl *dfl)
> >  {
> > -void __iomem *start = dfl->ioaddr;
> > -void __iomem *end = dfl->ioaddr + dfl->len;
> > +resource_size_t start, end;
> >  int ret = 0;
> >  u32 ofst = 0;
> >  u64 v;
> >
> > +ret = dfl_binfo_prepare(binfo, dfl->start, dfl->len);
> 
> Hm.. looks like dfl is only used for some initialization work, could we just pass
> Start and len from the function parameters? Then all parse_feature_xx functions
> don't need to know dfl data structure any more.

I think yes.

> 
> > +if (ret)
> > +return ret;
> > +
> > +start = dfl->start;
> > +end = start + dfl->len;
> 
> Above lines can be replaced with binfo, right?

When we just pass start & len as parameters of parse_feature_list(), I
think just

	end = start + len;

is good enough.

> 
> > +
> >  /* walk through the device feature list via DFH's next DFH pointer. */
> >  for (; start < end; start += ofst) {
> >  if (end - start < DFH_SIZE) {
> > @@ -910,11 +994,11 @@ static int parse_feature_list(struct
> > build_feature_devs_info *binfo,
> >  return -EINVAL;
> >  }
> >
> > -ret = parse_feature(binfo, dfl, start - dfl->ioaddr);
> > +ret = parse_feature(binfo, start - binfo->start);
> >  if (ret)
> >  return ret;
> >
> > -v = readq(start + DFH);
> > +v = readq(binfo->ioaddr + start - binfo->start + DFH);
> >  ofst = FIELD_GET(DFH_NEXT_HDR_OFST, v);
> >
> >  /* stop parsing if EOL(End of List) is set or offset is 0 */
> > @@ -923,6 +1007,8 @@ static int parse_feature_list(struct
> > build_feature_devs_info *binfo,
> >  }
> >
> >  /* commit current feature device when reach the end of list */
> > +dfl_binfo_finish(binfo);
> 
> Or complete a better name for this function?

I could change it.

> 
> > +
> >  return build_info_commit_dev(binfo);
> >  }
> >
> > @@ -976,7 +1062,6 @@ EXPORT_SYMBOL_GPL(dfl_fpga_enum_info_free);
> >   * @info: ptr to dfl_fpga_enum_info
> >   * @start: mmio resource address of the device feature list.
> >   * @len: mmio resource length of the device feature list.
> > - * @ioaddr: mapped mmio resource address of the device feature list.
> >   *
> >   * One FPGA device may have one or more Device Feature Lists (DFLs), use
> > this
> >   * function to add information of each DFL to common data structure for
> > next
> > @@ -985,8 +1070,7 @@ EXPORT_SYMBOL_GPL(dfl_fpga_enum_info_free);
> >   * Return: 0 on success, negative error code otherwise.
> >   */
> >  int dfl_fpga_enum_info_add_dfl(struct dfl_fpga_enum_info *info,
> > -       resource_size_t start, resource_size_t len,
> > -       void __iomem *ioaddr)
> > +       resource_size_t start, resource_size_t len)
> >  {
> >  struct dfl_fpga_enum_dfl *dfl;
> >
> > @@ -996,7 +1080,6 @@ int dfl_fpga_enum_info_add_dfl(struct
> > dfl_fpga_enum_info *info,
> >
> >  dfl->start = start;
> >  dfl->len = len;
> > -dfl->ioaddr = ioaddr;
> >
> >  list_add_tail(&dfl->node, &info->dfls);
> >
> > diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> > index a32dfba..f605c28 100644
> > --- a/drivers/fpga/dfl.h
> > +++ b/drivers/fpga/dfl.h
> > @@ -441,22 +441,18 @@ struct dfl_fpga_enum_info {
> >   *
> >   * @start: base address of this device feature list.
> >   * @len: size of this device feature list.
> > - * @ioaddr: mapped base address of this device feature list.
> >   * @node: node in list of device feature lists.
> >   */
> >  struct dfl_fpga_enum_dfl {
> >  resource_size_t start;
> >  resource_size_t len;
> >
> > -void __iomem *ioaddr;
> > -
> >  struct list_head node;
> >  };
> >
> >  struct dfl_fpga_enum_info *dfl_fpga_enum_info_alloc(struct device *dev);
> >  int dfl_fpga_enum_info_add_dfl(struct dfl_fpga_enum_info *info,
> > -       resource_size_t start, resource_size_t len,
> > -       void __iomem *ioaddr);
> > +       resource_size_t start, resource_size_t len);
> >  int dfl_fpga_enum_info_add_irq(struct dfl_fpga_enum_info *info,
> >         unsigned int nr_irqs, int *irq_table);
> >  void dfl_fpga_enum_info_free(struct dfl_fpga_enum_info *info);
> > --
> > 2.7.4

Re: [PATCH 1/2] fpga: dfl: map feature mmio resources in their own feature drivers

[Tom Rix]

Mostly little stuff.

Consider refactoring create_feature_instance.

Tom

> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
> index e220bec..22dc025 100644
> --- a/drivers/fpga/dfl-pci.c
> +++ b/drivers/fpga/dfl-pci.c
> @@ -39,6 +39,11 @@ static void __iomem *cci_pci_ioremap_bar(struct pci_dev *pcidev, int bar)
>  	return pcim_iomap_table(pcidev)[bar];
>  }
>  
> +static void cci_pci_iounmap_bars(struct pci_dev *pcidev, int mapped_bars)
> +{
> +	pcim_iounmap_regions(pcidev, mapped_bars);
> +}

A singleton, called only one place. Consider replacing with a direct call.


> +
>  static int cci_pci_alloc_irq(struct pci_dev *pcidev)
>  {
>  	int ret, nvec = pci_msix_vec_count(pcidev);
> @@ -123,7 +128,7 @@ static int *cci_pci_create_irq_table(struct pci_dev *pcidev, unsigned int nvec)
>  static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
>  {
>  	struct cci_drvdata *drvdata = pci_get_drvdata(pcidev);
> -	int port_num, bar, i, nvec, ret = 0;
> +	int port_num, bar, i, nvec, mapped_bars, ret = 0;

Shouldn't mapped_bars be at least unsigned and maybe either uint32_t or uint64_t ?

I see pcim_ioumap_regions has int as parameter. boo

>  	struct dfl_fpga_enum_info *info;
>  	struct dfl_fpga_cdev *cdev;
>  	resource_size_t start, len;
> @@ -163,6 +168,8 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
>  		goto irq_free_exit;
>  	}
>  
> +	mapped_bars = BIT(0);
> +
>  	/*
>  	 * PF device has FME and Ports/AFUs, and VF device only has one
>  	 * Port/AFU. Check them and add related "Device Feature List" info
> @@ -172,7 +179,7 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
>  		start = pci_resource_start(pcidev, 0);
>  		len = pci_resource_len(pcidev, 0);
>  
> -		dfl_fpga_enum_info_add_dfl(info, start, len, base);
> +		dfl_fpga_enum_info_add_dfl(info, start, len);
>  
>  		/*
>  		 * find more Device Feature Lists (e.g. Ports) per information
> @@ -200,22 +207,26 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
>  			if (!base)
>  				continue;
>  
> +			mapped_bars |= BIT(bar);
> +
>  			start = pci_resource_start(pcidev, bar) + offset;
>  			len = pci_resource_len(pcidev, bar) - offset;
>  
> -			dfl_fpga_enum_info_add_dfl(info, start, len,
> -						   base + offset);
> +			dfl_fpga_enum_info_add_dfl(info, start, len);
>  		}
>  	} else if (dfl_feature_is_port(base)) {
>  		start = pci_resource_start(pcidev, 0);
>  		len = pci_resource_len(pcidev, 0);
>  
> -		dfl_fpga_enum_info_add_dfl(info, start, len, base);
> +		dfl_fpga_enum_info_add_dfl(info, start, len);
>  	} else {
>  		ret = -ENODEV;
>  		goto irq_free_exit;
>  	}
>  
> +	/* release I/O mappings for next step enumeration */
> +	cci_pci_iounmap_bars(pcidev, mapped_bars);
There is no iounmap_bars in error handling, likely need to add this.
> +
>  	/* start enumeration with prepared enumeration information */
>  	cdev = dfl_fpga_feature_devs_enumerate(info);
>  	if (IS_ERR(cdev)) {
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index 649958a..7dc6411 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -250,6 +250,11 @@ int dfl_fpga_check_port_id(struct platform_device *pdev, void *pport_id)
>  }
>  EXPORT_SYMBOL_GPL(dfl_fpga_check_port_id);
>  
> +static bool is_header_feature(struct dfl_feature *feature)
> +{
> +	return feature->id == FEATURE_ID_FIU_HEADER;
Could this be a macro ?
> +}
> +
>  /**
>   * dfl_fpga_dev_feature_uinit - uinit for sub features of dfl feature device
>   * @pdev: feature device.
> @@ -273,8 +278,20 @@ static int dfl_feature_instance_init(struct platform_device *pdev,
>  				     struct dfl_feature *feature,
>  				     struct dfl_feature_driver *drv)
>  {
> +	void __iomem *base;
>  	int ret = 0;
>  
> +	if (!is_header_feature(feature)) {
> +		base = devm_platform_ioremap_resource(pdev,
> +						      feature->resource_index);
> +		if (IS_ERR(base)) {
> +			dev_err(&pdev->dev, "fail to get iomem resource!\n");
> +			return PTR_ERR(base);
> +		}
> +
> +		feature->ioaddr = base;
> +	}
> +
>  	if (drv->ops->init) {
>  		ret = drv->ops->init(pdev, feature);
>  		if (ret)
> @@ -427,7 +444,9 @@ EXPORT_SYMBOL_GPL(dfl_fpga_dev_ops_unregister);
>   * @irq_table: Linux IRQ numbers for all irqs, indexed by local irq index of
>   *	       this device.
>   * @feature_dev: current feature device.
> - * @ioaddr: header register region address of feature device in enumeration.
> + * @ioaddr: header register region address of current FIU in enumeration.
> + * @start: register resource start of current FIU.
> + * @len: max register resource length of current FIU.
>   * @sub_features: a sub features linked list for feature device in enumeration.
>   * @feature_num: number of sub features for feature device in enumeration.
>   */
> @@ -439,6 +458,9 @@ struct build_feature_devs_info {
>  
>  	struct platform_device *feature_dev;
>  	void __iomem *ioaddr;
> +	resource_size_t start;
> +	resource_size_t len;
> +
extra whitespace, remove
>  	struct list_head sub_features;
>  	int feature_num;
>  };
> @@ -484,10 +506,7 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
>  	struct dfl_feature_platform_data *pdata;
>  	struct dfl_feature_info *finfo, *p;
>  	enum dfl_id_type type;
> -	int ret, index = 0;
> -
> -	if (!fdev)
> -		return 0;
> +	int ret, index = 0, res_idx = 0;
>  
>  	type = feature_dev_id_type(fdev);
>  	if (WARN_ON_ONCE(type >= DFL_ID_MAX))
> @@ -530,16 +549,30 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
>  
>  	/* fill features and resource information for feature dev */
>  	list_for_each_entry_safe(finfo, p, &binfo->sub_features, node) {
> -		struct dfl_feature *feature = &pdata->features[index];
> +		struct dfl_feature *feature = &pdata->features[index++];
>  		struct dfl_feature_irq_ctx *ctx;
>  		unsigned int i;
>  
>  		/* save resource information for each feature */
>  		feature->dev = fdev;
>  		feature->id = finfo->fid;
> -		feature->resource_index = index;
> -		feature->ioaddr = finfo->ioaddr;
> -		fdev->resource[index++] = finfo->mmio_res;
> +
> +		/*
> +		 * map header resource for dfl bus device. Don't add header
> +		 * resource to feature devices, or the resource tree will be
> +		 * disordered and cause warning on resource release
> +		 */
> +		if (is_header_feature(feature)) {
> +			feature->resource_index = -1;
> +			feature->ioaddr =
> +				devm_ioremap_resource(binfo->dev,
> +						      &finfo->mmio_res);
> +			if (IS_ERR(feature->ioaddr))
> +				return PTR_ERR(feature->ioaddr);
feature->ioaddr is garbage, is this ok ?
> +		} else {
> +			feature->resource_index = res_idx;
> +			fdev->resource[res_idx++] = finfo->mmio_res;
> +		}
>  
>  		if (finfo->nr_irqs) {
>  			ctx = devm_kcalloc(binfo->dev, finfo->nr_irqs,
> @@ -582,19 +615,13 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
>  
>  static int
>  build_info_create_dev(struct build_feature_devs_info *binfo,
> -		      enum dfl_id_type type, void __iomem *ioaddr)
> +		      enum dfl_id_type type)
>  {
>  	struct platform_device *fdev;
> -	int ret;
>  
>  	if (type >= DFL_ID_MAX)
>  		return -EINVAL;
>  
> -	/* we will create a new device, commit current device first */
> -	ret = build_info_commit_dev(binfo);
> -	if (ret)
> -		return ret;
> -
>  	/*
>  	 * we use -ENODEV as the initialization indicator which indicates
>  	 * whether the id need to be reclaimed
> @@ -605,7 +632,7 @@ build_info_create_dev(struct build_feature_devs_info *binfo,
>  
>  	binfo->feature_dev = fdev;
>  	binfo->feature_num = 0;
> -	binfo->ioaddr = ioaddr;
> +
>  	INIT_LIST_HEAD(&binfo->sub_features);
>  
>  	fdev->id = dfl_id_alloc(type, &fdev->dev);
> @@ -747,18 +774,17 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
>   */
>  static int
>  create_feature_instance(struct build_feature_devs_info *binfo,
> -			struct dfl_fpga_enum_dfl *dfl, resource_size_t ofst,
> -			resource_size_t size, u64 fid)
> +			resource_size_t ofst, resource_size_t size, u64 fid)
>  {
>  	unsigned int irq_base, nr_irqs;
>  	struct dfl_feature_info *finfo;
>  	int ret;
>  
>  	/* read feature size and id if inputs are invalid */
> -	size = size ? size : feature_size(dfl->ioaddr + ofst);
> -	fid = fid ? fid : feature_id(dfl->ioaddr + ofst);
> +	size = size ? size : feature_size(binfo->ioaddr + ofst);
> +	fid = fid ? fid : feature_id(binfo->ioaddr + ofst);

This is complicated. based on fiu,afu or private  calling.  Why not have a function ptr

struct build_feature_devs_info {

void (*find_info) (...)

}

find_info_fiu (..) {

size = feature_size(..)

fid = feature_id(..)

}

>  
> -	if (dfl->len - ofst < size)
> +	if (binfo->len - ofst < size)
>  		return -EINVAL;
>  
>  	ret = parse_feature_irqs(binfo, ofst, fid, &irq_base, &nr_irqs);
> @@ -770,12 +796,11 @@ create_feature_instance(struct build_feature_devs_info *binfo,
>  		return -ENOMEM;
>  
>  	finfo->fid = fid;
> -	finfo->mmio_res.start = dfl->start + ofst;
> +	finfo->mmio_res.start = binfo->start + ofst;
>  	finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
>  	finfo->mmio_res.flags = IORESOURCE_MEM;
>  	finfo->irq_base = irq_base;
>  	finfo->nr_irqs = nr_irqs;
> -	finfo->ioaddr = dfl->ioaddr + ofst;
>  
>  	list_add_tail(&finfo->node, &binfo->sub_features);
>  	binfo->feature_num++;
> @@ -784,7 +809,6 @@ create_feature_instance(struct build_feature_devs_info *binfo,
>  }
>  
>  static int parse_feature_port_afu(struct build_feature_devs_info *binfo,
> -				  struct dfl_fpga_enum_dfl *dfl,
>  				  resource_size_t ofst)
>  {
>  	u64 v = readq(binfo->ioaddr + PORT_HDR_CAP);
> @@ -792,11 +816,10 @@ static int parse_feature_port_afu(struct build_feature_devs_info *binfo,
>  
>  	WARN_ON(!size);
>  
> -	return create_feature_instance(binfo, dfl, ofst, size, FEATURE_ID_AFU);
> +	return create_feature_instance(binfo, ofst, size, FEATURE_ID_AFU);
>  }
>  
>  static int parse_feature_afu(struct build_feature_devs_info *binfo,
> -			     struct dfl_fpga_enum_dfl *dfl,
>  			     resource_size_t ofst)
>  {
>  	if (!binfo->feature_dev) {
> @@ -806,7 +829,7 @@ static int parse_feature_afu(struct build_feature_devs_info *binfo,
>  
>  	switch (feature_dev_id_type(binfo->feature_dev)) {
>  	case PORT_ID:
> -		return parse_feature_port_afu(binfo, dfl, ofst);
> +		return parse_feature_port_afu(binfo, ofst);
>  	default:
>  		dev_info(binfo->dev, "AFU belonging to FIU %s is not supported yet.\n",
>  			 binfo->feature_dev->name);
> @@ -815,35 +838,91 @@ static int parse_feature_afu(struct build_feature_devs_info *binfo,
>  	return 0;
>  }
>  
> +static bool is_feature_dev_detected(struct build_feature_devs_info *binfo)
> +{
> +	return !!binfo->feature_dev;
Another macro.
> +}
> +
> +static void dfl_binfo_shift(struct build_feature_devs_info *binfo,
> +			    resource_size_t ofst)
shift? where is shifting happening.  A better name would be dfl_binfo_update or dfl_binfo_increment
> +{
> +	binfo->start = binfo->start + ofst;
> +	binfo->len = binfo->len - ofst;
> +}
> +
> +static int dfl_binfo_prepare(struct build_feature_devs_info *binfo,
> +			     resource_size_t start, resource_size_t len)
> +{
> +	struct device *dev = binfo->dev;
> +	void __iomem *ioaddr;
> +
> +	if (!devm_request_mem_region(dev, start, len, dev_name(dev))) {
> +		dev_err(dev, "request region fail, start:%pa, len:%pa\n",
> +			&start, &len);
> +		return -ENOMEM;
> +	}
> +
> +	ioaddr = devm_ioremap(dev, start, len);
> +	if (!ioaddr) {
> +		dev_err(dev, "ioremap region fail, start:%pa, len:%pa\n",
> +			&start, &len);
> +		devm_release_mem_region(dev, start, len);
> +		return -EFAULT;
> +	}
> +
> +	binfo->start = start;
> +	binfo->len = len;
> +	binfo->ioaddr = ioaddr;
> +
> +	return 0;
> +}
> +
> +static void dfl_binfo_finish(struct build_feature_devs_info *binfo)
> +{
> +	devm_iounmap(binfo->dev, binfo->ioaddr);
> +	devm_release_mem_region(binfo->dev, binfo->start, binfo->len);
> +}
> +
>  static int parse_feature_fiu(struct build_feature_devs_info *binfo,
> -			     struct dfl_fpga_enum_dfl *dfl,
>  			     resource_size_t ofst)
>  {
>  	u32 id, offset;
>  	u64 v;
>  	int ret = 0;
>  
> -	v = readq(dfl->ioaddr + ofst + DFH);
> +	if (is_feature_dev_detected(binfo)) {
> +		dfl_binfo_finish(binfo);
> +
> +		ret = build_info_commit_dev(binfo);
> +		if (ret)
> +			return ret;
> +
> +		dfl_binfo_prepare(binfo, binfo->start + ofst,
> +				  binfo->len - ofst);
Check status of dfl_binfo_prepare
> +	} else {
> +		dfl_binfo_shift(binfo, ofst);
> +	}
> +
> +	v = readq(binfo->ioaddr + DFH);
>  	id = FIELD_GET(DFH_ID, v);
>  
>  	/* create platform device for dfl feature dev */
> -	ret = build_info_create_dev(binfo, dfh_id_to_type(id),
> -				    dfl->ioaddr + ofst);
> +	ret = build_info_create_dev(binfo, dfh_id_to_type(id));
>  	if (ret)
>  		return ret;
>  
> -	ret = create_feature_instance(binfo, dfl, ofst, 0, 0);
> +	ret = create_feature_instance(binfo, 0, 0, 0);
>  	if (ret)
>  		return ret;
>  	/*
>  	 * find and parse FIU's child AFU via its NEXT_AFU register.
>  	 * please note that only Port has valid NEXT_AFU pointer per spec.
>  	 */
> -	v = readq(dfl->ioaddr + ofst + NEXT_AFU);
> +	v = readq(binfo->ioaddr + NEXT_AFU);
>  
>  	offset = FIELD_GET(NEXT_AFU_NEXT_DFH_OFST, v);
>  	if (offset)
> -		return parse_feature_afu(binfo, dfl, ofst + offset);
> +		return parse_feature_afu(binfo, offset);
>  
>  	dev_dbg(binfo->dev, "No AFUs detected on FIU %d\n", id);
>  
> @@ -851,16 +930,15 @@ static int parse_feature_fiu(struct build_feature_devs_info *binfo,
>  }
>  
>  static int parse_feature_private(struct build_feature_devs_info *binfo,
> -				 struct dfl_fpga_enum_dfl *dfl,
>  				 resource_size_t ofst)
>  {
>  	if (!binfo->feature_dev) {
>  		dev_err(binfo->dev, "the private feature %llx does not belong to any AFU.\n",
> -			(unsigned long long)feature_id(dfl->ioaddr + ofst));
> +			(unsigned long long)feature_id(binfo->ioaddr + ofst));
>  		return -EINVAL;
>  	}
>  
> -	return create_feature_instance(binfo, dfl, ofst, 0, 0);
> +	return create_feature_instance(binfo, ofst, 0, 0);
>  }
>  
>  /**
> @@ -868,24 +946,24 @@ static int parse_feature_private(struct build_feature_devs_info *binfo,
>   *
>   * @binfo: build feature devices information.
>   * @dfl: device feature list to parse
> - * @ofst: offset to feature header on this device feature list
> + * @ofst: offset to current FIU header
>   */
>  static int parse_feature(struct build_feature_devs_info *binfo,
> -			 struct dfl_fpga_enum_dfl *dfl, resource_size_t ofst)
> +			 resource_size_t ofst)
>  {
>  	u64 v;
>  	u32 type;
>  
> -	v = readq(dfl->ioaddr + ofst + DFH);
> +	v = readq(binfo->ioaddr + ofst + DFH);
>  	type = FIELD_GET(DFH_TYPE, v);
>  
>  	switch (type) {
>  	case DFH_TYPE_AFU:
> -		return parse_feature_afu(binfo, dfl, ofst);
> +		return parse_feature_afu(binfo, ofst);
>  	case DFH_TYPE_PRIVATE:
> -		return parse_feature_private(binfo, dfl, ofst);
> +		return parse_feature_private(binfo, ofst);
>  	case DFH_TYPE_FIU:
> -		return parse_feature_fiu(binfo, dfl, ofst);
> +		return parse_feature_fiu(binfo, ofst);
>  	default:
>  		dev_info(binfo->dev,
>  			 "Feature Type %x is not supported.\n", type);
> @@ -897,12 +975,18 @@ static int parse_feature(struct build_feature_devs_info *binfo,
>  static int parse_feature_list(struct build_feature_devs_info *binfo,
>  			      struct dfl_fpga_enum_dfl *dfl)
>  {
> -	void __iomem *start = dfl->ioaddr;
> -	void __iomem *end = dfl->ioaddr + dfl->len;
> +	resource_size_t start, end;
>  	int ret = 0;
>  	u32 ofst = 0;
>  	u64 v;
>  
> +	ret = dfl_binfo_prepare(binfo, dfl->start, dfl->len);
> +	if (ret)
> +		return ret;
> +
> +	start = dfl->start;
> +	end = start + dfl->len;
> +
>  	/* walk through the device feature list via DFH's next DFH pointer. */
>  	for (; start < end; start += ofst) {
>  		if (end - start < DFH_SIZE) {
> @@ -910,11 +994,11 @@ static int parse_feature_list(struct build_feature_devs_info *binfo,
>  			return -EINVAL;
>  		}
>  
> -		ret = parse_feature(binfo, dfl, start - dfl->ioaddr);
> +		ret = parse_feature(binfo, start - binfo->start);
>  		if (ret)
>  			return ret;
>  
> -		v = readq(start + DFH);
> +		v = readq(binfo->ioaddr + start - binfo->start + DFH);
>  		ofst = FIELD_GET(DFH_NEXT_HDR_OFST, v);
>  
>  		/* stop parsing if EOL(End of List) is set or offset is 0 */
> @@ -923,6 +1007,8 @@ static int parse_feature_list(struct build_feature_devs_info *binfo,
>  	}
>  
>  	/* commit current feature device when reach the end of list */
> +	dfl_binfo_finish(binfo);
> +
>  	return build_info_commit_dev(binfo);
>  }
>  
> @@ -976,7 +1062,6 @@ EXPORT_SYMBOL_GPL(dfl_fpga_enum_info_free);
>   * @info: ptr to dfl_fpga_enum_info
>   * @start: mmio resource address of the device feature list.
>   * @len: mmio resource length of the device feature list.
> - * @ioaddr: mapped mmio resource address of the device feature list.
>   *
>   * One FPGA device may have one or more Device Feature Lists (DFLs), use this
>   * function to add information of each DFL to common data structure for next
> @@ -985,8 +1070,7 @@ EXPORT_SYMBOL_GPL(dfl_fpga_enum_info_free);
>   * Return: 0 on success, negative error code otherwise.
>   */
>  int dfl_fpga_enum_info_add_dfl(struct dfl_fpga_enum_info *info,
> -			       resource_size_t start, resource_size_t len,
> -			       void __iomem *ioaddr)
> +			       resource_size_t start, resource_size_t len)
>  {
>  	struct dfl_fpga_enum_dfl *dfl;
>  
> @@ -996,7 +1080,6 @@ int dfl_fpga_enum_info_add_dfl(struct dfl_fpga_enum_info *info,
>  
>  	dfl->start = start;
>  	dfl->len = len;
> -	dfl->ioaddr = ioaddr;
>  
>  	list_add_tail(&dfl->node, &info->dfls);
>  
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index a32dfba..f605c28 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -441,22 +441,18 @@ struct dfl_fpga_enum_info {
>   *
>   * @start: base address of this device feature list.
>   * @len: size of this device feature list.
> - * @ioaddr: mapped base address of this device feature list.
>   * @node: node in list of device feature lists.
>   */
>  struct dfl_fpga_enum_dfl {
>  	resource_size_t start;
>  	resource_size_t len;
extra white space
>  
> -	void __iomem *ioaddr;
> -
>  	struct list_head node;
>  };
>  
>  struct dfl_fpga_enum_info *dfl_fpga_enum_info_alloc(struct device *dev);
>  int dfl_fpga_enum_info_add_dfl(struct dfl_fpga_enum_info *info,
> -			       resource_size_t start, resource_size_t len,
> -			       void __iomem *ioaddr);
> +			       resource_size_t start, resource_size_t len);
>  int dfl_fpga_enum_info_add_irq(struct dfl_fpga_enum_info *info,
>  			       unsigned int nr_irqs, int *irq_table);
>  void dfl_fpga_enum_info_free(struct dfl_fpga_enum_info *info);

Re: [PATCH 1/2] fpga: dfl: map feature mmio resources in their own feature drivers

[Xu Yilun]

On Fri, Jul 17, 2020 at 09:51:34AM -0700, Tom Rix wrote:
> Mostly little stuff.
> 
> Consider refactoring create_feature_instance.
> 
> Tom
> 
> > diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
> > index e220bec..22dc025 100644
> > --- a/drivers/fpga/dfl-pci.c
> > +++ b/drivers/fpga/dfl-pci.c
> > @@ -39,6 +39,11 @@ static void __iomem *cci_pci_ioremap_bar(struct pci_dev *pcidev, int bar)
> >  	return pcim_iomap_table(pcidev)[bar];
> >  }
> >  
> > +static void cci_pci_iounmap_bars(struct pci_dev *pcidev, int mapped_bars)
> > +{
> > +	pcim_iounmap_regions(pcidev, mapped_bars);
> > +}
> 
> A singleton, called only one place. Consider replacing with a direct call.

I could remove it.

> 
> 
> > +
> >  static int cci_pci_alloc_irq(struct pci_dev *pcidev)
> >  {
> >  	int ret, nvec = pci_msix_vec_count(pcidev);
> > @@ -123,7 +128,7 @@ static int *cci_pci_create_irq_table(struct pci_dev *pcidev, unsigned int nvec)
> >  static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
> >  {
> >  	struct cci_drvdata *drvdata = pci_get_drvdata(pcidev);
> > -	int port_num, bar, i, nvec, ret = 0;
> > +	int port_num, bar, i, nvec, mapped_bars, ret = 0;
> 
> Shouldn't mapped_bars be at least unsigned and maybe either uint32_t or uint64_t ?
> 
> I see pcim_ioumap_regions has int as parameter. boo

That's why I define the mapped_bars as an int, to avoid casting.

> 
> >  	struct dfl_fpga_enum_info *info;
> >  	struct dfl_fpga_cdev *cdev;
> >  	resource_size_t start, len;
> > @@ -163,6 +168,8 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
> >  		goto irq_free_exit;
> >  	}
> >  
> > +	mapped_bars = BIT(0);
> > +
> >  	/*
> >  	 * PF device has FME and Ports/AFUs, and VF device only has one
> >  	 * Port/AFU. Check them and add related "Device Feature List" info
> > @@ -172,7 +179,7 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
> >  		start = pci_resource_start(pcidev, 0);
> >  		len = pci_resource_len(pcidev, 0);
> >  
> > -		dfl_fpga_enum_info_add_dfl(info, start, len, base);
> > +		dfl_fpga_enum_info_add_dfl(info, start, len);
> >  
> >  		/*
> >  		 * find more Device Feature Lists (e.g. Ports) per information
> > @@ -200,22 +207,26 @@ static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
> >  			if (!base)
> >  				continue;
> >  
> > +			mapped_bars |= BIT(bar);
> > +
> >  			start = pci_resource_start(pcidev, bar) + offset;
> >  			len = pci_resource_len(pcidev, bar) - offset;
> >  
> > -			dfl_fpga_enum_info_add_dfl(info, start, len,
> > -						   base + offset);
> > +			dfl_fpga_enum_info_add_dfl(info, start, len);
> >  		}
> >  	} else if (dfl_feature_is_port(base)) {
> >  		start = pci_resource_start(pcidev, 0);
> >  		len = pci_resource_len(pcidev, 0);
> >  
> > -		dfl_fpga_enum_info_add_dfl(info, start, len, base);
> > +		dfl_fpga_enum_info_add_dfl(info, start, len);
> >  	} else {
> >  		ret = -ENODEV;
> >  		goto irq_free_exit;
> >  	}
> >  
> > +	/* release I/O mappings for next step enumeration */
> > +	cci_pci_iounmap_bars(pcidev, mapped_bars);
> There is no iounmap_bars in error handling, likely need to add this.
> > +
> >  	/* start enumeration with prepared enumeration information */
> >  	cdev = dfl_fpga_feature_devs_enumerate(info);
> >  	if (IS_ERR(cdev)) {
> > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> > index 649958a..7dc6411 100644
> > --- a/drivers/fpga/dfl.c
> > +++ b/drivers/fpga/dfl.c
> > @@ -250,6 +250,11 @@ int dfl_fpga_check_port_id(struct platform_device *pdev, void *pport_id)
> >  }
> >  EXPORT_SYMBOL_GPL(dfl_fpga_check_port_id);
> >  
> > +static bool is_header_feature(struct dfl_feature *feature)
> > +{
> > +	return feature->id == FEATURE_ID_FIU_HEADER;
> Could this be a macro ?
> > +}
> > +
> >  /**
> >   * dfl_fpga_dev_feature_uinit - uinit for sub features of dfl feature device
> >   * @pdev: feature device.
> > @@ -273,8 +278,20 @@ static int dfl_feature_instance_init(struct platform_device *pdev,
> >  				     struct dfl_feature *feature,
> >  				     struct dfl_feature_driver *drv)
> >  {
> > +	void __iomem *base;
> >  	int ret = 0;
> >  
> > +	if (!is_header_feature(feature)) {
> > +		base = devm_platform_ioremap_resource(pdev,
> > +						      feature->resource_index);
> > +		if (IS_ERR(base)) {
> > +			dev_err(&pdev->dev, "fail to get iomem resource!\n");
> > +			return PTR_ERR(base);
> > +		}
> > +
> > +		feature->ioaddr = base;
> > +	}
> > +
> >  	if (drv->ops->init) {
> >  		ret = drv->ops->init(pdev, feature);
> >  		if (ret)
> > @@ -427,7 +444,9 @@ EXPORT_SYMBOL_GPL(dfl_fpga_dev_ops_unregister);
> >   * @irq_table: Linux IRQ numbers for all irqs, indexed by local irq index of
> >   *	       this device.
> >   * @feature_dev: current feature device.
> > - * @ioaddr: header register region address of feature device in enumeration.
> > + * @ioaddr: header register region address of current FIU in enumeration.
> > + * @start: register resource start of current FIU.
> > + * @len: max register resource length of current FIU.
> >   * @sub_features: a sub features linked list for feature device in enumeration.
> >   * @feature_num: number of sub features for feature device in enumeration.
> >   */
> > @@ -439,6 +458,9 @@ struct build_feature_devs_info {
> >  
> >  	struct platform_device *feature_dev;
> >  	void __iomem *ioaddr;
> > +	resource_size_t start;
> > +	resource_size_t len;
> > +
> extra whitespace, remove
> >  	struct list_head sub_features;
> >  	int feature_num;
> >  };
> > @@ -484,10 +506,7 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
> >  	struct dfl_feature_platform_data *pdata;
> >  	struct dfl_feature_info *finfo, *p;
> >  	enum dfl_id_type type;
> > -	int ret, index = 0;
> > -
> > -	if (!fdev)
> > -		return 0;
> > +	int ret, index = 0, res_idx = 0;
> >  
> >  	type = feature_dev_id_type(fdev);
> >  	if (WARN_ON_ONCE(type >= DFL_ID_MAX))
> > @@ -530,16 +549,30 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
> >  
> >  	/* fill features and resource information for feature dev */
> >  	list_for_each_entry_safe(finfo, p, &binfo->sub_features, node) {
> > -		struct dfl_feature *feature = &pdata->features[index];
> > +		struct dfl_feature *feature = &pdata->features[index++];
> >  		struct dfl_feature_irq_ctx *ctx;
> >  		unsigned int i;
> >  
> >  		/* save resource information for each feature */
> >  		feature->dev = fdev;
> >  		feature->id = finfo->fid;
> > -		feature->resource_index = index;
> > -		feature->ioaddr = finfo->ioaddr;
> > -		fdev->resource[index++] = finfo->mmio_res;
> > +
> > +		/*
> > +		 * map header resource for dfl bus device. Don't add header
> > +		 * resource to feature devices, or the resource tree will be
> > +		 * disordered and cause warning on resource release
> > +		 */
> > +		if (is_header_feature(feature)) {
> > +			feature->resource_index = -1;
> > +			feature->ioaddr =
> > +				devm_ioremap_resource(binfo->dev,
> > +						      &finfo->mmio_res);
> > +			if (IS_ERR(feature->ioaddr))
> > +				return PTR_ERR(feature->ioaddr);
> feature->ioaddr is garbage, is this ok ?
> > +		} else {
> > +			feature->resource_index = res_idx;
> > +			fdev->resource[res_idx++] = finfo->mmio_res;
> > +		}
> >  
> >  		if (finfo->nr_irqs) {
> >  			ctx = devm_kcalloc(binfo->dev, finfo->nr_irqs,
> > @@ -582,19 +615,13 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
> >  
> >  static int
> >  build_info_create_dev(struct build_feature_devs_info *binfo,
> > -		      enum dfl_id_type type, void __iomem *ioaddr)
> > +		      enum dfl_id_type type)
> >  {
> >  	struct platform_device *fdev;
> > -	int ret;
> >  
> >  	if (type >= DFL_ID_MAX)
> >  		return -EINVAL;
> >  
> > -	/* we will create a new device, commit current device first */
> > -	ret = build_info_commit_dev(binfo);
> > -	if (ret)
> > -		return ret;
> > -
> >  	/*
> >  	 * we use -ENODEV as the initialization indicator which indicates
> >  	 * whether the id need to be reclaimed
> > @@ -605,7 +632,7 @@ build_info_create_dev(struct build_feature_devs_info *binfo,
> >  
> >  	binfo->feature_dev = fdev;
> >  	binfo->feature_num = 0;
> > -	binfo->ioaddr = ioaddr;
> > +
> >  	INIT_LIST_HEAD(&binfo->sub_features);
> >  
> >  	fdev->id = dfl_id_alloc(type, &fdev->dev);
> > @@ -747,18 +774,17 @@ static int parse_feature_irqs(struct build_feature_devs_info *binfo,
> >   */
> >  static int
> >  create_feature_instance(struct build_feature_devs_info *binfo,
> > -			struct dfl_fpga_enum_dfl *dfl, resource_size_t ofst,
> > -			resource_size_t size, u64 fid)
> > +			resource_size_t ofst, resource_size_t size, u64 fid)
> >  {
> >  	unsigned int irq_base, nr_irqs;
> >  	struct dfl_feature_info *finfo;
> >  	int ret;
> >  
> >  	/* read feature size and id if inputs are invalid */
> > -	size = size ? size : feature_size(dfl->ioaddr + ofst);
> > -	fid = fid ? fid : feature_id(dfl->ioaddr + ofst);
> > +	size = size ? size : feature_size(binfo->ioaddr + ofst);
> > +	fid = fid ? fid : feature_id(binfo->ioaddr + ofst);
> 
> This is complicated. based on fiu,afu or private  calling.  Why not have a function ptr
> 
> struct build_feature_devs_info {
> 
> void (*find_info) (...)
> 
> }
> 
> find_info_fiu (..) {
> 
> size = feature_size(..)
> 
> fid = feature_id(..)
> 
> }

I think we may have another patch to optimize the feature_id.

But in my opinion, the feature_id call is only used during enumeration.
So it may not be necessary we have a function ptr for the one-off
function.

> 
> >  
> > -	if (dfl->len - ofst < size)
> > +	if (binfo->len - ofst < size)
> >  		return -EINVAL;
> >  
> >  	ret = parse_feature_irqs(binfo, ofst, fid, &irq_base, &nr_irqs);
> > @@ -770,12 +796,11 @@ create_feature_instance(struct build_feature_devs_info *binfo,
> >  		return -ENOMEM;
> >  
> >  	finfo->fid = fid;
> > -	finfo->mmio_res.start = dfl->start + ofst;
> > +	finfo->mmio_res.start = binfo->start + ofst;
> >  	finfo->mmio_res.end = finfo->mmio_res.start + size - 1;
> >  	finfo->mmio_res.flags = IORESOURCE_MEM;
> >  	finfo->irq_base = irq_base;
> >  	finfo->nr_irqs = nr_irqs;
> > -	finfo->ioaddr = dfl->ioaddr + ofst;
> >  
> >  	list_add_tail(&finfo->node, &binfo->sub_features);
> >  	binfo->feature_num++;
> > @@ -784,7 +809,6 @@ create_feature_instance(struct build_feature_devs_info *binfo,
> >  }
> >  
> >  static int parse_feature_port_afu(struct build_feature_devs_info *binfo,
> > -				  struct dfl_fpga_enum_dfl *dfl,
> >  				  resource_size_t ofst)
> >  {
> >  	u64 v = readq(binfo->ioaddr + PORT_HDR_CAP);
> > @@ -792,11 +816,10 @@ static int parse_feature_port_afu(struct build_feature_devs_info *binfo,
> >  
> >  	WARN_ON(!size);
> >  
> > -	return create_feature_instance(binfo, dfl, ofst, size, FEATURE_ID_AFU);
> > +	return create_feature_instance(binfo, ofst, size, FEATURE_ID_AFU);
> >  }
> >  
> >  static int parse_feature_afu(struct build_feature_devs_info *binfo,
> > -			     struct dfl_fpga_enum_dfl *dfl,
> >  			     resource_size_t ofst)
> >  {
> >  	if (!binfo->feature_dev) {
> > @@ -806,7 +829,7 @@ static int parse_feature_afu(struct build_feature_devs_info *binfo,
> >  
> >  	switch (feature_dev_id_type(binfo->feature_dev)) {
> >  	case PORT_ID:
> > -		return parse_feature_port_afu(binfo, dfl, ofst);
> > +		return parse_feature_port_afu(binfo, ofst);
> >  	default:
> >  		dev_info(binfo->dev, "AFU belonging to FIU %s is not supported yet.\n",
> >  			 binfo->feature_dev->name);
> > @@ -815,35 +838,91 @@ static int parse_feature_afu(struct build_feature_devs_info *binfo,
> >  	return 0;
> >  }
> >  
> > +static bool is_feature_dev_detected(struct build_feature_devs_info *binfo)
> > +{
> > +	return !!binfo->feature_dev;
> Another macro.
> > +}
> > +
> > +static void dfl_binfo_shift(struct build_feature_devs_info *binfo,
> > +			    resource_size_t ofst)
> shift? where is shifting happening.  A better name would be dfl_binfo_update or dfl_binfo_increment
> > +{
> > +	binfo->start = binfo->start + ofst;
> > +	binfo->len = binfo->len - ofst;
> > +}
> > +
> > +static int dfl_binfo_prepare(struct build_feature_devs_info *binfo,
> > +			     resource_size_t start, resource_size_t len)
> > +{
> > +	struct device *dev = binfo->dev;
> > +	void __iomem *ioaddr;
> > +
> > +	if (!devm_request_mem_region(dev, start, len, dev_name(dev))) {
> > +		dev_err(dev, "request region fail, start:%pa, len:%pa\n",
> > +			&start, &len);
> > +		return -ENOMEM;
> > +	}
> > +
> > +	ioaddr = devm_ioremap(dev, start, len);
> > +	if (!ioaddr) {
> > +		dev_err(dev, "ioremap region fail, start:%pa, len:%pa\n",
> > +			&start, &len);
> > +		devm_release_mem_region(dev, start, len);
> > +		return -EFAULT;
> > +	}
> > +
> > +	binfo->start = start;
> > +	binfo->len = len;
> > +	binfo->ioaddr = ioaddr;
> > +
> > +	return 0;
> > +}
> > +
> > +static void dfl_binfo_finish(struct build_feature_devs_info *binfo)
> > +{
> > +	devm_iounmap(binfo->dev, binfo->ioaddr);
> > +	devm_release_mem_region(binfo->dev, binfo->start, binfo->len);
> > +}
> > +
> >  static int parse_feature_fiu(struct build_feature_devs_info *binfo,
> > -			     struct dfl_fpga_enum_dfl *dfl,
> >  			     resource_size_t ofst)
> >  {
> >  	u32 id, offset;
> >  	u64 v;
> >  	int ret = 0;
> >  
> > -	v = readq(dfl->ioaddr + ofst + DFH);
> > +	if (is_feature_dev_detected(binfo)) {
> > +		dfl_binfo_finish(binfo);
> > +
> > +		ret = build_info_commit_dev(binfo);
> > +		if (ret)
> > +			return ret;
> > +
> > +		dfl_binfo_prepare(binfo, binfo->start + ofst,
> > +				  binfo->len - ofst);
> Check status of dfl_binfo_prepare

Yes.

> > +	} else {
> > +		dfl_binfo_shift(binfo, ofst);
> > +	}
> > +
> > +	v = readq(binfo->ioaddr + DFH);
> >  	id = FIELD_GET(DFH_ID, v);
> >  
> >  	/* create platform device for dfl feature dev */
> > -	ret = build_info_create_dev(binfo, dfh_id_to_type(id),
> > -				    dfl->ioaddr + ofst);
> > +	ret = build_info_create_dev(binfo, dfh_id_to_type(id));
> >  	if (ret)
> >  		return ret;
> >  
> > -	ret = create_feature_instance(binfo, dfl, ofst, 0, 0);
> > +	ret = create_feature_instance(binfo, 0, 0, 0);
> >  	if (ret)
> >  		return ret;
> >  	/*
> >  	 * find and parse FIU's child AFU via its NEXT_AFU register.
> >  	 * please note that only Port has valid NEXT_AFU pointer per spec.
> >  	 */
> > -	v = readq(dfl->ioaddr + ofst + NEXT_AFU);
> > +	v = readq(binfo->ioaddr + NEXT_AFU);
> >  
> >  	offset = FIELD_GET(NEXT_AFU_NEXT_DFH_OFST, v);
> >  	if (offset)
> > -		return parse_feature_afu(binfo, dfl, ofst + offset);
> > +		return parse_feature_afu(binfo, offset);
> >  
> >  	dev_dbg(binfo->dev, "No AFUs detected on FIU %d\n", id);
> >  
> > @@ -851,16 +930,15 @@ static int parse_feature_fiu(struct build_feature_devs_info *binfo,
> >  }
> >  
> >  static int parse_feature_private(struct build_feature_devs_info *binfo,
> > -				 struct dfl_fpga_enum_dfl *dfl,
> >  				 resource_size_t ofst)
> >  {
> >  	if (!binfo->feature_dev) {
> >  		dev_err(binfo->dev, "the private feature %llx does not belong to any AFU.\n",
> > -			(unsigned long long)feature_id(dfl->ioaddr + ofst));
> > +			(unsigned long long)feature_id(binfo->ioaddr + ofst));
> >  		return -EINVAL;
> >  	}
> >  
> > -	return create_feature_instance(binfo, dfl, ofst, 0, 0);
> > +	return create_feature_instance(binfo, ofst, 0, 0);
> >  }
> >  
> >  /**
> > @@ -868,24 +946,24 @@ static int parse_feature_private(struct build_feature_devs_info *binfo,
> >   *
> >   * @binfo: build feature devices information.
> >   * @dfl: device feature list to parse
> > - * @ofst: offset to feature header on this device feature list
> > + * @ofst: offset to current FIU header
> >   */
> >  static int parse_feature(struct build_feature_devs_info *binfo,
> > -			 struct dfl_fpga_enum_dfl *dfl, resource_size_t ofst)
> > +			 resource_size_t ofst)
> >  {
> >  	u64 v;
> >  	u32 type;
> >  
> > -	v = readq(dfl->ioaddr + ofst + DFH);
> > +	v = readq(binfo->ioaddr + ofst + DFH);
> >  	type = FIELD_GET(DFH_TYPE, v);
> >  
> >  	switch (type) {
> >  	case DFH_TYPE_AFU:
> > -		return parse_feature_afu(binfo, dfl, ofst);
> > +		return parse_feature_afu(binfo, ofst);
> >  	case DFH_TYPE_PRIVATE:
> > -		return parse_feature_private(binfo, dfl, ofst);
> > +		return parse_feature_private(binfo, ofst);
> >  	case DFH_TYPE_FIU:
> > -		return parse_feature_fiu(binfo, dfl, ofst);
> > +		return parse_feature_fiu(binfo, ofst);
> >  	default:
> >  		dev_info(binfo->dev,
> >  			 "Feature Type %x is not supported.\n", type);
> > @@ -897,12 +975,18 @@ static int parse_feature(struct build_feature_devs_info *binfo,
> >  static int parse_feature_list(struct build_feature_devs_info *binfo,
> >  			      struct dfl_fpga_enum_dfl *dfl)
> >  {
> > -	void __iomem *start = dfl->ioaddr;
> > -	void __iomem *end = dfl->ioaddr + dfl->len;
> > +	resource_size_t start, end;
> >  	int ret = 0;
> >  	u32 ofst = 0;
> >  	u64 v;
> >  
> > +	ret = dfl_binfo_prepare(binfo, dfl->start, dfl->len);
> > +	if (ret)
> > +		return ret;
> > +
> > +	start = dfl->start;
> > +	end = start + dfl->len;
> > +
> >  	/* walk through the device feature list via DFH's next DFH pointer. */
> >  	for (; start < end; start += ofst) {
> >  		if (end - start < DFH_SIZE) {
> > @@ -910,11 +994,11 @@ static int parse_feature_list(struct build_feature_devs_info *binfo,
> >  			return -EINVAL;
> >  		}
> >  
> > -		ret = parse_feature(binfo, dfl, start - dfl->ioaddr);
> > +		ret = parse_feature(binfo, start - binfo->start);
> >  		if (ret)
> >  			return ret;
> >  
> > -		v = readq(start + DFH);
> > +		v = readq(binfo->ioaddr + start - binfo->start + DFH);
> >  		ofst = FIELD_GET(DFH_NEXT_HDR_OFST, v);
> >  
> >  		/* stop parsing if EOL(End of List) is set or offset is 0 */
> > @@ -923,6 +1007,8 @@ static int parse_feature_list(struct build_feature_devs_info *binfo,
> >  	}
> >  
> >  	/* commit current feature device when reach the end of list */
> > +	dfl_binfo_finish(binfo);
> > +
> >  	return build_info_commit_dev(binfo);
> >  }
> >  
> > @@ -976,7 +1062,6 @@ EXPORT_SYMBOL_GPL(dfl_fpga_enum_info_free);
> >   * @info: ptr to dfl_fpga_enum_info
> >   * @start: mmio resource address of the device feature list.
> >   * @len: mmio resource length of the device feature list.
> > - * @ioaddr: mapped mmio resource address of the device feature list.
> >   *
> >   * One FPGA device may have one or more Device Feature Lists (DFLs), use this
> >   * function to add information of each DFL to common data structure for next
> > @@ -985,8 +1070,7 @@ EXPORT_SYMBOL_GPL(dfl_fpga_enum_info_free);
> >   * Return: 0 on success, negative error code otherwise.
> >   */
> >  int dfl_fpga_enum_info_add_dfl(struct dfl_fpga_enum_info *info,
> > -			       resource_size_t start, resource_size_t len,
> > -			       void __iomem *ioaddr)
> > +			       resource_size_t start, resource_size_t len)
> >  {
> >  	struct dfl_fpga_enum_dfl *dfl;
> >  
> > @@ -996,7 +1080,6 @@ int dfl_fpga_enum_info_add_dfl(struct dfl_fpga_enum_info *info,
> >  
> >  	dfl->start = start;
> >  	dfl->len = len;
> > -	dfl->ioaddr = ioaddr;
> >  
> >  	list_add_tail(&dfl->node, &info->dfls);
> >  
> > diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> > index a32dfba..f605c28 100644
> > --- a/drivers/fpga/dfl.h
> > +++ b/drivers/fpga/dfl.h
> > @@ -441,22 +441,18 @@ struct dfl_fpga_enum_info {
> >   *
> >   * @start: base address of this device feature list.
> >   * @len: size of this device feature list.
> > - * @ioaddr: mapped base address of this device feature list.
> >   * @node: node in list of device feature lists.
> >   */
> >  struct dfl_fpga_enum_dfl {
> >  	resource_size_t start;
> >  	resource_size_t len;
> extra white space

I can remove it.

Thanks,
Yilun

> >  
> > -	void __iomem *ioaddr;
> > -
> >  	struct list_head node;
> >  };
> >  
> >  struct dfl_fpga_enum_info *dfl_fpga_enum_info_alloc(struct device *dev);
> >  int dfl_fpga_enum_info_add_dfl(struct dfl_fpga_enum_info *info,
> > -			       resource_size_t start, resource_size_t len,
> > -			       void __iomem *ioaddr);
> > +			       resource_size_t start, resource_size_t len);
> >  int dfl_fpga_enum_info_add_irq(struct dfl_fpga_enum_info *info,
> >  			       unsigned int nr_irqs, int *irq_table);
> >  void dfl_fpga_enum_info_free(struct dfl_fpga_enum_info *info);

Re: [PATCH 1/2] fpga: dfl: map feature mmio resources in their own feature drivers

[Xu Yilun]

Sorry, I missed some comments. Reply inline.

> > +	/* release I/O mappings for next step enumeration */
> > +	cci_pci_iounmap_bars(pcidev, mapped_bars);
> There is no iounmap_bars in error handling, likely need to add this.

The memory allocated by pcim_iomap_xxx will be released along with
pcidev destory.

> > +
> >  	/* start enumeration with prepared enumeration information */
> >  	cdev = dfl_fpga_feature_devs_enumerate(info);
> >  	if (IS_ERR(cdev)) {
> > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> > index 649958a..7dc6411 100644
> > --- a/drivers/fpga/dfl.c
> > +++ b/drivers/fpga/dfl.c
> > @@ -250,6 +250,11 @@ int dfl_fpga_check_port_id(struct platform_device *pdev, void *pport_id)
> >  }
> >  EXPORT_SYMBOL_GPL(dfl_fpga_check_port_id);
> >  
> > +static bool is_header_feature(struct dfl_feature *feature)
> > +{
> > +	return feature->id == FEATURE_ID_FIU_HEADER;
> Could this be a macro ?

Yes

> > +}
> > +
> >  /**
> >   * dfl_fpga_dev_feature_uinit - uinit for sub features of dfl feature device
> >   * @pdev: feature device.
> > @@ -273,8 +278,20 @@ static int dfl_feature_instance_init(struct platform_device *pdev,
> >  				     struct dfl_feature *feature,
> >  				     struct dfl_feature_driver *drv)
> >  {
> > +	void __iomem *base;
> >  	int ret = 0;
> >  
> > +	if (!is_header_feature(feature)) {
> > +		base = devm_platform_ioremap_resource(pdev,
> > +						      feature->resource_index);
> > +		if (IS_ERR(base)) {
> > +			dev_err(&pdev->dev, "fail to get iomem resource!\n");
> > +			return PTR_ERR(base);
> > +		}
> > +
> > +		feature->ioaddr = base;
> > +	}
> > +
> >  	if (drv->ops->init) {
> >  		ret = drv->ops->init(pdev, feature);
> >  		if (ret)
> > @@ -427,7 +444,9 @@ EXPORT_SYMBOL_GPL(dfl_fpga_dev_ops_unregister);
> >   * @irq_table: Linux IRQ numbers for all irqs, indexed by local irq index of
> >   *	       this device.
> >   * @feature_dev: current feature device.
> > - * @ioaddr: header register region address of feature device in enumeration.
> > + * @ioaddr: header register region address of current FIU in enumeration.
> > + * @start: register resource start of current FIU.
> > + * @len: max register resource length of current FIU.
> >   * @sub_features: a sub features linked list for feature device in enumeration.
> >   * @feature_num: number of sub features for feature device in enumeration.
> >   */
> > @@ -439,6 +458,9 @@ struct build_feature_devs_info {
> >  
> >  	struct platform_device *feature_dev;
> >  	void __iomem *ioaddr;
> > +	resource_size_t start;
> > +	resource_size_t len;
> > +
> extra whitespace, remove

Yes

> >  	struct list_head sub_features;
> >  	int feature_num;
> >  };
> > @@ -484,10 +506,7 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
> >  	struct dfl_feature_platform_data *pdata;
> >  	struct dfl_feature_info *finfo, *p;
> >  	enum dfl_id_type type;
> > -	int ret, index = 0;
> > -
> > -	if (!fdev)
> > -		return 0;
> > +	int ret, index = 0, res_idx = 0;
> >  
> >  	type = feature_dev_id_type(fdev);
> >  	if (WARN_ON_ONCE(type >= DFL_ID_MAX))
> > @@ -530,16 +549,30 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
> >  
> >  	/* fill features and resource information for feature dev */
> >  	list_for_each_entry_safe(finfo, p, &binfo->sub_features, node) {
> > -		struct dfl_feature *feature = &pdata->features[index];
> > +		struct dfl_feature *feature = &pdata->features[index++];
> >  		struct dfl_feature_irq_ctx *ctx;
> >  		unsigned int i;
> >  
> >  		/* save resource information for each feature */
> >  		feature->dev = fdev;
> >  		feature->id = finfo->fid;
> > -		feature->resource_index = index;
> > -		feature->ioaddr = finfo->ioaddr;
> > -		fdev->resource[index++] = finfo->mmio_res;
> > +
> > +		/*
> > +		 * map header resource for dfl bus device. Don't add header
> > +		 * resource to feature devices, or the resource tree will be
> > +		 * disordered and cause warning on resource release
> > +		 */
> > +		if (is_header_feature(feature)) {
> > +			feature->resource_index = -1;
> > +			feature->ioaddr =
> > +				devm_ioremap_resource(binfo->dev,
> > +						      &finfo->mmio_res);
> > +			if (IS_ERR(feature->ioaddr))
> > +				return PTR_ERR(feature->ioaddr);
> feature->ioaddr is garbage, is this ok ?

It should be OK. We will not touch this field during cleaning up.

> > +static bool is_feature_dev_detected(struct build_feature_devs_info *binfo)
> > +{
> > +	return !!binfo->feature_dev;
> Another macro.

Yes

> > +}
> > +
> > +static void dfl_binfo_shift(struct build_feature_devs_info *binfo,
> > +			    resource_size_t ofst)
> shift? where is shifting happening.  A better name would be dfl_binfo_update or dfl_binfo_increment

I'll delete this function, it is not useful.

Thanks,
Yilun

RE: [PATCH 1/2] fpga: dfl: map feature mmio resources in their own feature drivers

[Wu, Hao]

> Subject: [PATCH 1/2] fpga: dfl: map feature mmio resources in their own
> feature drivers
> 
> This patch makes preparation for modularization of DFL sub feature
> drivers.
> 
> Currently, if we need to support a new DFL sub feature, an entry should
> be added to fme/port_feature_drvs[] in dfl-fme/port-main.c. And we need
> to re-compile the whole DFL modules. That make the DFL drivers hard to be
> extended.
> 
> Another consideration is that DFL may contain some IP blocks which are
> already supported by kernel, most of them are supported by platform
> device drivers. We could create platform devices for these IP blocks and
> get them supported by these drivers.
> 
> An important issue is that platform device drivers usually requests mmio
> resources on probe. But now dfl mmio is mapped in dfl bus driver (e.g.
> dfl-pci) as a whole region. Then platform device drivers for sub features
> can't request their own mmio resources again. This is what the patch
> trying to resolve.
> 
> This patch changes the DFL enumeration. DFL bus driver will unmap mmio
> resources after first step enumeration and pass enumeration info to DFL
> framework. Then DFL framework will map the mmio resources again, do 2nd
> step enumeration, and also unmap the mmio resources. In this way, sub
> feature drivers could then request their own mmio resources as needed.
> 
> An exception is that mmio resource of FIU headers are still mapped in dfl
> bus driver. The FIU headers have some fundamental functions (sriov set,
> port enable/disable) needed for dfl bus devices and other sub features.
> They should not be unmapped as long as dfl bus device is alive.
> 
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> ---
>  drivers/fpga/dfl-pci.c |  21 ++++--
>  drivers/fpga/dfl.c     | 187 +++++++++++++++++++++++++++++++++++-----------
> ---
>  drivers/fpga/dfl.h     |   6 +-
>  3 files changed, 152 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
> index e220bec..22dc025 100644
> --- a/drivers/fpga/dfl-pci.c
> +++ b/drivers/fpga/dfl-pci.c
> @@ -39,6 +39,11 @@ static void __iomem *cci_pci_ioremap_bar(struct
> pci_dev *pcidev, int bar)
>  	return pcim_iomap_table(pcidev)[bar];
>  }
> 
> +static void cci_pci_iounmap_bars(struct pci_dev *pcidev, int mapped_bars)
> +{
> +	pcim_iounmap_regions(pcidev, mapped_bars);
> +}
> +
>  static int cci_pci_alloc_irq(struct pci_dev *pcidev)
>  {
>  	int ret, nvec = pci_msix_vec_count(pcidev);
> @@ -123,7 +128,7 @@ static int *cci_pci_create_irq_table(struct pci_dev
> *pcidev, unsigned int nvec)
>  static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
>  {
>  	struct cci_drvdata *drvdata = pci_get_drvdata(pcidev);
> -	int port_num, bar, i, nvec, ret = 0;
> +	int port_num, bar, i, nvec, mapped_bars, ret = 0;
>  	struct dfl_fpga_enum_info *info;
>  	struct dfl_fpga_cdev *cdev;
>  	resource_size_t start, len;
> @@ -163,6 +168,8 @@ static int cci_enumerate_feature_devs(struct pci_dev
> *pcidev)
>  		goto irq_free_exit;
>  	}
> 
> +	mapped_bars = BIT(0);
> +
>  	/*
>  	 * PF device has FME and Ports/AFUs, and VF device only has one
>  	 * Port/AFU. Check them and add related "Device Feature List" info
> @@ -172,7 +179,7 @@ static int cci_enumerate_feature_devs(struct pci_dev
> *pcidev)
>  		start = pci_resource_start(pcidev, 0);
>  		len = pci_resource_len(pcidev, 0);
> 
> -		dfl_fpga_enum_info_add_dfl(info, start, len, base);
> +		dfl_fpga_enum_info_add_dfl(info, start, len);
> 
>  		/*
>  		 * find more Device Feature Lists (e.g. Ports) per information
> @@ -200,22 +207,26 @@ static int cci_enumerate_feature_devs(struct
> pci_dev *pcidev)
>  			if (!base)
>  				continue;
> 
> +			mapped_bars |= BIT(bar);
> +

One more place,

As you removed base addr usage below, so ideally we don't need to map
more bars for port here? is my understanding correct?

>  			start = pci_resource_start(pcidev, bar) + offset;
>  			len = pci_resource_len(pcidev, bar) - offset;
> 
> -			dfl_fpga_enum_info_add_dfl(info, start, len,
> -						   base + offset);
> +			dfl_fpga_enum_info_add_dfl(info, start, len);

Thanks
Hao

Re: [PATCH 1/2] fpga: dfl: map feature mmio resources in their own feature drivers

[Xu Yilun]

On Mon, Jul 20, 2020 at 05:09:35PM +0800, Wu, Hao wrote:
> > Subject: [PATCH 1/2] fpga: dfl: map feature mmio resources in their own
> > feature drivers
> >
> > This patch makes preparation for modularization of DFL sub feature
> > drivers.
> >
> > Currently, if we need to support a new DFL sub feature, an entry should
> > be added to fme/port_feature_drvs[] in dfl-fme/port-main.c. And we need
> > to re-compile the whole DFL modules. That make the DFL drivers hard to be
> > extended.
> >
> > Another consideration is that DFL may contain some IP blocks which are
> > already supported by kernel, most of them are supported by platform
> > device drivers. We could create platform devices for these IP blocks and
> > get them supported by these drivers.
> >
> > An important issue is that platform device drivers usually requests mmio
> > resources on probe. But now dfl mmio is mapped in dfl bus driver (e.g.
> > dfl-pci) as a whole region. Then platform device drivers for sub features
> > can't request their own mmio resources again. This is what the patch
> > trying to resolve.
> >
> > This patch changes the DFL enumeration. DFL bus driver will unmap mmio
> > resources after first step enumeration and pass enumeration info to DFL
> > framework. Then DFL framework will map the mmio resources again, do 2nd
> > step enumeration, and also unmap the mmio resources. In this way, sub
> > feature drivers could then request their own mmio resources as needed.
> >
> > An exception is that mmio resource of FIU headers are still mapped in dfl
> > bus driver. The FIU headers have some fundamental functions (sriov set,
> > port enable/disable) needed for dfl bus devices and other sub features.
> > They should not be unmapped as long as dfl bus device is alive.
> >
> > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > Signed-off-by: Wu Hao <hao.wu@intel.com>
> > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> > ---
> >  drivers/fpga/dfl-pci.c |  21 ++++--
> >  drivers/fpga/dfl.c     | 187 +++++++++++++++++++++++++++++++++++-----------
> > ---
> >  drivers/fpga/dfl.h     |   6 +-
> >  3 files changed, 152 insertions(+), 62 deletions(-)
> >
> > diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
> > index e220bec..22dc025 100644
> > --- a/drivers/fpga/dfl-pci.c
> > +++ b/drivers/fpga/dfl-pci.c
> > @@ -39,6 +39,11 @@ static void __iomem *cci_pci_ioremap_bar(struct
> > pci_dev *pcidev, int bar)
> >  return pcim_iomap_table(pcidev)[bar];
> >  }
> >
> > +static void cci_pci_iounmap_bars(struct pci_dev *pcidev, int mapped_bars)
> > +{
> > +pcim_iounmap_regions(pcidev, mapped_bars);
> > +}
> > +
> >  static int cci_pci_alloc_irq(struct pci_dev *pcidev)
> >  {
> >  int ret, nvec = pci_msix_vec_count(pcidev);
> > @@ -123,7 +128,7 @@ static int *cci_pci_create_irq_table(struct pci_dev
> > *pcidev, unsigned int nvec)
> >  static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
> >  {
> >  struct cci_drvdata *drvdata = pci_get_drvdata(pcidev);
> > -int port_num, bar, i, nvec, ret = 0;
> > +int port_num, bar, i, nvec, mapped_bars, ret = 0;
> >  struct dfl_fpga_enum_info *info;
> >  struct dfl_fpga_cdev *cdev;
> >  resource_size_t start, len;
> > @@ -163,6 +168,8 @@ static int cci_enumerate_feature_devs(struct pci_dev
> > *pcidev)
> >  goto irq_free_exit;
> >  }
> >
> > +mapped_bars = BIT(0);
> > +
> >  /*
> >   * PF device has FME and Ports/AFUs, and VF device only has one
> >   * Port/AFU. Check them and add related "Device Feature List" info
> > @@ -172,7 +179,7 @@ static int cci_enumerate_feature_devs(struct pci_dev
> > *pcidev)
> >  start = pci_resource_start(pcidev, 0);
> >  len = pci_resource_len(pcidev, 0);
> >
> > -dfl_fpga_enum_info_add_dfl(info, start, len, base);
> > +dfl_fpga_enum_info_add_dfl(info, start, len);
> >
> >  /*
> >   * find more Device Feature Lists (e.g. Ports) per information
> > @@ -200,22 +207,26 @@ static int cci_enumerate_feature_devs(struct
> > pci_dev *pcidev)
> >  if (!base)
> >  continue;
> >
> > +mapped_bars |= BIT(bar);
> > +
> 
> One more place,
> 
> As you removed base addr usage below, so ideally we don't need to map
> more bars for port here? is my understanding correct?

Exactly, thanks for the catching. This makes the code much simpler.

Thanks,
Yilun

> 
> >  start = pci_resource_start(pcidev, bar) + offset;
> >  len = pci_resource_len(pcidev, bar) - offset;
> >
> > -dfl_fpga_enum_info_add_dfl(info, start, len,
> > -   base + offset);
> > +dfl_fpga_enum_info_add_dfl(info, start, len);
> 
> Thanks
> Hao

[PATCH 2/2] fpga: dfl: create a dfl bus type to support DFL devices

[Xu Yilun]

A new bus type "dfl" is introduced for private features which are not
initialized by DFL feature drivers (dfl-fme & dfl-afu drivers). So these
private features could be handled by separate driver modules.

DFL framework will create DFL devices on enumeration. DFL drivers could
be registered on this bus to match these DFL devices. They are matched by
dfl type & feature_id.

Signed-off-by: Xu Yilun <yilun.xu@intel.com>
Signed-off-by: Wu Hao <hao.wu@intel.com>
Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
Signed-off-by: Russ Weight <russell.h.weight@intel.com>
---
 Documentation/ABI/testing/sysfs-bus-dfl |  15 ++
 drivers/fpga/dfl.c                      | 248 ++++++++++++++++++++++++++++++--
 drivers/fpga/dfl.h                      |  85 +++++++++++
 3 files changed, 340 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl

diff --git a/Documentation/ABI/testing/sysfs-bus-dfl b/Documentation/ABI/testing/sysfs-bus-dfl
new file mode 100644
index 0000000..cd00abc
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-dfl
@@ -0,0 +1,15 @@
+What:		/sys/bus/dfl/devices/.../type
+Date:		March 2020
+KernelVersion:	5.7
+Contact:	Xu Yilun <yilun.xu@intel.com>
+Description:	Read-only. It returns type of DFL FIU of the device. Now DFL
+		supports 2 FIU types, 0 for FME, 1 for PORT.
+		Format: 0x%x
+
+What:		/sys/bus/dfl/devices/.../feature_id
+Date:		March 2020
+KernelVersion:	5.7
+Contact:	Xu Yilun <yilun.xu@intel.com>
+Description:	Read-only. It returns feature identifier local to its DFL FIU
+		type.
+		Format: 0x%llx
diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
index 7dc6411..93f9d6d 100644
--- a/drivers/fpga/dfl.c
+++ b/drivers/fpga/dfl.c
@@ -30,12 +30,6 @@ static DEFINE_MUTEX(dfl_id_mutex);
  * index to dfl_chardevs table. If no chardev support just set devt_type
  * as one invalid index (DFL_FPGA_DEVT_MAX).
  */
-enum dfl_id_type {
-	FME_ID,		/* fme id allocation and mapping */
-	PORT_ID,	/* port id allocation and mapping */
-	DFL_ID_MAX,
-};
-
 enum dfl_fpga_devt_type {
 	DFL_FPGA_DEVT_FME,
 	DFL_FPGA_DEVT_PORT,
@@ -255,6 +249,228 @@ static bool is_header_feature(struct dfl_feature *feature)
 	return feature->id == FEATURE_ID_FIU_HEADER;
 }
 
+static const struct dfl_device_id *
+dfl_match_one_device(const struct dfl_device_id *id,
+		     struct dfl_device *dfl_dev)
+{
+	if (id->type == dfl_dev->type &&
+	    id->feature_id == dfl_dev->feature_id)
+		return id;
+
+	return NULL;
+}
+
+static int dfl_bus_match(struct device *dev, struct device_driver *drv)
+{
+	struct dfl_device *dfl_dev = to_dfl_dev(dev);
+	struct dfl_driver *dfl_drv = to_dfl_drv(drv);
+	const struct dfl_device_id *id_entry = dfl_drv->id_table;
+
+	while (id_entry->feature_id) {
+		if (dfl_match_one_device(id_entry, dfl_dev)) {
+			dfl_dev->id_entry = id_entry;
+			return 1;
+		}
+		id_entry++;
+	}
+
+	return 0;
+}
+
+static int dfl_bus_probe(struct device *dev)
+{
+	struct dfl_device *dfl_dev = to_dfl_dev(dev);
+	struct dfl_driver *dfl_drv = to_dfl_drv(dev->driver);
+
+	return dfl_drv->probe(dfl_dev);
+}
+
+static int dfl_bus_remove(struct device *dev)
+{
+	struct dfl_device *dfl_dev = to_dfl_dev(dev);
+	struct dfl_driver *dfl_drv = to_dfl_drv(dev->driver);
+
+	if (dfl_drv->remove)
+		dfl_drv->remove(dfl_dev);
+
+	return 0;
+}
+
+static int dfl_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
+{
+	struct dfl_device *dfl_dev = to_dfl_dev(dev);
+
+	if (add_uevent_var(env, "MODALIAS=dfl:%08x:%016llx",
+			   dfl_dev->type, dfl_dev->feature_id))
+		return -ENOMEM;
+
+	return 0;
+}
+
+/* show dfl info fields */
+#define dfl_info_attr(field, format_string)				\
+static ssize_t								\
+field##_show(struct device *dev, struct device_attribute *attr,		\
+	     char *buf)							\
+{									\
+	struct dfl_device *dfl_dev = to_dfl_dev(dev);			\
+									\
+	return sprintf(buf, format_string, dfl_dev->field);		\
+}									\
+static DEVICE_ATTR_RO(field)
+
+dfl_info_attr(type, "0x%x\n");
+dfl_info_attr(feature_id, "0x%llx\n");
+
+static struct attribute *dfl_dev_attrs[] = {
+	&dev_attr_type.attr,
+	&dev_attr_feature_id.attr,
+	NULL,
+};
+
+ATTRIBUTE_GROUPS(dfl_dev);
+
+static struct bus_type dfl_bus_type = {
+	.name		= "dfl",
+	.match		= dfl_bus_match,
+	.probe		= dfl_bus_probe,
+	.remove		= dfl_bus_remove,
+	.uevent		= dfl_bus_uevent,
+	.dev_groups	= dfl_dev_groups,
+};
+
+static void release_dfl_dev(struct device *dev)
+{
+	struct dfl_device *dfl_dev = to_dfl_dev(dev);
+
+	release_resource(&dfl_dev->mmio_res);
+	kfree(dfl_dev->irqs);
+	kfree(dfl_dev);
+}
+
+static struct dfl_device *
+dfl_dev_add(struct dfl_feature_platform_data *pdata,
+	    struct dfl_feature *feature)
+{
+	struct platform_device *pdev = pdata->dev;
+	struct dfl_device *dfl_dev;
+	int i, ret;
+
+	dfl_dev = kzalloc(sizeof(*dfl_dev), GFP_KERNEL);
+	if (!dfl_dev)
+		return ERR_PTR(-ENOMEM);
+
+	dfl_dev->cdev = pdata->dfl_cdev;
+
+	dfl_dev->mmio_res.parent = &pdev->resource[feature->resource_index];
+	dfl_dev->mmio_res.flags = IORESOURCE_MEM;
+	dfl_dev->mmio_res.start =
+		pdev->resource[feature->resource_index].start;
+	dfl_dev->mmio_res.end = pdev->resource[feature->resource_index].end;
+
+	/* then add irq resource */
+	if (feature->nr_irqs) {
+		dfl_dev->irqs = kcalloc(feature->nr_irqs,
+					sizeof(*dfl_dev->irqs), GFP_KERNEL);
+		if (!dfl_dev->irqs) {
+			ret = -ENOMEM;
+			goto free_dfl_dev;
+		}
+
+		for (i = 0; i < feature->nr_irqs; i++)
+			dfl_dev->irqs[i] = feature->irq_ctx[i].irq;
+
+		dfl_dev->num_irqs = feature->nr_irqs;
+	}
+
+	dfl_dev->type = feature_dev_id_type(pdev);
+	dfl_dev->feature_id = (unsigned long long)feature->id;
+
+	dfl_dev->dev.parent  = &pdev->dev;
+	dfl_dev->dev.bus     = &dfl_bus_type;
+	dfl_dev->dev.release = release_dfl_dev;
+	dev_set_name(&dfl_dev->dev, "%s.%d", dev_name(&pdev->dev),
+		     feature->index);
+
+	dfl_dev->mmio_res.name = dev_name(&dfl_dev->dev);
+	ret = insert_resource(dfl_dev->mmio_res.parent, &dfl_dev->mmio_res);
+	if (ret) {
+		dev_err(&pdev->dev, "%s failed to claim resource: %pR\n",
+			dev_name(&dfl_dev->dev), &dfl_dev->mmio_res);
+		goto free_irqs;
+	}
+
+	ret = device_register(&dfl_dev->dev);
+	if (ret) {
+		put_device(&dfl_dev->dev);
+		return ERR_PTR(ret);
+	}
+
+	dev_info(&pdev->dev, "add dfl_dev: %s\n",
+		 dev_name(&dfl_dev->dev));
+	return dfl_dev;
+
+free_irqs:
+	kfree(dfl_dev->irqs);
+free_dfl_dev:
+	kfree(dfl_dev);
+	return ERR_PTR(ret);
+}
+
+static void dfl_devs_uinit(struct dfl_feature_platform_data *pdata)
+{
+	struct dfl_device *dfl_dev;
+	struct dfl_feature *feature;
+
+	dfl_fpga_dev_for_each_feature(pdata, feature) {
+		if (!feature->ioaddr && feature->priv) {
+			dfl_dev = feature->priv;
+			device_unregister(&dfl_dev->dev);
+			feature->priv = NULL;
+		}
+	}
+}
+
+static int dfl_devs_init(struct platform_device *pdev)
+{
+	struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
+	struct dfl_feature *feature;
+	struct dfl_device *dfl_dev;
+
+	dfl_fpga_dev_for_each_feature(pdata, feature) {
+		if (feature->ioaddr || feature->priv)
+			continue;
+
+		dfl_dev = dfl_dev_add(pdata, feature);
+		if (IS_ERR(dfl_dev)) {
+			dfl_devs_uinit(pdata);
+			return PTR_ERR(dfl_dev);
+		}
+
+		feature->priv = dfl_dev;
+	}
+
+	return 0;
+}
+
+int __dfl_driver_register(struct dfl_driver *dfl_drv, struct module *owner)
+{
+	if (!dfl_drv || !dfl_drv->probe || !dfl_drv->id_table)
+		return -EINVAL;
+
+	dfl_drv->drv.owner = owner;
+	dfl_drv->drv.bus = &dfl_bus_type;
+
+	return driver_register(&dfl_drv->drv);
+}
+EXPORT_SYMBOL(__dfl_driver_register);
+
+void dfl_driver_unregister(struct dfl_driver *dfl_drv)
+{
+	driver_unregister(&dfl_drv->drv);
+}
+EXPORT_SYMBOL(dfl_driver_unregister);
+
 /**
  * dfl_fpga_dev_feature_uinit - uinit for sub features of dfl feature device
  * @pdev: feature device.
@@ -264,12 +480,15 @@ void dfl_fpga_dev_feature_uinit(struct platform_device *pdev)
 	struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
 	struct dfl_feature *feature;
 
-	dfl_fpga_dev_for_each_feature(pdata, feature)
+	dfl_devs_uinit(pdata);
+
+	dfl_fpga_dev_for_each_feature(pdata, feature) {
 		if (feature->ops) {
 			if (feature->ops->uinit)
 				feature->ops->uinit(pdev, feature);
 			feature->ops = NULL;
 		}
+	}
 }
 EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_uinit);
 
@@ -348,6 +567,10 @@ int dfl_fpga_dev_feature_init(struct platform_device *pdev,
 		drv++;
 	}
 
+	ret = dfl_devs_init(pdev);
+	if (ret)
+		goto exit;
+
 	return 0;
 exit:
 	dfl_fpga_dev_feature_uinit(pdev);
@@ -553,6 +776,8 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
 		struct dfl_feature_irq_ctx *ctx;
 		unsigned int i;
 
+		feature->index = index;
+
 		/* save resource information for each feature */
 		feature->dev = fdev;
 		feature->id = finfo->fid;
@@ -1295,11 +1520,17 @@ static int __init dfl_fpga_init(void)
 {
 	int ret;
 
+	ret = bus_register(&dfl_bus_type);
+	if (ret)
+		return ret;
+
 	dfl_ids_init();
 
 	ret = dfl_chardev_init();
-	if (ret)
+	if (ret) {
 		dfl_ids_destroy();
+		bus_unregister(&dfl_bus_type);
+	}
 
 	return ret;
 }
@@ -1637,6 +1868,7 @@ static void __exit dfl_fpga_exit(void)
 {
 	dfl_chardev_uinit();
 	dfl_ids_destroy();
+	bus_unregister(&dfl_bus_type);
 }
 
 module_init(dfl_fpga_init);
diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
index f605c28..d00aa1c 100644
--- a/drivers/fpga/dfl.h
+++ b/drivers/fpga/dfl.h
@@ -229,6 +229,10 @@ struct dfl_feature_irq_ctx {
  *
  * @dev: ptr to pdev of the feature device which has the sub feature.
  * @id: sub feature id.
+ * @index: unique identifier for an sub feature within the feature device.
+ *	   It is possible that multiply sub features with same feature id are
+ *	   listed in one feature device. So an incremental index (start from 0)
+ *	   is needed to identify each sub feature.
  * @resource_index: each sub feature has one mmio resource for its registers.
  *		    this index is used to find its mmio resource from the
  *		    feature dev (platform device)'s reources.
@@ -241,6 +245,7 @@ struct dfl_feature_irq_ctx {
 struct dfl_feature {
 	struct platform_device *dev;
 	u64 id;
+	int index;
 	int resource_index;
 	void __iomem *ioaddr;
 	struct dfl_feature_irq_ctx *irq_ctx;
@@ -515,4 +520,84 @@ long dfl_feature_ioctl_set_irq(struct platform_device *pdev,
 			       struct dfl_feature *feature,
 			       unsigned long arg);
 
+/**
+ * enum dfl_id_type - define the DFL FIU types
+ */
+enum dfl_id_type {
+	FME_ID,
+	PORT_ID,
+	DFL_ID_MAX,
+};
+
+/**
+ * struct dfl_device_id -  dfl device identifier
+ * @type: Type of DFL FIU of the device. See enum dfl_id_type.
+ * @feature_id: 64 bits feature identifier local to its DFL FIU type.
+ * @driver_data: Driver specific data
+ */
+struct dfl_device_id {
+	unsigned int type;
+	unsigned long long feature_id;
+	unsigned long driver_data;
+};
+
+/**
+ * struct dfl_device - represent an dfl device on dfl bus
+ *
+ * @dev: Generic device interface.
+ * @type: Type of DFL FIU of the device. See enum dfl_id_type.
+ * @feature_id: 64 bits feature identifier local to its DFL FIU type.
+ * @mmio_res: MMIO resource of this dfl device.
+ * @irqs: List of Linux IRQ numbers of this dfl device.
+ * @num_irqs: number of IRQs supported by this dfl device.
+ * @cdev: pointer to DFL FPGA container device this dfl device belongs to.
+ * @id_entry: matched id entry in dfl driver's id table.
+ */
+struct dfl_device {
+	struct device dev;
+	unsigned int type;
+	unsigned long long feature_id;
+	struct resource mmio_res;
+	int *irqs;
+	unsigned int num_irqs;
+	struct dfl_fpga_cdev *cdev;
+	const struct dfl_device_id *id_entry;
+};
+
+/**
+ * struct dfl_driver - represent an dfl device driver
+ *
+ * @drv: Driver model structure.
+ * @id_table: Pointer to table of device IDs the driver is interested in.
+ * @probe: Callback for device binding.
+ * @remove: Callback for device unbinding.
+ */
+struct dfl_driver {
+	struct device_driver drv;
+	const struct dfl_device_id *id_table;
+
+	int (*probe)(struct dfl_device *dfl_dev);
+	int (*remove)(struct dfl_device *dfl_dev);
+};
+
+#define to_dfl_dev(d) container_of(d, struct dfl_device, dev)
+#define to_dfl_drv(d) container_of(d, struct dfl_driver, drv)
+
+/*
+ * use a macro to avoid include chaining to get THIS_MODULE
+ */
+#define dfl_driver_register(drv) \
+	__dfl_driver_register(drv, THIS_MODULE)
+int __dfl_driver_register(struct dfl_driver *dfl_drv, struct module *owner);
+void dfl_driver_unregister(struct dfl_driver *dfl_drv);
+
+/* module_dfl_driver() - Helper macro for drivers that don't do
+ * anything special in module init/exit.  This eliminates a lot of
+ * boilerplate.  Each module may only use this macro once, and
+ * calling it replaces module_init() and module_exit()
+ */
+#define module_dfl_driver(__dfl_driver) \
+	module_driver(__dfl_driver, dfl_driver_register, \
+		      dfl_driver_unregister)
+
 #endif /* __FPGA_DFL_H */
-- 
2.7.4

RE: [PATCH 2/2] fpga: dfl: create a dfl bus type to support DFL devices

[Wu, Hao]

> -----Original Message-----
> From: Xu, Yilun <yilun.xu@intel.com>
> Sent: Wednesday, July 15, 2020 1:38 PM
> To: mdf@kernel.org; linux-fpga@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Cc: trix@redhat.com; lgoncalv@redhat.com; Xu, Yilun <yilun.xu@intel.com>;
> Wu, Hao <hao.wu@intel.com>; Matthew Gerlach
> <matthew.gerlach@linux.intel.com>; Weight, Russell H
> <russell.h.weight@intel.com>
> Subject: [PATCH 2/2] fpga: dfl: create a dfl bus type to support DFL devices
> 
> A new bus type "dfl" is introduced for private features which are not
> initialized by DFL feature drivers (dfl-fme & dfl-afu drivers). So these
> private features could be handled by separate driver modules.
> 
> DFL framework will create DFL devices on enumeration. 

Actually these DFL devices are created in AFU/FME driver initialization or real
core DFL code, is my understanding correct?

> DFL drivers could
> be registered on this bus to match these DFL devices. They are matched by
> dfl type & feature_id.
> 
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-dfl |  15 ++
>  drivers/fpga/dfl.c                      | 248 ++++++++++++++++++++++++++++++--
>  drivers/fpga/dfl.h                      |  85 +++++++++++
>  3 files changed, 340 insertions(+), 8 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-dfl
> b/Documentation/ABI/testing/sysfs-bus-dfl
> new file mode 100644
> index 0000000..cd00abc
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-dfl
> @@ -0,0 +1,15 @@
> +What:		/sys/bus/dfl/devices/.../type
> +Date:		March 2020
> +KernelVersion:	5.7

I guess you need to update the date and target kernel version.

> +Contact:	Xu Yilun <yilun.xu@intel.com>
> +Description:	Read-only. It returns type of DFL FIU of the device. Now DFL
> +		supports 2 FIU types, 0 for FME, 1 for PORT.
> +		Format: 0x%x

Or consider just print the string instead here?

> +
> +What:		/sys/bus/dfl/devices/.../feature_id
> +Date:		March 2020
> +KernelVersion:	5.7

Ditto.

> +Contact:	Xu Yilun <yilun.xu@intel.com>
> +Description:	Read-only. It returns feature identifier local to its DFL FIU
> +		type.
> +		Format: 0x%llx
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index 7dc6411..93f9d6d 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -30,12 +30,6 @@ static DEFINE_MUTEX(dfl_id_mutex);
>   * index to dfl_chardevs table. If no chardev support just set devt_type
>   * as one invalid index (DFL_FPGA_DEVT_MAX).
>   */
> -enum dfl_id_type {
> -	FME_ID,		/* fme id allocation and mapping */
> -	PORT_ID,	/* port id allocation and mapping */
> -	DFL_ID_MAX,
> -};
> -
>  enum dfl_fpga_devt_type {
>  	DFL_FPGA_DEVT_FME,
>  	DFL_FPGA_DEVT_PORT,
> @@ -255,6 +249,228 @@ static bool is_header_feature(struct dfl_feature
> *feature)
>  	return feature->id == FEATURE_ID_FIU_HEADER;
>  }
> 
> +static const struct dfl_device_id *
> +dfl_match_one_device(const struct dfl_device_id *id,
> +		     struct dfl_device *dfl_dev)

Why start a new line here, it's just 80 char here. : )
BTW: you can use ddev instead of dfl_dev here for a shorter name.

> +{
> +	if (id->type == dfl_dev->type &&
> +	    id->feature_id == dfl_dev->feature_id)
> +		return id;
> +
> +	return NULL;
> +}
> +
> +static int dfl_bus_match(struct device *dev, struct device_driver *drv)
> +{
> +	struct dfl_device *dfl_dev = to_dfl_dev(dev);
> +	struct dfl_driver *dfl_drv = to_dfl_drv(drv);
> +	const struct dfl_device_id *id_entry = dfl_drv->id_table;
> +
> +	while (id_entry->feature_id) {
> +		if (dfl_match_one_device(id_entry, dfl_dev)) {
> +			dfl_dev->id_entry = id_entry;
> +			return 1;
> +		}
> +		id_entry++;
> +	}
> +
> +	return 0;
> +}
> +
> +static int dfl_bus_probe(struct device *dev)
> +{
> +	struct dfl_device *dfl_dev = to_dfl_dev(dev);
> +	struct dfl_driver *dfl_drv = to_dfl_drv(dev->driver);
> +
> +	return dfl_drv->probe(dfl_dev);
> +}
> +
> +static int dfl_bus_remove(struct device *dev)
> +{
> +	struct dfl_device *dfl_dev = to_dfl_dev(dev);
> +	struct dfl_driver *dfl_drv = to_dfl_drv(dev->driver);
> +
> +	if (dfl_drv->remove)
> +		dfl_drv->remove(dfl_dev);
> +
> +	return 0;
> +}
> +
> +static int dfl_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
> +{
> +	struct dfl_device *dfl_dev = to_dfl_dev(dev);
> +
> +	if (add_uevent_var(env, "MODALIAS=dfl:%08x:%016llx",
> +			   dfl_dev->type, dfl_dev->feature_id))

I see for pci bus, it's using v%08Xd%08X... should we consider adding one
"t" to indicate that value is for type? Will that be simpler to the users?

> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
> +/* show dfl info fields */
> +#define dfl_info_attr(field, format_string)				\
> +static ssize_t								\
> +field##_show(struct device *dev, struct device_attribute *attr,
> 	\
> +	     char *buf)							\
> +{									\
> +	struct dfl_device *dfl_dev = to_dfl_dev(dev);			\
> +									\
> +	return sprintf(buf, format_string, dfl_dev->field);		\
> +}									\
> +static DEVICE_ATTR_RO(field)
> +
> +dfl_info_attr(type, "0x%x\n");
> +dfl_info_attr(feature_id, "0x%llx\n");
> +
> +static struct attribute *dfl_dev_attrs[] = {
> +	&dev_attr_type.attr,
> +	&dev_attr_feature_id.attr,
> +	NULL,
> +};
> +
> +ATTRIBUTE_GROUPS(dfl_dev);
> +
> +static struct bus_type dfl_bus_type = {
> +	.name		= "dfl",
> +	.match		= dfl_bus_match,
> +	.probe		= dfl_bus_probe,
> +	.remove		= dfl_bus_remove,
> +	.uevent		= dfl_bus_uevent,
> +	.dev_groups	= dfl_dev_groups,
> +};
> +
> +static void release_dfl_dev(struct device *dev)
> +{
> +	struct dfl_device *dfl_dev = to_dfl_dev(dev);
> +
> +	release_resource(&dfl_dev->mmio_res);
> +	kfree(dfl_dev->irqs);
> +	kfree(dfl_dev);
> +}
> +
> +static struct dfl_device *
> +dfl_dev_add(struct dfl_feature_platform_data *pdata,
> +	    struct dfl_feature *feature)
> +{
> +	struct platform_device *pdev = pdata->dev;
> +	struct dfl_device *dfl_dev;
> +	int i, ret;
> +
> +	dfl_dev = kzalloc(sizeof(*dfl_dev), GFP_KERNEL);
> +	if (!dfl_dev)
> +		return ERR_PTR(-ENOMEM);
> +
> +	dfl_dev->cdev = pdata->dfl_cdev;
> +
> +	dfl_dev->mmio_res.parent = &pdev->resource[feature-
> >resource_index];
> +	dfl_dev->mmio_res.flags = IORESOURCE_MEM;
> +	dfl_dev->mmio_res.start =
> +		pdev->resource[feature->resource_index].start;
> +	dfl_dev->mmio_res.end = pdev->resource[feature-
> >resource_index].end;
> +
> +	/* then add irq resource */
> +	if (feature->nr_irqs) {
> +		dfl_dev->irqs = kcalloc(feature->nr_irqs,
> +					sizeof(*dfl_dev->irqs), GFP_KERNEL);
> +		if (!dfl_dev->irqs) {
> +			ret = -ENOMEM;
> +			goto free_dfl_dev;
> +		}
> +
> +		for (i = 0; i < feature->nr_irqs; i++)
> +			dfl_dev->irqs[i] = feature->irq_ctx[i].irq;
> +
> +		dfl_dev->num_irqs = feature->nr_irqs;
> +	}
> +
> +	dfl_dev->type = feature_dev_id_type(pdev);
> +	dfl_dev->feature_id = (unsigned long long)feature->id;
> +
> +	dfl_dev->dev.parent  = &pdev->dev;
> +	dfl_dev->dev.bus     = &dfl_bus_type;
> +	dfl_dev->dev.release = release_dfl_dev;
> +	dev_set_name(&dfl_dev->dev, "%s.%d", dev_name(&pdev->dev),
> +		     feature->index);

Or it's better to have a generic name for the device on the bus.

> +
> +	dfl_dev->mmio_res.name = dev_name(&dfl_dev->dev);
> +	ret = insert_resource(dfl_dev->mmio_res.parent, &dfl_dev-
> >mmio_res);
> +	if (ret) {
> +		dev_err(&pdev->dev, "%s failed to claim resource: %pR\n",
> +			dev_name(&dfl_dev->dev), &dfl_dev->mmio_res);
> +		goto free_irqs;
> +	}
> +
> +	ret = device_register(&dfl_dev->dev);
> +	if (ret) {
> +		put_device(&dfl_dev->dev);
> +		return ERR_PTR(ret);
> +	}
> +
> +	dev_info(&pdev->dev, "add dfl_dev: %s\n",
> +		 dev_name(&dfl_dev->dev));
> +	return dfl_dev;
> +
> +free_irqs:
> +	kfree(dfl_dev->irqs);
> +free_dfl_dev:
> +	kfree(dfl_dev);
> +	return ERR_PTR(ret);
> +}
> +
> +static void dfl_devs_uinit(struct dfl_feature_platform_data *pdata)
> +{
> +	struct dfl_device *dfl_dev;
> +	struct dfl_feature *feature;
> +
> +	dfl_fpga_dev_for_each_feature(pdata, feature) {
> +		if (!feature->ioaddr && feature->priv) {
> +			dfl_dev = feature->priv;
> +			device_unregister(&dfl_dev->dev);
> +			feature->priv = NULL;
> +		}
> +	}
> +}
> +
> +static int dfl_devs_init(struct platform_device *pdev)
> +{
> +	struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev-
> >dev);
> +	struct dfl_feature *feature;
> +	struct dfl_device *dfl_dev;
> +
> +	dfl_fpga_dev_for_each_feature(pdata, feature) {
> +		if (feature->ioaddr || feature->priv)
> +			continue;
> +
> +		dfl_dev = dfl_dev_add(pdata, feature);
> +		if (IS_ERR(dfl_dev)) {
> +			dfl_devs_uinit(pdata);
> +			return PTR_ERR(dfl_dev);
> +		}
> +
> +		feature->priv = dfl_dev;

If 

> +	}
> +
> +	return 0;
> +}
> +
> +int __dfl_driver_register(struct dfl_driver *dfl_drv, struct module *owner)
> +{
> +	if (!dfl_drv || !dfl_drv->probe || !dfl_drv->id_table)
> +		return -EINVAL;
> +
> +	dfl_drv->drv.owner = owner;
> +	dfl_drv->drv.bus = &dfl_bus_type;
> +
> +	return driver_register(&dfl_drv->drv);
> +}
> +EXPORT_SYMBOL(__dfl_driver_register);
> +
> +void dfl_driver_unregister(struct dfl_driver *dfl_drv)
> +{
> +	driver_unregister(&dfl_drv->drv);
> +}
> +EXPORT_SYMBOL(dfl_driver_unregister);
> +
>  /**
>   * dfl_fpga_dev_feature_uinit - uinit for sub features of dfl feature device
>   * @pdev: feature device.
> @@ -264,12 +480,15 @@ void dfl_fpga_dev_feature_uinit(struct
> platform_device *pdev)
>  	struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev-
> >dev);
>  	struct dfl_feature *feature;
> 
> -	dfl_fpga_dev_for_each_feature(pdata, feature)
> +	dfl_devs_uinit(pdata);
> +
> +	dfl_fpga_dev_for_each_feature(pdata, feature) {
>  		if (feature->ops) {
>  			if (feature->ops->uinit)
>  				feature->ops->uinit(pdev, feature);
>  			feature->ops = NULL;
>  		}
> +	}
>  }
>  EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_uinit);
> 
> @@ -348,6 +567,10 @@ int dfl_fpga_dev_feature_init(struct
> platform_device *pdev,
>  		drv++;
>  	}
> 
> +	ret = dfl_devs_init(pdev);
> +	if (ret)
> +		goto exit;
> +
>  	return 0;
>  exit:
>  	dfl_fpga_dev_feature_uinit(pdev);
> @@ -553,6 +776,8 @@ static int build_info_commit_dev(struct
> build_feature_devs_info *binfo)
>  		struct dfl_feature_irq_ctx *ctx;
>  		unsigned int i;
> 
> +		feature->index = index;
> +
>  		/* save resource information for each feature */
>  		feature->dev = fdev;
>  		feature->id = finfo->fid;
> @@ -1295,11 +1520,17 @@ static int __init dfl_fpga_init(void)
>  {
>  	int ret;
> 
> +	ret = bus_register(&dfl_bus_type);
> +	if (ret)
> +		return ret;
> +
>  	dfl_ids_init();
> 
>  	ret = dfl_chardev_init();
> -	if (ret)
> +	if (ret) {
>  		dfl_ids_destroy();
> +		bus_unregister(&dfl_bus_type);
> +	}
> 
>  	return ret;
>  }
> @@ -1637,6 +1868,7 @@ static void __exit dfl_fpga_exit(void)
>  {
>  	dfl_chardev_uinit();
>  	dfl_ids_destroy();
> +	bus_unregister(&dfl_bus_type);
>  }
> 
>  module_init(dfl_fpga_init);
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index f605c28..d00aa1c 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -229,6 +229,10 @@ struct dfl_feature_irq_ctx {
>   *
>   * @dev: ptr to pdev of the feature device which has the sub feature.
>   * @id: sub feature id.
> + * @index: unique identifier for an sub feature within the feature device.
> + *	   It is possible that multiply sub features with same feature id are
> + *	   listed in one feature device. So an incremental index (start from 0)
> + *	   is needed to identify each sub feature.
>   * @resource_index: each sub feature has one mmio resource for its
> registers.
>   *		    this index is used to find its mmio resource from the
>   *		    feature dev (platform device)'s reources.
> @@ -241,6 +245,7 @@ struct dfl_feature_irq_ctx {
>  struct dfl_feature {
>  	struct platform_device *dev;
>  	u64 id;
> +	int index;
>  	int resource_index;
>  	void __iomem *ioaddr;
>  	struct dfl_feature_irq_ctx *irq_ctx;
> @@ -515,4 +520,84 @@ long dfl_feature_ioctl_set_irq(struct
> platform_device *pdev,
>  			       struct dfl_feature *feature,
>  			       unsigned long arg);
> 
> +/**
> + * enum dfl_id_type - define the DFL FIU types
> + */
> +enum dfl_id_type {
> +	FME_ID,
> +	PORT_ID,
> +	DFL_ID_MAX,
> +};
> +
> +/**
> + * struct dfl_device_id -  dfl device identifier
> + * @type: Type of DFL FIU of the device. See enum dfl_id_type.
> + * @feature_id: 64 bits feature identifier local to its DFL FIU type.
> + * @driver_data: Driver specific data
> + */
> +struct dfl_device_id {
> +	unsigned int type;
> +	unsigned long long feature_id;
> +	unsigned long driver_data;

Seems not used yet for driver_data, or can be in later patch with real users.

> +};
> +
> +/**
> + * struct dfl_device - represent an dfl device on dfl bus
> + *
> + * @dev: Generic device interface.
> + * @type: Type of DFL FIU of the device. See enum dfl_id_type.
> + * @feature_id: 64 bits feature identifier local to its DFL FIU type.
> + * @mmio_res: MMIO resource of this dfl device.
> + * @irqs: List of Linux IRQ numbers of this dfl device.
> + * @num_irqs: number of IRQs supported by this dfl device.
> + * @cdev: pointer to DFL FPGA container device this dfl device belongs to.
> + * @id_entry: matched id entry in dfl driver's id table.
> + */
> +struct dfl_device {
> +	struct device dev;
> +	unsigned int type;
> +	unsigned long long feature_id;
> +	struct resource mmio_res;
> +	int *irqs;
> +	unsigned int num_irqs;
> +	struct dfl_fpga_cdev *cdev;
> +	const struct dfl_device_id *id_entry;
> +};
> +
> +/**
> + * struct dfl_driver - represent an dfl device driver
> + *
> + * @drv: Driver model structure.
> + * @id_table: Pointer to table of device IDs the driver is interested in.
> + * @probe: Callback for device binding.
> + * @remove: Callback for device unbinding.
> + */
> +struct dfl_driver {
> +	struct device_driver drv;
> +	const struct dfl_device_id *id_table;
> +
> +	int (*probe)(struct dfl_device *dfl_dev);
> +	int (*remove)(struct dfl_device *dfl_dev);
> +};
> +
> +#define to_dfl_dev(d) container_of(d, struct dfl_device, dev)
> +#define to_dfl_drv(d) container_of(d, struct dfl_driver, drv)
> +
> +/*
> + * use a macro to avoid include chaining to get THIS_MODULE
> + */
> +#define dfl_driver_register(drv) \
> +	__dfl_driver_register(drv, THIS_MODULE)
> +int __dfl_driver_register(struct dfl_driver *dfl_drv, struct module *owner);
> +void dfl_driver_unregister(struct dfl_driver *dfl_drv);
> +
> +/* module_dfl_driver() - Helper macro for drivers that don't do

/*
 * module_dfl_driver()

> + * anything special in module init/exit.  This eliminates a lot of
> + * boilerplate.  Each module may only use this macro once, and
> + * calling it replaces module_init() and module_exit()
> + */
> +#define module_dfl_driver(__dfl_driver) \
> +	module_driver(__dfl_driver, dfl_driver_register, \
> +		      dfl_driver_unregister)
> +
>  #endif /* __FPGA_DFL_H */

BTW: maybe it's better to have one patch to add a driver using this bus as an example?

Thanks
Hao

> --
> 2.7.4

Re: [PATCH 2/2] fpga: dfl: create a dfl bus type to support DFL devices

[Xu Yilun]

On Fri, Jul 17, 2020 at 06:26:37PM +0800, Wu, Hao wrote:
> > -----Original Message-----
> > From: Xu, Yilun <yilun.xu@intel.com>
> > Sent: Wednesday, July 15, 2020 1:38 PM
> > To: mdf@kernel.org; linux-fpga@vger.kernel.org; linux-
> > kernel@vger.kernel.org
> > Cc: trix@redhat.com; lgoncalv@redhat.com; Xu, Yilun <yilun.xu@intel.com>;
> > Wu, Hao <hao.wu@intel.com>; Matthew Gerlach
> > <matthew.gerlach@linux.intel.com>; Weight, Russell H
> > <russell.h.weight@intel.com>
> > Subject: [PATCH 2/2] fpga: dfl: create a dfl bus type to support DFL devices
> >
> > A new bus type "dfl" is introduced for private features which are not
> > initialized by DFL feature drivers (dfl-fme & dfl-afu drivers). So these
> > private features could be handled by separate driver modules.
> >
> > DFL framework will create DFL devices on enumeration.
> 
> Actually these DFL devices are created in AFU/FME driver initialization or real
> core DFL code, is my understanding correct?

Yes I could change the comments.

> 
> > DFL drivers could
> > be registered on this bus to match these DFL devices. They are matched by
> > dfl type & feature_id.
> >
> > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > Signed-off-by: Wu Hao <hao.wu@intel.com>
> > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> > ---
> >  Documentation/ABI/testing/sysfs-bus-dfl |  15 ++
> >  drivers/fpga/dfl.c                      | 248 ++++++++++++++++++++++++++++++--
> >  drivers/fpga/dfl.h                      |  85 +++++++++++
> >  3 files changed, 340 insertions(+), 8 deletions(-)
> >  create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-dfl
> > b/Documentation/ABI/testing/sysfs-bus-dfl
> > new file mode 100644
> > index 0000000..cd00abc
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-bus-dfl
> > @@ -0,0 +1,15 @@
> > +What:/sys/bus/dfl/devices/.../type
> > +Date:March 2020
> > +KernelVersion:5.7
> 
> I guess you need to update the date and target kernel version.

Yes

> 
> > +Contact:Xu Yilun <yilun.xu@intel.com>
> > +Description:Read-only. It returns type of DFL FIU of the device. Now DFL
> > +supports 2 FIU types, 0 for FME, 1 for PORT.
> > +Format: 0x%x
> 
> Or consider just print the string instead here?

I think we don't have to. Keeping it align with dfl_device_id.type may
be better. 

> 
> > +
> > +What:/sys/bus/dfl/devices/.../feature_id
> > +Date:March 2020
> > +KernelVersion:5.7
> 
> Ditto.

Yes

> 
> > +Contact:Xu Yilun <yilun.xu@intel.com>
> > +Description:Read-only. It returns feature identifier local to its DFL FIU
> > +type.
> > +Format: 0x%llx
> > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> > index 7dc6411..93f9d6d 100644
> > --- a/drivers/fpga/dfl.c
> > +++ b/drivers/fpga/dfl.c
> > @@ -30,12 +30,6 @@ static DEFINE_MUTEX(dfl_id_mutex);
> >   * index to dfl_chardevs table. If no chardev support just set devt_type
> >   * as one invalid index (DFL_FPGA_DEVT_MAX).
> >   */
> > -enum dfl_id_type {
> > -FME_ID,/* fme id allocation and mapping */
> > -PORT_ID,/* port id allocation and mapping */
> > -DFL_ID_MAX,
> > -};
> > -
> >  enum dfl_fpga_devt_type {
> >  DFL_FPGA_DEVT_FME,
> >  DFL_FPGA_DEVT_PORT,
> > @@ -255,6 +249,228 @@ static bool is_header_feature(struct dfl_feature
> > *feature)
> >  return feature->id == FEATURE_ID_FIU_HEADER;
> >  }
> >
> > +static const struct dfl_device_id *
> > +dfl_match_one_device(const struct dfl_device_id *id,
> > +     struct dfl_device *dfl_dev)
> 
> Why start a new line here, it's just 80 char here. : )
> BTW: you can use ddev instead of dfl_dev here for a shorter name.

Yes.

> 
> > +{
> > +if (id->type == dfl_dev->type &&
> > +    id->feature_id == dfl_dev->feature_id)
> > +return id;
> > +
> > +return NULL;
> > +}
> > +
> > +static int dfl_bus_match(struct device *dev, struct device_driver *drv)
> > +{
> > +struct dfl_device *dfl_dev = to_dfl_dev(dev);
> > +struct dfl_driver *dfl_drv = to_dfl_drv(drv);
> > +const struct dfl_device_id *id_entry = dfl_drv->id_table;
> > +
> > +while (id_entry->feature_id) {
> > +if (dfl_match_one_device(id_entry, dfl_dev)) {
> > +dfl_dev->id_entry = id_entry;
> > +return 1;
> > +}
> > +id_entry++;
> > +}
> > +
> > +return 0;
> > +}
> > +
> > +static int dfl_bus_probe(struct device *dev)
> > +{
> > +struct dfl_device *dfl_dev = to_dfl_dev(dev);
> > +struct dfl_driver *dfl_drv = to_dfl_drv(dev->driver);
> > +
> > +return dfl_drv->probe(dfl_dev);
> > +}
> > +
> > +static int dfl_bus_remove(struct device *dev)
> > +{
> > +struct dfl_device *dfl_dev = to_dfl_dev(dev);
> > +struct dfl_driver *dfl_drv = to_dfl_drv(dev->driver);
> > +
> > +if (dfl_drv->remove)
> > +dfl_drv->remove(dfl_dev);
> > +
> > +return 0;
> > +}
> > +
> > +static int dfl_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
> > +{
> > +struct dfl_device *dfl_dev = to_dfl_dev(dev);
> > +
> > +if (add_uevent_var(env, "MODALIAS=dfl:%08x:%016llx",
> > +   dfl_dev->type, dfl_dev->feature_id))
> 
> I see for pci bus, it's using v%08Xd%08X... should we consider adding one
> "t" to indicate that value is for type? Will that be simpler to the users?

Yes I could add the tags, maybe "dfl:t%08xf%016llx". So it will not
cause conflicted modalias if a new id field is added.

> 
> > +return -ENOMEM;
> > +
> > +return 0;
> > +}
> > +
> > +/* show dfl info fields */
> > +#define dfl_info_attr(field, format_string)\
> > +static ssize_t\
> > +field##_show(struct device *dev, struct device_attribute *attr,
> > \
> > +     char *buf)\
> > +{\
> > +struct dfl_device *dfl_dev = to_dfl_dev(dev);\
> > +\
> > +return sprintf(buf, format_string, dfl_dev->field);\
> > +}\
> > +static DEVICE_ATTR_RO(field)
> > +
> > +dfl_info_attr(type, "0x%x\n");
> > +dfl_info_attr(feature_id, "0x%llx\n");
> > +
> > +static struct attribute *dfl_dev_attrs[] = {
> > +&dev_attr_type.attr,
> > +&dev_attr_feature_id.attr,
> > +NULL,
> > +};
> > +
> > +ATTRIBUTE_GROUPS(dfl_dev);
> > +
> > +static struct bus_type dfl_bus_type = {
> > +.name= "dfl",
> > +.match= dfl_bus_match,
> > +.probe= dfl_bus_probe,
> > +.remove= dfl_bus_remove,
> > +.uevent= dfl_bus_uevent,
> > +.dev_groups= dfl_dev_groups,
> > +};
> > +
> > +static void release_dfl_dev(struct device *dev)
> > +{
> > +struct dfl_device *dfl_dev = to_dfl_dev(dev);
> > +
> > +release_resource(&dfl_dev->mmio_res);
> > +kfree(dfl_dev->irqs);
> > +kfree(dfl_dev);
> > +}
> > +
> > +static struct dfl_device *
> > +dfl_dev_add(struct dfl_feature_platform_data *pdata,
> > +    struct dfl_feature *feature)
> > +{
> > +struct platform_device *pdev = pdata->dev;
> > +struct dfl_device *dfl_dev;
> > +int i, ret;
> > +
> > +dfl_dev = kzalloc(sizeof(*dfl_dev), GFP_KERNEL);
> > +if (!dfl_dev)
> > +return ERR_PTR(-ENOMEM);
> > +
> > +dfl_dev->cdev = pdata->dfl_cdev;
> > +
> > +dfl_dev->mmio_res.parent = &pdev->resource[feature-
> > >resource_index];
> > +dfl_dev->mmio_res.flags = IORESOURCE_MEM;
> > +dfl_dev->mmio_res.start =
> > +pdev->resource[feature->resource_index].start;
> > +dfl_dev->mmio_res.end = pdev->resource[feature-
> > >resource_index].end;
> > +
> > +/* then add irq resource */
> > +if (feature->nr_irqs) {
> > +dfl_dev->irqs = kcalloc(feature->nr_irqs,
> > +sizeof(*dfl_dev->irqs), GFP_KERNEL);
> > +if (!dfl_dev->irqs) {
> > +ret = -ENOMEM;
> > +goto free_dfl_dev;
> > +}
> > +
> > +for (i = 0; i < feature->nr_irqs; i++)
> > +dfl_dev->irqs[i] = feature->irq_ctx[i].irq;
> > +
> > +dfl_dev->num_irqs = feature->nr_irqs;
> > +}
> > +
> > +dfl_dev->type = feature_dev_id_type(pdev);
> > +dfl_dev->feature_id = (unsigned long long)feature->id;
> > +
> > +dfl_dev->dev.parent  = &pdev->dev;
> > +dfl_dev->dev.bus     = &dfl_bus_type;
> > +dfl_dev->dev.release = release_dfl_dev;
> > +dev_set_name(&dfl_dev->dev, "%s.%d", dev_name(&pdev->dev),
> > +     feature->index);
> 
> Or it's better to have a generic name for the device on the bus.

mm.. It is good suggestion, we should have a unified name for dfl
devices.

How about ("dfl.%d.%d", pdev->id, feature->index)

> 
> > +
> > +dfl_dev->mmio_res.name = dev_name(&dfl_dev->dev);
> > +ret = insert_resource(dfl_dev->mmio_res.parent, &dfl_dev-
> > >mmio_res);
> > +if (ret) {
> > +dev_err(&pdev->dev, "%s failed to claim resource: %pR\n",
> > +dev_name(&dfl_dev->dev), &dfl_dev->mmio_res);
> > +goto free_irqs;
> > +}
> > +
> > +ret = device_register(&dfl_dev->dev);
> > +if (ret) {
> > +put_device(&dfl_dev->dev);
> > +return ERR_PTR(ret);
> > +}
> > +
> > +dev_info(&pdev->dev, "add dfl_dev: %s\n",
> > + dev_name(&dfl_dev->dev));
> > +return dfl_dev;
> > +
> > +free_irqs:
> > +kfree(dfl_dev->irqs);
> > +free_dfl_dev:
> > +kfree(dfl_dev);
> > +return ERR_PTR(ret);
> > +}
> > +
> > +static void dfl_devs_uinit(struct dfl_feature_platform_data *pdata)
> > +{
> > +struct dfl_device *dfl_dev;
> > +struct dfl_feature *feature;
> > +
> > +dfl_fpga_dev_for_each_feature(pdata, feature) {
> > +if (!feature->ioaddr && feature->priv) {
> > +dfl_dev = feature->priv;
> > +device_unregister(&dfl_dev->dev);
> > +feature->priv = NULL;
> > +}
> > +}
> > +}
> > +
> > +static int dfl_devs_init(struct platform_device *pdev)
> > +{
> > +struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev-
> > >dev);
> > +struct dfl_feature *feature;
> > +struct dfl_device *dfl_dev;
> > +
> > +dfl_fpga_dev_for_each_feature(pdata, feature) {
> > +if (feature->ioaddr || feature->priv)
> > +continue;
> > +
> > +dfl_dev = dfl_dev_add(pdata, feature);
> > +if (IS_ERR(dfl_dev)) {
> > +dfl_devs_uinit(pdata);
> > +return PTR_ERR(dfl_dev);
> > +}
> > +
> > +feature->priv = dfl_dev;
> 
> If
> 
> > +}
> > +
> > +return 0;
> > +}
> > +
> > +int __dfl_driver_register(struct dfl_driver *dfl_drv, struct module *owner)
> > +{
> > +if (!dfl_drv || !dfl_drv->probe || !dfl_drv->id_table)
> > +return -EINVAL;
> > +
> > +dfl_drv->drv.owner = owner;
> > +dfl_drv->drv.bus = &dfl_bus_type;
> > +
> > +return driver_register(&dfl_drv->drv);
> > +}
> > +EXPORT_SYMBOL(__dfl_driver_register);
> > +
> > +void dfl_driver_unregister(struct dfl_driver *dfl_drv)
> > +{
> > +driver_unregister(&dfl_drv->drv);
> > +}
> > +EXPORT_SYMBOL(dfl_driver_unregister);
> > +
> >  /**
> >   * dfl_fpga_dev_feature_uinit - uinit for sub features of dfl feature device
> >   * @pdev: feature device.
> > @@ -264,12 +480,15 @@ void dfl_fpga_dev_feature_uinit(struct
> > platform_device *pdev)
> >  struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev-
> > >dev);
> >  struct dfl_feature *feature;
> >
> > -dfl_fpga_dev_for_each_feature(pdata, feature)
> > +dfl_devs_uinit(pdata);
> > +
> > +dfl_fpga_dev_for_each_feature(pdata, feature) {
> >  if (feature->ops) {
> >  if (feature->ops->uinit)
> >  feature->ops->uinit(pdev, feature);
> >  feature->ops = NULL;
> >  }
> > +}
> >  }
> >  EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_uinit);
> >
> > @@ -348,6 +567,10 @@ int dfl_fpga_dev_feature_init(struct
> > platform_device *pdev,
> >  drv++;
> >  }
> >
> > +ret = dfl_devs_init(pdev);
> > +if (ret)
> > +goto exit;
> > +
> >  return 0;
> >  exit:
> >  dfl_fpga_dev_feature_uinit(pdev);
> > @@ -553,6 +776,8 @@ static int build_info_commit_dev(struct
> > build_feature_devs_info *binfo)
> >  struct dfl_feature_irq_ctx *ctx;
> >  unsigned int i;
> >
> > +feature->index = index;
> > +
> >  /* save resource information for each feature */
> >  feature->dev = fdev;
> >  feature->id = finfo->fid;
> > @@ -1295,11 +1520,17 @@ static int __init dfl_fpga_init(void)
> >  {
> >  int ret;
> >
> > +ret = bus_register(&dfl_bus_type);
> > +if (ret)
> > +return ret;
> > +
> >  dfl_ids_init();
> >
> >  ret = dfl_chardev_init();
> > -if (ret)
> > +if (ret) {
> >  dfl_ids_destroy();
> > +bus_unregister(&dfl_bus_type);
> > +}
> >
> >  return ret;
> >  }
> > @@ -1637,6 +1868,7 @@ static void __exit dfl_fpga_exit(void)
> >  {
> >  dfl_chardev_uinit();
> >  dfl_ids_destroy();
> > +bus_unregister(&dfl_bus_type);
> >  }
> >
> >  module_init(dfl_fpga_init);
> > diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> > index f605c28..d00aa1c 100644
> > --- a/drivers/fpga/dfl.h
> > +++ b/drivers/fpga/dfl.h
> > @@ -229,6 +229,10 @@ struct dfl_feature_irq_ctx {
> >   *
> >   * @dev: ptr to pdev of the feature device which has the sub feature.
> >   * @id: sub feature id.
> > + * @index: unique identifier for an sub feature within the feature device.
> > + *   It is possible that multiply sub features with same feature id are
> > + *   listed in one feature device. So an incremental index (start from 0)
> > + *   is needed to identify each sub feature.
> >   * @resource_index: each sub feature has one mmio resource for its
> > registers.
> >   *    this index is used to find its mmio resource from the
> >   *    feature dev (platform device)'s reources.
> > @@ -241,6 +245,7 @@ struct dfl_feature_irq_ctx {
> >  struct dfl_feature {
> >  struct platform_device *dev;
> >  u64 id;
> > +int index;
> >  int resource_index;
> >  void __iomem *ioaddr;
> >  struct dfl_feature_irq_ctx *irq_ctx;
> > @@ -515,4 +520,84 @@ long dfl_feature_ioctl_set_irq(struct
> > platform_device *pdev,
> >         struct dfl_feature *feature,
> >         unsigned long arg);
> >
> > +/**
> > + * enum dfl_id_type - define the DFL FIU types
> > + */
> > +enum dfl_id_type {
> > +FME_ID,
> > +PORT_ID,
> > +DFL_ID_MAX,
> > +};
> > +
> > +/**
> > + * struct dfl_device_id -  dfl device identifier
> > + * @type: Type of DFL FIU of the device. See enum dfl_id_type.
> > + * @feature_id: 64 bits feature identifier local to its DFL FIU type.
> > + * @driver_data: Driver specific data
> > + */
> > +struct dfl_device_id {
> > +unsigned int type;
> > +unsigned long long feature_id;
> > +unsigned long driver_data;
> 
> Seems not used yet for driver_data, or can be in later patch with real users.

I think we may keep this. Cause modpost also need this struct
dfl_device_id, I think it would be better we don't frequently change the
struct to avoid sync problem between kernel & modpost.

> 
> > +};
> > +
> > +/**
> > + * struct dfl_device - represent an dfl device on dfl bus
> > + *
> > + * @dev: Generic device interface.
> > + * @type: Type of DFL FIU of the device. See enum dfl_id_type.
> > + * @feature_id: 64 bits feature identifier local to its DFL FIU type.
> > + * @mmio_res: MMIO resource of this dfl device.
> > + * @irqs: List of Linux IRQ numbers of this dfl device.
> > + * @num_irqs: number of IRQs supported by this dfl device.
> > + * @cdev: pointer to DFL FPGA container device this dfl device belongs to.
> > + * @id_entry: matched id entry in dfl driver's id table.
> > + */
> > +struct dfl_device {
> > +struct device dev;
> > +unsigned int type;
> > +unsigned long long feature_id;
> > +struct resource mmio_res;
> > +int *irqs;
> > +unsigned int num_irqs;
> > +struct dfl_fpga_cdev *cdev;
> > +const struct dfl_device_id *id_entry;
> > +};
> > +
> > +/**
> > + * struct dfl_driver - represent an dfl device driver
> > + *
> > + * @drv: Driver model structure.
> > + * @id_table: Pointer to table of device IDs the driver is interested in.
> > + * @probe: Callback for device binding.
> > + * @remove: Callback for device unbinding.
> > + */
> > +struct dfl_driver {
> > +struct device_driver drv;
> > +const struct dfl_device_id *id_table;
> > +
> > +int (*probe)(struct dfl_device *dfl_dev);
> > +int (*remove)(struct dfl_device *dfl_dev);
> > +};
> > +
> > +#define to_dfl_dev(d) container_of(d, struct dfl_device, dev)
> > +#define to_dfl_drv(d) container_of(d, struct dfl_driver, drv)
> > +
> > +/*
> > + * use a macro to avoid include chaining to get THIS_MODULE
> > + */
> > +#define dfl_driver_register(drv) \
> > +__dfl_driver_register(drv, THIS_MODULE)
> > +int __dfl_driver_register(struct dfl_driver *dfl_drv, struct module *owner);
> > +void dfl_driver_unregister(struct dfl_driver *dfl_drv);
> > +
> > +/* module_dfl_driver() - Helper macro for drivers that don't do
> 
> /*
>  * module_dfl_driver()

Yes

> 
> > + * anything special in module init/exit.  This eliminates a lot of
> > + * boilerplate.  Each module may only use this macro once, and
> > + * calling it replaces module_init() and module_exit()
> > + */
> > +#define module_dfl_driver(__dfl_driver) \
> > +module_driver(__dfl_driver, dfl_driver_register, \
> > +      dfl_driver_unregister)
> > +
> >  #endif /* __FPGA_DFL_H */
> 
> BTW: maybe it's better to have one patch to add a driver using this bus as an example?

Yes I can also sent a dfl_n3000_nios driver in next version

Thanks,
Yilun

> 
> Thanks
> Hao
> 
> > --
> > 2.7.4

RE: [PATCH 2/2] fpga: dfl: create a dfl bus type to support DFL devices

[Wu, Hao]

> > > +}
> > > +
> > > +dfl_dev->type = feature_dev_id_type(pdev);
> > > +dfl_dev->feature_id = (unsigned long long)feature->id;
> > > +
> > > +dfl_dev->dev.parent  = &pdev->dev;
> > > +dfl_dev->dev.bus     = &dfl_bus_type;
> > > +dfl_dev->dev.release = release_dfl_dev;
> > > +dev_set_name(&dfl_dev->dev, "%s.%d", dev_name(&pdev->dev),
> > > +     feature->index);
> >
> > Or it's better to have a generic name for the device on the bus.
> 
> mm.. It is good suggestion, we should have a unified name for dfl
> devices.
> 
> How about ("dfl.%d.%d", pdev->id, feature->index)

It's quite difficult for people to use related information from these magic 
numbers. They are not ids defined in the spec, so just dfl_dev.x with one
unique id seems to be better. If you really need to expose some id
information, maybe you can consider adding some standard sysfs entry
to all dfl_dev, I think that will be easier for users. How do you think?

Thanks
Hao

Re: [PATCH 2/2] fpga: dfl: create a dfl bus type to support DFL devices

[Xu Yilun]

On Tue, Jul 21, 2020 at 07:41:27PM +0800, Wu, Hao wrote:
> > > > +}
> > > > +
> > > > +dfl_dev->type = feature_dev_id_type(pdev);
> > > > +dfl_dev->feature_id = (unsigned long long)feature->id;
> > > > +
> > > > +dfl_dev->dev.parent  = &pdev->dev;
> > > > +dfl_dev->dev.bus     = &dfl_bus_type;
> > > > +dfl_dev->dev.release = release_dfl_dev;
> > > > +dev_set_name(&dfl_dev->dev, "%s.%d", dev_name(&pdev->dev),
> > > > +     feature->index);
> > >
> > > Or it's better to have a generic name for the device on the bus.
> >
> > mm.. It is good suggestion, we should have a unified name for dfl
> > devices.
> >
> > How about ("dfl.%d.%d", pdev->id, feature->index)
> 
> It's quite difficult for people to use related information from these magic
> numbers. They are not ids defined in the spec, so just dfl_dev.x with one
> unique id seems to be better. If you really need to expose some id
> information, maybe you can consider adding some standard sysfs entry
> to all dfl_dev, I think that will be easier for users. How do you think?

I'm fine with the dfl_dev.x solution.

> 
> Thanks
> Hao

Re: [PATCH 2/2] fpga: dfl: create a dfl bus type to support DFL devices

[Tom Rix]

More small stuff.

Refactoring for feature_id conflict covered in other email.

Tom


On 7/14/20 10:38 PM, Xu Yilun wrote:
> A new bus type "dfl" is introduced for private features which are not
> initialized by DFL feature drivers (dfl-fme & dfl-afu drivers). So these
> private features could be handled by separate driver modules.
>
> DFL framework will create DFL devices on enumeration. DFL drivers could
> be registered on this bus to match these DFL devices. They are matched by
> dfl type & feature_id.
>
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-dfl |  15 ++
>  drivers/fpga/dfl.c                      | 248 ++++++++++++++++++++++++++++++--
>  drivers/fpga/dfl.h                      |  85 +++++++++++
>  3 files changed, 340 insertions(+), 8 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-dfl b/Documentation/ABI/testing/sysfs-bus-dfl
> new file mode 100644
> index 0000000..cd00abc
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-dfl
> @@ -0,0 +1,15 @@
> +What:		/sys/bus/dfl/devices/.../type
> +Date:		March 2020
> +KernelVersion:	5.7
5.8
> +Contact:	Xu Yilun <yilun.xu@intel.com>
> +Description:	Read-only. It returns type of DFL FIU of the device. Now DFL
> +		supports 2 FIU types, 0 for FME, 1 for PORT.
> +		Format: 0x%x
> +
> +What:		/sys/bus/dfl/devices/.../feature_id
> +Date:		March 2020
> +KernelVersion:	5.7
> +Contact:	Xu Yilun <yilun.xu@intel.com>
> +Description:	Read-only. It returns feature identifier local to its DFL FIU
> +		type.
> +		Format: 0x%llx
> diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> index 7dc6411..93f9d6d 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -30,12 +30,6 @@ static DEFINE_MUTEX(dfl_id_mutex);
>   * index to dfl_chardevs table. If no chardev support just set devt_type
>   * as one invalid index (DFL_FPGA_DEVT_MAX).
>   */
> -enum dfl_id_type {
> -	FME_ID,		/* fme id allocation and mapping */
> -	PORT_ID,	/* port id allocation and mapping */
> -	DFL_ID_MAX,
> -};
> -
>  enum dfl_fpga_devt_type {
>  	DFL_FPGA_DEVT_FME,
>  	DFL_FPGA_DEVT_PORT,
> @@ -255,6 +249,228 @@ static bool is_header_feature(struct dfl_feature *feature)
>  	return feature->id == FEATURE_ID_FIU_HEADER;
>  }
>  
> +static const struct dfl_device_id *
> +dfl_match_one_device(const struct dfl_device_id *id,
> +		     struct dfl_device *dfl_dev)
> +{
> +	if (id->type == dfl_dev->type &&
> +	    id->feature_id == dfl_dev->feature_id)
> +		return id;
> +
> +	return NULL;
> +}
> +
> +static int dfl_bus_match(struct device *dev, struct device_driver *drv)
> +{
> +	struct dfl_device *dfl_dev = to_dfl_dev(dev);
> +	struct dfl_driver *dfl_drv = to_dfl_drv(drv);
> +	const struct dfl_device_id *id_entry = dfl_drv->id_table;
Null check ?
> +
> +	while (id_entry->feature_id) {
Null check or document table has a sentinel.
> +		if (dfl_match_one_device(id_entry, dfl_dev)) {
> +			dfl_dev->id_entry = id_entry;
> +			return 1;
> +		}
> +		id_entry++;
> +	}
> +
> +	return 0;
> +}
> +
> +static int dfl_bus_probe(struct device *dev)
> +{
> +	struct dfl_device *dfl_dev = to_dfl_dev(dev);
> +	struct dfl_driver *dfl_drv = to_dfl_drv(dev->driver);
> +
> +	return dfl_drv->probe(dfl_dev);
> +}
> +
> +static int dfl_bus_remove(struct device *dev)
> +{
> +	struct dfl_device *dfl_dev = to_dfl_dev(dev);
> +	struct dfl_driver *dfl_drv = to_dfl_drv(dev->driver);
> +
> +	if (dfl_drv->remove)
> +		dfl_drv->remove(dfl_dev);
return dfl_drv->remove()
> +
> +	return 0;
> +}
> +
> +static int dfl_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
> +{
> +	struct dfl_device *dfl_dev = to_dfl_dev(dev);
> +
> +	if (add_uevent_var(env, "MODALIAS=dfl:%08x:%016llx",
> +			   dfl_dev->type, dfl_dev->feature_id))
> +		return -ENOMEM;

can simplify, change to

return add_uevent_var(...)

> +
> +	return 0;
> +}
> +
> +/* show dfl info fields */
> +#define dfl_info_attr(field, format_string)				\
> +static ssize_t								\
> +field##_show(struct device *dev, struct device_attribute *attr,		\
> +	     char *buf)							\
> +{									\
> +	struct dfl_device *dfl_dev = to_dfl_dev(dev);			\
> +									\
> +	return sprintf(buf, format_string, dfl_dev->field);		\
> +}									\
> +static DEVICE_ATTR_RO(field)
> +
> +dfl_info_attr(type, "0x%x\n");
> +dfl_info_attr(feature_id, "0x%llx\n");
> +
> +static struct attribute *dfl_dev_attrs[] = {
> +	&dev_attr_type.attr,
> +	&dev_attr_feature_id.attr,
> +	NULL,
> +};
> +
> +ATTRIBUTE_GROUPS(dfl_dev);
> +
> +static struct bus_type dfl_bus_type = {
> +	.name		= "dfl",
> +	.match		= dfl_bus_match,
> +	.probe		= dfl_bus_probe,
> +	.remove		= dfl_bus_remove,
> +	.uevent		= dfl_bus_uevent,
> +	.dev_groups	= dfl_dev_groups,
> +};
> +
> +static void release_dfl_dev(struct device *dev)
> +{
> +	struct dfl_device *dfl_dev = to_dfl_dev(dev);
> +
> +	release_resource(&dfl_dev->mmio_res);
Where is request_resource, shouldn't it be in dfl_dev_add ?
> +	kfree(dfl_dev->irqs);
> +	kfree(dfl_dev);
> +}
> +
> +static struct dfl_device *
> +dfl_dev_add(struct dfl_feature_platform_data *pdata,
> +	    struct dfl_feature *feature)
> +{
> +	struct platform_device *pdev = pdata->dev;
> +	struct dfl_device *dfl_dev;
> +	int i, ret;
> +
> +	dfl_dev = kzalloc(sizeof(*dfl_dev), GFP_KERNEL);
> +	if (!dfl_dev)
> +		return ERR_PTR(-ENOMEM);
> +
> +	dfl_dev->cdev = pdata->dfl_cdev;
> +
> +	dfl_dev->mmio_res.parent = &pdev->resource[feature->resource_index];
> +	dfl_dev->mmio_res.flags = IORESOURCE_MEM;
> +	dfl_dev->mmio_res.start =
> +		pdev->resource[feature->resource_index].start;
> +	dfl_dev->mmio_res.end = pdev->resource[feature->resource_index].end;
> +
> +	/* then add irq resource */
> +	if (feature->nr_irqs) {
> +		dfl_dev->irqs = kcalloc(feature->nr_irqs,
> +					sizeof(*dfl_dev->irqs), GFP_KERNEL);
> +		if (!dfl_dev->irqs) {
> +			ret = -ENOMEM;
> +			goto free_dfl_dev;
> +		}
> +
> +		for (i = 0; i < feature->nr_irqs; i++)
> +			dfl_dev->irqs[i] = feature->irq_ctx[i].irq;
> +
> +		dfl_dev->num_irqs = feature->nr_irqs;
> +	}
> +
> +	dfl_dev->type = feature_dev_id_type(pdev);
> +	dfl_dev->feature_id = (unsigned long long)feature->id;
> +
> +	dfl_dev->dev.parent  = &pdev->dev;
> +	dfl_dev->dev.bus     = &dfl_bus_type;
> +	dfl_dev->dev.release = release_dfl_dev;
> +	dev_set_name(&dfl_dev->dev, "%s.%d", dev_name(&pdev->dev),
> +		     feature->index);
this can fail
> +
> +	dfl_dev->mmio_res.name = dev_name(&dfl_dev->dev);
> +	ret = insert_resource(dfl_dev->mmio_res.parent, &dfl_dev->mmio_res);
> +	if (ret) {
> +		dev_err(&pdev->dev, "%s failed to claim resource: %pR\n",
> +			dev_name(&dfl_dev->dev), &dfl_dev->mmio_res);
> +		goto free_irqs;
> +	}
> +
> +	ret = device_register(&dfl_dev->dev);
> +	if (ret) {
> +		put_device(&dfl_dev->dev);
> +		return ERR_PTR(ret);
> +	}
> +
> +	dev_info(&pdev->dev, "add dfl_dev: %s\n",
> +		 dev_name(&dfl_dev->dev));
> +	return dfl_dev;
> +
> +free_irqs:
> +	kfree(dfl_dev->irqs);
> +free_dfl_dev:
> +	kfree(dfl_dev);
> +	return ERR_PTR(ret);
> +}
> +
> +static void dfl_devs_uinit(struct dfl_feature_platform_data *pdata)
> +{
> +	struct dfl_device *dfl_dev;
> +	struct dfl_feature *feature;
> +
> +	dfl_fpga_dev_for_each_feature(pdata, feature) {
> +		if (!feature->ioaddr && feature->priv) {
> +			dfl_dev = feature->priv;
> +			device_unregister(&dfl_dev->dev);
> +			feature->priv = NULL;
> +		}
> +	}
> +}
> +
> +static int dfl_devs_init(struct platform_device *pdev)
> +{
> +	struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> +	struct dfl_feature *feature;
> +	struct dfl_device *dfl_dev;
> +
> +	dfl_fpga_dev_for_each_feature(pdata, feature) {
> +		if (feature->ioaddr || feature->priv)
> +			continue;
> +
> +		dfl_dev = dfl_dev_add(pdata, feature);
> +		if (IS_ERR(dfl_dev)) {
> +			dfl_devs_uinit(pdata);
> +			return PTR_ERR(dfl_dev);
What happens to dfl_dev's that were successful. Need a clean up ?
> +		}
> +
> +		feature->priv = dfl_dev;
> +	}
> +
> +	return 0;
> +}
> +
> +int __dfl_driver_register(struct dfl_driver *dfl_drv, struct module *owner)
> +{
> +	if (!dfl_drv || !dfl_drv->probe || !dfl_drv->id_table)
> +		return -EINVAL;
> +
> +	dfl_drv->drv.owner = owner;
> +	dfl_drv->drv.bus = &dfl_bus_type;
> +
> +	return driver_register(&dfl_drv->drv);
> +}
> +EXPORT_SYMBOL(__dfl_driver_register);
> +
> +void dfl_driver_unregister(struct dfl_driver *dfl_drv)
> +{
> +	driver_unregister(&dfl_drv->drv);
> +}
> +EXPORT_SYMBOL(dfl_driver_unregister);
> +
>  /**
>   * dfl_fpga_dev_feature_uinit - uinit for sub features of dfl feature device
>   * @pdev: feature device.
> @@ -264,12 +480,15 @@ void dfl_fpga_dev_feature_uinit(struct platform_device *pdev)
>  	struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
>  	struct dfl_feature *feature;
>  
> -	dfl_fpga_dev_for_each_feature(pdata, feature)
> +	dfl_devs_uinit(pdata);
> +
> +	dfl_fpga_dev_for_each_feature(pdata, feature) {
>  		if (feature->ops) {
>  			if (feature->ops->uinit)
>  				feature->ops->uinit(pdev, feature);
>  			feature->ops = NULL;
>  		}
> +	}
>  }
>  EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_uinit);
>  
> @@ -348,6 +567,10 @@ int dfl_fpga_dev_feature_init(struct platform_device *pdev,
>  		drv++;
>  	}
>  
> +	ret = dfl_devs_init(pdev);
> +	if (ret)
> +		goto exit;
> +
>  	return 0;
>  exit:
>  	dfl_fpga_dev_feature_uinit(pdev);
> @@ -553,6 +776,8 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
>  		struct dfl_feature_irq_ctx *ctx;
>  		unsigned int i;
>  
> +		feature->index = index;
> +
>  		/* save resource information for each feature */
>  		feature->dev = fdev;
>  		feature->id = finfo->fid;
> @@ -1295,11 +1520,17 @@ static int __init dfl_fpga_init(void)
>  {
>  	int ret;
>  
> +	ret = bus_register(&dfl_bus_type);
> +	if (ret)
> +		return ret;
> +
>  	dfl_ids_init();
>  
>  	ret = dfl_chardev_init();
> -	if (ret)
> +	if (ret) {
>  		dfl_ids_destroy();
> +		bus_unregister(&dfl_bus_type);
> +	}
>  
>  	return ret;
>  }
> @@ -1637,6 +1868,7 @@ static void __exit dfl_fpga_exit(void)
>  {
>  	dfl_chardev_uinit();
>  	dfl_ids_destroy();
> +	bus_unregister(&dfl_bus_type);
>  }
>  
>  module_init(dfl_fpga_init);
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index f605c28..d00aa1c 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -229,6 +229,10 @@ struct dfl_feature_irq_ctx {
>   *
>   * @dev: ptr to pdev of the feature device which has the sub feature.
>   * @id: sub feature id.
> + * @index: unique identifier for an sub feature within the feature device.
> + *	   It is possible that multiply sub features with same feature id are
> + *	   listed in one feature device. So an incremental index (start from 0)
> + *	   is needed to identify each sub feature.
>   * @resource_index: each sub feature has one mmio resource for its registers.
>   *		    this index is used to find its mmio resource from the
>   *		    feature dev (platform device)'s reources.
> @@ -241,6 +245,7 @@ struct dfl_feature_irq_ctx {
>  struct dfl_feature {
>  	struct platform_device *dev;
>  	u64 id;
> +	int index;
>  	int resource_index;
>  	void __iomem *ioaddr;
>  	struct dfl_feature_irq_ctx *irq_ctx;
> @@ -515,4 +520,84 @@ long dfl_feature_ioctl_set_irq(struct platform_device *pdev,
>  			       struct dfl_feature *feature,
>  			       unsigned long arg);
>  
> +/**
> + * enum dfl_id_type - define the DFL FIU types
> + */
> +enum dfl_id_type {
> +	FME_ID,
> +	PORT_ID,
> +	DFL_ID_MAX,
> +};
> +
> +/**
> + * struct dfl_device_id -  dfl device identifier
> + * @type: Type of DFL FIU of the device. See enum dfl_id_type.
> + * @feature_id: 64 bits feature identifier local to its DFL FIU type.
> + * @driver_data: Driver specific data
> + */
> +struct dfl_device_id {
> +	unsigned int type;
> +	unsigned long long feature_id;
> +	unsigned long driver_data;
> +};
> +
> +/**
> + * struct dfl_device - represent an dfl device on dfl bus
> + *
> + * @dev: Generic device interface.
> + * @type: Type of DFL FIU of the device. See enum dfl_id_type.
> + * @feature_id: 64 bits feature identifier local to its DFL FIU type.
> + * @mmio_res: MMIO resource of this dfl device.
> + * @irqs: List of Linux IRQ numbers of this dfl device.
> + * @num_irqs: number of IRQs supported by this dfl device.
> + * @cdev: pointer to DFL FPGA container device this dfl device belongs to.
> + * @id_entry: matched id entry in dfl driver's id table.
> + */
> +struct dfl_device {
> +	struct device dev;
> +	unsigned int type;
> +	unsigned long long feature_id;
> +	struct resource mmio_res;
> +	int *irqs;
> +	unsigned int num_irqs;
> +	struct dfl_fpga_cdev *cdev;
> +	const struct dfl_device_id *id_entry;
> +};
> +
> +/**
> + * struct dfl_driver - represent an dfl device driver
> + *
> + * @drv: Driver model structure.
> + * @id_table: Pointer to table of device IDs the driver is interested in.
> + * @probe: Callback for device binding.
> + * @remove: Callback for device unbinding.
> + */
> +struct dfl_driver {
> +	struct device_driver drv;
> +	const struct dfl_device_id *id_table;
> +
> +	int (*probe)(struct dfl_device *dfl_dev);
> +	int (*remove)(struct dfl_device *dfl_dev);
> +};
> +
> +#define to_dfl_dev(d) container_of(d, struct dfl_device, dev)
> +#define to_dfl_drv(d) container_of(d, struct dfl_driver, drv)
> +
> +/*
> + * use a macro to avoid include chaining to get THIS_MODULE
> + */
> +#define dfl_driver_register(drv) \
> +	__dfl_driver_register(drv, THIS_MODULE)
> +int __dfl_driver_register(struct dfl_driver *dfl_drv, struct module *owner);
> +void dfl_driver_unregister(struct dfl_driver *dfl_drv);
> +
> +/* module_dfl_driver() - Helper macro for drivers that don't do
> + * anything special in module init/exit.  This eliminates a lot of
> + * boilerplate.  Each module may only use this macro once, and
> + * calling it replaces module_init() and module_exit()
> + */
> +#define module_dfl_driver(__dfl_driver) \
> +	module_driver(__dfl_driver, dfl_driver_register, \
> +		      dfl_driver_unregister)
> +
>  #endif /* __FPGA_DFL_H */

Re: [PATCH 2/2] fpga: dfl: create a dfl bus type to support DFL devices

[Xu Yilun]

On Fri, Jul 17, 2020 at 12:47:14PM -0700, Tom Rix wrote:
> More small stuff.
> 
> Refactoring for feature_id conflict covered in other email.
> 
> Tom
> 
> 
> On 7/14/20 10:38 PM, Xu Yilun wrote:
> > A new bus type "dfl" is introduced for private features which are not
> > initialized by DFL feature drivers (dfl-fme & dfl-afu drivers). So these
> > private features could be handled by separate driver modules.
> >
> > DFL framework will create DFL devices on enumeration. DFL drivers could
> > be registered on this bus to match these DFL devices. They are matched by
> > dfl type & feature_id.
> >
> > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > Signed-off-by: Wu Hao <hao.wu@intel.com>
> > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com>
> > Signed-off-by: Russ Weight <russell.h.weight@intel.com>
> > ---
> >  Documentation/ABI/testing/sysfs-bus-dfl |  15 ++
> >  drivers/fpga/dfl.c                      | 248 ++++++++++++++++++++++++++++++--
> >  drivers/fpga/dfl.h                      |  85 +++++++++++
> >  3 files changed, 340 insertions(+), 8 deletions(-)
> >  create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl
> >
> > diff --git a/Documentation/ABI/testing/sysfs-bus-dfl b/Documentation/ABI/testing/sysfs-bus-dfl
> > new file mode 100644
> > index 0000000..cd00abc
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-bus-dfl
> > @@ -0,0 +1,15 @@
> > +What:		/sys/bus/dfl/devices/.../type
> > +Date:		March 2020
> > +KernelVersion:	5.7
> 5.8

Yes, think it should be 5.9 for now.

> > +Contact:	Xu Yilun <yilun.xu@intel.com>
> > +Description:	Read-only. It returns type of DFL FIU of the device. Now DFL
> > +		supports 2 FIU types, 0 for FME, 1 for PORT.
> > +		Format: 0x%x
> > +
> > +What:		/sys/bus/dfl/devices/.../feature_id
> > +Date:		March 2020
> > +KernelVersion:	5.7
> > +Contact:	Xu Yilun <yilun.xu@intel.com>
> > +Description:	Read-only. It returns feature identifier local to its DFL FIU
> > +		type.
> > +		Format: 0x%llx
> > diff --git a/drivers/fpga/dfl.c b/drivers/fpga/dfl.c
> > index 7dc6411..93f9d6d 100644
> > --- a/drivers/fpga/dfl.c
> > +++ b/drivers/fpga/dfl.c
> > @@ -30,12 +30,6 @@ static DEFINE_MUTEX(dfl_id_mutex);
> >   * index to dfl_chardevs table. If no chardev support just set devt_type
> >   * as one invalid index (DFL_FPGA_DEVT_MAX).
> >   */
> > -enum dfl_id_type {
> > -	FME_ID,		/* fme id allocation and mapping */
> > -	PORT_ID,	/* port id allocation and mapping */
> > -	DFL_ID_MAX,
> > -};
> > -
> >  enum dfl_fpga_devt_type {
> >  	DFL_FPGA_DEVT_FME,
> >  	DFL_FPGA_DEVT_PORT,
> > @@ -255,6 +249,228 @@ static bool is_header_feature(struct dfl_feature *feature)
> >  	return feature->id == FEATURE_ID_FIU_HEADER;
> >  }
> >  
> > +static const struct dfl_device_id *
> > +dfl_match_one_device(const struct dfl_device_id *id,
> > +		     struct dfl_device *dfl_dev)
> > +{
> > +	if (id->type == dfl_dev->type &&
> > +	    id->feature_id == dfl_dev->feature_id)
> > +		return id;
> > +
> > +	return NULL;
> > +}
> > +
> > +static int dfl_bus_match(struct device *dev, struct device_driver *drv)
> > +{
> > +	struct dfl_device *dfl_dev = to_dfl_dev(dev);
> > +	struct dfl_driver *dfl_drv = to_dfl_drv(drv);
> > +	const struct dfl_device_id *id_entry = dfl_drv->id_table;
> Null check ?

Yes. Thanks for catching this.

> > +
> > +	while (id_entry->feature_id) {
> Null check or document table has a sentinel.

I'll document that table needs a sentinel.

> > +		if (dfl_match_one_device(id_entry, dfl_dev)) {
> > +			dfl_dev->id_entry = id_entry;
> > +			return 1;
> > +		}
> > +		id_entry++;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int dfl_bus_probe(struct device *dev)
> > +{
> > +	struct dfl_device *dfl_dev = to_dfl_dev(dev);
> > +	struct dfl_driver *dfl_drv = to_dfl_drv(dev->driver);
> > +
> > +	return dfl_drv->probe(dfl_dev);
> > +}
> > +
> > +static int dfl_bus_remove(struct device *dev)
> > +{
> > +	struct dfl_device *dfl_dev = to_dfl_dev(dev);
> > +	struct dfl_driver *dfl_drv = to_dfl_drv(dev->driver);
> > +
> > +	if (dfl_drv->remove)
> > +		dfl_drv->remove(dfl_dev);
> return dfl_drv->remove()

I think we could define  void (*remove)(struct dfl_device *dfl_dev) for
dfl_driver.remove

> > +
> > +	return 0;
> > +}
> > +
> > +static int dfl_bus_uevent(struct device *dev, struct kobj_uevent_env *env)
> > +{
> > +	struct dfl_device *dfl_dev = to_dfl_dev(dev);
> > +
> > +	if (add_uevent_var(env, "MODALIAS=dfl:%08x:%016llx",
> > +			   dfl_dev->type, dfl_dev->feature_id))
> > +		return -ENOMEM;
> 
> can simplify, change to
> 
> return add_uevent_var(...)

Yes

> 
> > +
> > +	return 0;
> > +}
> > +
> > +/* show dfl info fields */
> > +#define dfl_info_attr(field, format_string)				\
> > +static ssize_t								\
> > +field##_show(struct device *dev, struct device_attribute *attr,		\
> > +	     char *buf)							\
> > +{									\
> > +	struct dfl_device *dfl_dev = to_dfl_dev(dev);			\
> > +									\
> > +	return sprintf(buf, format_string, dfl_dev->field);		\
> > +}									\
> > +static DEVICE_ATTR_RO(field)
> > +
> > +dfl_info_attr(type, "0x%x\n");
> > +dfl_info_attr(feature_id, "0x%llx\n");
> > +
> > +static struct attribute *dfl_dev_attrs[] = {
> > +	&dev_attr_type.attr,
> > +	&dev_attr_feature_id.attr,
> > +	NULL,
> > +};
> > +
> > +ATTRIBUTE_GROUPS(dfl_dev);
> > +
> > +static struct bus_type dfl_bus_type = {
> > +	.name		= "dfl",
> > +	.match		= dfl_bus_match,
> > +	.probe		= dfl_bus_probe,
> > +	.remove		= dfl_bus_remove,
> > +	.uevent		= dfl_bus_uevent,
> > +	.dev_groups	= dfl_dev_groups,
> > +};
> > +
> > +static void release_dfl_dev(struct device *dev)
> > +{
> > +	struct dfl_device *dfl_dev = to_dfl_dev(dev);
> > +
> > +	release_resource(&dfl_dev->mmio_res);
> Where is request_resource, shouldn't it be in dfl_dev_add ?

insert_resource() is used in dfl_dev_add

> > +	kfree(dfl_dev->irqs);
> > +	kfree(dfl_dev);
> > +}
> > +
> > +static struct dfl_device *
> > +dfl_dev_add(struct dfl_feature_platform_data *pdata,
> > +	    struct dfl_feature *feature)
> > +{
> > +	struct platform_device *pdev = pdata->dev;
> > +	struct dfl_device *dfl_dev;
> > +	int i, ret;
> > +
> > +	dfl_dev = kzalloc(sizeof(*dfl_dev), GFP_KERNEL);
> > +	if (!dfl_dev)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	dfl_dev->cdev = pdata->dfl_cdev;
> > +
> > +	dfl_dev->mmio_res.parent = &pdev->resource[feature->resource_index];
> > +	dfl_dev->mmio_res.flags = IORESOURCE_MEM;
> > +	dfl_dev->mmio_res.start =
> > +		pdev->resource[feature->resource_index].start;
> > +	dfl_dev->mmio_res.end = pdev->resource[feature->resource_index].end;
> > +
> > +	/* then add irq resource */
> > +	if (feature->nr_irqs) {
> > +		dfl_dev->irqs = kcalloc(feature->nr_irqs,
> > +					sizeof(*dfl_dev->irqs), GFP_KERNEL);
> > +		if (!dfl_dev->irqs) {
> > +			ret = -ENOMEM;
> > +			goto free_dfl_dev;
> > +		}
> > +
> > +		for (i = 0; i < feature->nr_irqs; i++)
> > +			dfl_dev->irqs[i] = feature->irq_ctx[i].irq;
> > +
> > +		dfl_dev->num_irqs = feature->nr_irqs;
> > +	}
> > +
> > +	dfl_dev->type = feature_dev_id_type(pdev);
> > +	dfl_dev->feature_id = (unsigned long long)feature->id;
> > +
> > +	dfl_dev->dev.parent  = &pdev->dev;
> > +	dfl_dev->dev.bus     = &dfl_bus_type;
> > +	dfl_dev->dev.release = release_dfl_dev;
> > +	dev_set_name(&dfl_dev->dev, "%s.%d", dev_name(&pdev->dev),
> > +		     feature->index);
> this can fail

Yes, I could add the check.

> > +
> > +	dfl_dev->mmio_res.name = dev_name(&dfl_dev->dev);
> > +	ret = insert_resource(dfl_dev->mmio_res.parent, &dfl_dev->mmio_res);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "%s failed to claim resource: %pR\n",
> > +			dev_name(&dfl_dev->dev), &dfl_dev->mmio_res);
> > +		goto free_irqs;
> > +	}
> > +
> > +	ret = device_register(&dfl_dev->dev);
> > +	if (ret) {
> > +		put_device(&dfl_dev->dev);
> > +		return ERR_PTR(ret);
> > +	}
> > +
> > +	dev_info(&pdev->dev, "add dfl_dev: %s\n",
> > +		 dev_name(&dfl_dev->dev));
> > +	return dfl_dev;
> > +
> > +free_irqs:
> > +	kfree(dfl_dev->irqs);
> > +free_dfl_dev:
> > +	kfree(dfl_dev);
> > +	return ERR_PTR(ret);
> > +}
> > +
> > +static void dfl_devs_uinit(struct dfl_feature_platform_data *pdata)
> > +{
> > +	struct dfl_device *dfl_dev;
> > +	struct dfl_feature *feature;
> > +
> > +	dfl_fpga_dev_for_each_feature(pdata, feature) {
> > +		if (!feature->ioaddr && feature->priv) {
> > +			dfl_dev = feature->priv;
> > +			device_unregister(&dfl_dev->dev);
> > +			feature->priv = NULL;
> > +		}
> > +	}
> > +}
> > +
> > +static int dfl_devs_init(struct platform_device *pdev)
> > +{
> > +	struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> > +	struct dfl_feature *feature;
> > +	struct dfl_device *dfl_dev;
> > +
> > +	dfl_fpga_dev_for_each_feature(pdata, feature) {
> > +		if (feature->ioaddr || feature->priv)
> > +			continue;
> > +
> > +		dfl_dev = dfl_dev_add(pdata, feature);
> > +		if (IS_ERR(dfl_dev)) {
> > +			dfl_devs_uinit(pdata);
> > +			return PTR_ERR(dfl_dev);
> What happens to dfl_dev's that were successful. Need a clean up ?
> > +		}
> > +
> > +		feature->priv = dfl_dev;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +int __dfl_driver_register(struct dfl_driver *dfl_drv, struct module *owner)
> > +{
> > +	if (!dfl_drv || !dfl_drv->probe || !dfl_drv->id_table)
> > +		return -EINVAL;
> > +
> > +	dfl_drv->drv.owner = owner;
> > +	dfl_drv->drv.bus = &dfl_bus_type;
> > +
> > +	return driver_register(&dfl_drv->drv);
> > +}
> > +EXPORT_SYMBOL(__dfl_driver_register);
> > +
> > +void dfl_driver_unregister(struct dfl_driver *dfl_drv)
> > +{
> > +	driver_unregister(&dfl_drv->drv);
> > +}
> > +EXPORT_SYMBOL(dfl_driver_unregister);
> > +
> >  /**
> >   * dfl_fpga_dev_feature_uinit - uinit for sub features of dfl feature device
> >   * @pdev: feature device.
> > @@ -264,12 +480,15 @@ void dfl_fpga_dev_feature_uinit(struct platform_device *pdev)
> >  	struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> >  	struct dfl_feature *feature;
> >  
> > -	dfl_fpga_dev_for_each_feature(pdata, feature)
> > +	dfl_devs_uinit(pdata);
> > +
> > +	dfl_fpga_dev_for_each_feature(pdata, feature) {
> >  		if (feature->ops) {
> >  			if (feature->ops->uinit)
> >  				feature->ops->uinit(pdev, feature);
> >  			feature->ops = NULL;
> >  		}
> > +	}
> >  }
> >  EXPORT_SYMBOL_GPL(dfl_fpga_dev_feature_uinit);
> >  
> > @@ -348,6 +567,10 @@ int dfl_fpga_dev_feature_init(struct platform_device *pdev,
> >  		drv++;
> >  	}
> >  
> > +	ret = dfl_devs_init(pdev);
> > +	if (ret)
> > +		goto exit;
> > +
> >  	return 0;
> >  exit:
> >  	dfl_fpga_dev_feature_uinit(pdev);
> > @@ -553,6 +776,8 @@ static int build_info_commit_dev(struct build_feature_devs_info *binfo)
> >  		struct dfl_feature_irq_ctx *ctx;
> >  		unsigned int i;
> >  
> > +		feature->index = index;
> > +
> >  		/* save resource information for each feature */
> >  		feature->dev = fdev;
> >  		feature->id = finfo->fid;
> > @@ -1295,11 +1520,17 @@ static int __init dfl_fpga_init(void)
> >  {
> >  	int ret;
> >  
> > +	ret = bus_register(&dfl_bus_type);
> > +	if (ret)
> > +		return ret;
> > +
> >  	dfl_ids_init();
> >  
> >  	ret = dfl_chardev_init();
> > -	if (ret)
> > +	if (ret) {
> >  		dfl_ids_destroy();
> > +		bus_unregister(&dfl_bus_type);
> > +	}
> >  
> >  	return ret;
> >  }
> > @@ -1637,6 +1868,7 @@ static void __exit dfl_fpga_exit(void)
> >  {
> >  	dfl_chardev_uinit();
> >  	dfl_ids_destroy();
> > +	bus_unregister(&dfl_bus_type);
> >  }
> >  
> >  module_init(dfl_fpga_init);
> > diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> > index f605c28..d00aa1c 100644
> > --- a/drivers/fpga/dfl.h
> > +++ b/drivers/fpga/dfl.h
> > @@ -229,6 +229,10 @@ struct dfl_feature_irq_ctx {
> >   *
> >   * @dev: ptr to pdev of the feature device which has the sub feature.
> >   * @id: sub feature id.
> > + * @index: unique identifier for an sub feature within the feature device.
> > + *	   It is possible that multiply sub features with same feature id are
> > + *	   listed in one feature device. So an incremental index (start from 0)
> > + *	   is needed to identify each sub feature.
> >   * @resource_index: each sub feature has one mmio resource for its registers.
> >   *		    this index is used to find its mmio resource from the
> >   *		    feature dev (platform device)'s reources.
> > @@ -241,6 +245,7 @@ struct dfl_feature_irq_ctx {
> >  struct dfl_feature {
> >  	struct platform_device *dev;
> >  	u64 id;
> > +	int index;
> >  	int resource_index;
> >  	void __iomem *ioaddr;
> >  	struct dfl_feature_irq_ctx *irq_ctx;
> > @@ -515,4 +520,84 @@ long dfl_feature_ioctl_set_irq(struct platform_device *pdev,
> >  			       struct dfl_feature *feature,
> >  			       unsigned long arg);
> >  
> > +/**
> > + * enum dfl_id_type - define the DFL FIU types
> > + */
> > +enum dfl_id_type {
> > +	FME_ID,
> > +	PORT_ID,
> > +	DFL_ID_MAX,
> > +};
> > +
> > +/**
> > + * struct dfl_device_id -  dfl device identifier
> > + * @type: Type of DFL FIU of the device. See enum dfl_id_type.
> > + * @feature_id: 64 bits feature identifier local to its DFL FIU type.
> > + * @driver_data: Driver specific data
> > + */
> > +struct dfl_device_id {
> > +	unsigned int type;
> > +	unsigned long long feature_id;
> > +	unsigned long driver_data;
> > +};
> > +
> > +/**
> > + * struct dfl_device - represent an dfl device on dfl bus
> > + *
> > + * @dev: Generic device interface.
> > + * @type: Type of DFL FIU of the device. See enum dfl_id_type.
> > + * @feature_id: 64 bits feature identifier local to its DFL FIU type.
> > + * @mmio_res: MMIO resource of this dfl device.
> > + * @irqs: List of Linux IRQ numbers of this dfl device.
> > + * @num_irqs: number of IRQs supported by this dfl device.
> > + * @cdev: pointer to DFL FPGA container device this dfl device belongs to.
> > + * @id_entry: matched id entry in dfl driver's id table.
> > + */
> > +struct dfl_device {
> > +	struct device dev;
> > +	unsigned int type;
> > +	unsigned long long feature_id;
> > +	struct resource mmio_res;
> > +	int *irqs;
> > +	unsigned int num_irqs;
> > +	struct dfl_fpga_cdev *cdev;
> > +	const struct dfl_device_id *id_entry;
> > +};
> > +
> > +/**
> > + * struct dfl_driver - represent an dfl device driver
> > + *
> > + * @drv: Driver model structure.
> > + * @id_table: Pointer to table of device IDs the driver is interested in.
> > + * @probe: Callback for device binding.
> > + * @remove: Callback for device unbinding.
> > + */
> > +struct dfl_driver {
> > +	struct device_driver drv;
> > +	const struct dfl_device_id *id_table;
> > +
> > +	int (*probe)(struct dfl_device *dfl_dev);
> > +	int (*remove)(struct dfl_device *dfl_dev);
> > +};
> > +
> > +#define to_dfl_dev(d) container_of(d, struct dfl_device, dev)
> > +#define to_dfl_drv(d) container_of(d, struct dfl_driver, drv)
> > +
> > +/*
> > + * use a macro to avoid include chaining to get THIS_MODULE
> > + */
> > +#define dfl_driver_register(drv) \
> > +	__dfl_driver_register(drv, THIS_MODULE)
> > +int __dfl_driver_register(struct dfl_driver *dfl_drv, struct module *owner);
> > +void dfl_driver_unregister(struct dfl_driver *dfl_drv);
> > +
> > +/* module_dfl_driver() - Helper macro for drivers that don't do
> > + * anything special in module init/exit.  This eliminates a lot of
> > + * boilerplate.  Each module may only use this macro once, and
> > + * calling it replaces module_init() and module_exit()
> > + */
> > +#define module_dfl_driver(__dfl_driver) \
> > +	module_driver(__dfl_driver, dfl_driver_register, \
> > +		      dfl_driver_unregister)
> > +
> >  #endif /* __FPGA_DFL_H */

Re: [PATCH 2/2] fpga: dfl: create a dfl bus type to support DFL devices

[Xu Yilun]

> > +static int dfl_devs_init(struct platform_device *pdev)
> > +{
> > +	struct dfl_feature_platform_data *pdata = dev_get_platdata(&pdev->dev);
> > +	struct dfl_feature *feature;
> > +	struct dfl_device *dfl_dev;
> > +
> > +	dfl_fpga_dev_for_each_feature(pdata, feature) {
> > +		if (feature->ioaddr || feature->priv)
> > +			continue;
> > +
> > +		dfl_dev = dfl_dev_add(pdata, feature);
> > +		if (IS_ERR(dfl_dev)) {
> > +			dfl_devs_uinit(pdata);
> > +			return PTR_ERR(dfl_dev);
> What happens to dfl_dev's that were successful. Need a clean up ?

Yes, the already added dfl devices under this pdev will be unregistered
in function dfl_devs_uinit()

> > +		}
> > +
> > +		feature->priv = dfl_dev;
> > +	}
> > +
> > +	return 0;
> > +}

Re: [PATCH 0/2] Modularization of DFL private feature drivers

[Tom Rix]

Generally i think this is a good approach.

However I do have concern.

The feature_id in dfl is magic number, similar to pci id but without a vendor id.

Is it possible to add something like a vendor id so different vendors would not have to be so careful to use a unique id ?

This touches some of the matching function of the bus ops.  Could there be a way for bus ops to be used so that there could be multiple matching function.  Maybe the one in 0002 as a default so users could override it ?

The use case I am worrying about is an OEM.  The OEM uses an unclaimed feature_id and supplies their own platform device device driver to match the feature_id.  But some later version of the kernel claims this feature_id and the OEM's driver no longer works and since the value comes from pci config space it is difficult to change.

So looking for something like

register_feature_matcher((*bus_match)(struct device *dev, struct device_driver *drv))

static int dfl_bus_match_default(struct device *dev, struct device_driver *drv)
{
    struct dfl_device *dfl_dev = to_dfl_dev(dev);
    struct dfl_driver *dfl_drv = to_dfl_drv(drv);
    const struct dfl_device_id *id_entry = dfl_drv->id_table;

    while (id_entry->feature_id) {
        if (dfl_match_one_device(id_entry, dfl_dev)) {
            dfl_dev->id_entry = id_entry;
            return 1;
        }
        id_entry++;
    }

    return 0;
}

register_feature_matcher(&dfl_bus_match_default)

static int dfl_bus_match(struct device *dev, struct device_driver *drv)
{

    struct dfl_device *dfl_dev = to_dfl_dev(dev);
    struct dfl_driver *dfl_drv = to_dfl_drv(drv);
    const struct dfl_device_id *id_entry = dfl_drv->id_table;

    while (id_entry->feature_id) {

        matcher = Loop over matchers()

        if (matcher(dev, drv)) {
            dfl_dev->id_entry = id_entry;
            return 1;
        }
        id_entry++;
    }

    return 0;
}

Or maybe use some of the unused bits in the dfl header to add a second vendor-like id and change existing matcher to look feature_id and vendor_like_id.

Tom

 

On 7/14/20 10:38 PM, Xu Yilun wrote:
> This patchset makes it possible to develop independent driver modules
> for DFL private features. It also helps to leverage existing kernel
> drivers to enable some IP blocks in DFL.
>
> Xu Yilun (2):
>   fpga: dfl: map feature mmio resources in their own feature drivers
>   fpga: dfl: create a dfl bus type to support DFL devices
>
>  Documentation/ABI/testing/sysfs-bus-dfl |  15 ++
>  drivers/fpga/dfl-pci.c                  |  21 +-
>  drivers/fpga/dfl.c                      | 435 +++++++++++++++++++++++++++-----
>  drivers/fpga/dfl.h                      |  91 ++++++-
>  4 files changed, 492 insertions(+), 70 deletions(-)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl
>

RE: [PATCH 0/2] Modularization of DFL private feature drivers

[Wu, Hao]

> Subject: Re: [PATCH 0/2] Modularization of DFL private feature drivers
> 
> Generally i think this is a good approach.
> 
> However I do have concern.
> 
> The feature_id in dfl is magic number, similar to pci id but without a vendor
> id.
> 
> Is it possible to add something like a vendor id so different vendors would
> not have to be so careful to use a unique id ?

Hi Tom,

Thanks for the comments.

Here is only one field defined in spec, that is feature id to distinguish one
feature with another one. There is no vendor id here I think, and several
cards are using this for now and seems not possible to change DFH format
for these products. : (

I fully understand the concern is the feature id management, and potential
conflicts there from different vendors. One possible method to resolve this
is managing the ids in a public place (web? Or just the driver header file for
feature id definition), all vendors can request some feature ids, and other
people can see which feature ids have been used so that they can avoid
using conflict ones with other people.

And in the later version DFH, this feature id will be replaced with GUID
I think, so it can resolve the conflict problems from different vendors?
But now, we still need to handle the existing ones. : )

Thanks
Hao

> 
> This touches some of the matching function of the bus ops.  Could there be a
> way for bus ops to be used so that there could be multiple matching
> function.  Maybe the one in 0002 as a default so users could override it ?
> 
> The use case I am worrying about is an OEM.  The OEM uses an unclaimed
> feature_id and supplies their own platform device device driver to match the
> feature_id.  But some later version of the kernel claims this feature_id and
> the OEM's driver no longer works and since the value comes from pci config
> space it is difficult to change.
> 
> So looking for something like
> 
> register_feature_matcher((*bus_match)(struct device *dev, struct
> device_driver *drv))
> 
> static int dfl_bus_match_default(struct device *dev, struct device_driver *drv)
> {
>     struct dfl_device *dfl_dev = to_dfl_dev(dev);
>     struct dfl_driver *dfl_drv = to_dfl_drv(drv);
>     const struct dfl_device_id *id_entry = dfl_drv->id_table;
> 
>     while (id_entry->feature_id) {
>         if (dfl_match_one_device(id_entry, dfl_dev)) {
>             dfl_dev->id_entry = id_entry;
>             return 1;
>         }
>         id_entry++;
>     }
> 
>     return 0;
> }
> 
> register_feature_matcher(&dfl_bus_match_default)
> 
> static int dfl_bus_match(struct device *dev, struct device_driver *drv)
> {
> 
>     struct dfl_device *dfl_dev = to_dfl_dev(dev);
>     struct dfl_driver *dfl_drv = to_dfl_drv(drv);
>     const struct dfl_device_id *id_entry = dfl_drv->id_table;
> 
>     while (id_entry->feature_id) {
> 
>         matcher = Loop over matchers()
> 
>         if (matcher(dev, drv)) {
>             dfl_dev->id_entry = id_entry;
>             return 1;
>         }
>         id_entry++;
>     }
> 
>     return 0;
> }
> 
> Or maybe use some of the unused bits in the dfl header to add a second
> vendor-like id and change existing matcher to look feature_id and
> vendor_like_id.
> 
> Tom
> 
> 
> 
> On 7/14/20 10:38 PM, Xu Yilun wrote:
> > This patchset makes it possible to develop independent driver modules
> > for DFL private features. It also helps to leverage existing kernel
> > drivers to enable some IP blocks in DFL.
> >
> > Xu Yilun (2):
> >   fpga: dfl: map feature mmio resources in their own feature drivers
> >   fpga: dfl: create a dfl bus type to support DFL devices
> >
> >  Documentation/ABI/testing/sysfs-bus-dfl |  15 ++
> >  drivers/fpga/dfl-pci.c                  |  21 +-
> >  drivers/fpga/dfl.c                      | 435 +++++++++++++++++++++++++++-----
> >  drivers/fpga/dfl.h                      |  91 ++++++-
> >  4 files changed, 492 insertions(+), 70 deletions(-)
> >  create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl
> >

Re: [PATCH 0/2] Modularization of DFL private feature drivers

[Tom Rix]

On 7/16/20 8:48 PM, Wu, Hao wrote:
>> Subject: Re: [PATCH 0/2] Modularization of DFL private feature drivers
>>
>> Generally i think this is a good approach.
>>
>> However I do have concern.
>>
>> The feature_id in dfl is magic number, similar to pci id but without a vendor
>> id.
>>
>> Is it possible to add something like a vendor id so different vendors would
>> not have to be so careful to use a unique id ?
> Hi Tom,
>
> Thanks for the comments.
>
> Here is only one field defined in spec, that is feature id to distinguish one
> feature with another one. There is no vendor id here I think, and several
> cards are using this for now and seems not possible to change DFH format
> for these products. : (

There looks like some unused bits in the first word, how about

#define DFH_EOL            BIT_ULL(40)        /* End of list */

+define DFH_VENDOR    GENMAKE_ULL(49,41) /* Vendor ID */

#define DFH_TYPE        GENMASK_ULL(63, 60)    /* Feature type */

And Intel gets id 0.

>
> I fully understand the concern is the feature id management, and potential
> conflicts there from different vendors. One possible method to resolve this
> is managing the ids in a public place (web? Or just the driver header file for
> feature id definition), all vendors can request some feature ids, and other
> people can see which feature ids have been used so that they can avoid
> using conflict ones with other people.

The conflict will come in development when two vendors use the same unpublished feature id.

Also keeping the truth of id's in the kernel source isn't that great because the public kernel always lags development repositories.

>
> And in the later version DFH, this feature id will be replaced with GUID
> I think, so it can resolve the conflict problems from different vendors?
> But now, we still need to handle the existing ones. : )

This is why I proposed needing to generalize the matching.

>
> Thanks
> Hao
>
>> This touches some of the matching function of the bus ops.  Could there be a
>> way for bus ops to be used so that there could be multiple matching
>> function.  Maybe the one in 0002 as a default so users could override it ?
>>
>> The use case I am worrying about is an OEM.  The OEM uses an unclaimed
>> feature_id and supplies their own platform device device driver to match the
>> feature_id.  But some later version of the kernel claims this feature_id and
>> the OEM's driver no longer works and since the value comes from pci config
>> space it is difficult to change.
>>
>> So looking for something like
>>
>> register_feature_matcher((*bus_match)(struct device *dev, struct
>> device_driver *drv))
>>
>> static int dfl_bus_match_default(struct device *dev, struct device_driver *drv)
>> {
>>     struct dfl_device *dfl_dev = to_dfl_dev(dev);
>>     struct dfl_driver *dfl_drv = to_dfl_drv(drv);
>>     const struct dfl_device_id *id_entry = dfl_drv->id_table;
>>
>>     while (id_entry->feature_id) {
>>         if (dfl_match_one_device(id_entry, dfl_dev)) {
>>             dfl_dev->id_entry = id_entry;
>>             return 1;
>>         }
>>         id_entry++;
>>     }
>>
>>     return 0;
>> }
>>
>> register_feature_matcher(&dfl_bus_match_default)
>>
>> static int dfl_bus_match(struct device *dev, struct device_driver *drv)
>> {
>>
>>     struct dfl_device *dfl_dev = to_dfl_dev(dev);
>>     struct dfl_driver *dfl_drv = to_dfl_drv(drv);
>>     const struct dfl_device_id *id_entry = dfl_drv->id_table;
>>
>>     while (id_entry->feature_id) {
>>
>>         matcher = Loop over matchers()
>>
>>         if (matcher(dev, drv)) {
>>             dfl_dev->id_entry = id_entry;
>>             return 1;
>>         }
>>         id_entry++;
>>     }
>>
>>     return 0;
>> }
>>
>> Or maybe use some of the unused bits in the dfl header to add a second
>> vendor-like id and change existing matcher to look feature_id and
>> vendor_like_id.
>>
>> Tom
>>
>>
>>
>> On 7/14/20 10:38 PM, Xu Yilun wrote:
>>> This patchset makes it possible to develop independent driver modules
>>> for DFL private features. It also helps to leverage existing kernel
>>> drivers to enable some IP blocks in DFL.
>>>
>>> Xu Yilun (2):
>>>   fpga: dfl: map feature mmio resources in their own feature drivers
>>>   fpga: dfl: create a dfl bus type to support DFL devices
>>>
>>>  Documentation/ABI/testing/sysfs-bus-dfl |  15 ++
>>>  drivers/fpga/dfl-pci.c                  |  21 +-
>>>  drivers/fpga/dfl.c                      | 435 +++++++++++++++++++++++++++-----
>>>  drivers/fpga/dfl.h                      |  91 ++++++-
>>>  4 files changed, 492 insertions(+), 70 deletions(-)
>>>  create mode 100644 Documentation/ABI/testing/sysfs-bus-dfl
>>>

RE: [PATCH 0/2] Modularization of DFL private feature drivers

[Wu, Hao]

> On 7/16/20 8:48 PM, Wu, Hao wrote:
> >> Subject: Re: [PATCH 0/2] Modularization of DFL private feature drivers
> >>
> >> Generally i think this is a good approach.
> >>
> >> However I do have concern.
> >>
> >> The feature_id in dfl is magic number, similar to pci id but without a
> vendor
> >> id.
> >>
> >> Is it possible to add something like a vendor id so different vendors would
> >> not have to be so careful to use a unique id ?
> > Hi Tom,
> >
> > Thanks for the comments.
> >
> > Here is only one field defined in spec, that is feature id to distinguish one
> > feature with another one. There is no vendor id here I think, and several
> > cards are using this for now and seems not possible to change DFH format
> > for these products. : (
> 
> There looks like some unused bits in the first word, how about
> 
> #define DFH_EOL            BIT_ULL(40)        /* End of list */
> 
> +define DFH_VENDOR    GENMAKE_ULL(49,41) /* Vendor ID */
> 
> #define DFH_TYPE        GENMASK_ULL(63, 60)    /* Feature type */
> 
> And Intel gets id 0.
> 
> >
> > I fully understand the concern is the feature id management, and potential
> > conflicts there from different vendors. One possible method to resolve this
> > is managing the ids in a public place (web? Or just the driver header file for
> > feature id definition), all vendors can request some feature ids, and other
> > people can see which feature ids have been used so that they can avoid
> > using conflict ones with other people.
> 
> The conflict will come in development when two vendors use the same
> unpublished feature id.
> 
> Also keeping the truth of id's in the kernel source isn't that great because the
> public kernel always lags development repositories.

I fully understand your point, and it's a good idea to me, but I am not sure if 
we can update the spec for DFHv0 at this moment. Let me check with spec
owner about this. Actually I believe we don't want to add anything in driver
code which is not defined in the spec at all. : ( 

> 
> >
> > And in the later version DFH, this feature id will be replaced with GUID
> > I think, so it can resolve the conflict problems from different vendors?
> > But now, we still need to handle the existing ones. : )
> 
> This is why I proposed needing to generalize the matching.

Personally I prefer that we can have standard matching functions per DFH
specs.

Yilun, how do you think about this?

Thanks
Hao

Re: [PATCH 0/2] Modularization of DFL private feature drivers

[Xu Yilun]

On Mon, Jul 20, 2020 at 05:21:43PM +0800, Wu, Hao wrote:
> > On 7/16/20 8:48 PM, Wu, Hao wrote:
> > >> Subject: Re: [PATCH 0/2] Modularization of DFL private feature drivers
> > >>
> > >> Generally i think this is a good approach.
> > >>
> > >> However I do have concern.
> > >>
> > >> The feature_id in dfl is magic number, similar to pci id but without a
> > vendor
> > >> id.
> > >>
> > >> Is it possible to add something like a vendor id so different vendors would
> > >> not have to be so careful to use a unique id ?
> > > Hi Tom,
> > >
> > > Thanks for the comments.
> > >
> > > Here is only one field defined in spec, that is feature id to distinguish one
> > > feature with another one. There is no vendor id here I think, and several
> > > cards are using this for now and seems not possible to change DFH format
> > > for these products. : (
> >
> > There looks like some unused bits in the first word, how about
> >
> > #define DFH_EOL            BIT_ULL(40)        /* End of list */
> >
> > +define DFH_VENDOR    GENMAKE_ULL(49,41) /* Vendor ID */
> >
> > #define DFH_TYPE        GENMASK_ULL(63, 60)    /* Feature type */
> >
> > And Intel gets id 0.
> >
> > >
> > > I fully understand the concern is the feature id management, and potential
> > > conflicts there from different vendors. One possible method to resolve this
> > > is managing the ids in a public place (web? Or just the driver header file for
> > > feature id definition), all vendors can request some feature ids, and other
> > > people can see which feature ids have been used so that they can avoid
> > > using conflict ones with other people.
> >
> > The conflict will come in development when two vendors use the same
> > unpublished feature id.
> >
> > Also keeping the truth of id's in the kernel source isn't that great because the
> > public kernel always lags development repositories.
> 
> I fully understand your point, and it's a good idea to me, but I am not sure if
> we can update the spec for DFHv0 at this moment. Let me check with spec
> owner about this. Actually I believe we don't want to add anything in driver
> code which is not defined in the spec at all. : (
> 
> >
> > >
> > > And in the later version DFH, this feature id will be replaced with GUID
> > > I think, so it can resolve the conflict problems from different vendors?
> > > But now, we still need to handle the existing ones. : )
> >
> > This is why I proposed needing to generalize the matching.
> 
> Personally I prefer that we can have standard matching functions per DFH
> specs.
> 
> Yilun, how do you think about this?

I also prefer we don't add anything now before DFL spec is updated.

The idea of extending the DFLv0 is good, but I'm not sure if it is necessary
now. It depends on how the customer is using the card, are they
developing more FME features themselves on current cards and want to make
them public.

I think we could discuss about the strategy. And we could add another
patchset if we finally decide and update the DFLv0 spec.

Thanks
Yilun

> 
> Thanks
> Hao
>