RE: [PATCH v3 2/4] fpga: dfl: map feature mmio resources in their own feature drivers
From: Wu, Hao
Date: Wed Aug 05 2020 - 12:34:11 EST
> Subject: [PATCH v3 2/4] 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@xxxxxxxxx>
> Signed-off-by: Wu Hao <hao.wu@xxxxxxxxx>
> Signed-off-by: Matthew Gerlach <matthew.gerlach@xxxxxxxxxxxxxxx>
> Signed-off-by: Russ Weight <russell.h.weight@xxxxxxxxx>
> Reviewed-by: Tom Rix <trix@xxxxxxxxxx>
> ---
> v2: delete dfl_binfo_shift() cause no shift is possible for FIU parsing.
> Only map bar0 for first phase enumeration in dfl-pci, we can find
> all dfl mmio resource info in bar0.
> Some minor fixes for comments from Hao & Tom.
> v3: no change.
> ---
> drivers/fpga/dfl-pci.c | 24 +++----
> drivers/fpga/dfl.c | 185 ++++++++++++++++++++++++++++++++++------------
> ---
> drivers/fpga/dfl.h | 7 +-
> 3 files changed, 140 insertions(+), 76 deletions(-)
>
> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
> index e220bec..a2203d0 100644
> --- a/drivers/fpga/dfl-pci.c
> +++ b/drivers/fpga/dfl-pci.c
> @@ -31,12 +31,12 @@ struct cci_drvdata {
> struct dfl_fpga_cdev *cdev; /* container device */
> };
>
> -static void __iomem *cci_pci_ioremap_bar(struct pci_dev *pcidev, int bar)
> +static void __iomem *cci_pci_ioremap_bar0(struct pci_dev *pcidev)
> {
> - if (pcim_iomap_regions(pcidev, BIT(bar), DRV_NAME))
> + if (pcim_iomap_regions(pcidev, BIT(0), DRV_NAME))
> return NULL;
>
> - return pcim_iomap_table(pcidev)[bar];
> + return pcim_iomap_table(pcidev)[0];
> }
>
> static int cci_pci_alloc_irq(struct pci_dev *pcidev)
> @@ -156,8 +156,8 @@ static int cci_enumerate_feature_devs(struct pci_dev
> *pcidev)
> goto irq_free_exit;
> }
>
> - /* start to find Device Feature List from Bar 0 */
> - base = cci_pci_ioremap_bar(pcidev, 0);
> + /* start to find Device Feature List in Bar 0 */
> + base = cci_pci_ioremap_bar0(pcidev);
> if (!base) {
> ret = -ENOMEM;
> goto irq_free_exit;
> @@ -172,7 +172,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
> @@ -196,26 +196,24 @@ static int cci_enumerate_feature_devs(struct
> pci_dev *pcidev)
> */
> bar = FIELD_GET(FME_PORT_OFST_BAR_ID, v);
> offset = FIELD_GET(FME_PORT_OFST_DFH_OFST, v);
> - base = cci_pci_ioremap_bar(pcidev, bar);
> - if (!base)
> - continue;
> -
> 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 */
> + pcim_iounmap_regions(pcidev, BIT(0));
> +
> /* 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 18575d9..c649239 100644
> --- a/drivers/fpga/dfl.c
> +++ b/drivers/fpga/dfl.c
> @@ -250,6 +250,8 @@ int dfl_fpga_check_port_id(struct platform_device
> *pdev, void *pport_id)
> }
> EXPORT_SYMBOL_GPL(dfl_fpga_check_port_id);
>
> +#define is_header_feature(feature) ((feature)->id ==
> FEATURE_ID_FIU_HEADER)
> +
> /**
> * dfl_fpga_dev_feature_uinit - uinit for sub features of dfl feature device
> * @pdev: feature device.
> @@ -273,8 +275,22 @@ 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,
> + "ioremap failed for feature 0x%x!\n",
> + feature->id);
> + return PTR_ERR(base);
> + }
> +
> + feature->ioaddr = base;
> + }
> +
> if (drv->ops->init) {
> ret = drv->ops->init(pdev, feature);
> if (ret)
> @@ -427,7 +443,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 +457,8 @@ 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 +504,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 +547,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 +613,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 +630,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 +772,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, u16 fid)
> + resource_size_t ofst, resource_size_t size, u16 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 +794,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 +807,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,21 +814,22 @@ 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);
> }
>
> +#define is_feature_dev_detected(binfo) (!!(binfo)->feature_dev)
> +
> 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) {
> + if (!is_feature_dev_detected(binfo)) {
> dev_err(binfo->dev, "this AFU does not belong to any
> FIU.\n");
> return -EINVAL;
> }
>
> 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,8 +838,40 @@ static int parse_feature_afu(struct
> build_feature_devs_info *binfo,
> return 0;
> }
>
> +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 -EBUSY;
> + }
> +
> + 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);
as it's devm_request_mem_region, do we still need to release it here?
> + return -ENOMEM;
> + }
> +
> + binfo->start = start;
> + binfo->len = len;
> + binfo->ioaddr = ioaddr;
> +
> + return 0;
> +}
> +
> +static void dfl_binfo_complete(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)
> {
> int ret = 0;
> @@ -824,27 +879,39 @@ static int parse_feature_fiu(struct
> build_feature_devs_info *binfo,
> u16 id;
> u64 v;
>
> - v = readq(dfl->ioaddr + ofst + DFH);
> + if (is_feature_dev_detected(binfo)) {
> + dfl_binfo_complete(binfo);
> +
> + ret = build_info_commit_dev(binfo);
> + if (ret)
> + return ret;
> +
> + ret = dfl_binfo_prepare(binfo, binfo->start + ofst,
> + binfo->len - ofst);
> + if (ret)
> + return ret;
> + }
> +
> + 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);
>
> @@ -852,16 +919,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) {
> + if (!is_feature_dev_detected(binfo)) {
> dev_err(binfo->dev, "the private feature 0x%x does not
> belong to any AFU.\n",
> - feature_id(dfl->ioaddr + ofst));
> + feature_id(binfo->ioaddr + ofst));
> return -EINVAL;
> }
>
> - return create_feature_instance(binfo, dfl, ofst, 0, 0);
> + return create_feature_instance(binfo, ofst, 0, 0);
> }
>
> /**
> @@ -869,24 +935,24 @@ static int parse_feature_private(struct
> build_feature_devs_info *binfo,
> *
> * @binfo: build feature devices information.
> * @dfl: device feature list to parse
Remove this line.
Other place looks good to me.
Thanks
Hao
> - * @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);
> @@ -896,14 +962,17 @@ 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)
> + resource_size_t start, resource_size_t len)
> {
> - void __iomem *start = dfl->ioaddr;
> - void __iomem *end = dfl->ioaddr + dfl->len;
> + resource_size_t end = start + len;
> int ret = 0;
> u32 ofst = 0;
> u64 v;
>
> + ret = dfl_binfo_prepare(binfo, start, len);
> + if (ret)
> + return ret;
> +
> /* walk through the device feature list via DFH's next DFH pointer. */
> for (; start < end; start += ofst) {
> if (end - start < DFH_SIZE) {
> @@ -911,11 +980,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 */
> @@ -924,7 +993,12 @@ static int parse_feature_list(struct
> build_feature_devs_info *binfo,
> }
>
> /* commit current feature device when reach the end of list */
> - return build_info_commit_dev(binfo);
> + dfl_binfo_complete(binfo);
> +
> + if (is_feature_dev_detected(binfo))
> + ret = build_info_commit_dev(binfo);
> +
> + return ret;
> }
>
> struct dfl_fpga_enum_info *dfl_fpga_enum_info_alloc(struct device *dev)
> @@ -977,7 +1051,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
> @@ -986,8 +1059,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;
>
> @@ -997,7 +1069,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);
>
> @@ -1120,7 +1191,7 @@ dfl_fpga_feature_devs_enumerate(struct
> dfl_fpga_enum_info *info)
> * Lists.
> */
> list_for_each_entry(dfl, &info->dfls, node) {
> - ret = parse_feature_list(binfo, dfl);
> + ret = parse_feature_list(binfo, dfl->start, dfl->len);
> if (ret) {
> remove_feature_devs(cdev);
> build_info_free(binfo);
> diff --git a/drivers/fpga/dfl.h b/drivers/fpga/dfl.h
> index bc61942..5973769 100644
> --- a/drivers/fpga/dfl.h
> +++ b/drivers/fpga/dfl.h
> @@ -441,22 +441,17 @@ 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