From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752709AbdHHS0B (ORCPT ); Tue, 8 Aug 2017 14:26:01 -0400 Received: from mga11.intel.com ([192.55.52.93]:30602 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752265AbdHHSZl (ORCPT ); Tue, 8 Aug 2017 14:25:41 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.41,344,1498546800"; d="scan'208";a="887904022" From: "Wu, Hao" To: Alan Tull CC: Rob Herring , Moritz Fischer , "linux-fpga@vger.kernel.org" , linux-kernel , "Kang, Luwei" , "Zhang, Yi Z" Subject: RE: [PATCH RFC] fpga: add FPGA Bus device framework Thread-Topic: [PATCH RFC] fpga: add FPGA Bus device framework Thread-Index: AQHTC4qo2+f/GmRZrkO7bnSnyVf00qJ5FuzcgAGodlA= Date: Tue, 8 Aug 2017 18:25:37 +0000 Message-ID: References: <1501676385-32551-1-git-send-email-hao.wu@intel.com> <20170803075335.GA4120@hao-dev> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: dlp-product: dlpe-windows dlp-version: 10.0.102.7 dlp-reaction: no-action x-originating-ip: [10.239.127.40] Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by nfs id v78IQ8FC017995 > On Thu, Aug 3, 2017 at 2:53 AM, Wu Hao 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 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//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 > >> > --- > >> > 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//name > >> > +Date: August 2017 > >> > +KernelVersion: 4.13 > >> > +Contact: Wu Hao > >> > +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 > >> > +#include > >> > +#include > >> > +#include > >> > + > >> > +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@vger.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@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html