Re: [PATCH 1/2] fpga: dfl: map feature mmio resources in their own feature drivers
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)
> > +{
>
Re: [PATCH 1/2] fpga: dfl: map feature mmio resources in their own feature drivers
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
> > Signed-off-by: Wu Hao
> > Signed-off-by: Matthew Gerlach
> > Signed-off-by: Russ Weight
> > ---
> > 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
Re: [PATCH 1/2] fpga: dfl: map feature mmio resources in their own feature drivers
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) {
> >
Re: [PATCH 1/2] fpga: dfl: map feature mmio resources in their own feature drivers
On Fri, Jul 17, 2020 at 05:48:55PM +0800, Wu, Hao wrote:
> > -Original Message-
> > From: linux-fpga-ow...@vger.kernel.org
> > On Behalf Of Xu Yilun
> > Sent: Wednesday, July 15, 2020 1:38 PM
> > To: m...@kernel.org; linux-f...@vger.kernel.org; linux-
> > ker...@vger.kernel.org
> > Cc: t...@redhat.com; lgonc...@redhat.com; Xu, Yilun ;
> > Wu, Hao ; 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
> > Signed-off-by: Wu Hao
> > Signed-off-by: Matthew Gerlach
> > Signed-off-by: Russ Weight
> > ---
> > 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
RE: [PATCH 1/2] fpga: dfl: map feature mmio resources in their own feature drivers
> 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
> Signed-off-by: Wu Hao
> Signed-off-by: Matthew Gerlach
> Signed-off-by: Russ Weight
> ---
> drivers/fpga/dfl-pci.c | 21 --
> drivers/fpga/dfl.c | 187 +++---
> ---
> drivers/fpga/dfl.h | 6 +-
> 3 files changed, 152 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
> index e220bec..22dc025 100644
> --- a/drivers/fpga/dfl-pci.c
> +++ b/drivers/fpga/dfl-pci.c
> @@ -39,6 +39,11 @@ static void __iomem *cci_pci_ioremap_bar(struct
> pci_dev *pcidev, int bar)
> return pcim_iomap_table(pcidev)[bar];
> }
>
> +static void cci_pci_iounmap_bars(struct pci_dev *pcidev, int mapped_bars)
> +{
> + pcim_iounmap_regions(pcidev, mapped_bars);
> +}
> +
> static int cci_pci_alloc_irq(struct pci_dev *pcidev)
> {
> int ret, nvec = pci_msix_vec_count(pcidev);
> @@ -123,7 +128,7 @@ static int *cci_pci_create_irq_table(struct pci_dev
> *pcidev, unsigned int nvec)
> static int cci_enumerate_feature_devs(struct pci_dev *pcidev)
> {
> struct cci_drvdata *drvdata = pci_get_drvdata(pcidev);
> - int port_num, bar, i, nvec, ret = 0;
> + int port_num, bar, i, nvec, mapped_bars, ret = 0;
> struct dfl_fpga_enum_info *info;
> struct dfl_fpga_cdev *cdev;
> resource_size_t start, len;
> @@ -163,6 +168,8 @@ static int cci_enumerate_feature_devs(struct pci_dev
> *pcidev)
> goto irq_free_exit;
> }
>
> + mapped_bars = BIT(0);
> +
> /*
>* PF device has FME and Ports/AFUs, and VF device only has one
>* Port/AFU. Check them and add related "Device Feature List" info
> @@ -172,7 +179,7 @@ static int cci_enumerate_feature_devs(struct pci_dev
> *pcidev)
> start = pci_resource_start(pcidev, 0);
> len = pci_resource_len(pcidev, 0);
>
> - dfl_fpga_enum_info_add_dfl(info, start, len, base);
> + dfl_fpga_enum_info_add_dfl(info, start, len);
>
> /*
>* find more Device Feature Lists (e.g. Ports) per information
> @@ -200,22 +207,26 @@ static int cci_enumerate_feature_devs(struct
> pci_dev *pcidev)
> if (!base)
> continue;
>
> + mapped_bars |= BIT(bar);
> +
One more place,
As you removed base addr usage below, so ideally we don't need to map
more bars for port here? is my understanding correct?
> start = pci_resource_start(pcidev, bar) + offset;
> len = pci_resource_len(pcidev, bar) - offset;
>
> - dfl_fpga_enum_info_add_dfl(info, start, len,
> -base + offset);
> + dfl_fpga_enum_info_add_dfl(info, start, len);
Thanks
Hao
Re: [PATCH 1/2] fpga: dfl: map feature mmio resources in their own feature drivers
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:
RE: [PATCH 1/2] fpga: dfl: map feature mmio resources in their own feature drivers
> -Original Message-
> From: linux-fpga-ow...@vger.kernel.org
> On Behalf Of Xu Yilun
> Sent: Wednesday, July 15, 2020 1:38 PM
> To: m...@kernel.org; linux-f...@vger.kernel.org; linux-
> ker...@vger.kernel.org
> Cc: t...@redhat.com; lgonc...@redhat.com; Xu, Yilun ;
> Wu, Hao ; 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
> Signed-off-by: Wu Hao
> Signed-off-by: Matthew Gerlach
> Signed-off-by: Russ Weight
> ---
> 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 {
>
