Re: [PATCH RFC] fpga: add FPGA Bus device framework

From: Alan Tull
Date: Mon Aug 07 2017 - 12:05:37 EST

Next message: SF Markus Elfring: "[PATCH] z2ram: Delete four error messages for a failed memory allocation in z2_open()"
Previous message: Kishon Vijay Abraham I: "[RFC PATCH 3/7] mmc: sdhci: Add callback to set bus mode"
In reply to: Wu Hao: "Re: [PATCH RFC] fpga: add FPGA Bus device framework"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Thu, Aug 3, 2017 at 2:53 AM, Wu Hao <hao.wu@xxxxxxxxx> wrote:
> On Wed, Aug 02, 2017 at 04:16:32PM -0500, Alan Tull wrote:
>> On Wed, Aug 2, 2017 at 7:19 AM, Wu Hao <hao.wu@xxxxxxxxx> wrote:
>> > This patch is a RFC patch which replaces the patch[1] which
>> > creates 'fpga-dev' class as container device. It introduces
>> > a 'fpga' bus type, and provides interfaces to create/destroy
>> > fpga bus devices. This fpga bus device only could be used as
>> > a container device, and no drivers needed for it.
>> >
>> > There is no interface change, so this patch could be used
>> > together with other patches of the original patch set[2].
>> >
>>
>> I am wondering whether this could be added to fpga-bridge.c so that
>> fpga-bridge becomes the fpga bus and fpga bus devices are under it.
>> The reasons for doing this are discussed in the other thread.
>>
>> > This following APIs are provided by FPGA Bus device framework:
>> > * fpga_dev_create
>> > Create fpga bus device under the given parent device.
>> > * fpga_dev_destroy
>> > Destroy fpga bus device
>>
>> This is being used in such that each fpga-dev is a container for
>> platform devices rather than fpga devices. That's not what I was
>> expecting. :)
>
> Hi Alan
>
> So does that mean in Intel FPGA PCIe driver, it needs to create
> a fpga-bridge (as base bridge?),

Yes

> and this fpga-bridge should register
> a fpga-bus and have a fpga bus device as its child, after that we can
> use this fpga bus device as container device,

Could the bus code be added to fpga-bridge? Then the base bridge is
the container device. A fpga-region would be under that and the AFU
and FME fpga devices would be under it.

> and create sub feature
> devices (e.g AFU and FME platform device)

We're talking about adding a new bus to the kernel here, not platform bus.

> under it, and user application
> could locate it in /sys/bus/fpga/devices/. Is my understanding correct? :)

So sysfs may end up something like this in your case:

/sys/bus/fpga/devices/fpga.0/intel-fpga-fme.0
/sys/bus/fpga/devices/fpga.0/intel-fpga-port.0
/sys/bus/fpga/devices/fpga.0/intel-fpga-port.1
/sys/bus/fpga/devices/fpga.0/fpga-mgr0
/sys/bus/fpga/devices/fpga.0/fpga-br1
/sys/bus/fpga/devices/fpga.0/fpga-br2

/sys/bus/fpga/devices/fpga.1/fpga-region1
/sys/bus/fpga/devices/fpga.2/fpga-region2

/sys/bus/fpga/devices/fpga.3/intel-fpga-fme.1
/sys/bus/fpga/devices/fpga.3/intel-fpga-port.2
/sys/bus/fpga/devices/fpga.3/intel-fpga-port.3
/sys/bus/fpga/devices/fpga.0/fpga-mgr1
/sys/bus/fpga/devices/fpga.3/fpga-br4
/sys/bus/fpga/devices/fpga.3/fpga-br5

/sys/bus/fpga/devices/fpga.4/fpga-region4
/sys/bus/fpga/devices/fpga.5/fpga-region5

fpga-br0 and 3 are base bridges (on top of PCIe) which show up as
fpga.0 and 3. Regions 0 and 3 are base regions.

fpga.0 and fpga.3 correspond to the real FPGA devices.

>
> And if we have second level fpga-bridge for PR regions, then they
> should register fpga-bus and fpga bus type device as child too?
> If yes, then we need a method for user application to distinguish
> which one represents the FPGA device in /sys/bus/fpga/devices/, right?

To find a bus that is a fpga, userspace only needs to look for busses
that have an FME (or a mgr).

> It seems to be a similar case that we see a lot of regions in
> /sys/class/fpga_region/ but not sure which one is the base region. :)

I understand that the goal of fpga-dev was to describe the topology
(which is the function of a bus, not a class as Rob explained). To be
honest, I'm still pondering the implications of adding a fpga bus.

Alan

>
>>
>> Alan
>>
>> >
>> > The following sysfs files are created:
>> > * /sys/bus/fpga/devices/<fpga.x>/name
>> > Name of the fpga bus device.
>> >
>> > [1] http://marc.info/?l=linux-fpga&m=149844237209829&w=2
>> > [2] http://marc.info/?l=linux-fpga&m=149844232609819&w=2
>> >
>> > Signed-off-by: Wu Hao <hao.wu@xxxxxxxxx>
>> > ---
>> > Documentation/ABI/testing/sysfs-bus-fpga | 5 ++
>> > drivers/fpga/Kconfig | 6 ++
>> > drivers/fpga/Makefile | 3 +
>> > drivers/fpga/fpga-dev.c | 132 +++++++++++++++++++++++++++++++
>> > include/linux/fpga/fpga-dev.h | 31 ++++++++
>> > 5 files changed, 177 insertions(+)
>> > create mode 100644 Documentation/ABI/testing/sysfs-bus-fpga
>> > create mode 100644 drivers/fpga/fpga-dev.c
>> > create mode 100644 include/linux/fpga/fpga-dev.h
>> >
>> > diff --git a/Documentation/ABI/testing/sysfs-bus-fpga b/Documentation/ABI/testing/sysfs-bus-fpga
>> > new file mode 100644
>> > index 0000000..414e946
>> > --- /dev/null
>> > +++ b/Documentation/ABI/testing/sysfs-bus-fpga
>> > @@ -0,0 +1,5 @@
>> > +What: /sys/bus/fpga/devices/<fpga.x>/name
>> > +Date: August 2017
>> > +KernelVersion: 4.13
>> > +Contact: Wu Hao <hao.wu@xxxxxxxxx>
>> > +Description: Name of FPGA bus device
>> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
>> > index d89eb52..b97a90e 100644
>> > --- a/drivers/fpga/Kconfig
>> > +++ b/drivers/fpga/Kconfig
>> > @@ -12,6 +12,12 @@ config FPGA
>> > manager drivers.
>> >
>> > if FPGA
>> > +config FPGA_DEVICE
>> > + tristate "FPGA Bus Device Framework"
>> > + help
>> > + Say Y here if you want support for FPGA Bus devices from the
>> > + kernel. The FPGA Bus Device Framework adds a FPGA Bus type and
>> > + provide interfaces to create FPGA devices on the bus.
>> >
>> > config FPGA_REGION
>> > tristate "FPGA Region"
>> > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
>> > index 08fc728..3eb254e 100644
>> > --- a/drivers/fpga/Makefile
>> > +++ b/drivers/fpga/Makefile
>> > @@ -5,6 +5,9 @@
>> > # Core FPGA Manager Framework
>> > obj-$(CONFIG_FPGA) += fpga-mgr.o
>> >
>> > +# FPGA Bus Device Framework
>> > +obj-$(CONFIG_FPGA_DEVICE) += fpga-dev.o
>> > +
>> > # FPGA Manager Drivers
>> > obj-$(CONFIG_FPGA_MGR_ICE40_SPI) += ice40-spi.o
>> > obj-$(CONFIG_FPGA_MGR_SOCFPGA) += socfpga.o
>> > diff --git a/drivers/fpga/fpga-dev.c b/drivers/fpga/fpga-dev.c
>> > new file mode 100644
>> > index 0000000..ddf67bb
>> > --- /dev/null
>> > +++ b/drivers/fpga/fpga-dev.c
>> > @@ -0,0 +1,132 @@
>> > +/*
>> > + * FPGA Bus Device Framework Driver
>> > + *
>> > + * Copyright (C) 2017 Intel Corporation, Inc.
>> > + *
>> > + * This work is licensed under the terms of the GNU GPL version 2. See
>> > + * the COPYING file in the top-level directory.
>> > + */
>> > +#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 bool is_bus_registered;
>>
>> I wouldn't think init will be called more than once. Did you run into
>> a problem where it did?
>
> It's used to prevent bus device creation if bus_register function
> returns error.
>
> Thanks
> Hao
>
>>
>> > +
>> > +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 const struct device_type fpga_dev_type = {
>> > + .name = "fpga_dev",
>> > + .release = fpga_dev_release,
>> > +};
>> > +
>> > +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);
>> > +
>> > +static struct attribute *fpga_dev_attrs[] = {
>> > + &dev_attr_name.attr,
>> > + NULL,
>> > +};
>> > +ATTRIBUTE_GROUPS(fpga_dev);
>> > +
>> > +static struct bus_type fpga_bus_type = {
>> > + .name = "fpga",
>> > +};
>> > +
>> > +/**
>> > + * fpga_dev_create - create a fpga device on fpga bus
>> > + * @parent: parent device
>> > + * @name: fpga bus 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 (WARN_ON(!is_bus_registered))
>> > + return ERR_PTR(-ENODEV);
>> > +
>> > + 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.type = &fpga_dev_type;
>> > + fdev->dev.bus = &fpga_bus_type;
>> > + fdev->dev.groups = fpga_dev_groups;
>> > + 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 bus 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 int __init fpga_bus_init(void)
>> > +{
>> > + int ret;
>> > +
>> > + pr_info("FPGA Bus Device Framework\n");
>> > +
>> > + ret = bus_register(&fpga_bus_type);
>> > + if (ret)
>> > + return ret;
>> > +
>> > + is_bus_registered = true;
>> > + return 0;
>> > +}
>> > +
>> > +static void __exit fpga_bus_exit(void)
>> > +{
>> > + bus_unregister(&fpga_bus_type);
>> > +}
>> > +
>> > +MODULE_DESCRIPTION("FPGA Bus Device Framework");
>> > +MODULE_LICENSE("GPL v2");
>> > +
>> > +subsys_initcall(fpga_bus_init);
>> > +module_exit(fpga_bus_exit);
>> > diff --git a/include/linux/fpga/fpga-dev.h b/include/linux/fpga/fpga-dev.h
>> > new file mode 100644
>> > index 0000000..7f6deb4
>> > --- /dev/null
>> > +++ b/include/linux/fpga/fpga-dev.h
>> > @@ -0,0 +1,31 @@
>> > +/*
>> > + * FPGA Bus Device Framework driver Header
>> > + *
>> > + * Copyright (C) 2017 Intel Corporation, Inc.
>> > + *
>> > + * This work is licensed under the terms of the GNU GPL version 2. See
>> > + * the COPYING file in the top-level directory.
>> > + */
>> > +#ifndef _LINUX_FPGA_DEV_H
>> > +#define _LINUX_FPGA_DEV_H
>> > +
>> > +/**
>> > + * struct fpga_dev - fpga bus device structure
>> > + * @name: name of fpga bus device
>> > + * @dev: fpga bus device
>> > + */
>> > +struct fpga_dev {
>> > + const char *name;
>> > + struct device dev;
>> > +};
>> > +
>> > +#define to_fpga_dev(d) container_of(d, struct fpga_dev, dev)
>> > +
>> > +struct fpga_dev *fpga_dev_create(struct device *parent, const char *name);
>> > +
>> > +static inline void fpga_dev_destroy(struct fpga_dev *fdev)
>> > +{
>> > + device_unregister(&fdev->dev);
>> > +}
>> > +
>> > +#endif
>> > --
>> > 1.8.3.1
>> >
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
>> the body of a message to majordomo@xxxxxxxxxxxxxxx
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html

Next message: SF Markus Elfring: "[PATCH] z2ram: Delete four error messages for a failed memory allocation in z2_open()"
Previous message: Kishon Vijay Abraham I: "[RFC PATCH 3/7] mmc: sdhci: Add callback to set bus mode"
In reply to: Wu Hao: "Re: [PATCH RFC] fpga: add FPGA Bus device framework"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]