* RE: [PATCH 1/2] fpga: dfl: map feature mmio resources in their own feature drivers
2020-07-15 5:38 ` [PATCH 1/2] fpga: dfl: map feature mmio resources in their own " Xu Yilun
@ 2020-07-17 9:48 ` Wu, Hao
2020-07-17 16:51 ` Tom Rix
2020-07-20 9:09 ` Wu, Hao
2 siblings, 0 replies; 26+ messages in thread
From: Wu, Hao @ 2020-07-17 9:48 UTC (permalink / raw)
To: Xu, Yilun, mdf, linux-fpga, linux-kernel
Cc: trix, lgoncalv, Xu, Yilun, Matthew Gerlach, Weight, Russell H
> -----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
^ permalink raw reply [flat|nested] 26+ messages in thread* RE: [PATCH 1/2] fpga: dfl: map feature mmio resources in their own feature drivers
@ 2020-07-17 9:48 ` Wu, Hao
0 siblings, 0 replies; 26+ messages in thread
From: Wu, Hao @ 2020-07-17 9:48 UTC (permalink / raw)
To: Xu, Yilun, mdf, linux-fpga, linux-kernel
Cc: trix, lgoncalv, Matthew Gerlach, Weight, Russell H
> -----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
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 1/2] fpga: dfl: map feature mmio resources in their own feature drivers
2020-07-17 9:48 ` Wu, Hao
(?)
@ 2020-07-21 6:57 ` Xu Yilun
-1 siblings, 0 replies; 26+ messages in thread
From: Xu Yilun @ 2020-07-21 6:57 UTC (permalink / raw)
To: Wu, Hao
Cc: mdf, linux-fpga, linux-kernel, trix, lgoncalv, Matthew Gerlach,
Weight, Russell H
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] fpga: dfl: map feature mmio resources in their own feature drivers
2020-07-15 5:38 ` [PATCH 1/2] fpga: dfl: map feature mmio resources in their own " Xu Yilun
2020-07-17 9:48 ` Wu, Hao
@ 2020-07-17 16:51 ` Tom Rix
2020-07-21 7:34 ` Xu Yilun
2020-07-22 6:51 ` Xu Yilun
2020-07-20 9:09 ` Wu, Hao
2 siblings, 2 replies; 26+ messages in thread
From: Tom Rix @ 2020-07-17 16:51 UTC (permalink / raw)
To: Xu Yilun, mdf, linux-fpga, linux-kernel
Cc: lgoncalv, Wu Hao, Matthew Gerlach, Russ Weight
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);
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 1/2] fpga: dfl: map feature mmio resources in their own feature drivers
2020-07-17 16:51 ` Tom Rix
@ 2020-07-21 7:34 ` Xu Yilun
2020-07-22 6:51 ` Xu Yilun
1 sibling, 0 replies; 26+ messages in thread
From: Xu Yilun @ 2020-07-21 7:34 UTC (permalink / raw)
To: Tom Rix
Cc: mdf, linux-fpga, linux-kernel, lgoncalv, Wu Hao, Matthew Gerlach,
Russ Weight
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);
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 1/2] fpga: dfl: map feature mmio resources in their own feature drivers
@ 2020-07-21 7:34 ` Xu Yilun
0 siblings, 0 replies; 26+ messages in thread
From: Xu Yilun @ 2020-07-21 7:34 UTC (permalink / raw)
To: Tom Rix
Cc: mdf, linux-fpga, linux-kernel, lgoncalv, Wu Hao, Matthew Gerlach,
Russ Weight
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);
^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] fpga: dfl: map feature mmio resources in their own feature drivers
2020-07-17 16:51 ` Tom Rix
@ 2020-07-22 6:51 ` Xu Yilun
2020-07-22 6:51 ` Xu Yilun
1 sibling, 0 replies; 26+ messages in thread
From: Xu Yilun @ 2020-07-22 6:51 UTC (permalink / raw)
To: Tom Rix
Cc: mdf, linux-fpga, linux-kernel, lgoncalv, Wu Hao, Matthew Gerlach,
Russ Weight
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
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 1/2] fpga: dfl: map feature mmio resources in their own feature drivers
@ 2020-07-22 6:51 ` Xu Yilun
0 siblings, 0 replies; 26+ messages in thread
From: Xu Yilun @ 2020-07-22 6:51 UTC (permalink / raw)
To: Tom Rix
Cc: mdf, linux-fpga, linux-kernel, lgoncalv, Wu Hao, Matthew Gerlach,
Russ Weight
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
^ permalink raw reply [flat|nested] 26+ messages in thread
* RE: [PATCH 1/2] fpga: dfl: map feature mmio resources in their own feature drivers
2020-07-15 5:38 ` [PATCH 1/2] fpga: dfl: map feature mmio resources in their own " Xu Yilun
@ 2020-07-20 9:09 ` Wu, Hao
2020-07-17 16:51 ` Tom Rix
2020-07-20 9:09 ` Wu, Hao
2 siblings, 0 replies; 26+ messages in thread
From: Wu, Hao @ 2020-07-20 9:09 UTC (permalink / raw)
To: Xu, Yilun, mdf, linux-fpga, linux-kernel
Cc: trix, lgoncalv, Xu, Yilun, Matthew Gerlach, Weight, Russell H
> 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
^ permalink raw reply [flat|nested] 26+ messages in thread* RE: [PATCH 1/2] fpga: dfl: map feature mmio resources in their own feature drivers
@ 2020-07-20 9:09 ` Wu, Hao
0 siblings, 0 replies; 26+ messages in thread
From: Wu, Hao @ 2020-07-20 9:09 UTC (permalink / raw)
To: Xu, Yilun, mdf, linux-fpga, linux-kernel
Cc: trix, lgoncalv, Matthew Gerlach, Weight, Russell H
> 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
^ permalink raw reply [flat|nested] 26+ messages in thread* Re: [PATCH 1/2] fpga: dfl: map feature mmio resources in their own feature drivers
2020-07-20 9:09 ` Wu, Hao
(?)
@ 2020-07-21 7:40 ` Xu Yilun
-1 siblings, 0 replies; 26+ messages in thread
From: Xu Yilun @ 2020-07-21 7:40 UTC (permalink / raw)
To: Wu, Hao
Cc: mdf, linux-fpga, linux-kernel, trix, lgoncalv, Matthew Gerlach,
Weight, Russell H
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
^ permalink raw reply [flat|nested] 26+ messages in thread