[PATCH v5 0/2] UIO support for dfl devices

[PATCH v5 0/2] UIO support for dfl devices

[Xu Yilun]

This patchset supports some dfl device drivers written in userspace.

In the patchset v1, the "driver_override" interface should be used to bind
the DFL UIO driver to DFL devices. But there is concern that the
"driver_override" interface is not OK itself.

In v2, we use a new matching algorithem. The "driver_override" interface
is abandoned, the DFL UIO driver matches any DFL device which could not be
handled by other DFL drivers. So the DFL UIO driver could be used for new
DFL devices which are not supported by kernel. The concern is the UIO may
not be suitable as a default/generic driver for all dfl features, such as
features with multiple interrupts.

In v4, we specify each matching device in the id_table of the UIO driver,
just the same as other dfl drivers do. Now the UIO driver supports Ether
Group feature. To support more DFL features, their feature ids should be
added to the driver's id_table.


Main changes from v1:
- switch to the new matching algorithem. It matches DFL devices which could
  not be handled by other DFL drivers.
- refacor the code about device resources filling.
- add the documentation.

Main changes from v2:
- split the match ops changes in dfl.c to an independent patch.
- move the declarations needed for dfl-uio-pdev from include/linux/dfl.h
  to driver/fpga/dfl.h
- some minor fixes.

Main changes from v3:
- switch to specifying each matching device in the driver's id_table.
- refactor the irq handling code.

Main changes from v4:
- refactor the irq handling code.

Xu Yilun (2):
  fpga: dfl: add the userspace I/O device support for DFL devices
  Documentation: fpga: dfl: Add description for DFL UIO support

 Documentation/fpga/dfl.rst  | 24 ++++++++++++
 drivers/fpga/Kconfig        | 10 +++++
 drivers/fpga/Makefile       |  1 +
 drivers/fpga/dfl-uio-pdev.c | 91 +++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 126 insertions(+)
 create mode 100644 drivers/fpga/dfl-uio-pdev.c

-- 
2.7.4

[PATCH v5 1/2] fpga: dfl: add the userspace I/O device support for DFL devices

[Xu Yilun]

This patch supports the DFL drivers be written in userspace. This is
realized by exposing the userspace I/O device interfaces.

The driver leverages the uio_pdrv_genirq, it adds the uio_pdrv_genirq
platform device with the DFL device's resources, and let the generic UIO
platform device driver provide support to userspace access to kernel
interrupts and memory locations.

The driver now supports the ether group feature. To support a new DFL
feature been directly accessed via UIO, its feature id should be added to
the driver's id_table.

Signed-off-by: Xu Yilun <yilun.xu@intel.com>
Reviewed-by: Tom Rix <trix@redhat.com>
---
v2: switch to the new matching algorithem. It matches DFL devices which
     could not be handled by other DFL drivers.
    refacor the code about device resources filling.
    fix some comments.
v3: split the dfl.c changes out of this patch.
    some minor fixes
v4: drop the idea of a generic matching algorithem, instead we specify
     each matching device in id_table.
    to make clear that only one irq is supported, the irq handling code
     is refactored.
v5: refactor the irq resource code.
---
 drivers/fpga/Kconfig        | 10 +++++
 drivers/fpga/Makefile       |  1 +
 drivers/fpga/dfl-uio-pdev.c | 91 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 102 insertions(+)
 create mode 100644 drivers/fpga/dfl-uio-pdev.c

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 5ff9438..61445be 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -203,6 +203,16 @@ config FPGA_DFL_NIOS_INTEL_PAC_N3000
 	  the card. It also instantiates the SPI master (spi-altera) for
 	  the card's BMC (Board Management Controller).
 
+config FPGA_DFL_UIO_PDEV
+	tristate "FPGA DFL Driver for Userspace I/O platform devices"
+	depends on FPGA_DFL && UIO_PDRV_GENIRQ
+	help
+	  Enable this to allow some DFL drivers be written in userspace. It
+	  adds the uio_pdrv_genirq platform device with the DFL feature's
+	  resources, and lets the generic UIO platform device driver provide
+	  support for userspace access to kernel interrupts and memory
+	  locations.
+
 config FPGA_DFL_PCI
 	tristate "FPGA DFL PCIe Device Driver"
 	depends on PCI && FPGA_DFL
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index 18dc9885..8847fe0 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -45,6 +45,7 @@ dfl-afu-objs := dfl-afu-main.o dfl-afu-region.o dfl-afu-dma-region.o
 dfl-afu-objs += dfl-afu-error.o
 
 obj-$(CONFIG_FPGA_DFL_NIOS_INTEL_PAC_N3000)	+= dfl-n3000-nios.o
+obj-$(CONFIG_FPGA_DFL_UIO_PDEV)		+= dfl-uio-pdev.o
 
 # Drivers for FPGAs which implement DFL
 obj-$(CONFIG_FPGA_DFL_PCI)		+= dfl-pci.o
diff --git a/drivers/fpga/dfl-uio-pdev.c b/drivers/fpga/dfl-uio-pdev.c
new file mode 100644
index 0000000..a4cd581
--- /dev/null
+++ b/drivers/fpga/dfl-uio-pdev.c
@@ -0,0 +1,91 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * DFL driver for Userspace I/O platform devices
+ *
+ * Copyright (C) 2020 Intel Corporation, Inc.
+ */
+#include <linux/dfl.h>
+#include <linux/errno.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/uio_driver.h>
+
+#define DRIVER_NAME "dfl-uio-pdev"
+
+static int dfl_uio_pdev_probe(struct dfl_device *ddev)
+{
+	struct platform_device_info pdevinfo = { 0 };
+	struct uio_info uio_pdata = { 0 };
+	struct platform_device *uio_pdev;
+	struct device *dev = &ddev->dev;
+	unsigned int num_res = 1;
+	struct resource res[2];
+
+	res[0].parent = &ddev->mmio_res;
+	res[0].flags = IORESOURCE_MEM;
+	res[0].start = ddev->mmio_res.start;
+	res[0].end = ddev->mmio_res.end;
+
+	if (ddev->num_irqs) {
+		if (ddev->num_irqs > 1)
+			dev_warn(&ddev->dev,
+				 "%d irqs for %s, but UIO only supports the first one\n",
+				 ddev->num_irqs, dev_name(&ddev->dev));
+
+		res[1].flags = IORESOURCE_IRQ;
+		res[1].start = ddev->irqs[0];
+		res[1].end = ddev->irqs[0];
+		num_res++;
+	}
+
+	uio_pdata.name = DRIVER_NAME;
+	uio_pdata.version = "0";
+
+	pdevinfo.name = "uio_pdrv_genirq";
+	pdevinfo.res = res;
+	pdevinfo.num_res = num_res;
+	pdevinfo.parent = &ddev->dev;
+	pdevinfo.id = PLATFORM_DEVID_AUTO;
+	pdevinfo.data = &uio_pdata;
+	pdevinfo.size_data = sizeof(uio_pdata);
+
+	uio_pdev = platform_device_register_full(&pdevinfo);
+	if (!IS_ERR(uio_pdev))
+		dev_set_drvdata(dev, uio_pdev);
+
+	return PTR_ERR_OR_ZERO(uio_pdev);
+}
+
+static void dfl_uio_pdev_remove(struct dfl_device *ddev)
+{
+	struct platform_device *uio_pdev = dev_get_drvdata(&ddev->dev);
+
+	platform_device_unregister(uio_pdev);
+}
+
+#define FME_FEATURE_ID_ETH_GROUP	0x10
+
+static const struct dfl_device_id dfl_uio_pdev_ids[] = {
+	{ FME_ID, FME_FEATURE_ID_ETH_GROUP },
+
+	/* Add your new id entries here to support uio for more dfl features */
+
+	{ }
+};
+MODULE_DEVICE_TABLE(dfl, dfl_uio_pdev_ids);
+
+static struct dfl_driver dfl_uio_pdev_driver = {
+	.drv	= {
+		.name       = DRIVER_NAME,
+	},
+	.id_table = dfl_uio_pdev_ids,
+	.probe	= dfl_uio_pdev_probe,
+	.remove	= dfl_uio_pdev_remove,
+};
+module_dfl_driver(dfl_uio_pdev_driver);
+
+MODULE_DESCRIPTION("DFL driver for Userspace I/O platform devices");
+MODULE_AUTHOR("Intel Corporation");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

[PATCH v5 2/2] Documentation: fpga: dfl: Add description for DFL UIO support

[Xu Yilun]

This patch adds description for UIO support for dfl devices on DFL
bus.

Signed-off-by: Xu Yilun <yilun.xu@intel.com>
---
v2: no doc in v1, add it for v2.
v3: some documentation fixes.
v4: documentation change since the driver matching is changed.
v5: no change.
---
 Documentation/fpga/dfl.rst | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
index 0404fe6..b8497f3 100644
--- a/Documentation/fpga/dfl.rst
+++ b/Documentation/fpga/dfl.rst
@@ -7,6 +7,7 @@ Authors:
 - Enno Luebbers <enno.luebbers@intel.com>
 - Xiao Guangrong <guangrong.xiao@linux.intel.com>
 - Wu Hao <hao.wu@intel.com>
+- Xu Yilun <yilun.xu@intel.com>
 
 The Device Feature List (DFL) FPGA framework (and drivers according to
 this framework) hides the very details of low layer hardwares and provides
@@ -502,6 +503,29 @@ FME Partial Reconfiguration Sub Feature driver (see drivers/fpga/dfl-fme-pr.c)
 could be a reference.
 
 
+UIO support for DFL devices
+===========================
+The purpose of an FPGA is to be reprogrammed with newly developed hardware
+components. New hardware can instantiate a new private feature in the DFL, and
+then get a DFL device in their system. In some cases users may need a userspace
+driver for the DFL device:
+
+* Users may need to run some diagnostic test for their hardwares.
+* Users may prototype the kernel driver in user space.
+* Some hardware is designed for specific purposes and does not fit into one of
+  the standard kernel subsystems.
+
+This requires the direct access to the MMIO space and interrupt handling in
+userspace. The dfl-uio-pdev module exposes the UIO device interfaces for this
+purpose. It adds the uio_pdrv_genirq platform device with the resources of
+the DFL feature, and lets the generic UIO platform device driver provide UIO
+support to userspace.
+
+FPGA_DFL_UIO_PDEV should be selected to enable the dfl-uio-pdev module driver.
+To support a new DFL feature been directly accessed via UIO, its feature id
+should be added to the driver's id_table.
+
+
 Open discussion
 ===============
 FME driver exports one ioctl (DFL_FPGA_FME_PORT_PR) for partial reconfiguration
-- 
2.7.4

RE: [PATCH v5 1/2] fpga: dfl: add the userspace I/O device support for DFL devices

[Wu, Hao]

> Subject: [PATCH v5 1/2] fpga: dfl: add the userspace I/O device support for
> DFL devices
> 
> This patch supports the DFL drivers be written in userspace. This is
> realized by exposing the userspace I/O device interfaces.
> 
> The driver leverages the uio_pdrv_genirq, it adds the uio_pdrv_genirq
> platform device with the DFL device's resources, and let the generic UIO
> platform device driver provide support to userspace access to kernel
> interrupts and memory locations.
> 
> The driver now supports the ether group feature. To support a new DFL
> feature been directly accessed via UIO, its feature id should be added to
> the driver's id_table.
> 
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> Reviewed-by: Tom Rix <trix@redhat.com>
> ---
> v2: switch to the new matching algorithem. It matches DFL devices which
>      could not be handled by other DFL drivers.
>     refacor the code about device resources filling.
>     fix some comments.
> v3: split the dfl.c changes out of this patch.
>     some minor fixes
> v4: drop the idea of a generic matching algorithem, instead we specify
>      each matching device in id_table.
>     to make clear that only one irq is supported, the irq handling code
>      is refactored.
> v5: refactor the irq resource code.
> ---
>  drivers/fpga/Kconfig        | 10 +++++
>  drivers/fpga/Makefile       |  1 +
>  drivers/fpga/dfl-uio-pdev.c | 91
> +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 102 insertions(+)
>  create mode 100644 drivers/fpga/dfl-uio-pdev.c
> 
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index 5ff9438..61445be 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -203,6 +203,16 @@ config FPGA_DFL_NIOS_INTEL_PAC_N3000
>  	  the card. It also instantiates the SPI master (spi-altera) for
>  	  the card's BMC (Board Management Controller).
> 
> +config FPGA_DFL_UIO_PDEV
> +	tristate "FPGA DFL Driver for Userspace I/O platform devices"
> +	depends on FPGA_DFL && UIO_PDRV_GENIRQ
> +	help
> +	  Enable this to allow some DFL drivers be written in userspace. It
> +	  adds the uio_pdrv_genirq platform device with the DFL feature's
> +	  resources, and lets the generic UIO platform device driver provide
> +	  support for userspace access to kernel interrupts and memory
> +	  locations.
> +
>  config FPGA_DFL_PCI
>  	tristate "FPGA DFL PCIe Device Driver"
>  	depends on PCI && FPGA_DFL
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 18dc9885..8847fe0 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -45,6 +45,7 @@ dfl-afu-objs := dfl-afu-main.o dfl-afu-region.o dfl-afu-
> dma-region.o
>  dfl-afu-objs += dfl-afu-error.o
> 
>  obj-$(CONFIG_FPGA_DFL_NIOS_INTEL_PAC_N3000)	+= dfl-n3000-nios.o
> +obj-$(CONFIG_FPGA_DFL_UIO_PDEV)		+= dfl-uio-pdev.o
> 
>  # Drivers for FPGAs which implement DFL
>  obj-$(CONFIG_FPGA_DFL_PCI)		+= dfl-pci.o
> diff --git a/drivers/fpga/dfl-uio-pdev.c b/drivers/fpga/dfl-uio-pdev.c
> new file mode 100644
> index 0000000..a4cd581
> --- /dev/null
> +++ b/drivers/fpga/dfl-uio-pdev.c
> @@ -0,0 +1,91 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * DFL driver for Userspace I/O platform devices
> + *
> + * Copyright (C) 2020 Intel Corporation, Inc.
> + */
> +#include <linux/dfl.h>
> +#include <linux/errno.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/uio_driver.h>
> +
> +#define DRIVER_NAME "dfl-uio-pdev"
> +
> +static int dfl_uio_pdev_probe(struct dfl_device *ddev)
> +{
> +	struct platform_device_info pdevinfo = { 0 };
> +	struct uio_info uio_pdata = { 0 };
> +	struct platform_device *uio_pdev;
> +	struct device *dev = &ddev->dev;
> +	unsigned int num_res = 1;
> +	struct resource res[2];
> +
> +	res[0].parent = &ddev->mmio_res;
> +	res[0].flags = IORESOURCE_MEM;
> +	res[0].start = ddev->mmio_res.start;
> +	res[0].end = ddev->mmio_res.end;
> +
> +	if (ddev->num_irqs) {
> +		if (ddev->num_irqs > 1)
> +			dev_warn(&ddev->dev,
> +				 "%d irqs for %s, but UIO only supports the
> first one\n",
> +				 ddev->num_irqs, dev_name(&ddev->dev));
> +
> +		res[1].flags = IORESOURCE_IRQ;
> +		res[1].start = ddev->irqs[0];
> +		res[1].end = ddev->irqs[0];

Looks like res[] needs to be zeroed for other fields not used here.
other places look good to me.

Hao

> +		num_res++;
> +	}
> +
> +	uio_pdata.name = DRIVER_NAME;
> +	uio_pdata.version = "0";
> +
> +	pdevinfo.name = "uio_pdrv_genirq";
> +	pdevinfo.res = res;
> +	pdevinfo.num_res = num_res;
> +	pdevinfo.parent = &ddev->dev;
> +	pdevinfo.id = PLATFORM_DEVID_AUTO;
> +	pdevinfo.data = &uio_pdata;
> +	pdevinfo.size_data = sizeof(uio_pdata);
> +
> +	uio_pdev = platform_device_register_full(&pdevinfo);
> +	if (!IS_ERR(uio_pdev))
> +		dev_set_drvdata(dev, uio_pdev);
> +
> +	return PTR_ERR_OR_ZERO(uio_pdev);
> +}
> +
> +static void dfl_uio_pdev_remove(struct dfl_device *ddev)
> +{
> +	struct platform_device *uio_pdev = dev_get_drvdata(&ddev->dev);
> +
> +	platform_device_unregister(uio_pdev);
> +}
> +
> +#define FME_FEATURE_ID_ETH_GROUP	0x10
> +
> +static const struct dfl_device_id dfl_uio_pdev_ids[] = {
> +	{ FME_ID, FME_FEATURE_ID_ETH_GROUP },
> +
> +	/* Add your new id entries here to support uio for more dfl features
> */
> +
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(dfl, dfl_uio_pdev_ids);
> +
> +static struct dfl_driver dfl_uio_pdev_driver = {
> +	.drv	= {
> +		.name       = DRIVER_NAME,
> +	},
> +	.id_table = dfl_uio_pdev_ids,
> +	.probe	= dfl_uio_pdev_probe,
> +	.remove	= dfl_uio_pdev_remove,
> +};
> +module_dfl_driver(dfl_uio_pdev_driver);
> +
> +MODULE_DESCRIPTION("DFL driver for Userspace I/O platform devices");
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4

RE: [PATCH v5 2/2] Documentation: fpga: dfl: Add description for DFL UIO support

[Wu, Hao]

> Subject: [PATCH v5 2/2] Documentation: fpga: dfl: Add description for DFL
> UIO support
> 
> This patch adds description for UIO support for dfl devices on DFL
> bus.
> 
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> ---
> v2: no doc in v1, add it for v2.
> v3: some documentation fixes.
> v4: documentation change since the driver matching is changed.
> v5: no change.
> ---
>  Documentation/fpga/dfl.rst | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
> index 0404fe6..b8497f3 100644
> --- a/Documentation/fpga/dfl.rst
> +++ b/Documentation/fpga/dfl.rst
> @@ -7,6 +7,7 @@ Authors:
>  - Enno Luebbers <enno.luebbers@intel.com>
>  - Xiao Guangrong <guangrong.xiao@linux.intel.com>
>  - Wu Hao <hao.wu@intel.com>
> +- Xu Yilun <yilun.xu@intel.com>
> 
>  The Device Feature List (DFL) FPGA framework (and drivers according to
>  this framework) hides the very details of low layer hardwares and provides
> @@ -502,6 +503,29 @@ FME Partial Reconfiguration Sub Feature driver (see
> drivers/fpga/dfl-fme-pr.c)
>  could be a reference.
> 
> 
> +UIO support for DFL devices
> +===========================

I think the main purpose is to support userspace driver for DFL devices, and
UIO is the option for now. Do you think if the title could be userspace driver
support for DFL devices? 

Hao

> +The purpose of an FPGA is to be reprogrammed with newly developed
> hardware
> +components. New hardware can instantiate a new private feature in the
> DFL, and
> +then get a DFL device in their system. In some cases users may need a
> userspace
> +driver for the DFL device:
> +
> +* Users may need to run some diagnostic test for their hardwares.
> +* Users may prototype the kernel driver in user space.
> +* Some hardware is designed for specific purposes and does not fit into one
> of
> +  the standard kernel subsystems.
> +
> +This requires the direct access to the MMIO space and interrupt handling in
> +userspace. The dfl-uio-pdev module exposes the UIO device interfaces for
> this
> +purpose. It adds the uio_pdrv_genirq platform device with the resources of
> +the DFL feature, and lets the generic UIO platform device driver provide UIO
> +support to userspace.
> +
> +FPGA_DFL_UIO_PDEV should be selected to enable the dfl-uio-pdev module
> driver.
> +To support a new DFL feature been directly accessed via UIO, its feature id
> +should be added to the driver's id_table.
> +
> +
>  Open discussion
>  ===============
>  FME driver exports one ioctl (DFL_FPGA_FME_PORT_PR) for partial
> reconfiguration
> --
> 2.7.4

Re: [PATCH v5 1/2] fpga: dfl: add the userspace I/O device support for DFL devices

[Xu Yilun]

On Mon, Jan 04, 2021 at 12:16:30PM +0800, Wu, Hao wrote:
> > Subject: [PATCH v5 1/2] fpga: dfl: add the userspace I/O device support for
> > DFL devices
> >
> > This patch supports the DFL drivers be written in userspace. This is
> > realized by exposing the userspace I/O device interfaces.
> >
> > The driver leverages the uio_pdrv_genirq, it adds the uio_pdrv_genirq
> > platform device with the DFL device's resources, and let the generic UIO
> > platform device driver provide support to userspace access to kernel
> > interrupts and memory locations.
> >
> > The driver now supports the ether group feature. To support a new DFL
> > feature been directly accessed via UIO, its feature id should be added to
> > the driver's id_table.
> >
> > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > Reviewed-by: Tom Rix <trix@redhat.com>
> > ---
> > v2: switch to the new matching algorithem. It matches DFL devices which
> >      could not be handled by other DFL drivers.
> >     refacor the code about device resources filling.
> >     fix some comments.
> > v3: split the dfl.c changes out of this patch.
> >     some minor fixes
> > v4: drop the idea of a generic matching algorithem, instead we specify
> >      each matching device in id_table.
> >     to make clear that only one irq is supported, the irq handling code
> >      is refactored.
> > v5: refactor the irq resource code.
> > ---
> >  drivers/fpga/Kconfig        | 10 +++++
> >  drivers/fpga/Makefile       |  1 +
> >  drivers/fpga/dfl-uio-pdev.c | 91
> > +++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 102 insertions(+)
> >  create mode 100644 drivers/fpga/dfl-uio-pdev.c
> >
> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > index 5ff9438..61445be 100644
> > --- a/drivers/fpga/Kconfig
> > +++ b/drivers/fpga/Kconfig
> > @@ -203,6 +203,16 @@ config FPGA_DFL_NIOS_INTEL_PAC_N3000
> >    the card. It also instantiates the SPI master (spi-altera) for
> >    the card's BMC (Board Management Controller).
> >
> > +config FPGA_DFL_UIO_PDEV
> > +tristate "FPGA DFL Driver for Userspace I/O platform devices"
> > +depends on FPGA_DFL && UIO_PDRV_GENIRQ
> > +help
> > +  Enable this to allow some DFL drivers be written in userspace. It
> > +  adds the uio_pdrv_genirq platform device with the DFL feature's
> > +  resources, and lets the generic UIO platform device driver provide
> > +  support for userspace access to kernel interrupts and memory
> > +  locations.
> > +
> >  config FPGA_DFL_PCI
> >  tristate "FPGA DFL PCIe Device Driver"
> >  depends on PCI && FPGA_DFL
> > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> > index 18dc9885..8847fe0 100644
> > --- a/drivers/fpga/Makefile
> > +++ b/drivers/fpga/Makefile
> > @@ -45,6 +45,7 @@ dfl-afu-objs := dfl-afu-main.o dfl-afu-region.o dfl-afu-
> > dma-region.o
> >  dfl-afu-objs += dfl-afu-error.o
> >
> >  obj-$(CONFIG_FPGA_DFL_NIOS_INTEL_PAC_N3000)+= dfl-n3000-nios.o
> > +obj-$(CONFIG_FPGA_DFL_UIO_PDEV)+= dfl-uio-pdev.o
> >
> >  # Drivers for FPGAs which implement DFL
> >  obj-$(CONFIG_FPGA_DFL_PCI)+= dfl-pci.o
> > diff --git a/drivers/fpga/dfl-uio-pdev.c b/drivers/fpga/dfl-uio-pdev.c
> > new file mode 100644
> > index 0000000..a4cd581
> > --- /dev/null
> > +++ b/drivers/fpga/dfl-uio-pdev.c
> > @@ -0,0 +1,91 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * DFL driver for Userspace I/O platform devices
> > + *
> > + * Copyright (C) 2020 Intel Corporation, Inc.
> > + */
> > +#include <linux/dfl.h>
> > +#include <linux/errno.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +#include <linux/uio_driver.h>
> > +
> > +#define DRIVER_NAME "dfl-uio-pdev"
> > +
> > +static int dfl_uio_pdev_probe(struct dfl_device *ddev)
> > +{
> > +struct platform_device_info pdevinfo = { 0 };
> > +struct uio_info uio_pdata = { 0 };
> > +struct platform_device *uio_pdev;
> > +struct device *dev = &ddev->dev;
> > +unsigned int num_res = 1;
> > +struct resource res[2];
> > +
> > +res[0].parent = &ddev->mmio_res;
> > +res[0].flags = IORESOURCE_MEM;
> > +res[0].start = ddev->mmio_res.start;
> > +res[0].end = ddev->mmio_res.end;
> > +
> > +if (ddev->num_irqs) {
> > +if (ddev->num_irqs > 1)
> > +dev_warn(&ddev->dev,
> > + "%d irqs for %s, but UIO only supports the
> > first one\n",
> > + ddev->num_irqs, dev_name(&ddev->dev));
> > +
> > +res[1].flags = IORESOURCE_IRQ;
> > +res[1].start = ddev->irqs[0];
> > +res[1].end = ddev->irqs[0];
> 
> Looks like res[] needs to be zeroed for other fields not used here.

Thanks for catching this. Will fix it.

Yilun

Re: [PATCH v5 2/2] Documentation: fpga: dfl: Add description for DFL UIO support

[Xu Yilun]

On Mon, Jan 04, 2021 at 12:24:05PM +0800, Wu, Hao wrote:
> > Subject: [PATCH v5 2/2] Documentation: fpga: dfl: Add description for DFL
> > UIO support
> >
> > This patch adds description for UIO support for dfl devices on DFL
> > bus.
> >
> > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > ---
> > v2: no doc in v1, add it for v2.
> > v3: some documentation fixes.
> > v4: documentation change since the driver matching is changed.
> > v5: no change.
> > ---
> >  Documentation/fpga/dfl.rst | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> >
> > diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
> > index 0404fe6..b8497f3 100644
> > --- a/Documentation/fpga/dfl.rst
> > +++ b/Documentation/fpga/dfl.rst
> > @@ -7,6 +7,7 @@ Authors:
> >  - Enno Luebbers <enno.luebbers@intel.com>
> >  - Xiao Guangrong <guangrong.xiao@linux.intel.com>
> >  - Wu Hao <hao.wu@intel.com>
> > +- Xu Yilun <yilun.xu@intel.com>
> >
> >  The Device Feature List (DFL) FPGA framework (and drivers according to
> >  this framework) hides the very details of low layer hardwares and provides
> > @@ -502,6 +503,29 @@ FME Partial Reconfiguration Sub Feature driver (see
> > drivers/fpga/dfl-fme-pr.c)
> >  could be a reference.
> >
> >
> > +UIO support for DFL devices
> > +===========================
> 
> I think the main purpose is to support userspace driver for DFL devices, and
> UIO is the option for now. Do you think if the title could be userspace driver
> support for DFL devices?

Yes, I could change it.

Thanks,
Yilun

Re: [PATCH v5 2/2] Documentation: fpga: dfl: Add description for DFL UIO support

[Russ Weight]

On 1/1/21 7:13 PM, Xu Yilun wrote:
> This patch adds description for UIO support for dfl devices on DFL
> bus.
>
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> ---
> v2: no doc in v1, add it for v2.
> v3: some documentation fixes.
> v4: documentation change since the driver matching is changed.
> v5: no change.
> ---
>  Documentation/fpga/dfl.rst | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
> index 0404fe6..b8497f3 100644
> --- a/Documentation/fpga/dfl.rst
> +++ b/Documentation/fpga/dfl.rst
> @@ -7,6 +7,7 @@ Authors:
>  - Enno Luebbers <enno.luebbers@intel.com>
>  - Xiao Guangrong <guangrong.xiao@linux.intel.com>
>  - Wu Hao <hao.wu@intel.com>
> +- Xu Yilun <yilun.xu@intel.com>
>  
>  The Device Feature List (DFL) FPGA framework (and drivers according to
>  this framework) hides the very details of low layer hardwares and provides
> @@ -502,6 +503,29 @@ FME Partial Reconfiguration Sub Feature driver (see drivers/fpga/dfl-fme-pr.c)
>  could be a reference.
>  
>  
> +UIO support for DFL devices
> +===========================
> +The purpose of an FPGA is to be reprogrammed with newly developed hardware
> +components. New hardware can instantiate a new private feature in the DFL, and
> +then get a DFL device in their system. In some cases users may need a userspace
> +driver for the DFL device:
> +
> +* Users may need to run some diagnostic test for their hardwares.
> +* Users may prototype the kernel driver in user space.
> +* Some hardware is designed for specific purposes and does not fit into one of
> +  the standard kernel subsystems.
> +
> +This requires the direct access to the MMIO space and interrupt handling in
This may be better stated as:

"This requires direct access to MMIO space and interrupt handling from userspace."

- Russ

> +userspace. The dfl-uio-pdev module exposes the UIO device interfaces for this
> +purpose. It adds the uio_pdrv_genirq platform device with the resources of
> +the DFL feature, and lets the generic UIO platform device driver provide UIO
> +support to userspace.
> +
> +FPGA_DFL_UIO_PDEV should be selected to enable the dfl-uio-pdev module driver.
> +To support a new DFL feature been directly accessed via UIO, its feature id
> +should be added to the driver's id_table.
> +
> +
>  Open discussion
>  ===============
>  FME driver exports one ioctl (DFL_FPGA_FME_PORT_PR) for partial reconfiguration

Re: [PATCH v5 2/2] Documentation: fpga: dfl: Add description for DFL UIO support

[Xu Yilun]

On Mon, Jan 04, 2021 at 09:19:24AM -0800, Russ Weight wrote:
> 
> 
> On 1/1/21 7:13 PM, Xu Yilun wrote:
> > This patch adds description for UIO support for dfl devices on DFL
> > bus.
> >
> > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > ---
> > v2: no doc in v1, add it for v2.
> > v3: some documentation fixes.
> > v4: documentation change since the driver matching is changed.
> > v5: no change.
> > ---
> >  Documentation/fpga/dfl.rst | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> >
> > diff --git a/Documentation/fpga/dfl.rst b/Documentation/fpga/dfl.rst
> > index 0404fe6..b8497f3 100644
> > --- a/Documentation/fpga/dfl.rst
> > +++ b/Documentation/fpga/dfl.rst
> > @@ -7,6 +7,7 @@ Authors:
> >  - Enno Luebbers <enno.luebbers@intel.com>
> >  - Xiao Guangrong <guangrong.xiao@linux.intel.com>
> >  - Wu Hao <hao.wu@intel.com>
> > +- Xu Yilun <yilun.xu@intel.com>
> >  
> >  The Device Feature List (DFL) FPGA framework (and drivers according to
> >  this framework) hides the very details of low layer hardwares and provides
> > @@ -502,6 +503,29 @@ FME Partial Reconfiguration Sub Feature driver (see drivers/fpga/dfl-fme-pr.c)
> >  could be a reference.
> >  
> >  
> > +UIO support for DFL devices
> > +===========================
> > +The purpose of an FPGA is to be reprogrammed with newly developed hardware
> > +components. New hardware can instantiate a new private feature in the DFL, and
> > +then get a DFL device in their system. In some cases users may need a userspace
> > +driver for the DFL device:
> > +
> > +* Users may need to run some diagnostic test for their hardwares.
> > +* Users may prototype the kernel driver in user space.
> > +* Some hardware is designed for specific purposes and does not fit into one of
> > +  the standard kernel subsystems.
> > +
> > +This requires the direct access to the MMIO space and interrupt handling in
> This may be better stated as:
> 
> "This requires direct access to MMIO space and interrupt handling from userspace."

Thanks for the correction, I'll change it.

Yilun

Re: [PATCH v5 0/2] UIO support for dfl devices

[Moritz Fischer]

Hi Xu,

On Sat, Jan 02, 2021 at 11:13:00AM +0800, Xu Yilun wrote:
> This patchset supports some dfl device drivers written in userspace.
> 
> In the patchset v1, the "driver_override" interface should be used to bind
> the DFL UIO driver to DFL devices. But there is concern that the
> "driver_override" interface is not OK itself.
> 
> In v2, we use a new matching algorithem. The "driver_override" interface
> is abandoned, the DFL UIO driver matches any DFL device which could not be
> handled by other DFL drivers. So the DFL UIO driver could be used for new
> DFL devices which are not supported by kernel. The concern is the UIO may
> not be suitable as a default/generic driver for all dfl features, such as
> features with multiple interrupts.
> 
> In v4, we specify each matching device in the id_table of the UIO driver,
> just the same as other dfl drivers do. Now the UIO driver supports Ether
> Group feature. To support more DFL features, their feature ids should be
> added to the driver's id_table.

I think this is what you want, yes. Instead of doing a driver override
or such, add devices that should always be bound to UIO to a device id
table. For those you temporarily want to bind, make sure you can unbind
them and use 'new_id' or 'bind' in sysfs, similar to what sysfs does.

> 
> 
> Main changes from v1:
> - switch to the new matching algorithem. It matches DFL devices which could
>   not be handled by other DFL drivers.
> - refacor the code about device resources filling.
> - add the documentation.
> 
> Main changes from v2:
> - split the match ops changes in dfl.c to an independent patch.
> - move the declarations needed for dfl-uio-pdev from include/linux/dfl.h
>   to driver/fpga/dfl.h
> - some minor fixes.
> 
> Main changes from v3:
> - switch to specifying each matching device in the driver's id_table.
> - refactor the irq handling code.
> 
> Main changes from v4:
> - refactor the irq handling code.
> 
> Xu Yilun (2):
>   fpga: dfl: add the userspace I/O device support for DFL devices
>   Documentation: fpga: dfl: Add description for DFL UIO support
> 
>  Documentation/fpga/dfl.rst  | 24 ++++++++++++
>  drivers/fpga/Kconfig        | 10 +++++
>  drivers/fpga/Makefile       |  1 +
>  drivers/fpga/dfl-uio-pdev.c | 91 +++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 126 insertions(+)
>  create mode 100644 drivers/fpga/dfl-uio-pdev.c
> 
> -- 
> 2.7.4
> 

- Moritz

Re: [PATCH v5 1/2] fpga: dfl: add the userspace I/O device support for DFL devices

[Moritz Fischer]

On Sat, Jan 02, 2021 at 11:13:01AM +0800, Xu Yilun wrote:
> This patch supports the DFL drivers be written in userspace. This is
> realized by exposing the userspace I/O device interfaces.
> 
> The driver leverages the uio_pdrv_genirq, it adds the uio_pdrv_genirq
> platform device with the DFL device's resources, and let the generic UIO
> platform device driver provide support to userspace access to kernel
> interrupts and memory locations.
> 
> The driver now supports the ether group feature. To support a new DFL
> feature been directly accessed via UIO, its feature id should be added to
> the driver's id_table.
> 
> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> Reviewed-by: Tom Rix <trix@redhat.com>
> ---
> v2: switch to the new matching algorithem. It matches DFL devices which
>      could not be handled by other DFL drivers.
>     refacor the code about device resources filling.
>     fix some comments.
> v3: split the dfl.c changes out of this patch.
>     some minor fixes
> v4: drop the idea of a generic matching algorithem, instead we specify
>      each matching device in id_table.
>     to make clear that only one irq is supported, the irq handling code
>      is refactored.
> v5: refactor the irq resource code.
> ---
>  drivers/fpga/Kconfig        | 10 +++++
>  drivers/fpga/Makefile       |  1 +
>  drivers/fpga/dfl-uio-pdev.c | 91 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 102 insertions(+)
>  create mode 100644 drivers/fpga/dfl-uio-pdev.c
> 
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index 5ff9438..61445be 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -203,6 +203,16 @@ config FPGA_DFL_NIOS_INTEL_PAC_N3000
>  	  the card. It also instantiates the SPI master (spi-altera) for
>  	  the card's BMC (Board Management Controller).
>  
> +config FPGA_DFL_UIO_PDEV
> +	tristate "FPGA DFL Driver for Userspace I/O platform devices"
> +	depends on FPGA_DFL && UIO_PDRV_GENIRQ
> +	help
> +	  Enable this to allow some DFL drivers be written in userspace. It
> +	  adds the uio_pdrv_genirq platform device with the DFL feature's
> +	  resources, and lets the generic UIO platform device driver provide
> +	  support for userspace access to kernel interrupts and memory
> +	  locations.
> +
>  config FPGA_DFL_PCI
>  	tristate "FPGA DFL PCIe Device Driver"
>  	depends on PCI && FPGA_DFL
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 18dc9885..8847fe0 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -45,6 +45,7 @@ dfl-afu-objs := dfl-afu-main.o dfl-afu-region.o dfl-afu-dma-region.o
>  dfl-afu-objs += dfl-afu-error.o
>  
>  obj-$(CONFIG_FPGA_DFL_NIOS_INTEL_PAC_N3000)	+= dfl-n3000-nios.o
> +obj-$(CONFIG_FPGA_DFL_UIO_PDEV)		+= dfl-uio-pdev.o
>  
>  # Drivers for FPGAs which implement DFL
>  obj-$(CONFIG_FPGA_DFL_PCI)		+= dfl-pci.o
> diff --git a/drivers/fpga/dfl-uio-pdev.c b/drivers/fpga/dfl-uio-pdev.c
> new file mode 100644
> index 0000000..a4cd581
> --- /dev/null
> +++ b/drivers/fpga/dfl-uio-pdev.c
> @@ -0,0 +1,91 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * DFL driver for Userspace I/O platform devices
> + *
> + * Copyright (C) 2020 Intel Corporation, Inc.
> + */
> +#include <linux/dfl.h>
> +#include <linux/errno.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/uio_driver.h>
> +
> +#define DRIVER_NAME "dfl-uio-pdev"
> +
> +static int dfl_uio_pdev_probe(struct dfl_device *ddev)
> +{
> +	struct platform_device_info pdevinfo = { 0 };
> +	struct uio_info uio_pdata = { 0 };
> +	struct platform_device *uio_pdev;
> +	struct device *dev = &ddev->dev;
> +	unsigned int num_res = 1;
> +	struct resource res[2];
> +
> +	res[0].parent = &ddev->mmio_res;
> +	res[0].flags = IORESOURCE_MEM;
> +	res[0].start = ddev->mmio_res.start;
> +	res[0].end = ddev->mmio_res.end;
> +
> +	if (ddev->num_irqs) {
> +		if (ddev->num_irqs > 1)
> +			dev_warn(&ddev->dev,
> +				 "%d irqs for %s, but UIO only supports the first one\n",
> +				 ddev->num_irqs, dev_name(&ddev->dev));
> +
> +		res[1].flags = IORESOURCE_IRQ;
> +		res[1].start = ddev->irqs[0];
> +		res[1].end = ddev->irqs[0];
> +		num_res++;
> +	}
> +
> +	uio_pdata.name = DRIVER_NAME;
> +	uio_pdata.version = "0";
> +
> +	pdevinfo.name = "uio_pdrv_genirq";
> +	pdevinfo.res = res;
> +	pdevinfo.num_res = num_res;
> +	pdevinfo.parent = &ddev->dev;
> +	pdevinfo.id = PLATFORM_DEVID_AUTO;
> +	pdevinfo.data = &uio_pdata;
> +	pdevinfo.size_data = sizeof(uio_pdata);
> +
> +	uio_pdev = platform_device_register_full(&pdevinfo);
> +	if (!IS_ERR(uio_pdev))
> +		dev_set_drvdata(dev, uio_pdev);

I'm not sure if this is more readable than:

   	uio_pdev = platform_device_register_full(&pdevinfo);
   	if (IS_ERR(uio_pdev))
   		return PTR_ERR(uio_pdev);

	dev_set_drvdata(dev, uio_pdev);
	return 0;

No strong preference, though ... :)
> +
> +	return PTR_ERR_OR_ZERO(uio_pdev);
> +}
> +
> +static void dfl_uio_pdev_remove(struct dfl_device *ddev)
> +{
> +	struct platform_device *uio_pdev = dev_get_drvdata(&ddev->dev);
> +
> +	platform_device_unregister(uio_pdev);
> +}
> +
> +#define FME_FEATURE_ID_ETH_GROUP	0x10
> +
> +static const struct dfl_device_id dfl_uio_pdev_ids[] = {
> +	{ FME_ID, FME_FEATURE_ID_ETH_GROUP },

Will you want to always bind FME_FEATURE_ID_ETH_GROUP? If not I'd suggest not
to add it here.

If you want to provide an option to somewhat non-ABI fixed bind things
you could look at what vfio-pci does (provide a module parameter),
otherwise use sysfs 'new_id' or 'bind'.

Does that sound reasonable?
> +
> +	/* Add your new id entries here to support uio for more dfl features */
> +
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(dfl, dfl_uio_pdev_ids);
> +
> +static struct dfl_driver dfl_uio_pdev_driver = {
> +	.drv	= {
> +		.name       = DRIVER_NAME,
> +	},
> +	.id_table = dfl_uio_pdev_ids,
> +	.probe	= dfl_uio_pdev_probe,
> +	.remove	= dfl_uio_pdev_remove,
> +};
> +module_dfl_driver(dfl_uio_pdev_driver);
> +
> +MODULE_DESCRIPTION("DFL driver for Userspace I/O platform devices");
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_LICENSE("GPL v2");
> -- 
> 2.7.4
> 
Thanks,

Moritz

Re: [PATCH v5 1/2] fpga: dfl: add the userspace I/O device support for DFL devices

[Xu Yilun]

On Sun, Jan 10, 2021 at 12:11:17PM -0800, Moritz Fischer wrote:
> On Sat, Jan 02, 2021 at 11:13:01AM +0800, Xu Yilun wrote:
> > This patch supports the DFL drivers be written in userspace. This is
> > realized by exposing the userspace I/O device interfaces.
> > 
> > The driver leverages the uio_pdrv_genirq, it adds the uio_pdrv_genirq
> > platform device with the DFL device's resources, and let the generic UIO
> > platform device driver provide support to userspace access to kernel
> > interrupts and memory locations.
> > 
> > The driver now supports the ether group feature. To support a new DFL
> > feature been directly accessed via UIO, its feature id should be added to
> > the driver's id_table.
> > 
> > Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > Reviewed-by: Tom Rix <trix@redhat.com>
> > ---
> > v2: switch to the new matching algorithem. It matches DFL devices which
> >      could not be handled by other DFL drivers.
> >     refacor the code about device resources filling.
> >     fix some comments.
> > v3: split the dfl.c changes out of this patch.
> >     some minor fixes
> > v4: drop the idea of a generic matching algorithem, instead we specify
> >      each matching device in id_table.
> >     to make clear that only one irq is supported, the irq handling code
> >      is refactored.
> > v5: refactor the irq resource code.
> > ---
> >  drivers/fpga/Kconfig        | 10 +++++
> >  drivers/fpga/Makefile       |  1 +
> >  drivers/fpga/dfl-uio-pdev.c | 91 +++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 102 insertions(+)
> >  create mode 100644 drivers/fpga/dfl-uio-pdev.c
> > 
> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > index 5ff9438..61445be 100644
> > --- a/drivers/fpga/Kconfig
> > +++ b/drivers/fpga/Kconfig
> > @@ -203,6 +203,16 @@ config FPGA_DFL_NIOS_INTEL_PAC_N3000
> >  	  the card. It also instantiates the SPI master (spi-altera) for
> >  	  the card's BMC (Board Management Controller).
> >  
> > +config FPGA_DFL_UIO_PDEV
> > +	tristate "FPGA DFL Driver for Userspace I/O platform devices"
> > +	depends on FPGA_DFL && UIO_PDRV_GENIRQ
> > +	help
> > +	  Enable this to allow some DFL drivers be written in userspace. It
> > +	  adds the uio_pdrv_genirq platform device with the DFL feature's
> > +	  resources, and lets the generic UIO platform device driver provide
> > +	  support for userspace access to kernel interrupts and memory
> > +	  locations.
> > +
> >  config FPGA_DFL_PCI
> >  	tristate "FPGA DFL PCIe Device Driver"
> >  	depends on PCI && FPGA_DFL
> > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> > index 18dc9885..8847fe0 100644
> > --- a/drivers/fpga/Makefile
> > +++ b/drivers/fpga/Makefile
> > @@ -45,6 +45,7 @@ dfl-afu-objs := dfl-afu-main.o dfl-afu-region.o dfl-afu-dma-region.o
> >  dfl-afu-objs += dfl-afu-error.o
> >  
> >  obj-$(CONFIG_FPGA_DFL_NIOS_INTEL_PAC_N3000)	+= dfl-n3000-nios.o
> > +obj-$(CONFIG_FPGA_DFL_UIO_PDEV)		+= dfl-uio-pdev.o
> >  
> >  # Drivers for FPGAs which implement DFL
> >  obj-$(CONFIG_FPGA_DFL_PCI)		+= dfl-pci.o
> > diff --git a/drivers/fpga/dfl-uio-pdev.c b/drivers/fpga/dfl-uio-pdev.c
> > new file mode 100644
> > index 0000000..a4cd581
> > --- /dev/null
> > +++ b/drivers/fpga/dfl-uio-pdev.c
> > @@ -0,0 +1,91 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * DFL driver for Userspace I/O platform devices
> > + *
> > + * Copyright (C) 2020 Intel Corporation, Inc.
> > + */
> > +#include <linux/dfl.h>
> > +#include <linux/errno.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +#include <linux/uio_driver.h>
> > +
> > +#define DRIVER_NAME "dfl-uio-pdev"
> > +
> > +static int dfl_uio_pdev_probe(struct dfl_device *ddev)
> > +{
> > +	struct platform_device_info pdevinfo = { 0 };
> > +	struct uio_info uio_pdata = { 0 };
> > +	struct platform_device *uio_pdev;
> > +	struct device *dev = &ddev->dev;
> > +	unsigned int num_res = 1;
> > +	struct resource res[2];
> > +
> > +	res[0].parent = &ddev->mmio_res;
> > +	res[0].flags = IORESOURCE_MEM;
> > +	res[0].start = ddev->mmio_res.start;
> > +	res[0].end = ddev->mmio_res.end;
> > +
> > +	if (ddev->num_irqs) {
> > +		if (ddev->num_irqs > 1)
> > +			dev_warn(&ddev->dev,
> > +				 "%d irqs for %s, but UIO only supports the first one\n",
> > +				 ddev->num_irqs, dev_name(&ddev->dev));
> > +
> > +		res[1].flags = IORESOURCE_IRQ;
> > +		res[1].start = ddev->irqs[0];
> > +		res[1].end = ddev->irqs[0];
> > +		num_res++;
> > +	}
> > +
> > +	uio_pdata.name = DRIVER_NAME;
> > +	uio_pdata.version = "0";
> > +
> > +	pdevinfo.name = "uio_pdrv_genirq";
> > +	pdevinfo.res = res;
> > +	pdevinfo.num_res = num_res;
> > +	pdevinfo.parent = &ddev->dev;
> > +	pdevinfo.id = PLATFORM_DEVID_AUTO;
> > +	pdevinfo.data = &uio_pdata;
> > +	pdevinfo.size_data = sizeof(uio_pdata);
> > +
> > +	uio_pdev = platform_device_register_full(&pdevinfo);
> > +	if (!IS_ERR(uio_pdev))
> > +		dev_set_drvdata(dev, uio_pdev);
> 
> I'm not sure if this is more readable than:
> 
>    	uio_pdev = platform_device_register_full(&pdevinfo);
>    	if (IS_ERR(uio_pdev))
>    		return PTR_ERR(uio_pdev);
> 
> 	dev_set_drvdata(dev, uio_pdev);
> 	return 0;
> 
> No strong preference, though ... :)

I think your version is more readable, I'll change it.

> > +
> > +	return PTR_ERR_OR_ZERO(uio_pdev);
> > +}
> > +
> > +static void dfl_uio_pdev_remove(struct dfl_device *ddev)
> > +{
> > +	struct platform_device *uio_pdev = dev_get_drvdata(&ddev->dev);
> > +
> > +	platform_device_unregister(uio_pdev);
> > +}
> > +
> > +#define FME_FEATURE_ID_ETH_GROUP	0x10
> > +
> > +static const struct dfl_device_id dfl_uio_pdev_ids[] = {
> > +	{ FME_ID, FME_FEATURE_ID_ETH_GROUP },
> 
> Will you want to always bind FME_FEATURE_ID_ETH_GROUP? If not I'd suggest not
> to add it here.

Actually this is not the most preferable to me. I'm always looking for a
generic way to bind the uio driver to user assigned dfl devices. But there
is concern that userspace should not be responsible for the device driver
matching in previous mail thread:

https://lore.kernel.org/linux-fpga/1602828151-24784-2-git-send-email-yilun.xu@intel.com/

But TBH I still didn't figure out why driver_override is not OK in this
case.

> 
> If you want to provide an option to somewhat non-ABI fixed bind things
> you could look at what vfio-pci does (provide a module parameter),
> otherwise use sysfs 'new_id' or 'bind'.

I would like to have a "new_id" for dfl bus driver. It is not generic to
all drivers, I need to add the attr for dfl drivers like pci do.

My concern is how the module param or "new_id" is different from
"driver_override", seems userspace is also taking part in the device
matching.

But since we've restarted the discussion, I'm very much willing to have
a try on the "new_id".

Thanks,
Yilun

Re: [PATCH v5 0/2] UIO support for dfl devices

[Xu Yilun]

On Sun, Jan 10, 2021 at 11:58:44AM -0800, Moritz Fischer wrote:
> Hi Xu,
> 
> On Sat, Jan 02, 2021 at 11:13:00AM +0800, Xu Yilun wrote:
> > This patchset supports some dfl device drivers written in userspace.
> > 
> > In the patchset v1, the "driver_override" interface should be used to bind
> > the DFL UIO driver to DFL devices. But there is concern that the
> > "driver_override" interface is not OK itself.
> > 
> > In v2, we use a new matching algorithem. The "driver_override" interface
> > is abandoned, the DFL UIO driver matches any DFL device which could not be
> > handled by other DFL drivers. So the DFL UIO driver could be used for new
> > DFL devices which are not supported by kernel. The concern is the UIO may
> > not be suitable as a default/generic driver for all dfl features, such as
> > features with multiple interrupts.
> > 
> > In v4, we specify each matching device in the id_table of the UIO driver,
> > just the same as other dfl drivers do. Now the UIO driver supports Ether
> > Group feature. To support more DFL features, their feature ids should be
> > added to the driver's id_table.
> 
> I think this is what you want, yes. Instead of doing a driver override
> or such, add devices that should always be bound to UIO to a device id
> table. For those you temporarily want to bind, make sure you can unbind
> them and use 'new_id' or 'bind' in sysfs, similar to what sysfs does.

"new_id" is not generic to all bus drivers, we need to add the attr in
dfl bus driver like pci do, actually I think quite similar to
"driver_override", How do you think?

I'm glad we restarted the discussion for the temporary binding of UIO
driver.

Thanks,
Yilun

Re: [PATCH v5 1/2] fpga: dfl: add the userspace I/O device support for DFL devices

[Tom Rix]

On 1/10/21 10:16 PM, Xu Yilun wrote:
> On Sun, Jan 10, 2021 at 12:11:17PM -0800, Moritz Fischer wrote:
>> On Sat, Jan 02, 2021 at 11:13:01AM +0800, Xu Yilun wrote:
>>> This patch supports the DFL drivers be written in userspace. This is
>>> realized by exposing the userspace I/O device interfaces.
>>>
>>> The driver leverages the uio_pdrv_genirq, it adds the uio_pdrv_genirq
>>> platform device with the DFL device's resources, and let the generic UIO
>>> platform device driver provide support to userspace access to kernel
>>> interrupts and memory locations.
>>>
>>> The driver now supports the ether group feature. To support a new DFL
>>> feature been directly accessed via UIO, its feature id should be added to
>>> the driver's id_table.
>>>
>>> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
>>> Reviewed-by: Tom Rix <trix@redhat.com>
>>> ---
>>> v2: switch to the new matching algorithem. It matches DFL devices which
>>>      could not be handled by other DFL drivers.
>>>     refacor the code about device resources filling.
>>>     fix some comments.
>>> v3: split the dfl.c changes out of this patch.
>>>     some minor fixes
>>> v4: drop the idea of a generic matching algorithem, instead we specify
>>>      each matching device in id_table.
>>>     to make clear that only one irq is supported, the irq handling code
>>>      is refactored.
>>> v5: refactor the irq resource code.
>>> ---
>>>  drivers/fpga/Kconfig        | 10 +++++
>>>  drivers/fpga/Makefile       |  1 +
>>>  drivers/fpga/dfl-uio-pdev.c | 91 +++++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 102 insertions(+)
>>>  create mode 100644 drivers/fpga/dfl-uio-pdev.c
>>>
>>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
>>> index 5ff9438..61445be 100644
>>> --- a/drivers/fpga/Kconfig
>>> +++ b/drivers/fpga/Kconfig
>>> @@ -203,6 +203,16 @@ config FPGA_DFL_NIOS_INTEL_PAC_N3000
>>>  	  the card. It also instantiates the SPI master (spi-altera) for
>>>  	  the card's BMC (Board Management Controller).
>>>  
>>> +config FPGA_DFL_UIO_PDEV
>>> +	tristate "FPGA DFL Driver for Userspace I/O platform devices"
>>> +	depends on FPGA_DFL && UIO_PDRV_GENIRQ
>>> +	help
>>> +	  Enable this to allow some DFL drivers be written in userspace. It
>>> +	  adds the uio_pdrv_genirq platform device with the DFL feature's
>>> +	  resources, and lets the generic UIO platform device driver provide
>>> +	  support for userspace access to kernel interrupts and memory
>>> +	  locations.
>>> +
>>>  config FPGA_DFL_PCI
>>>  	tristate "FPGA DFL PCIe Device Driver"
>>>  	depends on PCI && FPGA_DFL
>>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
>>> index 18dc9885..8847fe0 100644
>>> --- a/drivers/fpga/Makefile
>>> +++ b/drivers/fpga/Makefile
>>> @@ -45,6 +45,7 @@ dfl-afu-objs := dfl-afu-main.o dfl-afu-region.o dfl-afu-dma-region.o
>>>  dfl-afu-objs += dfl-afu-error.o
>>>  
>>>  obj-$(CONFIG_FPGA_DFL_NIOS_INTEL_PAC_N3000)	+= dfl-n3000-nios.o
>>> +obj-$(CONFIG_FPGA_DFL_UIO_PDEV)		+= dfl-uio-pdev.o
>>>  
>>>  # Drivers for FPGAs which implement DFL
>>>  obj-$(CONFIG_FPGA_DFL_PCI)		+= dfl-pci.o
>>> diff --git a/drivers/fpga/dfl-uio-pdev.c b/drivers/fpga/dfl-uio-pdev.c
>>> new file mode 100644
>>> index 0000000..a4cd581
>>> --- /dev/null
>>> +++ b/drivers/fpga/dfl-uio-pdev.c
>>> @@ -0,0 +1,91 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * DFL driver for Userspace I/O platform devices
>>> + *
>>> + * Copyright (C) 2020 Intel Corporation, Inc.
>>> + */
>>> +#include <linux/dfl.h>
>>> +#include <linux/errno.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/uio_driver.h>
>>> +
>>> +#define DRIVER_NAME "dfl-uio-pdev"
>>> +
>>> +static int dfl_uio_pdev_probe(struct dfl_device *ddev)
>>> +{
>>> +	struct platform_device_info pdevinfo = { 0 };
>>> +	struct uio_info uio_pdata = { 0 };
>>> +	struct platform_device *uio_pdev;
>>> +	struct device *dev = &ddev->dev;
>>> +	unsigned int num_res = 1;
>>> +	struct resource res[2];
>>> +
>>> +	res[0].parent = &ddev->mmio_res;
>>> +	res[0].flags = IORESOURCE_MEM;
>>> +	res[0].start = ddev->mmio_res.start;
>>> +	res[0].end = ddev->mmio_res.end;
>>> +
>>> +	if (ddev->num_irqs) {
>>> +		if (ddev->num_irqs > 1)
>>> +			dev_warn(&ddev->dev,
>>> +				 "%d irqs for %s, but UIO only supports the first one\n",
>>> +				 ddev->num_irqs, dev_name(&ddev->dev));
>>> +
>>> +		res[1].flags = IORESOURCE_IRQ;
>>> +		res[1].start = ddev->irqs[0];
>>> +		res[1].end = ddev->irqs[0];
>>> +		num_res++;
>>> +	}
>>> +
>>> +	uio_pdata.name = DRIVER_NAME;
>>> +	uio_pdata.version = "0";
>>> +
>>> +	pdevinfo.name = "uio_pdrv_genirq";
>>> +	pdevinfo.res = res;
>>> +	pdevinfo.num_res = num_res;
>>> +	pdevinfo.parent = &ddev->dev;
>>> +	pdevinfo.id = PLATFORM_DEVID_AUTO;
>>> +	pdevinfo.data = &uio_pdata;
>>> +	pdevinfo.size_data = sizeof(uio_pdata);
>>> +
>>> +	uio_pdev = platform_device_register_full(&pdevinfo);
>>> +	if (!IS_ERR(uio_pdev))
>>> +		dev_set_drvdata(dev, uio_pdev);
>> I'm not sure if this is more readable than:
>>
>>    	uio_pdev = platform_device_register_full(&pdevinfo);
>>    	if (IS_ERR(uio_pdev))
>>    		return PTR_ERR(uio_pdev);
>>
>> 	dev_set_drvdata(dev, uio_pdev);
>> 	return 0;
>>
>> No strong preference, though ... :)
> I think your version is more readable, I'll change it.
>
>>> +
>>> +	return PTR_ERR_OR_ZERO(uio_pdev);
>>> +}
>>> +
>>> +static void dfl_uio_pdev_remove(struct dfl_device *ddev)
>>> +{
>>> +	struct platform_device *uio_pdev = dev_get_drvdata(&ddev->dev);
>>> +
>>> +	platform_device_unregister(uio_pdev);
>>> +}
>>> +
>>> +#define FME_FEATURE_ID_ETH_GROUP	0x10
>>> +
>>> +static const struct dfl_device_id dfl_uio_pdev_ids[] = {
>>> +	{ FME_ID, FME_FEATURE_ID_ETH_GROUP },
>> Will you want to always bind FME_FEATURE_ID_ETH_GROUP? If not I'd suggest not
>> to add it here.
> Actually this is not the most preferable to me. I'm always looking for a
> generic way to bind the uio driver to user assigned dfl devices. But there
> is concern that userspace should not be responsible for the device driver
> matching in previous mail thread:
>
> https://lore.kernel.org/linux-fpga/1602828151-24784-2-git-send-email-yilun.xu@intel.com/
>
> But TBH I still didn't figure out why driver_override is not OK in this
> case.
>
>> If you want to provide an option to somewhat non-ABI fixed bind things
>> you could look at what vfio-pci does (provide a module parameter),
>> otherwise use sysfs 'new_id' or 'bind'.
> I would like to have a "new_id" for dfl bus driver. It is not generic to
> all drivers, I need to add the attr for dfl drivers like pci do.
>
> My concern is how the module param or "new_id" is different from
> "driver_override", seems userspace is also taking part in the device
> matching.
>
> But since we've restarted the discussion, I'm very much willing to have
> a try on the "new_id".

I don't believe there is any problem with the basic platform uio driver.

Can we split it out and work the new_id change in parallel ?

Tom

>
> Thanks,
> Yilun
>

Re: [PATCH v5 1/2] fpga: dfl: add the userspace I/O device support for DFL devices

[Xu Yilun]

On Mon, Jan 11, 2021 at 06:59:10AM -0800, Tom Rix wrote:
> 
> On 1/10/21 10:16 PM, Xu Yilun wrote:
> > On Sun, Jan 10, 2021 at 12:11:17PM -0800, Moritz Fischer wrote:
> >> On Sat, Jan 02, 2021 at 11:13:01AM +0800, Xu Yilun wrote:
> >>> This patch supports the DFL drivers be written in userspace. This is
> >>> realized by exposing the userspace I/O device interfaces.
> >>>
> >>> The driver leverages the uio_pdrv_genirq, it adds the uio_pdrv_genirq
> >>> platform device with the DFL device's resources, and let the generic UIO
> >>> platform device driver provide support to userspace access to kernel
> >>> interrupts and memory locations.
> >>>
> >>> The driver now supports the ether group feature. To support a new DFL
> >>> feature been directly accessed via UIO, its feature id should be added to
> >>> the driver's id_table.
> >>>
> >>> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> >>> Reviewed-by: Tom Rix <trix@redhat.com>
> >>> ---
> >>> v2: switch to the new matching algorithem. It matches DFL devices which
> >>>      could not be handled by other DFL drivers.
> >>>     refacor the code about device resources filling.
> >>>     fix some comments.
> >>> v3: split the dfl.c changes out of this patch.
> >>>     some minor fixes
> >>> v4: drop the idea of a generic matching algorithem, instead we specify
> >>>      each matching device in id_table.
> >>>     to make clear that only one irq is supported, the irq handling code
> >>>      is refactored.
> >>> v5: refactor the irq resource code.
> >>> ---
> >>>  drivers/fpga/Kconfig        | 10 +++++
> >>>  drivers/fpga/Makefile       |  1 +
> >>>  drivers/fpga/dfl-uio-pdev.c | 91 +++++++++++++++++++++++++++++++++++++++++++++
> >>>  3 files changed, 102 insertions(+)
> >>>  create mode 100644 drivers/fpga/dfl-uio-pdev.c
> >>>
> >>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> >>> index 5ff9438..61445be 100644
> >>> --- a/drivers/fpga/Kconfig
> >>> +++ b/drivers/fpga/Kconfig
> >>> @@ -203,6 +203,16 @@ config FPGA_DFL_NIOS_INTEL_PAC_N3000
> >>>  	  the card. It also instantiates the SPI master (spi-altera) for
> >>>  	  the card's BMC (Board Management Controller).
> >>>  
> >>> +config FPGA_DFL_UIO_PDEV
> >>> +	tristate "FPGA DFL Driver for Userspace I/O platform devices"
> >>> +	depends on FPGA_DFL && UIO_PDRV_GENIRQ
> >>> +	help
> >>> +	  Enable this to allow some DFL drivers be written in userspace. It
> >>> +	  adds the uio_pdrv_genirq platform device with the DFL feature's
> >>> +	  resources, and lets the generic UIO platform device driver provide
> >>> +	  support for userspace access to kernel interrupts and memory
> >>> +	  locations.
> >>> +
> >>>  config FPGA_DFL_PCI
> >>>  	tristate "FPGA DFL PCIe Device Driver"
> >>>  	depends on PCI && FPGA_DFL
> >>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> >>> index 18dc9885..8847fe0 100644
> >>> --- a/drivers/fpga/Makefile
> >>> +++ b/drivers/fpga/Makefile
> >>> @@ -45,6 +45,7 @@ dfl-afu-objs := dfl-afu-main.o dfl-afu-region.o dfl-afu-dma-region.o
> >>>  dfl-afu-objs += dfl-afu-error.o
> >>>  
> >>>  obj-$(CONFIG_FPGA_DFL_NIOS_INTEL_PAC_N3000)	+= dfl-n3000-nios.o
> >>> +obj-$(CONFIG_FPGA_DFL_UIO_PDEV)		+= dfl-uio-pdev.o
> >>>  
> >>>  # Drivers for FPGAs which implement DFL
> >>>  obj-$(CONFIG_FPGA_DFL_PCI)		+= dfl-pci.o
> >>> diff --git a/drivers/fpga/dfl-uio-pdev.c b/drivers/fpga/dfl-uio-pdev.c
> >>> new file mode 100644
> >>> index 0000000..a4cd581
> >>> --- /dev/null
> >>> +++ b/drivers/fpga/dfl-uio-pdev.c
> >>> @@ -0,0 +1,91 @@
> >>> +// SPDX-License-Identifier: GPL-2.0
> >>> +/*
> >>> + * DFL driver for Userspace I/O platform devices
> >>> + *
> >>> + * Copyright (C) 2020 Intel Corporation, Inc.
> >>> + */
> >>> +#include <linux/dfl.h>
> >>> +#include <linux/errno.h>
> >>> +#include <linux/kernel.h>
> >>> +#include <linux/module.h>
> >>> +#include <linux/platform_device.h>
> >>> +#include <linux/slab.h>
> >>> +#include <linux/uio_driver.h>
> >>> +
> >>> +#define DRIVER_NAME "dfl-uio-pdev"
> >>> +
> >>> +static int dfl_uio_pdev_probe(struct dfl_device *ddev)
> >>> +{
> >>> +	struct platform_device_info pdevinfo = { 0 };
> >>> +	struct uio_info uio_pdata = { 0 };
> >>> +	struct platform_device *uio_pdev;
> >>> +	struct device *dev = &ddev->dev;
> >>> +	unsigned int num_res = 1;
> >>> +	struct resource res[2];
> >>> +
> >>> +	res[0].parent = &ddev->mmio_res;
> >>> +	res[0].flags = IORESOURCE_MEM;
> >>> +	res[0].start = ddev->mmio_res.start;
> >>> +	res[0].end = ddev->mmio_res.end;
> >>> +
> >>> +	if (ddev->num_irqs) {
> >>> +		if (ddev->num_irqs > 1)
> >>> +			dev_warn(&ddev->dev,
> >>> +				 "%d irqs for %s, but UIO only supports the first one\n",
> >>> +				 ddev->num_irqs, dev_name(&ddev->dev));
> >>> +
> >>> +		res[1].flags = IORESOURCE_IRQ;
> >>> +		res[1].start = ddev->irqs[0];
> >>> +		res[1].end = ddev->irqs[0];
> >>> +		num_res++;
> >>> +	}
> >>> +
> >>> +	uio_pdata.name = DRIVER_NAME;
> >>> +	uio_pdata.version = "0";
> >>> +
> >>> +	pdevinfo.name = "uio_pdrv_genirq";
> >>> +	pdevinfo.res = res;
> >>> +	pdevinfo.num_res = num_res;
> >>> +	pdevinfo.parent = &ddev->dev;
> >>> +	pdevinfo.id = PLATFORM_DEVID_AUTO;
> >>> +	pdevinfo.data = &uio_pdata;
> >>> +	pdevinfo.size_data = sizeof(uio_pdata);
> >>> +
> >>> +	uio_pdev = platform_device_register_full(&pdevinfo);
> >>> +	if (!IS_ERR(uio_pdev))
> >>> +		dev_set_drvdata(dev, uio_pdev);
> >> I'm not sure if this is more readable than:
> >>
> >>    	uio_pdev = platform_device_register_full(&pdevinfo);
> >>    	if (IS_ERR(uio_pdev))
> >>    		return PTR_ERR(uio_pdev);
> >>
> >> 	dev_set_drvdata(dev, uio_pdev);
> >> 	return 0;
> >>
> >> No strong preference, though ... :)
> > I think your version is more readable, I'll change it.
> >
> >>> +
> >>> +	return PTR_ERR_OR_ZERO(uio_pdev);
> >>> +}
> >>> +
> >>> +static void dfl_uio_pdev_remove(struct dfl_device *ddev)
> >>> +{
> >>> +	struct platform_device *uio_pdev = dev_get_drvdata(&ddev->dev);
> >>> +
> >>> +	platform_device_unregister(uio_pdev);
> >>> +}
> >>> +
> >>> +#define FME_FEATURE_ID_ETH_GROUP	0x10
> >>> +
> >>> +static const struct dfl_device_id dfl_uio_pdev_ids[] = {
> >>> +	{ FME_ID, FME_FEATURE_ID_ETH_GROUP },
> >> Will you want to always bind FME_FEATURE_ID_ETH_GROUP? If not I'd suggest not
> >> to add it here.
> > Actually this is not the most preferable to me. I'm always looking for a
> > generic way to bind the uio driver to user assigned dfl devices. But there
> > is concern that userspace should not be responsible for the device driver
> > matching in previous mail thread:
> >
> > https://lore.kernel.org/linux-fpga/1602828151-24784-2-git-send-email-yilun.xu@intel.com/
> >
> > But TBH I still didn't figure out why driver_override is not OK in this
> > case.
> >
> >> If you want to provide an option to somewhat non-ABI fixed bind things
> >> you could look at what vfio-pci does (provide a module parameter),
> >> otherwise use sysfs 'new_id' or 'bind'.
> > I would like to have a "new_id" for dfl bus driver. It is not generic to
> > all drivers, I need to add the attr for dfl drivers like pci do.
> >
> > My concern is how the module param or "new_id" is different from
> > "driver_override", seems userspace is also taking part in the device
> > matching.
> >
> > But since we've restarted the discussion, I'm very much willing to have
> > a try on the "new_id".
> 
> I don't believe there is any problem with the basic platform uio driver.
> 
> Can we split it out and work the new_id change in parallel ?

Yes. The Ether Group is always bind to the uio driver for Intel PAC
N3000. So this patch works properly now. We could have a separate patch
for the new_id.

Thanks,
Yilun

Re: [PATCH v5 1/2] fpga: dfl: add the userspace I/O device support for DFL devices

[Moritz Fischer]

Hi Xu,

On Tue, Jan 12, 2021 at 08:16:15AM +0800, Xu Yilun wrote:
> On Mon, Jan 11, 2021 at 06:59:10AM -0800, Tom Rix wrote:
> > 
> > On 1/10/21 10:16 PM, Xu Yilun wrote:
> > > On Sun, Jan 10, 2021 at 12:11:17PM -0800, Moritz Fischer wrote:
> > >> On Sat, Jan 02, 2021 at 11:13:01AM +0800, Xu Yilun wrote:
> > >>> This patch supports the DFL drivers be written in userspace. This is
> > >>> realized by exposing the userspace I/O device interfaces.
> > >>>
> > >>> The driver leverages the uio_pdrv_genirq, it adds the uio_pdrv_genirq
> > >>> platform device with the DFL device's resources, and let the generic UIO
> > >>> platform device driver provide support to userspace access to kernel
> > >>> interrupts and memory locations.
> > >>>
> > >>> The driver now supports the ether group feature. To support a new DFL
> > >>> feature been directly accessed via UIO, its feature id should be added to
> > >>> the driver's id_table.
> > >>>
> > >>> Signed-off-by: Xu Yilun <yilun.xu@intel.com>
> > >>> Reviewed-by: Tom Rix <trix@redhat.com>
> > >>> ---
> > >>> v2: switch to the new matching algorithem. It matches DFL devices which
> > >>>      could not be handled by other DFL drivers.
> > >>>     refacor the code about device resources filling.
> > >>>     fix some comments.
> > >>> v3: split the dfl.c changes out of this patch.
> > >>>     some minor fixes
> > >>> v4: drop the idea of a generic matching algorithem, instead we specify
> > >>>      each matching device in id_table.
> > >>>     to make clear that only one irq is supported, the irq handling code
> > >>>      is refactored.
> > >>> v5: refactor the irq resource code.
> > >>> ---
> > >>>  drivers/fpga/Kconfig        | 10 +++++
> > >>>  drivers/fpga/Makefile       |  1 +
> > >>>  drivers/fpga/dfl-uio-pdev.c | 91 +++++++++++++++++++++++++++++++++++++++++++++
> > >>>  3 files changed, 102 insertions(+)
> > >>>  create mode 100644 drivers/fpga/dfl-uio-pdev.c
> > >>>
> > >>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > >>> index 5ff9438..61445be 100644
> > >>> --- a/drivers/fpga/Kconfig
> > >>> +++ b/drivers/fpga/Kconfig
> > >>> @@ -203,6 +203,16 @@ config FPGA_DFL_NIOS_INTEL_PAC_N3000
> > >>>  	  the card. It also instantiates the SPI master (spi-altera) for
> > >>>  	  the card's BMC (Board Management Controller).
> > >>>  
> > >>> +config FPGA_DFL_UIO_PDEV
> > >>> +	tristate "FPGA DFL Driver for Userspace I/O platform devices"
> > >>> +	depends on FPGA_DFL && UIO_PDRV_GENIRQ
> > >>> +	help
> > >>> +	  Enable this to allow some DFL drivers be written in userspace. It
> > >>> +	  adds the uio_pdrv_genirq platform device with the DFL feature's
> > >>> +	  resources, and lets the generic UIO platform device driver provide
> > >>> +	  support for userspace access to kernel interrupts and memory
> > >>> +	  locations.
> > >>> +
> > >>>  config FPGA_DFL_PCI
> > >>>  	tristate "FPGA DFL PCIe Device Driver"
> > >>>  	depends on PCI && FPGA_DFL
> > >>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> > >>> index 18dc9885..8847fe0 100644
> > >>> --- a/drivers/fpga/Makefile
> > >>> +++ b/drivers/fpga/Makefile
> > >>> @@ -45,6 +45,7 @@ dfl-afu-objs := dfl-afu-main.o dfl-afu-region.o dfl-afu-dma-region.o
> > >>>  dfl-afu-objs += dfl-afu-error.o
> > >>>  
> > >>>  obj-$(CONFIG_FPGA_DFL_NIOS_INTEL_PAC_N3000)	+= dfl-n3000-nios.o
> > >>> +obj-$(CONFIG_FPGA_DFL_UIO_PDEV)		+= dfl-uio-pdev.o
> > >>>  
> > >>>  # Drivers for FPGAs which implement DFL
> > >>>  obj-$(CONFIG_FPGA_DFL_PCI)		+= dfl-pci.o
> > >>> diff --git a/drivers/fpga/dfl-uio-pdev.c b/drivers/fpga/dfl-uio-pdev.c
> > >>> new file mode 100644
> > >>> index 0000000..a4cd581
> > >>> --- /dev/null
> > >>> +++ b/drivers/fpga/dfl-uio-pdev.c
> > >>> @@ -0,0 +1,91 @@
> > >>> +// SPDX-License-Identifier: GPL-2.0
> > >>> +/*
> > >>> + * DFL driver for Userspace I/O platform devices
> > >>> + *
> > >>> + * Copyright (C) 2020 Intel Corporation, Inc.
> > >>> + */
> > >>> +#include <linux/dfl.h>
> > >>> +#include <linux/errno.h>
> > >>> +#include <linux/kernel.h>
> > >>> +#include <linux/module.h>
> > >>> +#include <linux/platform_device.h>
> > >>> +#include <linux/slab.h>
> > >>> +#include <linux/uio_driver.h>
> > >>> +
> > >>> +#define DRIVER_NAME "dfl-uio-pdev"
> > >>> +
> > >>> +static int dfl_uio_pdev_probe(struct dfl_device *ddev)
> > >>> +{
> > >>> +	struct platform_device_info pdevinfo = { 0 };
> > >>> +	struct uio_info uio_pdata = { 0 };
> > >>> +	struct platform_device *uio_pdev;
> > >>> +	struct device *dev = &ddev->dev;
> > >>> +	unsigned int num_res = 1;
> > >>> +	struct resource res[2];
> > >>> +
> > >>> +	res[0].parent = &ddev->mmio_res;
> > >>> +	res[0].flags = IORESOURCE_MEM;
> > >>> +	res[0].start = ddev->mmio_res.start;
> > >>> +	res[0].end = ddev->mmio_res.end;
> > >>> +
> > >>> +	if (ddev->num_irqs) {
> > >>> +		if (ddev->num_irqs > 1)
> > >>> +			dev_warn(&ddev->dev,
> > >>> +				 "%d irqs for %s, but UIO only supports the first one\n",
> > >>> +				 ddev->num_irqs, dev_name(&ddev->dev));
> > >>> +
> > >>> +		res[1].flags = IORESOURCE_IRQ;
> > >>> +		res[1].start = ddev->irqs[0];
> > >>> +		res[1].end = ddev->irqs[0];
> > >>> +		num_res++;
> > >>> +	}
> > >>> +
> > >>> +	uio_pdata.name = DRIVER_NAME;
> > >>> +	uio_pdata.version = "0";
> > >>> +
> > >>> +	pdevinfo.name = "uio_pdrv_genirq";
> > >>> +	pdevinfo.res = res;
> > >>> +	pdevinfo.num_res = num_res;
> > >>> +	pdevinfo.parent = &ddev->dev;
> > >>> +	pdevinfo.id = PLATFORM_DEVID_AUTO;
> > >>> +	pdevinfo.data = &uio_pdata;
> > >>> +	pdevinfo.size_data = sizeof(uio_pdata);
> > >>> +
> > >>> +	uio_pdev = platform_device_register_full(&pdevinfo);
> > >>> +	if (!IS_ERR(uio_pdev))
> > >>> +		dev_set_drvdata(dev, uio_pdev);
> > >> I'm not sure if this is more readable than:
> > >>
> > >>    	uio_pdev = platform_device_register_full(&pdevinfo);
> > >>    	if (IS_ERR(uio_pdev))
> > >>    		return PTR_ERR(uio_pdev);
> > >>
> > >> 	dev_set_drvdata(dev, uio_pdev);
> > >> 	return 0;
> > >>
> > >> No strong preference, though ... :)
> > > I think your version is more readable, I'll change it.
> > >
> > >>> +
> > >>> +	return PTR_ERR_OR_ZERO(uio_pdev);
> > >>> +}
> > >>> +
> > >>> +static void dfl_uio_pdev_remove(struct dfl_device *ddev)
> > >>> +{
> > >>> +	struct platform_device *uio_pdev = dev_get_drvdata(&ddev->dev);
> > >>> +
> > >>> +	platform_device_unregister(uio_pdev);
> > >>> +}
> > >>> +
> > >>> +#define FME_FEATURE_ID_ETH_GROUP	0x10
> > >>> +
> > >>> +static const struct dfl_device_id dfl_uio_pdev_ids[] = {
> > >>> +	{ FME_ID, FME_FEATURE_ID_ETH_GROUP },
> > >> Will you want to always bind FME_FEATURE_ID_ETH_GROUP? If not I'd suggest not
> > >> to add it here.
> > > Actually this is not the most preferable to me. I'm always looking for a
> > > generic way to bind the uio driver to user assigned dfl devices. But there
> > > is concern that userspace should not be responsible for the device driver
> > > matching in previous mail thread:
> > >
> > > https://lore.kernel.org/linux-fpga/1602828151-24784-2-git-send-email-yilun.xu@intel.com/
> > >
> > > But TBH I still didn't figure out why driver_override is not OK in this
> > > case.
> > >
> > >> If you want to provide an option to somewhat non-ABI fixed bind things
> > >> you could look at what vfio-pci does (provide a module parameter),
> > >> otherwise use sysfs 'new_id' or 'bind'.
> > > I would like to have a "new_id" for dfl bus driver. It is not generic to
> > > all drivers, I need to add the attr for dfl drivers like pci do.
> > >
> > > My concern is how the module param or "new_id" is different from
> > > "driver_override", seems userspace is also taking part in the device
> > > matching.
> > >
> > > But since we've restarted the discussion, I'm very much willing to have
> > > a try on the "new_id".
> > 
> > I don't believe there is any problem with the basic platform uio driver.
> > 
> > Can we split it out and work the new_id change in parallel ?
> 
> Yes. The Ether Group is always bind to the uio driver for Intel PAC
> N3000. So this patch works properly now. We could have a separate patch
> for the new_id.
> 
> Thanks,
> Yilun

Sounds good, can you resend your series with the other changes
discussed?

Thanks
- Moritz