| Message ID | 1610502848-30345-2-git-send-email-yilun.xu@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | UIO support for dfl devices | expand |
Hi Xu, On Wed, Jan 13, 2021 at 09:54:07AM +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> > Acked-by: Wu Hao <hao.wu@intel.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. > v6: fix the res[] zero initialization issue. > improve the return code for probe(). > --- > drivers/fpga/Kconfig | 10 +++++ > drivers/fpga/Makefile | 1 + > drivers/fpga/dfl-uio-pdev.c | 93 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 104 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 Nit: Enable this to allow DFL drivers to be written in userspace. > + 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..12b47bf > --- /dev/null > +++ b/drivers/fpga/dfl-uio-pdev.c > @@ -0,0 +1,93 @@ > +// 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 resource res[2] = { { 0 } }; > + struct uio_info uio_pdata = { 0 }; > + struct platform_device *uio_pdev; > + struct device *dev = &ddev->dev; > + unsigned int num_res = 1; > + > + 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); It looks like: platform_device_register_resndata(&ddev->dev, "uio_pdrv_genirq", PLATFORM_DEVID_AUTO, res, num_res, &uio_pdata, sizeof(uio_pdata)) would work? > + if (IS_ERR(uio_pdev)) > + return PTR_ERR(uio_pdev); > + > + dev_set_drvdata(dev, uio_pdev); > + > + return 0; > +} > + > +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 */ This is fairly common, we can maybe drop this comment? > + > + { } > +}; > +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 > Otherwise, looks good to me. - Moritz
On Wed, Jan 13, 2021 at 09:54:07AM +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. Why doesn't the existing uio driver work for this, why do you need a new one? > --- > drivers/fpga/Kconfig | 10 +++++ > drivers/fpga/Makefile | 1 + > drivers/fpga/dfl-uio-pdev.c | 93 +++++++++++++++++++++++++++++++++++++++++++++ uio drivers traditionally go in drivers/uio/ and start with "uio", so shouldn't this be drivers/uio/uio_dfl_pdev.c to match the same naming scheme? But again, you need to explain in detail, why the existing uio driver doesn't work properly, or why you can't just add a few lines to an existing one. thanks, greg k-h
Greg, On Sun, Jan 17, 2021 at 04:45:04PM +0100, Greg KH wrote: > On Wed, Jan 13, 2021 at 09:54:07AM +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. > > Why doesn't the existing uio driver work for this, why do you need a new > one? > > > --- > > drivers/fpga/Kconfig | 10 +++++ > > drivers/fpga/Makefile | 1 + > > drivers/fpga/dfl-uio-pdev.c | 93 +++++++++++++++++++++++++++++++++++++++++++++ > > uio drivers traditionally go in drivers/uio/ and start with "uio", so > shouldn't this be drivers/uio/uio_dfl_pdev.c to match the same naming > scheme? I had considered suggesting that, but ultimately this driver only creates a 'uio_pdrv_genirq' platform device, so it didn't seem like a good fit. > > But again, you need to explain in detail, why the existing uio driver > doesn't work properly, or why you can't just add a few lines to an > existing one. Ultimately there are three options I see: 1) Do what Xu does, which is re-use the 'uio_pdrv_genirq' uio driver by creating a platform device for it as sub-device of the dfl device that we bind to uio_pdrv_genirq 2) Add a module_dfl_driver part to drivers/uio/uio_pdrv_genirq.c and corresponding id table 3) Create a new uio_dfl_genirq kind of driver that uses the dfl bus and that would make sense to then put into drivers/uio. (This would duplicate code in uio_pdrv_genirq to some extend) Overall I think in terms of code re-use I think Xu's choice might be less new code as it simply wraps the uio platform device driver, and allows for defining the resources passed to the UIO driver to be defined by hardware through a DFL. I've seen the pattern that Xu proposed used in other places like the macb network driver where you'd have macb_main (the platform driver) and macb_pci that wraps it for a pci usage. - Moritz
On Sat, Jan 16, 2021 at 07:56:08PM -0800, Moritz Fischer wrote: > Hi Xu, > > On Wed, Jan 13, 2021 at 09:54:07AM +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> > > Acked-by: Wu Hao <hao.wu@intel.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. > > v6: fix the res[] zero initialization issue. > > improve the return code for probe(). > > --- > > drivers/fpga/Kconfig | 10 +++++ > > drivers/fpga/Makefile | 1 + > > drivers/fpga/dfl-uio-pdev.c | 93 +++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 104 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 > Nit: Enable this to allow DFL drivers to be written in userspace. Yes, will fix 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..12b47bf > > --- /dev/null > > +++ b/drivers/fpga/dfl-uio-pdev.c > > @@ -0,0 +1,93 @@ > > +// 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 resource res[2] = { { 0 } }; > > + struct uio_info uio_pdata = { 0 }; > > + struct platform_device *uio_pdev; > > + struct device *dev = &ddev->dev; > > + unsigned int num_res = 1; > > + > > + 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); > It looks like: > platform_device_register_resndata(&ddev->dev, "uio_pdrv_genirq", > PLATFORM_DEVID_AUTO, res, num_res, > &uio_pdata, sizeof(uio_pdata)) > > would work? It works. I'll change it. > > > > + if (IS_ERR(uio_pdev)) > > + return PTR_ERR(uio_pdev); > > + > > + dev_set_drvdata(dev, uio_pdev); > > + > > + return 0; > > +} > > + > > +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 */ > This is fairly common, we can maybe drop this comment? Yes. Thanks, Yilun
On 1/17/21 8:22 AM, Moritz Fischer wrote: > Greg, > > On Sun, Jan 17, 2021 at 04:45:04PM +0100, Greg KH wrote: >> On Wed, Jan 13, 2021 at 09:54:07AM +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. >> Why doesn't the existing uio driver work for this, why do you need a new >> one? >> >>> --- >>> drivers/fpga/Kconfig | 10 +++++ >>> drivers/fpga/Makefile | 1 + >>> drivers/fpga/dfl-uio-pdev.c | 93 +++++++++++++++++++++++++++++++++++++++++++++ >> uio drivers traditionally go in drivers/uio/ and start with "uio", so >> shouldn't this be drivers/uio/uio_dfl_pdev.c to match the same naming >> scheme? > I had considered suggesting that, but ultimately this driver only > creates a 'uio_pdrv_genirq' platform device, so it didn't seem like a > good fit. >> But again, you need to explain in detail, why the existing uio driver >> doesn't work properly, or why you can't just add a few lines to an >> existing one. > Ultimately there are three options I see: > 1) Do what Xu does, which is re-use the 'uio_pdrv_genirq' uio driver by > creating a platform device for it as sub-device of the dfl device that > we bind to uio_pdrv_genirq > 2) Add a module_dfl_driver part to drivers/uio/uio_pdrv_genirq.c and > corresponding id table > 3) Create a new uio_dfl_genirq kind of driver that uses the dfl bus and > that would make sense to then put into drivers/uio. (This would > duplicate code in uio_pdrv_genirq to some extend) > > Overall I think in terms of code re-use I think Xu's choice might be > less new code as it simply wraps the uio platform device driver, and > allows for defining the resources passed to the UIO driver to be defined > by hardware through a DFL. > > I've seen the pattern that Xu proposed used in other places like the > macb network driver where you'd have macb_main (the platform driver) and > macb_pci that wraps it for a pci usage. > > - Moritz Thinking of this problem more generally. Every fpga will have a handful of sub devices. Do we want to carry them in the fpga subsystem or carry them in the other subsystems ? Consider the short term reviewing and long term maintenance of the sub devices by the subsystem maintainers. It easier for them if the sub devices are in the other subsystems. Applying this to specifically for dfl_uio. No one from the uio subsystem reviewing this change is a problem. I think this change needs to go to the uio subsystem. And a new entry in the MAINTAINERS file to keep the fpga subsystem in the loop for reviews and acks Tom
Hi Tom, On Thu, Jan 21, 2021 at 06:30:20AM -0800, Tom Rix wrote: > > On 1/17/21 8:22 AM, Moritz Fischer wrote: > > Greg, > > > > On Sun, Jan 17, 2021 at 04:45:04PM +0100, Greg KH wrote: > >> On Wed, Jan 13, 2021 at 09:54:07AM +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. > >> Why doesn't the existing uio driver work for this, why do you need a new > >> one? > >> > >>> --- > >>> drivers/fpga/Kconfig | 10 +++++ > >>> drivers/fpga/Makefile | 1 + > >>> drivers/fpga/dfl-uio-pdev.c | 93 +++++++++++++++++++++++++++++++++++++++++++++ > >> uio drivers traditionally go in drivers/uio/ and start with "uio", so > >> shouldn't this be drivers/uio/uio_dfl_pdev.c to match the same naming > >> scheme? > > I had considered suggesting that, but ultimately this driver only > > creates a 'uio_pdrv_genirq' platform device, so it didn't seem like a > > good fit. > >> But again, you need to explain in detail, why the existing uio driver > >> doesn't work properly, or why you can't just add a few lines to an > >> existing one. > > Ultimately there are three options I see: > > 1) Do what Xu does, which is re-use the 'uio_pdrv_genirq' uio driver by > > creating a platform device for it as sub-device of the dfl device that > > we bind to uio_pdrv_genirq > > 2) Add a module_dfl_driver part to drivers/uio/uio_pdrv_genirq.c and > > corresponding id table > > 3) Create a new uio_dfl_genirq kind of driver that uses the dfl bus and > > that would make sense to then put into drivers/uio. (This would > > duplicate code in uio_pdrv_genirq to some extend) > > > > Overall I think in terms of code re-use I think Xu's choice might be > > less new code as it simply wraps the uio platform device driver, and > > allows for defining the resources passed to the UIO driver to be defined > > by hardware through a DFL. > > > > I've seen the pattern that Xu proposed used in other places like the > > macb network driver where you'd have macb_main (the platform driver) and > > macb_pci that wraps it for a pci usage. > > > > - Moritz > > Thinking of this problem more generally. > > Every fpga will have a handful of sub devices. > > Do we want to carry them in the fpga subsystem or carry them in the other subsystems ? Yeah no we really don't. I think that was the point of the whole DFL bus :) > > Consider the short term reviewing and long term maintenance of the sub devices by the subsystem maintainers. > > It easier for them if the sub devices are in the other subsystems. Agreed. > > > Applying this to specifically for dfl_uio. > > No one from the uio subsystem reviewing this change is a problem. Greg will. > I think this change needs to go to the uio subsystem. Yeah I've thought about this some for the last few days, maybe it's easier that way. Tbh, there's so little code here even if we went with option 3 above it's probably fairly short. It would set a better prcedent. Xu, how do you feel about giving that a spin? See if option 3 will be way more code. - Moritz
On Thu, Jan 21, 2021 at 12:03:47PM -0800, Moritz Fischer wrote: > Hi Tom, > > On Thu, Jan 21, 2021 at 06:30:20AM -0800, Tom Rix wrote: > > > > On 1/17/21 8:22 AM, Moritz Fischer wrote: > > > Greg, > > > > > > On Sun, Jan 17, 2021 at 04:45:04PM +0100, Greg KH wrote: > > >> On Wed, Jan 13, 2021 at 09:54:07AM +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. > > >> Why doesn't the existing uio driver work for this, why do you need a new > > >> one? > > >> > > >>> --- > > >>> drivers/fpga/Kconfig | 10 +++++ > > >>> drivers/fpga/Makefile | 1 + > > >>> drivers/fpga/dfl-uio-pdev.c | 93 +++++++++++++++++++++++++++++++++++++++++++++ > > >> uio drivers traditionally go in drivers/uio/ and start with "uio", so > > >> shouldn't this be drivers/uio/uio_dfl_pdev.c to match the same naming > > >> scheme? > > > I had considered suggesting that, but ultimately this driver only > > > creates a 'uio_pdrv_genirq' platform device, so it didn't seem like a > > > good fit. > > >> But again, you need to explain in detail, why the existing uio driver > > >> doesn't work properly, or why you can't just add a few lines to an > > >> existing one. > > > Ultimately there are three options I see: > > > 1) Do what Xu does, which is re-use the 'uio_pdrv_genirq' uio driver by > > > creating a platform device for it as sub-device of the dfl device that > > > we bind to uio_pdrv_genirq > > > 2) Add a module_dfl_driver part to drivers/uio/uio_pdrv_genirq.c and > > > corresponding id table > > > 3) Create a new uio_dfl_genirq kind of driver that uses the dfl bus and > > > that would make sense to then put into drivers/uio. (This would > > > duplicate code in uio_pdrv_genirq to some extend) > > > > > > Overall I think in terms of code re-use I think Xu's choice might be > > > less new code as it simply wraps the uio platform device driver, and > > > allows for defining the resources passed to the UIO driver to be defined > > > by hardware through a DFL. > > > > > > I've seen the pattern that Xu proposed used in other places like the > > > macb network driver where you'd have macb_main (the platform driver) and > > > macb_pci that wraps it for a pci usage. > > > > > > - Moritz > > > > Thinking of this problem more generally. > > > > Every fpga will have a handful of sub devices. > > > > Do we want to carry them in the fpga subsystem or carry them in the other subsystems ? > > Yeah no we really don't. I think that was the point of the whole DFL > bus :) > > > > Consider the short term reviewing and long term maintenance of the sub devices by the subsystem maintainers. > > > > It easier for them if the sub devices are in the other subsystems. > > Agreed. > > > > > > Applying this to specifically for dfl_uio. > > > > No one from the uio subsystem reviewing this change is a problem. > > Greg will. > > I think this change needs to go to the uio subsystem. > > Yeah I've thought about this some for the last few days, maybe it's > easier that way. > > Tbh, there's so little code here even if we went with option 3 above > it's probably fairly short. It would set a better prcedent. > > Xu, how do you feel about giving that a spin? See if option 3 will be > way more code. Yes, I'll try to put it to drivers/uio. I see the implementation in vfio_platform.c/vfio_amba.c/vfio_platform_common.c. I'm wondering if we could handle it in that way. Thanks, yilun
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..12b47bf --- /dev/null +++ b/drivers/fpga/dfl-uio-pdev.c @@ -0,0 +1,93 @@ +// 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 resource res[2] = { { 0 } }; + struct uio_info uio_pdata = { 0 }; + struct platform_device *uio_pdev; + struct device *dev = &ddev->dev; + unsigned int num_res = 1; + + 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)) + return PTR_ERR(uio_pdev); + + dev_set_drvdata(dev, uio_pdev); + + return 0; +} + +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");