> On Thu, Mar 30, 2017 at 08:08:02PM +0800, Wu Hao wrote:
> > During FPGA device (e.g PCI-based) discovery, platform devices are
> > registered for different FPGA function units. But the device node path
> > isn't quite friendly to applications.
> >
> > Consider this case, applications want to access child device's sysfs
> > file for some information.
> >
> > 1) Access using bus-based path (e.g PCI)
> >
> > /sys/bus/pci/devices/xxxxx/fpga_func_a.0/sysfs_file
> >
> > From the path, it's clear which PCI device is the parent, but not perfect
> > solution for applications. PCI device BDF is not fixed, application may
> > need to search all PCI device to find the actual FPGA Device.
> >
> > 2) Or access using platform device path
> >
> > /sys/bus/platform/devices/fpga_func_a.0/sysfs_file
> >
> > Applications find the actual function by name easily, but no information
> > about which fpga device it belongs to. It's quite confusing if multiple
> > FPGA devices are in one system.
> >
> > 'FPGA Device' class is introduced to resolve this problem. Each node
> > under this class represents a fpga device, which may have one or more
> > child devices. Applications only need to search under this FPGA Device
> > class folder to find the child device node it needs.
> >
> > For example, for the platform has 2 fpga devices, each fpga device has
> > 3 child devices, the hierarchy looks like this.
> >
> > Two nodes are under /sys/class/fpga/:
> > /sys/class/fpga/fpga.0
> > /sys/class/fpga/fpga.1
> >
> > Each node has 1 function A device and 2 function B devices:
> > /sys/class/fpga/fpga.0/func_a.0
> > /sys/class/fpga/fpga.0/func_b.0
> > /sys/class/fpga/fpga.0/func_b.1
> >
> > /sys/class/fpga/fpga.1/func_a.1
> > /sys/class/fpga/fpga.1/func_b.2
> > /sys/class/fpga/fpga.1/func_b.3
> >
> > This following APIs are provided by FPGA device framework:
> > * fpga_dev_create
> > Create fpga device under the given parent device.
> > * fpga_dev_destroy
> > Destroy fpga device
> >
> > The following sysfs files are created:
> > * /sys/class/fpga/<fpga.x>/name
> > Name of the fpga device.
> >
> > Signed-off-by: Tim Whisonant <tim.whison...@intel.com>
> > Signed-off-by: Enno Luebbers <enno.luebb...@intel.com>
> > Signed-off-by: Shiva Rao <shiva....@intel.com>
> > Signed-off-by: Christopher Rauer <christopher.ra...@intel.com>
> > Signed-off-by: Xiao Guangrong <guangrong.x...@linux.intel.com>
> > Signed-off-by: Wu Hao <hao...@intel.com>
> > ---
> > drivers/fpga/Kconfig | 6 +++
> > drivers/fpga/Makefile | 3 ++
> > drivers/fpga/fpga-dev.c | 120
> > ++++++++++++++++++++++++++++++++++++++++++
> > include/linux/fpga/fpga-dev.h | 34 ++++++++++++
> > 4 files changed, 163 insertions(+)
> > create mode 100644 drivers/fpga/fpga-dev.c create mode 100644
> > include/linux/fpga/fpga-dev.h
> >
> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig index
> > ce861a2..d99b640 100644
> > --- a/drivers/fpga/Kconfig
> > +++ b/drivers/fpga/Kconfig
> > @@ -12,6 +12,12 @@ config FPGA
> > manager drivers.
> >
> > if FPGA
> > +config FPGA_DEVICE
> > + tristate "FPGA Device Framework"
> > + help
> > + Say Y here if you want support for FPGA Devices from the kernel.
> > + The FPGA Device Framework adds a FPGA device class and provide
> > + interfaces to create FPGA devices.
> >
> > config FPGA_REGION
> > tristate "FPGA Region"
> > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile index
> > 8df07bc..53a41d2 100644
> > --- a/drivers/fpga/Makefile
> > +++ b/drivers/fpga/Makefile
> > @@ -5,6 +5,9 @@
> > # Core FPGA Manager Framework
> > obj-$(CONFIG_FPGA) += fpga-mgr.o
> >
> > +# FPGA Device Framework
> > +obj-$(CONFIG_FPGA_DEVICE) += fpga-dev.o
> > +
> > # FPGA Manager Drivers
> > obj-$(CONFIG_FPGA_MGR_SOCFPGA) += socfpga.o
> > obj-$(CONFIG_FPGA_MGR_SOCFPGA_A10) += socfpga-a10.o
> > diff --git a/drivers/fpga/fpga-dev.c b/drivers/fpga/fpga-dev.c new
> > file mode 100644 index 0000000..0f4c0ed
> > --- /dev/null
> > +++ b/drivers/fpga/fpga-dev.c
> > @@ -0,0 +1,120 @@
> > +/*
> > + * FPGA Device Framework Driver
> > + *
> > + * Copyright (C) 2017 Intel Corporation, Inc.
> > + *
> > + * This work is licensed under a dual BSD/GPLv2 license. When using
> > +or
> > + * redistributing this file, you may do so under either license. See
> > +the
> > + * LICENSE.BSD file under drivers/fpga/intel for the BSD license and
> > +see
> > + * the COPYING file in the top-level directory for the GPLv2 license.
>
> Really? A BSD licened file that does EXPORT_SYMBOL_GPL and interacts
> directly with the driver core? Please go talk to some of your lawyers about
> this before you resubmit this correctly...
Sorry, will check and fix this in next version.
>
>
> > + */
> > +#include <linux/device.h>
> > +#include <linux/module.h>
> > +#include <linux/slab.h>
> > +#include <linux/fpga/fpga-dev.h>
> > +
> > +static DEFINE_IDA(fpga_dev_ida);
> > +static struct class *fpga_dev_class;
> > +
> > +static ssize_t name_show(struct device *dev,
> > + struct device_attribute *attr, char *buf) {
> > + struct fpga_dev *fdev = to_fpga_dev(dev);
> > +
> > + return sprintf(buf, "%s\n", fdev->name); } static
> > +DEVICE_ATTR_RO(name);
>
> There already is a name for the device, it's the directory name.
For current implementation, the directory will have a common name like
/sys/class/fpga/fpga.0
/sys/class/fpga/fpga.1
/sys/class/fpga/fpga.2
...
For the 'name' sysfs interface, driver can put more device specific info
into this 'name', e.g intel-fpga-dev. Userspace can use this information
to know which kind of FPGA device it is. e.g if applications read the
/sys/class/fpga/fpga.5/name as intel-fpga-dev, it means the 5th fpga
device on the system is a Intel FPGA device, and then application applies
related method to enumerate the accelerators for it.
And other existing fpga class has similar sysfs interface too, so I would
like to keep it aligned with others.
>
> > +
> > +static struct attribute *fpga_dev_attrs[] = {
> > + &dev_attr_name.attr,
> > + NULL,
> > +};
> > +ATTRIBUTE_GROUPS(fpga_dev);
> > +
> > +/**
> > + * fpga_dev_create - create a fpga device
> > + * @parent: parent device
> > + * @name: fpga device name
> > + *
> > + * Return fpga_dev struct for success, error code otherwise.
> > + */
> > +struct fpga_dev *fpga_dev_create(struct device *parent, const char
> > +*name) {
> > + struct fpga_dev *fdev;
> > + int id, ret = 0;
> > +
> > + if (!name || !strlen(name)) {
> > + dev_err(parent, "Attempt to register with no name!\n");
> > + return ERR_PTR(-EINVAL);
> > + }
> > +
> > + fdev = kzalloc(sizeof(*fdev), GFP_KERNEL);
> > + if (!fdev)
> > + return ERR_PTR(-ENOMEM);
> > +
> > + id = ida_simple_get(&fpga_dev_ida, 0, 0, GFP_KERNEL);
> > + if (id < 0) {
> > + ret = id;
> > + goto error_kfree;
> > + }
> > +
> > + fdev->name = name;
> > +
> > + device_initialize(&fdev->dev);
> > + fdev->dev.class = fpga_dev_class;
> > + fdev->dev.parent = parent;
> > + fdev->dev.id = id;
> > +
> > + ret = dev_set_name(&fdev->dev, "fpga.%d", id);
> > + if (ret)
> > + goto error_device;
> > +
> > + ret = device_add(&fdev->dev);
> > + if (ret)
> > + goto error_device;
> > +
> > + dev_dbg(fdev->dev.parent, "fpga device [%s] created\n", fdev->name);
> > +
> > + return fdev;
> > +
> > +error_device:
> > + ida_simple_remove(&fpga_dev_ida, id);
> > +error_kfree:
> > + kfree(fdev);
> > +
> > + return ERR_PTR(ret);
> > +}
> > +EXPORT_SYMBOL_GPL(fpga_dev_create);
> > +
> > +static void fpga_dev_release(struct device *dev) {
> > + struct fpga_dev *fdev = to_fpga_dev(dev);
> > +
> > + ida_simple_remove(&fpga_dev_ida, fdev->dev.id);
> > + kfree(fdev);
> > +}
> > +
> > +static int __init fpga_dev_class_init(void) {
> > + pr_info("FPGA Device framework\n");
> > +
> > + fpga_dev_class = class_create(THIS_MODULE, "fpga");
> > + if (IS_ERR(fpga_dev_class))
> > + return PTR_ERR(fpga_dev_class);
> > +
> > + fpga_dev_class->dev_groups = fpga_dev_groups;
> > + fpga_dev_class->dev_release = fpga_dev_release;
> > +
> > + return 0;
> > +}
> > +
> > +static void __exit fpga_dev_class_exit(void) {
> > + class_destroy(fpga_dev_class);
> > +}
> > +
> > +MODULE_DESCRIPTION("FPGA Device framework"); MODULE_LICENSE("Dual
> > +BSD/GPL");
> > +
> > +subsys_initcall(fpga_dev_class_init);
> > +module_exit(fpga_dev_class_exit);
> > diff --git a/include/linux/fpga/fpga-dev.h
> > b/include/linux/fpga/fpga-dev.h new file mode 100644 index
> > 0000000..7b58356
> > --- /dev/null
> > +++ b/include/linux/fpga/fpga-dev.h
> > @@ -0,0 +1,34 @@
> > +/*
> > + * FPGA Device Driver Header
> > + *
> > + * Copyright (C) 2017 Intel Corporation, Inc.
> > + *
> > + * This work is licensed under a dual BSD/GPLv2 license. When using
> > +or
> > + * redistributing this file, you may do so under either license. See
> > +the
> > + * LICENSE.BSD file under drivers/fpga/intel for the BSD license and
> > +see
> > + * the COPYING file in the top-level directory for the GPLv2 license.
>
> Again with the dual license, please fix up.
>
Sorry, will fix this in the next version.
> > + *
> > + */
> > +#ifndef _LINUX_FPGA_DEV_H
> > +#define _LINUX_FPGA_DEV_H
> > +
> > +/**
> > + * struct fpga_dev - fpga device structure
> > + * @name: name of fpga device
> > + * @dev: fpga device
> > + */
> > +struct fpga_dev {
> > + const char *name;
> > + struct device dev;
>
> struct device already has a name, why duplicate it here?
As mentioned above, dev has common name as fpga.x, but 'name' could have
more meaningful information, e.g venodr and device information.
Thanks
Hao
>
> thanks,
>
> greg k-h
