Message-ID: <BE8371DA886269458E0220A16DC1F8277E07031A@SHSMSX104.ccr.corp.intel.com>
Date:   Tue, 8 Aug 2017 18:25:37 +0000
From:   "Wu, Hao" <hao.wu@...el.com>
To:     Alan Tull <atull@...nel.org>
CC:     Rob Herring <robh+dt@...nel.org>, Moritz Fischer <mdf@...nel.org>,
        "linux-fpga@...r.kernel.org" <linux-fpga@...r.kernel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        "Kang, Luwei" <luwei.kang@...el.com>,
        "Zhang, Yi Z" <yi.z.zhang@...el.com>
Subject: RE: [PATCH RFC] fpga: add FPGA Bus device framework

> On Thu, Aug 3, 2017 at 2:53 AM, Wu Hao <hao.wu@...el.com> 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@...el.com> 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

Hi Alan

I am a little confused on this.

It seems that we could not have multiple fpga-br/region/mgr under one device.
As in patch set2, intel-fpga-fme.0 creates platform devices as children, and register
fpga-bridges/regions/mgr under these children platform devices. This is why 3 new
platform device driver introduced in this patch set 2 to match with those new created
children platform devices.

So it is something like this
/sys/bus/fpga/devices/fpga.0/intel-fpga-fme.0/intel-fpga-fme-region.0/fpga_region/region0
/sys/bus/fpga/devices/fpga.0/intel-fpga-fme.0/intel-fpga-fme-region.1/fpga_region/region1
/sys/bus/fpga/devices/fpga.0/intel-fpga-fme.0/intel-fpga-fme-br.0/fpga_bridge/br1
/sys/bus/fpga/devices/fpga.0/intel-fpga-fme.0/intel-fpga-fme-br.1/fpga_region/br2
/sys/bus/fpga/devices/fpga.1
/sys/bus/fpga/devices/fpga.2 

br0 should be the base bridge. fpga.1 and 2 are the child fpga bus device of fpga bridge.

> 
> /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).

Do you mean that check all fpga-dev.x folder to see if anyone has FME?

Then it is still not friendly to user space, as we may have a lot of bridges
(and regions) on one system. 

And looks like no big difference that we reuse base fpga-region as
container. Search all regionx in /sys/class/fpga_region/ to see if anyone
has a FME.

How do you think? : )

Hao

> 
> > 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@...el.com>
> >> > ---
> >> >  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@...el.com>
> >> > +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@...r.kernel.org
> >> 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@...r.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists