On Wed, 2013-09-18 at 17:56 +0200, Michal Simek wrote:
> This new subsystem should unify all fpga drivers which
> do the same things. Load configuration data to fpga
> or another programmable logic through common interface.
> It doesn't matter if it is MMIO device, gpio bitbanging,
> etc. connection. The point is to have the same
> inteface for these drivers.
Is this really a "driver".
Perhaps it's more of a core kernel function
and belongs in kernel.
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
The error messages are a little shouty with all
the unnecessary "!" uses.
[]
> +/**
> + * fpga_mgr_read - Read data from fpga
> + * @mgr: Pointer to the fpga manager structure
> + * @buf: Pointer to the buffer location
> + * @count: Pointer to the number of copied bytes
> + *
> + * Function reads fpga bitstream and copy them to output buffer.
> + *
> + * Returns the number of bytes copied to @buf, a negative error number otherwise
> + */
> +static int fpga_mgr_read(struct fpga_manager *mgr, char *buf, ssize_t *count)
> +{
> + int ret;
> +
> + if (test_and_set_bit_lock(FPGA_MGR_DEV_BUSY, &mgr->flags))
> + return -EBUSY;
> +
> + if (!mgr->mops || !mgr->mops->read) {
> + dev_err(mgr->dev,
> + "Controller doesn't support read operations!\n");
> + return -EPERM;
> + }
> +
> + if (mgr->mops->read_init) {
> + ret = mgr->mops->read_init(mgr);
> + if (ret) {
> + dev_err(mgr->dev, "Failed in read-init!\n");
> + return ret;
> + }
> + }
> +
> + if (mgr->mops->read) {
> + ret = mgr->mops->read(mgr, buf, count);
> + if (ret) {
> + dev_err(mgr->dev, "Failed to read firmware!\n");
> + return ret;
> + }
> + }
> +
> + if (mgr->mops->read_complete) {
> + ret = mgr->mops->read_complete(mgr);
> + if (ret) {
> + dev_err(mgr->dev, "Failed in read-complete!\n");
> + return ret;
> + }
> + }
> +
> + clear_bit_unlock(FPGA_MGR_DEV_BUSY, &mgr->flags);
> +
> + return 0;
> +}
> +
> +/**
> + * fpga_mgr_attr_read - Read data from fpga
> + * @dev: Pointer to the device structure
> + * @attr: Pointer to the device attribute structure
> + * @buf: Pointer to the buffer location
> + *
> + * Function reads fpga bitstream and copy them to output buffer
> + *
> + * Returns the number of bytes copied to @buf, a negative error number otherwise
> + */
> +static ssize_t fpga_mgr_attr_read(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct fpga_manager *mgr = dev_get_drvdata(dev);
> + ssize_t count;
> + int ret;
> +
> + if (mgr && mgr->fpga_read)
> + ret = mgr->fpga_read(mgr, buf, &count);
> +
> + return ret == 0 ? count : -EPERM;
EPERM isn't the only error return from fpga_read.
> +}
> +
> +/**
> + * fpga_mgr_write - Write data to fpga
> + * @mgr: Pointer to the fpga manager structure
> + * @fw_name: Pointer to the buffer location with bistream firmware filename
> + *
> + * @buf contains firmware filename which is loading through firmware
> + * interface and passed to the fpga driver.
> + *
> + * Returns string lenght added to @fw_name, a negative error number otherwise
> + */
> +static int fpga_mgr_write(struct fpga_manager *mgr, const char *fw_name)
> +{
> + int ret = 0;
> + const struct firmware *fw;
> +
> + if (!fw_name || !strlen(fw_name)) {
> + dev_err(mgr->dev, "Firmware name is not specified!\n");
> + return -EINVAL;
> + }
> +
> + if (!mgr->mops || !mgr->mops->write) {
> + dev_err(mgr->dev,
> + "Controller doesn't support write operations!\n");
> + return -EPERM;
I think you should revisit the return codes.
> +/**
> + * fpga_mgr_attr_write - Write data to fpga
> + * @dev: Pointer to the device structure
> + * @attr: Pointer to the device attribute structure
> + * @buf: Pointer to the buffer location with bistream firmware filename
> + * @count: Number of characters in @buf
> + *
> + * @buf contains firmware filename which is loading through firmware
> + * interface and passed to the fpga driver.
> + *
> + * Returns string lenght added to @buf, a negative error number otherwise
> + */
> +static ssize_t fpga_mgr_attr_write(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct fpga_manager *mgr = dev_get_drvdata(dev);
> + int ret;
> +
> + if (mgr && mgr->fpga_write)
> + ret = mgr->fpga_write(mgr, buf);
> +
> + return ret == 0 ? strlen(buf) : -EPERM;
> +}
Same -EPERM issue as read
> + mgr = devm_kzalloc(&pdev->dev, sizeof(*mgr), GFP_KERNEL);
> + if (!mgr) {
> + dev_err(&pdev->dev, "Failed to alloc mgr struct!\n");
Unnecessary OOM message as there's a general dump_stack()
already done on any OOM without GFP_NOWARN
> diff --git a/include/linux/fpga.h b/include/linux/fpga.h
[]
> +struct fpga_manager {
> + char name[48];
Maybe a #define instead of 48?
Hi Michal,
On Wed, 2013-09-18 at 17:56 +0200, Michal Simek wrote:
> This new subsystem should unify all fpga drivers which
> do the same things. Load configuration data to fpga
> or another programmable logic through common interface.
> It doesn't matter if it is MMIO device, gpio bitbanging,
> etc. connection. The point is to have the same
> inteface for these drivers.
>
> Signed-off-by: Michal Simek <[email protected]>
[CC] Alan Tull <[email protected]>
[CC] Yves Vandervennet <[email protected]>
Thanks for doing this. Myself and Alan Tull had plans to send this
upstream, but we wanted to make sure that this framework would also work
on the Xilinx platform as well. Have you had a chance to test this on a
SOCFGPA platform? If not, we can go do that.
>
> ---
> MAINTAINERS | 7 +
> drivers/Kconfig | 2 +
> drivers/Makefile | 1 +
> drivers/fpga/Kconfig | 18 ++
> drivers/fpga/Makefile | 5 +
> drivers/fpga/fpga-mgr.c | 433 ++++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/fpga.h | 105 ++++++++++++
> 7 files changed, 571 insertions(+)
> create mode 100644 drivers/fpga/Kconfig
> create mode 100644 drivers/fpga/Makefile
> create mode 100644 drivers/fpga/fpga-mgr.c
> create mode 100644 include/linux/fpga.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e61c2e8..5c7597b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3427,6 +3427,13 @@ F: include/linux/fmc*.h
> F: include/linux/ipmi-fru.h
> K: fmc_d.*register
>
> +FPGA SUBSYSTEM
> +M: Michal Simek <[email protected]>
Since the basic framework of this driver was done by Alan Tull and
myself, can you please add Alan and myself as the maintainer here?
> +T: git git://git.xilinx.com/linux-xlnx.git
> +S: Supported
> +F: drivers/fpga/
> +F: include/linux/fpga.h
> +
> FPU EMULATOR
> M: Bill Metzenthen <[email protected]>
> W: http://floatingpoint.sourceforge.net/emulator/index.html
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index aa43b91..17f3caa 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -166,4 +166,6 @@ source "drivers/reset/Kconfig"
>
> source "drivers/fmc/Kconfig"
>
> +source "drivers/fpga/Kconfig"
> +
> endmenu
> diff --git a/drivers/Makefile b/drivers/Makefile
> index ab93de8..2b5e73b 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -152,3 +152,4 @@ obj-$(CONFIG_VME_BUS) += vme/
> obj-$(CONFIG_IPACK_BUS) += ipack/
> obj-$(CONFIG_NTB) += ntb/
> obj-$(CONFIG_FMC) += fmc/
> +obj-$(CONFIG_FPGA) += fpga/
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> new file mode 100644
> index 0000000..5357645
> --- /dev/null
> +++ b/drivers/fpga/Kconfig
> @@ -0,0 +1,18 @@
> +#
> +# FPGA framework configuration
> +#
> +
> +menu "FPGA devices"
> +
> +config FPGA
> + tristate "FPGA Framework"
> + help
> + Say Y here if you want support for configuring FPGAs from the
> + kernel. The FPGA framework adds a FPGA manager class and FPGA
> + manager drivers.
> +
> +if FPGA
Not sure what this is for?
Thanks,
Dinh
> +
> +endif
> +
> +endmenu
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> new file mode 100644
> index 0000000..3c7f29b
> --- /dev/null
> +++ b/drivers/fpga/Makefile
> @@ -0,0 +1,5 @@
> +#
> +# Makefile for the fpga framework and fpga manager drivers.
> +#
> +
> +obj-$(CONFIG_FPGA) += fpga-mgr.o
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> new file mode 100644
> index 0000000..7312efd
> --- /dev/null
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -0,0 +1,433 @@
> +/*
> + * FPGA Framework
> + *
> + * Copyright (C) 2013 Xilinx, Inc.
> + *
> + * based on origin code from
> + * Copyright (C) 2013 Altera Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/firmware.h>
> +#include <linux/fpga.h>
> +#include <linux/fs.h>
> +#include <linux/idr.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +
> +static struct idr fpga_mgr_idr;
> +static spinlock_t fpga_mgr_idr_lock;
> +static struct class *fpga_mgr_class;
> +
> +/**
> + * fpga_mgr_name_show - Show fpga manager name
> + * @dev: Pointer to the device structure
> + * @attr: Pointer to the device attribute structure
> + * @buf: Pointer to the buffer location
> + *
> + * Returns the number of bytes copied to @buf, a negative error number otherwise
> + */
> +static ssize_t fpga_mgr_name_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct fpga_manager *mgr = dev_get_drvdata(dev);
> +
> + if (!mgr)
> + return -ENODEV;
> +
> + return snprintf(buf, sizeof(mgr->name), "%s\n", mgr->name);
> +}
> +
> +/**
> + * fpga_mgr_status_show - Show fpga manager status
> + * @dev: Pointer to the device structure
> + * @attr: Pointer to the device attribute structure
> + * @buf: Pointer to the buffer location
> + *
> + * Returns the number of bytes copied to @buf, a negative error number otherwise
> + */
> +static ssize_t fpga_mgr_status_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct fpga_manager *mgr = dev_get_drvdata(dev);
> +
> + if (!mgr || !mgr->mops || !mgr->mops->status)
> + return -ENODEV;
> +
> + return mgr->mops->status(mgr, buf);
> +}
> +
> +/**
> + * fpga_mgr_read - Read data from fpga
> + * @mgr: Pointer to the fpga manager structure
> + * @buf: Pointer to the buffer location
> + * @count: Pointer to the number of copied bytes
> + *
> + * Function reads fpga bitstream and copy them to output buffer.
> + *
> + * Returns the number of bytes copied to @buf, a negative error number otherwise
> + */
> +static int fpga_mgr_read(struct fpga_manager *mgr, char *buf, ssize_t *count)
> +{
> + int ret;
> +
> + if (test_and_set_bit_lock(FPGA_MGR_DEV_BUSY, &mgr->flags))
> + return -EBUSY;
> +
> + if (!mgr->mops || !mgr->mops->read) {
> + dev_err(mgr->dev,
> + "Controller doesn't support read operations!\n");
> + return -EPERM;
> + }
> +
> + if (mgr->mops->read_init) {
> + ret = mgr->mops->read_init(mgr);
> + if (ret) {
> + dev_err(mgr->dev, "Failed in read-init!\n");
> + return ret;
> + }
> + }
> +
> + if (mgr->mops->read) {
> + ret = mgr->mops->read(mgr, buf, count);
> + if (ret) {
> + dev_err(mgr->dev, "Failed to read firmware!\n");
> + return ret;
> + }
> + }
> +
> + if (mgr->mops->read_complete) {
> + ret = mgr->mops->read_complete(mgr);
> + if (ret) {
> + dev_err(mgr->dev, "Failed in read-complete!\n");
> + return ret;
> + }
> + }
> +
> + clear_bit_unlock(FPGA_MGR_DEV_BUSY, &mgr->flags);
> +
> + return 0;
> +}
> +
> +/**
> + * fpga_mgr_attr_read - Read data from fpga
> + * @dev: Pointer to the device structure
> + * @attr: Pointer to the device attribute structure
> + * @buf: Pointer to the buffer location
> + *
> + * Function reads fpga bitstream and copy them to output buffer
> + *
> + * Returns the number of bytes copied to @buf, a negative error number otherwise
> + */
> +static ssize_t fpga_mgr_attr_read(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct fpga_manager *mgr = dev_get_drvdata(dev);
> + ssize_t count;
> + int ret;
> +
> + if (mgr && mgr->fpga_read)
> + ret = mgr->fpga_read(mgr, buf, &count);
> +
> + return ret == 0 ? count : -EPERM;
> +}
> +
> +/**
> + * fpga_mgr_write - Write data to fpga
> + * @mgr: Pointer to the fpga manager structure
> + * @fw_name: Pointer to the buffer location with bistream firmware filename
> + *
> + * @buf contains firmware filename which is loading through firmware
> + * interface and passed to the fpga driver.
> + *
> + * Returns string lenght added to @fw_name, a negative error number otherwise
> + */
> +static int fpga_mgr_write(struct fpga_manager *mgr, const char *fw_name)
> +{
> + int ret = 0;
> + const struct firmware *fw;
> +
> + if (!fw_name || !strlen(fw_name)) {
> + dev_err(mgr->dev, "Firmware name is not specified!\n");
> + return -EINVAL;
> + }
> +
> + if (!mgr->mops || !mgr->mops->write) {
> + dev_err(mgr->dev,
> + "Controller doesn't support write operations!\n");
> + return -EPERM;
> + }
> +
> + ret = request_firmware(&fw, fw_name, mgr->dev);
> + if (ret) {
> + dev_err(mgr->dev, "Failed to load firmware %s!\n", fw_name);
> + return ret;
> + }
> +
> + if (test_and_set_bit_lock(FPGA_MGR_DEV_BUSY, &mgr->flags))
> + return -EBUSY;
> +
> + /* Init controller */
> + if (mgr->mops->write_init) {
> + ret = mgr->mops->write_init(mgr);
> + if (ret) {
> + dev_err(mgr->dev, "Failed to write_init!\n");
> + return ret;
> + }
> + }
> +
> + /* Do write */
> + ret = mgr->mops->write(mgr, (void *)fw->data, fw->size);
> + if (ret) {
> + dev_err(mgr->dev, "Failed to write data!\n");
> + return ret;
> + }
> +
> + if (mgr->mops->write_complete) {
> + ret = mgr->mops->write_complete(mgr);
> + if (ret) {
> + dev_err(mgr->dev, "Failed to write_init!\n");
> + return ret;
> + }
> + }
> +
> + release_firmware(fw);
> +
> + clear_bit_unlock(FPGA_MGR_DEV_BUSY, &mgr->flags);
> +
> + return 0;
> +}
> +
> +/**
> + * fpga_mgr_attr_write - Write data to fpga
> + * @dev: Pointer to the device structure
> + * @attr: Pointer to the device attribute structure
> + * @buf: Pointer to the buffer location with bistream firmware filename
> + * @count: Number of characters in @buf
> + *
> + * @buf contains firmware filename which is loading through firmware
> + * interface and passed to the fpga driver.
> + *
> + * Returns string lenght added to @buf, a negative error number otherwise
> + */
> +static ssize_t fpga_mgr_attr_write(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct fpga_manager *mgr = dev_get_drvdata(dev);
> + int ret;
> +
> + if (mgr && mgr->fpga_write)
> + ret = mgr->fpga_write(mgr, buf);
> +
> + return ret == 0 ? strlen(buf) : -EPERM;
> +}
> +
> +static struct device_attribute fpga_mgr_attrs[] = {
> + __ATTR(name, S_IRUGO, fpga_mgr_name_show, NULL),
> + __ATTR(status, S_IRUGO, fpga_mgr_status_show, NULL),
> + __ATTR(fpga, S_IRUGO | S_IWUGO, fpga_mgr_attr_read,
> + fpga_mgr_attr_write),
> + __ATTR_NULL
> +};
> +
> +/**
> + * fpga_mgr_register: Register new fpga manager
> + * @pdev: Pointer to the platform device of fpga manager
> + * @mops: Pointer to the fpga manager operations
> + * @name: Name of fpga manager
> + * @priv: Pointer to the fpga manager private data
> + *
> + * Function register fpga manager with uniq ID and create device
> + * for accessing the device.
> + *
> + * Returns 0 on success, a negative error number otherwise
> + */
> +int fpga_mgr_register(struct platform_device *pdev,
> + struct fpga_manager_ops *mops, char *name, void *priv)
> +{
> + struct fpga_manager *mgr;
> + int ret;
> +
> + if (!mops) {
> + dev_err(&pdev->dev, "Register failed: NO fpga_manager_ops!\n");
> + return -EINVAL;
> + }
> +
> + if (!name || !strlen(name)) {
> + dev_err(&pdev->dev, "Register failed: NO name specific!\n");
> + return -EINVAL;
> + }
> +
> + mgr = devm_kzalloc(&pdev->dev, sizeof(*mgr), GFP_KERNEL);
> + if (!mgr) {
> + dev_err(&pdev->dev, "Failed to alloc mgr struct!\n");
> + return -ENOMEM;
> + }
> +
> + /* Get unique number */
> + idr_preload(GFP_KERNEL);
> + spin_lock(&fpga_mgr_idr_lock);
> + ret = idr_alloc(&fpga_mgr_idr, mgr, 0, 0, GFP_NOWAIT);
> + if (ret >= 0)
> + mgr->nr = ret;
> + spin_unlock(&fpga_mgr_idr_lock);
> + idr_preload_end();
> + if (ret < 0)
> + return ret;
> +
> + /* Setup all necessary information */
> + mgr->dev = &pdev->dev;
> + mgr->mops = mops;
> + mgr->priv = priv;
> + mgr->fpga_read = fpga_mgr_read;
> + mgr->fpga_write = fpga_mgr_write;
> + strlcpy(mgr->name, name, sizeof(mgr->name));
> +
> + mgr->dev = device_create(fpga_mgr_class, get_device(&pdev->dev),
> + MKDEV(0, 0), mgr, "fpga%d", mgr->nr);
> + if (IS_ERR(mgr->dev)) {
> + spin_lock(&fpga_mgr_idr_lock);
> + idr_remove(&fpga_mgr_idr, mgr->nr);
> + spin_unlock(&fpga_mgr_idr_lock);
> +
> + dev_err(&pdev->dev, "Failed to create device!\n");
> + return PTR_ERR(mgr->dev);
> + }
> +
> + platform_set_drvdata(pdev, mgr);
> +
> + dev_info(&pdev->dev, "Fpga manager [%s] registered as minor %d\n",
> + mgr->name, mgr->nr);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(fpga_mgr_register);
> +
> +/**
> + * fpga_mgr_unregister: Remove fpga manager
> + * @pdev: Pointer to the platform device of fpga manager
> + *
> + * Function unregister fpga manager and release all temporary structures
> + *
> + * Returns 0 for all cases
> + */
> +int fpga_mgr_unregister(struct platform_device *pdev)
> +{
> + struct fpga_manager *mgr = platform_get_drvdata(pdev);
> +
> + if (mgr && mgr->mops && mgr->mops->fpga_remove)
> + mgr->mops->fpga_remove(mgr);
> +
> + device_unregister(mgr->dev);
> +
> + spin_lock(&fpga_mgr_idr_lock);
> + idr_remove(&fpga_mgr_idr, mgr->nr);
> + spin_unlock(&fpga_mgr_idr_lock);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(fpga_mgr_unregister);
> +
> +/**
> + * of_dev_node_match: Compare associated device tree node with
> + * @dev: Pointer to the device
> + * @data: Pointer to the device tree node
> + *
> + * Returns 1 on success
> + */
> +static int of_dev_node_match(struct device *dev, void *data)
> +{
> + return dev->of_node == data;
> +}
> +
> +/**
> + * of_find_fpga_mgr_by_node - Find the fpga_manager associated with a node
> + * @node: Pointer to the device tree node
> + *
> + * Returns fpga_manager pointer, or NULL if not found
> + */
> +struct fpga_manager *of_find_fpga_mgr_by_node(struct device_node *node)
> +{
> + struct device *dev;
> + struct fpga_manager *mgr;
> +
> + dev = bus_find_device(&platform_bus_type, NULL, node,
> + of_dev_node_match);
> + if (!dev)
> + return NULL;
> +
> + mgr = dev_get_drvdata(dev);
> +
> + return mgr ? mgr : NULL;
> +}
> +EXPORT_SYMBOL(of_find_fpga_mgr_by_node);
> +
> +/**
> + * of_find_fpga_mgr_by_phandle - Find the fpga_manager associated with a node
> + * @node: Pointer to the device tree node
> + * @phandle_name: Pointer to the phandle_name
> + *
> + * Returns fpga_manager pointer, or NULL if not found
> + */
> +struct fpga_manager *of_find_fpga_mgr_by_phandle(struct device_node *node,
> + const char *phandle_name)
> +{
> + struct device_node *fpga_node;
> +
> + fpga_node = of_parse_phandle(node, phandle_name, 0);
> + if (!fpga_node) {
> + pr_err("Phandle not found\n");
> + return ERR_PTR(-ENODEV);
> + }
> +
> + return of_find_fpga_mgr_by_node(fpga_node);
> +}
> +EXPORT_SYMBOL(of_find_fpga_mgr_by_phandle);
> +
> +/**
> + * fpga_mgr_init: Create fpga class
> + */
> +static int __init fpga_mgr_init(void)
> +{
> + pr_info("FPGA Manager framework driver\n");
> +
> + fpga_mgr_class = class_create(THIS_MODULE, "fpga");
> + if (IS_ERR(fpga_mgr_class))
> + return PTR_ERR(fpga_mgr_class);
> +
> + idr_init(&fpga_mgr_idr);
> + spin_lock_init(&fpga_mgr_idr_lock);
> +
> + fpga_mgr_class->dev_attrs = fpga_mgr_attrs;
> +
> + return 0;
> +}
> +subsys_initcall(fpga_mgr_init);
> +
> +/**
> + * fpga_mgr_exit: Destroy fpga class
> + */
> +static void __exit fpga_mgr_exit(void)
> +{
> + class_destroy(fpga_mgr_class);
> + idr_destroy(&fpga_mgr_idr);
> +}
> +module_exit(fpga_mgr_exit);
> +
> +MODULE_AUTHOR("Xilinx, Inc.");
> +MODULE_DESCRIPTION("FPGA Manager framework driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/fpga.h b/include/linux/fpga.h
> new file mode 100644
> index 0000000..970c42e
> --- /dev/null
> +++ b/include/linux/fpga.h
> @@ -0,0 +1,105 @@
> +/*
> + * FPGA Framework
> + *
> + * Copyright (C) 2013 Xilinx, Inc.
> + *
> + * based on origin code from
> + * Copyright (C) 2013 Altera Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +
> +#ifndef _LINUX_FPGA_H
> +#define _LINUX_FPGA_H
> +
> +struct fpga_manager;
> +
> +/**
> + * struct fpga_manager_ops - FPGA manager driver operations
> + *
> + * @status: The function to call for getting fpga manager status.
> + * Returns number of characters written to the @buf and
> + * a string of the FPGA's status in @bug.
> + * @read_init: The function to call for preparing the FPGA for reading
> + * its confuration data. Returns 0 on success, a negative error
> + * number otherwise.
> + * @read: Read configuration data from the FPGA. Return 0 on success,
> + * a negative error number otherwise.
> + * @read_complete: Return FPGA to a default state after reading is done.
> + * Return 0 on success, a negative error number otherwise.
> + * @write_init: Prepare the FPGA to receive configuration data.
> + * Return 0 on success, a negative error number otherwise.
> + * @write: Write count bytes of configuration data to the FPGA placed in @buf.
> + * Return 0 on success, a negative error number otherwise.
> + * @write_complete: Return FPGA to default state after writing is done.
> + * Return 0 on success, a negative error number otherwise.
> + * @fpga_remove: Set FPGA into a specific state during driver remove.
> + *
> + * fpga_manager_ops are the low level functions implemented by a specific
> + * fpga manager driver. Leaving any of these out that aren't needed is fine
> + * as they are all tested for NULL before being called.
> + */
> +struct fpga_manager_ops {
> + int (*status)(struct fpga_manager *mgr, char *buf);
> + int (*read_init)(struct fpga_manager *mgr);
> + int (*read)(struct fpga_manager *mgr, char *buf, ssize_t *count);
> + int (*read_complete)(struct fpga_manager *mgr);
> + int (*write_init)(struct fpga_manager *mgr);
> + int (*write)(struct fpga_manager *mgr, char *buf, ssize_t count);
> + int (*write_complete)(struct fpga_manager *mgr);
> + void (*fpga_remove)(struct fpga_manager *mgr);
> +};
> +
> +/* flag bits */
> +#define FPGA_MGR_DEV_BUSY 0
> +
> +/**
> + * struct fpga_manager - FPGA manager driver structure
> + *
> + * @name: Name of fpga manager
> + * @dev: Pointer to the device structure
> + * @mops: Pointer to the fpga manager operations
> + * @priv: Pointer to fpga manager private data
> + * @nr: Unique manager number in the system
> + * @flags: For saving temporary
> + * @fpga_read: The function to call for reading configuration data from
> + * the FPGA.
> + * @fpga_write: The function to call for writing configuration data to the FPGA.
> + */
> +struct fpga_manager {
> + char name[48];
> + struct device *dev;
> + struct fpga_manager_ops *mops;
> + void *priv;
> + unsigned int nr;
> + unsigned long flags;
> + int (*fpga_read)(struct fpga_manager *mgr, char *buf,
> + ssize_t *count);
> + int (*fpga_write)(struct fpga_manager *mgr, const char *fw_name);
> +};
> +
> +int fpga_mgr_register(struct platform_device *pdev,
> + struct fpga_manager_ops *mops, char *name, void *priv);
> +
> +int fpga_mgr_unregister(struct platform_device *pdev);
> +
> +struct fpga_manager *of_find_fpga_mgr_by_node(struct device_node *node);
> +struct fpga_manager *of_find_fpga_mgr_by_phandle(struct device_node *node,
> + const char *phandle_name);
> +
> +#endif /*_LINUX_FPGA_H */
> --
> 1.8.2.3
>
+ Jason Gunthorpe
On Wed, Sep 18, 2013 at 05:56:39PM +0200, Michal Simek wrote:
> This new subsystem should unify all fpga drivers which
> do the same things. Load configuration data to fpga
> or another programmable logic through common interface.
> It doesn't matter if it is MMIO device, gpio bitbanging,
> etc. connection. The point is to have the same
> inteface for these drivers.
>
> Signed-off-by: Michal Simek <[email protected]>
>
> ---
> MAINTAINERS | 7 +
> drivers/Kconfig | 2 +
> drivers/Makefile | 1 +
> drivers/fpga/Kconfig | 18 ++
> drivers/fpga/Makefile | 5 +
> drivers/fpga/fpga-mgr.c | 433 ++++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/fpga.h | 105 ++++++++++++
> 7 files changed, 571 insertions(+)
> create mode 100644 drivers/fpga/Kconfig
> create mode 100644 drivers/fpga/Makefile
> create mode 100644 drivers/fpga/fpga-mgr.c
> create mode 100644 include/linux/fpga.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e61c2e8..5c7597b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3427,6 +3427,13 @@ F: include/linux/fmc*.h
> F: include/linux/ipmi-fru.h
> K: fmc_d.*register
>
> +FPGA SUBSYSTEM
> +M: Michal Simek <[email protected]>
> +T: git git://git.xilinx.com/linux-xlnx.git
> +S: Supported
> +F: drivers/fpga/
> +F: include/linux/fpga.h
> +
> FPU EMULATOR
> M: Bill Metzenthen <[email protected]>
> W: http://floatingpoint.sourceforge.net/emulator/index.html
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index aa43b91..17f3caa 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -166,4 +166,6 @@ source "drivers/reset/Kconfig"
>
> source "drivers/fmc/Kconfig"
>
> +source "drivers/fpga/Kconfig"
> +
> endmenu
> diff --git a/drivers/Makefile b/drivers/Makefile
> index ab93de8..2b5e73b 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -152,3 +152,4 @@ obj-$(CONFIG_VME_BUS) += vme/
> obj-$(CONFIG_IPACK_BUS) += ipack/
> obj-$(CONFIG_NTB) += ntb/
> obj-$(CONFIG_FMC) += fmc/
> +obj-$(CONFIG_FPGA) += fpga/
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> new file mode 100644
> index 0000000..5357645
> --- /dev/null
> +++ b/drivers/fpga/Kconfig
> @@ -0,0 +1,18 @@
> +#
> +# FPGA framework configuration
> +#
> +
> +menu "FPGA devices"
> +
> +config FPGA
> + tristate "FPGA Framework"
> + help
> + Say Y here if you want support for configuring FPGAs from the
> + kernel. The FPGA framework adds a FPGA manager class and FPGA
> + manager drivers.
> +
> +if FPGA
> +
> +endif
> +
> +endmenu
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> new file mode 100644
> index 0000000..3c7f29b
> --- /dev/null
> +++ b/drivers/fpga/Makefile
> @@ -0,0 +1,5 @@
> +#
> +# Makefile for the fpga framework and fpga manager drivers.
> +#
> +
> +obj-$(CONFIG_FPGA) += fpga-mgr.o
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> new file mode 100644
> index 0000000..7312efd
> --- /dev/null
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -0,0 +1,433 @@
> +/*
> + * FPGA Framework
> + *
> + * Copyright (C) 2013 Xilinx, Inc.
> + *
> + * based on origin code from
> + * Copyright (C) 2013 Altera Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/firmware.h>
> +#include <linux/fpga.h>
> +#include <linux/fs.h>
> +#include <linux/idr.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +
> +static struct idr fpga_mgr_idr;
> +static spinlock_t fpga_mgr_idr_lock;
> +static struct class *fpga_mgr_class;
> +
> +/**
> + * fpga_mgr_name_show - Show fpga manager name
> + * @dev: Pointer to the device structure
> + * @attr: Pointer to the device attribute structure
> + * @buf: Pointer to the buffer location
> + *
> + * Returns the number of bytes copied to @buf, a negative error number otherwise
> + */
> +static ssize_t fpga_mgr_name_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct fpga_manager *mgr = dev_get_drvdata(dev);
> +
> + if (!mgr)
> + return -ENODEV;
> +
> + return snprintf(buf, sizeof(mgr->name), "%s\n", mgr->name);
> +}
> +
> +/**
> + * fpga_mgr_status_show - Show fpga manager status
> + * @dev: Pointer to the device structure
> + * @attr: Pointer to the device attribute structure
> + * @buf: Pointer to the buffer location
> + *
> + * Returns the number of bytes copied to @buf, a negative error number otherwise
> + */
> +static ssize_t fpga_mgr_status_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct fpga_manager *mgr = dev_get_drvdata(dev);
> +
> + if (!mgr || !mgr->mops || !mgr->mops->status)
> + return -ENODEV;
> +
> + return mgr->mops->status(mgr, buf);
> +}
> +
> +/**
> + * fpga_mgr_read - Read data from fpga
> + * @mgr: Pointer to the fpga manager structure
> + * @buf: Pointer to the buffer location
> + * @count: Pointer to the number of copied bytes
> + *
> + * Function reads fpga bitstream and copy them to output buffer.
> + *
> + * Returns the number of bytes copied to @buf, a negative error number otherwise
> + */
> +static int fpga_mgr_read(struct fpga_manager *mgr, char *buf, ssize_t *count)
> +{
> + int ret;
> +
> + if (test_and_set_bit_lock(FPGA_MGR_DEV_BUSY, &mgr->flags))
> + return -EBUSY;
> +
> + if (!mgr->mops || !mgr->mops->read) {
> + dev_err(mgr->dev,
> + "Controller doesn't support read operations!\n");
> + return -EPERM;
> + }
> +
> + if (mgr->mops->read_init) {
> + ret = mgr->mops->read_init(mgr);
> + if (ret) {
> + dev_err(mgr->dev, "Failed in read-init!\n");
> + return ret;
> + }
> + }
> +
> + if (mgr->mops->read) {
> + ret = mgr->mops->read(mgr, buf, count);
> + if (ret) {
> + dev_err(mgr->dev, "Failed to read firmware!\n");
> + return ret;
> + }
> + }
> +
> + if (mgr->mops->read_complete) {
> + ret = mgr->mops->read_complete(mgr);
> + if (ret) {
> + dev_err(mgr->dev, "Failed in read-complete!\n");
> + return ret;
> + }
> + }
> +
> + clear_bit_unlock(FPGA_MGR_DEV_BUSY, &mgr->flags);
> +
> + return 0;
> +}
> +
> +/**
> + * fpga_mgr_attr_read - Read data from fpga
> + * @dev: Pointer to the device structure
> + * @attr: Pointer to the device attribute structure
> + * @buf: Pointer to the buffer location
> + *
> + * Function reads fpga bitstream and copy them to output buffer
> + *
> + * Returns the number of bytes copied to @buf, a negative error number otherwise
> + */
> +static ssize_t fpga_mgr_attr_read(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct fpga_manager *mgr = dev_get_drvdata(dev);
> + ssize_t count;
> + int ret;
> +
> + if (mgr && mgr->fpga_read)
> + ret = mgr->fpga_read(mgr, buf, &count);
> +
> + return ret == 0 ? count : -EPERM;
> +}
> +
> +/**
> + * fpga_mgr_write - Write data to fpga
> + * @mgr: Pointer to the fpga manager structure
> + * @fw_name: Pointer to the buffer location with bistream firmware filename
> + *
> + * @buf contains firmware filename which is loading through firmware
> + * interface and passed to the fpga driver.
> + *
> + * Returns string lenght added to @fw_name, a negative error number otherwise
> + */
> +static int fpga_mgr_write(struct fpga_manager *mgr, const char *fw_name)
> +{
> + int ret = 0;
> + const struct firmware *fw;
> +
> + if (!fw_name || !strlen(fw_name)) {
> + dev_err(mgr->dev, "Firmware name is not specified!\n");
> + return -EINVAL;
> + }
> +
> + if (!mgr->mops || !mgr->mops->write) {
> + dev_err(mgr->dev,
> + "Controller doesn't support write operations!\n");
> + return -EPERM;
> + }
> +
> + ret = request_firmware(&fw, fw_name, mgr->dev);
> + if (ret) {
> + dev_err(mgr->dev, "Failed to load firmware %s!\n", fw_name);
> + return ret;
> + }
> +
> + if (test_and_set_bit_lock(FPGA_MGR_DEV_BUSY, &mgr->flags))
> + return -EBUSY;
> +
> + /* Init controller */
> + if (mgr->mops->write_init) {
> + ret = mgr->mops->write_init(mgr);
> + if (ret) {
> + dev_err(mgr->dev, "Failed to write_init!\n");
> + return ret;
> + }
> + }
> +
> + /* Do write */
> + ret = mgr->mops->write(mgr, (void *)fw->data, fw->size);
> + if (ret) {
> + dev_err(mgr->dev, "Failed to write data!\n");
> + return ret;
> + }
> +
> + if (mgr->mops->write_complete) {
> + ret = mgr->mops->write_complete(mgr);
> + if (ret) {
> + dev_err(mgr->dev, "Failed to write_init!\n");
> + return ret;
> + }
> + }
> +
> + release_firmware(fw);
> +
> + clear_bit_unlock(FPGA_MGR_DEV_BUSY, &mgr->flags);
> +
> + return 0;
> +}
> +
> +/**
> + * fpga_mgr_attr_write - Write data to fpga
> + * @dev: Pointer to the device structure
> + * @attr: Pointer to the device attribute structure
> + * @buf: Pointer to the buffer location with bistream firmware filename
> + * @count: Number of characters in @buf
> + *
> + * @buf contains firmware filename which is loading through firmware
> + * interface and passed to the fpga driver.
> + *
> + * Returns string lenght added to @buf, a negative error number otherwise
> + */
> +static ssize_t fpga_mgr_attr_write(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct fpga_manager *mgr = dev_get_drvdata(dev);
> + int ret;
> +
> + if (mgr && mgr->fpga_write)
> + ret = mgr->fpga_write(mgr, buf);
> +
> + return ret == 0 ? strlen(buf) : -EPERM;
> +}
> +
> +static struct device_attribute fpga_mgr_attrs[] = {
> + __ATTR(name, S_IRUGO, fpga_mgr_name_show, NULL),
> + __ATTR(status, S_IRUGO, fpga_mgr_status_show, NULL),
> + __ATTR(fpga, S_IRUGO | S_IWUGO, fpga_mgr_attr_read,
> + fpga_mgr_attr_write),
> + __ATTR_NULL
> +};
> +
> +/**
> + * fpga_mgr_register: Register new fpga manager
> + * @pdev: Pointer to the platform device of fpga manager
> + * @mops: Pointer to the fpga manager operations
> + * @name: Name of fpga manager
> + * @priv: Pointer to the fpga manager private data
> + *
> + * Function register fpga manager with uniq ID and create device
> + * for accessing the device.
> + *
> + * Returns 0 on success, a negative error number otherwise
> + */
> +int fpga_mgr_register(struct platform_device *pdev,
> + struct fpga_manager_ops *mops, char *name, void *priv)
> +{
> + struct fpga_manager *mgr;
> + int ret;
> +
> + if (!mops) {
> + dev_err(&pdev->dev, "Register failed: NO fpga_manager_ops!\n");
> + return -EINVAL;
> + }
> +
> + if (!name || !strlen(name)) {
> + dev_err(&pdev->dev, "Register failed: NO name specific!\n");
> + return -EINVAL;
> + }
> +
> + mgr = devm_kzalloc(&pdev->dev, sizeof(*mgr), GFP_KERNEL);
> + if (!mgr) {
> + dev_err(&pdev->dev, "Failed to alloc mgr struct!\n");
> + return -ENOMEM;
> + }
> +
> + /* Get unique number */
> + idr_preload(GFP_KERNEL);
> + spin_lock(&fpga_mgr_idr_lock);
> + ret = idr_alloc(&fpga_mgr_idr, mgr, 0, 0, GFP_NOWAIT);
> + if (ret >= 0)
> + mgr->nr = ret;
> + spin_unlock(&fpga_mgr_idr_lock);
> + idr_preload_end();
> + if (ret < 0)
> + return ret;
> +
> + /* Setup all necessary information */
> + mgr->dev = &pdev->dev;
> + mgr->mops = mops;
> + mgr->priv = priv;
> + mgr->fpga_read = fpga_mgr_read;
> + mgr->fpga_write = fpga_mgr_write;
> + strlcpy(mgr->name, name, sizeof(mgr->name));
> +
> + mgr->dev = device_create(fpga_mgr_class, get_device(&pdev->dev),
> + MKDEV(0, 0), mgr, "fpga%d", mgr->nr);
> + if (IS_ERR(mgr->dev)) {
> + spin_lock(&fpga_mgr_idr_lock);
> + idr_remove(&fpga_mgr_idr, mgr->nr);
> + spin_unlock(&fpga_mgr_idr_lock);
> +
> + dev_err(&pdev->dev, "Failed to create device!\n");
> + return PTR_ERR(mgr->dev);
> + }
> +
> + platform_set_drvdata(pdev, mgr);
> +
> + dev_info(&pdev->dev, "Fpga manager [%s] registered as minor %d\n",
> + mgr->name, mgr->nr);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(fpga_mgr_register);
> +
> +/**
> + * fpga_mgr_unregister: Remove fpga manager
> + * @pdev: Pointer to the platform device of fpga manager
> + *
> + * Function unregister fpga manager and release all temporary structures
> + *
> + * Returns 0 for all cases
> + */
> +int fpga_mgr_unregister(struct platform_device *pdev)
> +{
> + struct fpga_manager *mgr = platform_get_drvdata(pdev);
> +
> + if (mgr && mgr->mops && mgr->mops->fpga_remove)
> + mgr->mops->fpga_remove(mgr);
> +
> + device_unregister(mgr->dev);
> +
> + spin_lock(&fpga_mgr_idr_lock);
> + idr_remove(&fpga_mgr_idr, mgr->nr);
> + spin_unlock(&fpga_mgr_idr_lock);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(fpga_mgr_unregister);
> +
> +/**
> + * of_dev_node_match: Compare associated device tree node with
> + * @dev: Pointer to the device
> + * @data: Pointer to the device tree node
> + *
> + * Returns 1 on success
> + */
> +static int of_dev_node_match(struct device *dev, void *data)
> +{
> + return dev->of_node == data;
> +}
> +
> +/**
> + * of_find_fpga_mgr_by_node - Find the fpga_manager associated with a node
> + * @node: Pointer to the device tree node
> + *
> + * Returns fpga_manager pointer, or NULL if not found
> + */
> +struct fpga_manager *of_find_fpga_mgr_by_node(struct device_node *node)
> +{
> + struct device *dev;
> + struct fpga_manager *mgr;
> +
> + dev = bus_find_device(&platform_bus_type, NULL, node,
> + of_dev_node_match);
> + if (!dev)
> + return NULL;
> +
> + mgr = dev_get_drvdata(dev);
> +
> + return mgr ? mgr : NULL;
> +}
> +EXPORT_SYMBOL(of_find_fpga_mgr_by_node);
> +
> +/**
> + * of_find_fpga_mgr_by_phandle - Find the fpga_manager associated with a node
> + * @node: Pointer to the device tree node
> + * @phandle_name: Pointer to the phandle_name
> + *
> + * Returns fpga_manager pointer, or NULL if not found
> + */
> +struct fpga_manager *of_find_fpga_mgr_by_phandle(struct device_node *node,
> + const char *phandle_name)
> +{
> + struct device_node *fpga_node;
> +
> + fpga_node = of_parse_phandle(node, phandle_name, 0);
> + if (!fpga_node) {
> + pr_err("Phandle not found\n");
> + return ERR_PTR(-ENODEV);
> + }
> +
> + return of_find_fpga_mgr_by_node(fpga_node);
> +}
> +EXPORT_SYMBOL(of_find_fpga_mgr_by_phandle);
> +
> +/**
> + * fpga_mgr_init: Create fpga class
> + */
> +static int __init fpga_mgr_init(void)
> +{
> + pr_info("FPGA Manager framework driver\n");
> +
> + fpga_mgr_class = class_create(THIS_MODULE, "fpga");
> + if (IS_ERR(fpga_mgr_class))
> + return PTR_ERR(fpga_mgr_class);
> +
> + idr_init(&fpga_mgr_idr);
> + spin_lock_init(&fpga_mgr_idr_lock);
> +
> + fpga_mgr_class->dev_attrs = fpga_mgr_attrs;
> +
> + return 0;
> +}
> +subsys_initcall(fpga_mgr_init);
> +
> +/**
> + * fpga_mgr_exit: Destroy fpga class
> + */
> +static void __exit fpga_mgr_exit(void)
> +{
> + class_destroy(fpga_mgr_class);
> + idr_destroy(&fpga_mgr_idr);
> +}
> +module_exit(fpga_mgr_exit);
> +
> +MODULE_AUTHOR("Xilinx, Inc.");
> +MODULE_DESCRIPTION("FPGA Manager framework driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/fpga.h b/include/linux/fpga.h
> new file mode 100644
> index 0000000..970c42e
> --- /dev/null
> +++ b/include/linux/fpga.h
> @@ -0,0 +1,105 @@
> +/*
> + * FPGA Framework
> + *
> + * Copyright (C) 2013 Xilinx, Inc.
> + *
> + * based on origin code from
> + * Copyright (C) 2013 Altera Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +
> +#ifndef _LINUX_FPGA_H
> +#define _LINUX_FPGA_H
> +
> +struct fpga_manager;
> +
> +/**
> + * struct fpga_manager_ops - FPGA manager driver operations
> + *
> + * @status: The function to call for getting fpga manager status.
> + * Returns number of characters written to the @buf and
> + * a string of the FPGA's status in @bug.
> + * @read_init: The function to call for preparing the FPGA for reading
> + * its confuration data. Returns 0 on success, a negative error
> + * number otherwise.
> + * @read: Read configuration data from the FPGA. Return 0 on success,
> + * a negative error number otherwise.
> + * @read_complete: Return FPGA to a default state after reading is done.
> + * Return 0 on success, a negative error number otherwise.
> + * @write_init: Prepare the FPGA to receive configuration data.
> + * Return 0 on success, a negative error number otherwise.
> + * @write: Write count bytes of configuration data to the FPGA placed in @buf.
> + * Return 0 on success, a negative error number otherwise.
> + * @write_complete: Return FPGA to default state after writing is done.
> + * Return 0 on success, a negative error number otherwise.
> + * @fpga_remove: Set FPGA into a specific state during driver remove.
> + *
> + * fpga_manager_ops are the low level functions implemented by a specific
> + * fpga manager driver. Leaving any of these out that aren't needed is fine
> + * as they are all tested for NULL before being called.
> + */
> +struct fpga_manager_ops {
> + int (*status)(struct fpga_manager *mgr, char *buf);
> + int (*read_init)(struct fpga_manager *mgr);
> + int (*read)(struct fpga_manager *mgr, char *buf, ssize_t *count);
> + int (*read_complete)(struct fpga_manager *mgr);
> + int (*write_init)(struct fpga_manager *mgr);
> + int (*write)(struct fpga_manager *mgr, char *buf, ssize_t count);
> + int (*write_complete)(struct fpga_manager *mgr);
> + void (*fpga_remove)(struct fpga_manager *mgr);
> +};
> +
> +/* flag bits */
> +#define FPGA_MGR_DEV_BUSY 0
> +
> +/**
> + * struct fpga_manager - FPGA manager driver structure
> + *
> + * @name: Name of fpga manager
> + * @dev: Pointer to the device structure
> + * @mops: Pointer to the fpga manager operations
> + * @priv: Pointer to fpga manager private data
> + * @nr: Unique manager number in the system
> + * @flags: For saving temporary
> + * @fpga_read: The function to call for reading configuration data from
> + * the FPGA.
> + * @fpga_write: The function to call for writing configuration data to the FPGA.
> + */
> +struct fpga_manager {
> + char name[48];
> + struct device *dev;
> + struct fpga_manager_ops *mops;
> + void *priv;
> + unsigned int nr;
> + unsigned long flags;
> + int (*fpga_read)(struct fpga_manager *mgr, char *buf,
> + ssize_t *count);
> + int (*fpga_write)(struct fpga_manager *mgr, const char *fw_name);
> +};
> +
> +int fpga_mgr_register(struct platform_device *pdev,
> + struct fpga_manager_ops *mops, char *name, void *priv);
> +
> +int fpga_mgr_unregister(struct platform_device *pdev);
> +
> +struct fpga_manager *of_find_fpga_mgr_by_node(struct device_node *node);
> +struct fpga_manager *of_find_fpga_mgr_by_phandle(struct device_node *node,
> + const char *phandle_name);
> +
> +#endif /*_LINUX_FPGA_H */
> --
> 1.8.2.3
>
On Wed, Sep 18, 2013 at 03:15:17PM -0400, Jason Cooper wrote:
> + Jason Gunthorpe
Thanks, looks interesting, we could possibly use this interface if it
met our needs..
> On Wed, Sep 18, 2013 at 05:56:39PM +0200, Michal Simek wrote:
> > This new subsystem should unify all fpga drivers which
> > do the same things. Load configuration data to fpga
> > or another programmable logic through common interface.
> > It doesn't matter if it is MMIO device, gpio bitbanging,
> > etc. connection. The point is to have the same
> > inteface for these drivers.
So, we have many years of in-field experience with this and this API
doesn't really match what we do.
Here are the steps we perform, from userspace:
- Ask kernel to place FPGA into reset and prepare for programming
* Kernel can return an error (eg FPGA failed to erase, etc)
* this is the PROG_N low -> DONE high, PROG_N high -> INIT_N high
sequencing on Xilinx chips
- Ask kernel to load a bitstream.
* Userspace locats the bitstream file to load, and the mmaps it.
* Userspace passes the entire file in a single write() call to the
kernel which streams it over the configuration bus
* The kernel can report an erro rhere (eg Xilinx can report CRC
error)
- Ask the kernel to verify that configuration is complete.
* On Xilinx this wait for done to go high
- Ask the kernel to release the configuration bus (tristate
all drivers) (or sometimes we have to drive the bus low,
it depends on the bitfile, user space knows what to do)
It is very important that userspace know exactly which step fails
because the resolution is different. We use this in a manufacturing
setting, so failures are expected and need quick root cause
determination.
You could probably address that need by very clearly defining a
variety of errno values for the various cases. However, it would be a
disaster if every driver did something a little different :|
Using request_firmware exclusively is not useful for us. We
format the bitfile with a header that contains our internal tracking
information. Sometimes we need to bitswap the bitfile. Our userspace
handles all of this and can pass a bitfile in memory to write().
request_firmware would be horrible to use :)
Our API uses a binary sysfs attribute to stream the FPGA data, you
might want to consider that.
Regards,
Jason
On Wed, 2013-09-18 at 14:32 -0600, Jason Gunthorpe wrote:
> On Wed, Sep 18, 2013 at 03:15:17PM -0400, Jason Cooper wrote:
>
> > + Jason Gunthorpe
>
> Thanks, looks interesting, we could possibly use this interface if it
> met our needs..
>
> > On Wed, Sep 18, 2013 at 05:56:39PM +0200, Michal Simek wrote:
> > > This new subsystem should unify all fpga drivers which
> > > do the same things. Load configuration data to fpga
> > > or another programmable logic through common interface.
> > > It doesn't matter if it is MMIO device, gpio bitbanging,
> > > etc. connection. The point is to have the same
> > > inteface for these drivers.
>
> So, we have many years of in-field experience with this and this API
> doesn't really match what we do.
>
> Here are the steps we perform, from userspace:
> - Ask kernel to place FPGA into reset and prepare for programming
> * Kernel can return an error (eg FPGA failed to erase, etc)
> * this is the PROG_N low -> DONE high, PROG_N high -> INIT_N high
> sequencing on Xilinx chips
> - Ask kernel to load a bitstream.
> * Userspace locats the bitstream file to load, and the mmaps it.
> * Userspace passes the entire file in a single write() call to the
> kernel which streams it over the configuration bus
> * The kernel can report an erro rhere (eg Xilinx can report CRC
> error)
> - Ask the kernel to verify that configuration is complete.
> * On Xilinx this wait for done to go high
> - Ask the kernel to release the configuration bus (tristate
> all drivers) (or sometimes we have to drive the bus low,
> it depends on the bitfile, user space knows what to do)
>
> It is very important that userspace know exactly which step fails
> because the resolution is different. We use this in a manufacturing
> setting, so failures are expected and need quick root cause
> determination.
>
> You could probably address that need by very clearly defining a
> variety of errno values for the various cases. However, it would be a
> disaster if every driver did something a little different :|
>
> Using request_firmware exclusively is not useful for us. We
> format the bitfile with a header that contains our internal tracking
> information. Sometimes we need to bitswap the bitfile. Our userspace
> handles all of this and can pass a bitfile in memory to write().
>
> request_firmware would be horrible to use :)
>
> Our API uses a binary sysfs attribute to stream the FPGA data, you
> might want to consider that.
>
> Regards,
> Jason
The firmware approach is interesting. It might be less flexible
compared with my original code (see link to git below) that this is
based on. The original code created a devnode like /dev/fpga0 and a raw
bitstream could be loaded by doing 'cat bitstream > /dev/fpga0'. Or
some other userspace app could write the /dev/fpga0 to handle any
headers that needed to be added to the bitstream.
This code also creates a set of files under /sys for each separate fpga.
I.e. checking status by looking at /sys/class/fpga/fpag0/status. It
would be pretty small changes to control reseting the fpga by adding a
'reset' file there also (added first to the framework, and an interface
into the low level fpga manager driver).
I am trying this out with my low level fpga manager driver. I'm very
curious about your approach and I am wondering whether the firmware
approach will work for us or not.
Will this framework handle more than one fpga at a time?
Is there some way a per-device userspace helper can be added that can
handle adding the headers? Such that different fpga types get different
helpers?
My fpga framework code is in git at:
http://rocketboards.org/gitweb/?p=linux-socfpga.git;a=commit;h=7b7c04ef3f8589349211bdfe884e42d6b7554b27
and
http://rocketboards.org/gitweb/?p=linux-socfpga.git;a=commit;h=57ee6197d65015620cd6aad435a695ce00a48a8c
Best Regards,
Alan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
On 19/09/13 01:56, Michal Simek wrote:
> This new subsystem should unify all fpga drivers which
> do the same things. Load configuration data to fpga
> or another programmable logic through common interface.
> It doesn't matter if it is MMIO device, gpio bitbanging,
> etc. connection. The point is to have the same
> inteface for these drivers.
>
> Signed-off-by: Michal Simek <[email protected]>
Hi Michal,
Some comments below.
~Ryan
>
> ---
> MAINTAINERS | 7 +
> drivers/Kconfig | 2 +
> drivers/Makefile | 1 +
> drivers/fpga/Kconfig | 18 ++
> drivers/fpga/Makefile | 5 +
> drivers/fpga/fpga-mgr.c | 433 ++++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/fpga.h | 105 ++++++++++++
> 7 files changed, 571 insertions(+)
> create mode 100644 drivers/fpga/Kconfig
> create mode 100644 drivers/fpga/Makefile
> create mode 100644 drivers/fpga/fpga-mgr.c
> create mode 100644 include/linux/fpga.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e61c2e8..5c7597b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3427,6 +3427,13 @@ F: include/linux/fmc*.h
> F: include/linux/ipmi-fru.h
> K: fmc_d.*register
>
> +FPGA SUBSYSTEM
> +M: Michal Simek <[email protected]>
> +T: git git://git.xilinx.com/linux-xlnx.git
> +S: Supported
> +F: drivers/fpga/
> +F: include/linux/fpga.h
> +
> FPU EMULATOR
> M: Bill Metzenthen <[email protected]>
> W: http://floatingpoint.sourceforge.net/emulator/index.html
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index aa43b91..17f3caa 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -166,4 +166,6 @@ source "drivers/reset/Kconfig"
>
> source "drivers/fmc/Kconfig"
>
> +source "drivers/fpga/Kconfig"
> +
> endmenu
> diff --git a/drivers/Makefile b/drivers/Makefile
> index ab93de8..2b5e73b 100644
> --- a/drivers/Makefile
> +++ b/drivers/Makefile
> @@ -152,3 +152,4 @@ obj-$(CONFIG_VME_BUS) += vme/
> obj-$(CONFIG_IPACK_BUS) += ipack/
> obj-$(CONFIG_NTB) += ntb/
> obj-$(CONFIG_FMC) += fmc/
> +obj-$(CONFIG_FPGA) += fpga/
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> new file mode 100644
> index 0000000..5357645
> --- /dev/null
> +++ b/drivers/fpga/Kconfig
> @@ -0,0 +1,18 @@
> +#
> +# FPGA framework configuration
> +#
> +
> +menu "FPGA devices"
> +
> +config FPGA
> + tristate "FPGA Framework"
> + help
> + Say Y here if you want support for configuring FPGAs from the
> + kernel. The FPGA framework adds a FPGA manager class and FPGA
> + manager drivers.
> +
> +if FPGA
> +
> +endif
> +
> +endmenu
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> new file mode 100644
> index 0000000..3c7f29b
> --- /dev/null
> +++ b/drivers/fpga/Makefile
> @@ -0,0 +1,5 @@
> +#
> +# Makefile for the fpga framework and fpga manager drivers.
> +#
> +
> +obj-$(CONFIG_FPGA) += fpga-mgr.o
> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> new file mode 100644
> index 0000000..7312efd
> --- /dev/null
> +++ b/drivers/fpga/fpga-mgr.c
> @@ -0,0 +1,433 @@
> +/*
> + * FPGA Framework
> + *
> + * Copyright (C) 2013 Xilinx, Inc.
> + *
> + * based on origin code from
> + * Copyright (C) 2013 Altera Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/firmware.h>
> +#include <linux/fpga.h>
> +#include <linux/fs.h>
> +#include <linux/idr.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +
> +static struct idr fpga_mgr_idr;
> +static spinlock_t fpga_mgr_idr_lock;
> +static struct class *fpga_mgr_class;
Use the initialisers where available:
static DEFINE_IDR(fpga_mgr_idr);
static DEFINE_SPINLOCK(fpga_mgr_idr_lock);
> +/**
> + * fpga_mgr_name_show - Show fpga manager name
> + * @dev: Pointer to the device structure
> + * @attr: Pointer to the device attribute structure
> + * @buf: Pointer to the buffer location
> + *
> + * Returns the number of bytes copied to @buf, a negative error number otherwise
> + */
> +static ssize_t fpga_mgr_name_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct fpga_manager *mgr = dev_get_drvdata(dev);
> +
> + if (!mgr)
> + return -ENODEV;
> +
> + return snprintf(buf, sizeof(mgr->name), "%s\n", mgr->name);
> +}
> +
> +/**
> + * fpga_mgr_status_show - Show fpga manager status
> + * @dev: Pointer to the device structure
> + * @attr: Pointer to the device attribute structure
> + * @buf: Pointer to the buffer location
> + *
> + * Returns the number of bytes copied to @buf, a negative error number otherwise
> + */
> +static ssize_t fpga_mgr_status_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct fpga_manager *mgr = dev_get_drvdata(dev);
> +
> + if (!mgr || !mgr->mops || !mgr->mops->status)
> + return -ENODEV;
> +
> + return mgr->mops->status(mgr, buf);
> +}
> +
> +/**
> + * fpga_mgr_read - Read data from fpga
> + * @mgr: Pointer to the fpga manager structure
> + * @buf: Pointer to the buffer location
> + * @count: Pointer to the number of copied bytes
> + *
> + * Function reads fpga bitstream and copy them to output buffer.
> + *
> + * Returns the number of bytes copied to @buf, a negative error number otherwise
> + */
> +static int fpga_mgr_read(struct fpga_manager *mgr, char *buf, ssize_t *count)
> +{
> + int ret;
> +
> + if (test_and_set_bit_lock(FPGA_MGR_DEV_BUSY, &mgr->flags))
> + return -EBUSY;
> +
> + if (!mgr->mops || !mgr->mops->read) {
> + dev_err(mgr->dev,
> + "Controller doesn't support read operations!\n");
> + return -EPERM;
This error path leaves the FPGA_MGR_DEV_BUSY locked. Same on the error
paths below.
> + }
> +
> + if (mgr->mops->read_init) {
> + ret = mgr->mops->read_init(mgr);
> + if (ret) {
> + dev_err(mgr->dev, "Failed in read-init!\n");
Error messages like this can be useful for debugging, but can also be
used to spam the system log if the user can easily trigger the failure
condition (e.g. by passing deliberately incorrect arguments). Consider
dropping these, and other messages, to dev_dbg.
> + return ret;
> + }
> + }
> +
> + if (mgr->mops->read) {
> + ret = mgr->mops->read(mgr, buf, count);
> + if (ret) {
> + dev_err(mgr->dev, "Failed to read firmware!\n");
> + return ret;
> + }
> + }
> +
> + if (mgr->mops->read_complete) {
> + ret = mgr->mops->read_complete(mgr);
> + if (ret) {
> + dev_err(mgr->dev, "Failed in read-complete!\n");
> + return ret;
> + }
> + }
> +
> + clear_bit_unlock(FPGA_MGR_DEV_BUSY, &mgr->flags);
> +
> + return 0;
> +}
> +
> +/**
> + * fpga_mgr_attr_read - Read data from fpga
> + * @dev: Pointer to the device structure
> + * @attr: Pointer to the device attribute structure
> + * @buf: Pointer to the buffer location
> + *
> + * Function reads fpga bitstream and copy them to output buffer
> + *
> + * Returns the number of bytes copied to @buf, a negative error number otherwise
> + */
> +static ssize_t fpga_mgr_attr_read(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct fpga_manager *mgr = dev_get_drvdata(dev);
> + ssize_t count;
> + int ret;
> +
> + if (mgr && mgr->fpga_read)
> + ret = mgr->fpga_read(mgr, buf, &count);
> +
> + return ret == 0 ? count : -EPERM;
Why doesn't this return the value of ret from mgr->fpga_read if there is
an error?
> +}
> +
> +/**
> + * fpga_mgr_write - Write data to fpga
> + * @mgr: Pointer to the fpga manager structure
> + * @fw_name: Pointer to the buffer location with bistream firmware filename
> + *
> + * @buf contains firmware filename which is loading through firmware
> + * interface and passed to the fpga driver.
> + *
> + * Returns string lenght added to @fw_name, a negative error number otherwise
Typo: "length"
> + */
> +static int fpga_mgr_write(struct fpga_manager *mgr, const char *fw_name)
> +{
> + int ret = 0;
> + const struct firmware *fw;
> +
> + if (!fw_name || !strlen(fw_name)) {
> + dev_err(mgr->dev, "Firmware name is not specified!\n");
> + return -EINVAL;
> + }
> +
> + if (!mgr->mops || !mgr->mops->write) {
> + dev_err(mgr->dev,
> + "Controller doesn't support write operations!\n");
> + return -EPERM;
> + }
> +
> + ret = request_firmware(&fw, fw_name, mgr->dev);
> + if (ret) {
> + dev_err(mgr->dev, "Failed to load firmware %s!\n", fw_name);
> + return ret;
> + }
> +
> + if (test_and_set_bit_lock(FPGA_MGR_DEV_BUSY, &mgr->flags))
> + return -EBUSY;
> +
> + /* Init controller */
> + if (mgr->mops->write_init) {
> + ret = mgr->mops->write_init(mgr);
> + if (ret) {
> + dev_err(mgr->dev, "Failed to write_init!\n");
> + return ret;
Missing lock and firmware release. Same on error paths below.
> + }
> + }
> +
> + /* Do write */
> + ret = mgr->mops->write(mgr, (void *)fw->data, fw->size);
It might be better to change the second argument of the write callback
to take const u8 * so that you don't need to cast.
> + if (ret) {
> + dev_err(mgr->dev, "Failed to write data!\n");
> + return ret;
> + }
> +
> + if (mgr->mops->write_complete) {
> + ret = mgr->mops->write_complete(mgr);
> + if (ret) {
> + dev_err(mgr->dev, "Failed to write_init!\n");
> + return ret;
> + }
> + }
> +
> + release_firmware(fw);
> +
> + clear_bit_unlock(FPGA_MGR_DEV_BUSY, &mgr->flags);
> +
> + return 0;
> +}
> +
> +/**
> + * fpga_mgr_attr_write - Write data to fpga
> + * @dev: Pointer to the device structure
> + * @attr: Pointer to the device attribute structure
> + * @buf: Pointer to the buffer location with bistream firmware filename
> + * @count: Number of characters in @buf
> + *
> + * @buf contains firmware filename which is loading through firmware
> + * interface and passed to the fpga driver.
> + *
> + * Returns string lenght added to @buf, a negative error number otherwise
Typo: "length". Check elsewhere.
> + */
> +static ssize_t fpga_mgr_attr_write(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct fpga_manager *mgr = dev_get_drvdata(dev);
> + int ret;
> +
> + if (mgr && mgr->fpga_write)
> + ret = mgr->fpga_write(mgr, buf);
> +
> + return ret == 0 ? strlen(buf) : -EPERM;
Again, this should return the error code from mgr->fpga_write.
> +}
> +
> +static struct device_attribute fpga_mgr_attrs[] = {
> + __ATTR(name, S_IRUGO, fpga_mgr_name_show, NULL),
> + __ATTR(status, S_IRUGO, fpga_mgr_status_show, NULL),
> + __ATTR(fpga, S_IRUGO | S_IWUGO, fpga_mgr_attr_read,
> + fpga_mgr_attr_write),
> + __ATTR_NULL
> +};
> +
> +/**
> + * fpga_mgr_register: Register new fpga manager
> + * @pdev: Pointer to the platform device of fpga manager
> + * @mops: Pointer to the fpga manager operations
> + * @name: Name of fpga manager
> + * @priv: Pointer to the fpga manager private data
> + *
> + * Function register fpga manager with uniq ID and create device
> + * for accessing the device.
> + *
> + * Returns 0 on success, a negative error number otherwise
> + */
> +int fpga_mgr_register(struct platform_device *pdev,
> + struct fpga_manager_ops *mops, char *name, void *priv)
> +{
> + struct fpga_manager *mgr;
> + int ret;
> +
> + if (!mops) {
> + dev_err(&pdev->dev, "Register failed: NO fpga_manager_ops!\n");
> + return -EINVAL;
> + }
> +
> + if (!name || !strlen(name)) {
> + dev_err(&pdev->dev, "Register failed: NO name specific!\n");
> + return -EINVAL;
> + }
You could probably omit these checks, since this is an in-kernel
interface. Callers are expected to follow the documentation correctly.
> +
> + mgr = devm_kzalloc(&pdev->dev, sizeof(*mgr), GFP_KERNEL);
> + if (!mgr) {
> + dev_err(&pdev->dev, "Failed to alloc mgr struct!\n");
kmalloc and friends already print a warning and stack trace if
__GFP_NOWARN is not passed, so this error message is not required.
> + return -ENOMEM;
> + }
> +
> + /* Get unique number */
> + idr_preload(GFP_KERNEL);
> + spin_lock(&fpga_mgr_idr_lock);
> + ret = idr_alloc(&fpga_mgr_idr, mgr, 0, 0, GFP_NOWAIT);
> + if (ret >= 0)
> + mgr->nr = ret;
> + spin_unlock(&fpga_mgr_idr_lock);
> + idr_preload_end();
> + if (ret < 0)
> + return ret;
> +
> + /* Setup all necessary information */
> + mgr->dev = &pdev->dev;
> + mgr->mops = mops;
> + mgr->priv = priv;
> + mgr->fpga_read = fpga_mgr_read;
> + mgr->fpga_write = fpga_mgr_write;
> + strlcpy(mgr->name, name, sizeof(mgr->name));
> +
> + mgr->dev = device_create(fpga_mgr_class, get_device(&pdev->dev),
> + MKDEV(0, 0), mgr, "fpga%d", mgr->nr);
> + if (IS_ERR(mgr->dev)) {
> + spin_lock(&fpga_mgr_idr_lock);
> + idr_remove(&fpga_mgr_idr, mgr->nr);
> + spin_unlock(&fpga_mgr_idr_lock);
> +
> + dev_err(&pdev->dev, "Failed to create device!\n");
> + return PTR_ERR(mgr->dev);
put_device or kfree to release the devm_kzalloc above?
> + }
> +
> + platform_set_drvdata(pdev, mgr);
> +
> + dev_info(&pdev->dev, "Fpga manager [%s] registered as minor %d\n",
> + mgr->name, mgr->nr);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(fpga_mgr_register);
> +
> +/**
> + * fpga_mgr_unregister: Remove fpga manager
> + * @pdev: Pointer to the platform device of fpga manager
> + *
> + * Function unregister fpga manager and release all temporary structures
> + *
> + * Returns 0 for all cases
> + */
> +int fpga_mgr_unregister(struct platform_device *pdev)
> +{
> + struct fpga_manager *mgr = platform_get_drvdata(pdev);
> +
> + if (mgr && mgr->mops && mgr->mops->fpga_remove)
> + mgr->mops->fpga_remove(mgr);
> +
> + device_unregister(mgr->dev);
> +
> + spin_lock(&fpga_mgr_idr_lock);
> + idr_remove(&fpga_mgr_idr, mgr->nr);
> + spin_unlock(&fpga_mgr_idr_lock);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(fpga_mgr_unregister);
> +
> +/**
> + * of_dev_node_match: Compare associated device tree node with
> + * @dev: Pointer to the device
> + * @data: Pointer to the device tree node
> + *
> + * Returns 1 on success
> + */
> +static int of_dev_node_match(struct device *dev, void *data)
> +{
> + return dev->of_node == data;
> +}
> +
> +/**
> + * of_find_fpga_mgr_by_node - Find the fpga_manager associated with a node
> + * @node: Pointer to the device tree node
> + *
> + * Returns fpga_manager pointer, or NULL if not found
> + */
> +struct fpga_manager *of_find_fpga_mgr_by_node(struct device_node *node)
> +{
> + struct device *dev;
> + struct fpga_manager *mgr;
> +
> + dev = bus_find_device(&platform_bus_type, NULL, node,
> + of_dev_node_match);
> + if (!dev)
> + return NULL;
> +
> + mgr = dev_get_drvdata(dev);
> +
> + return mgr ? mgr : NULL;
Umm,
return dev_get_drvdata(dev);
?
> +}
> +EXPORT_SYMBOL(of_find_fpga_mgr_by_node);
> +
> +/**
> + * of_find_fpga_mgr_by_phandle - Find the fpga_manager associated with a node
> + * @node: Pointer to the device tree node
> + * @phandle_name: Pointer to the phandle_name
> + *
> + * Returns fpga_manager pointer, or NULL if not found
> + */
> +struct fpga_manager *of_find_fpga_mgr_by_phandle(struct device_node *node,
> + const char *phandle_name)
> +{
> + struct device_node *fpga_node;
> +
> + fpga_node = of_parse_phandle(node, phandle_name, 0);
> + if (!fpga_node) {
> + pr_err("Phandle not found\n");
> + return ERR_PTR(-ENODEV);
> + }
> +
> + return of_find_fpga_mgr_by_node(fpga_node);
> +}
> +EXPORT_SYMBOL(of_find_fpga_mgr_by_phandle);
> +
> +/**
> + * fpga_mgr_init: Create fpga class
> + */
> +static int __init fpga_mgr_init(void)
> +{
> + pr_info("FPGA Manager framework driver\n");
> +
> + fpga_mgr_class = class_create(THIS_MODULE, "fpga");
Maybe "fpga_manager" since these are manager devices, rather than the
fpga itself.
> + if (IS_ERR(fpga_mgr_class))
> + return PTR_ERR(fpga_mgr_class);
> +
> + idr_init(&fpga_mgr_idr);
> + spin_lock_init(&fpga_mgr_idr_lock);
Remove. This can be done statically as suggested above.
> +
> + fpga_mgr_class->dev_attrs = fpga_mgr_attrs;
> +
> + return 0;
> +}
> +subsys_initcall(fpga_mgr_init);
> +
> +/**
> + * fpga_mgr_exit: Destroy fpga class
> + */
> +static void __exit fpga_mgr_exit(void)
> +{
> + class_destroy(fpga_mgr_class);
> + idr_destroy(&fpga_mgr_idr);
> +}
> +module_exit(fpga_mgr_exit);
> +
> +MODULE_AUTHOR("Xilinx, Inc.");
> +MODULE_DESCRIPTION("FPGA Manager framework driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/fpga.h b/include/linux/fpga.h
> new file mode 100644
> index 0000000..970c42e
> --- /dev/null
> +++ b/include/linux/fpga.h
> @@ -0,0 +1,105 @@
> +/*
> + * FPGA Framework
> + *
> + * Copyright (C) 2013 Xilinx, Inc.
> + *
> + * based on origin code from
> + * Copyright (C) 2013 Altera Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +
> +#ifndef _LINUX_FPGA_H
> +#define _LINUX_FPGA_H
> +
> +struct fpga_manager;
> +
> +/**
> + * struct fpga_manager_ops - FPGA manager driver operations
> + *
> + * @status: The function to call for getting fpga manager status.
> + * Returns number of characters written to the @buf and
> + * a string of the FPGA's status in @bug.
> + * @read_init: The function to call for preparing the FPGA for reading
> + * its confuration data. Returns 0 on success, a negative error
> + * number otherwise.
> + * @read: Read configuration data from the FPGA. Return 0 on success,
> + * a negative error number otherwise.
> + * @read_complete: Return FPGA to a default state after reading is done.
> + * Return 0 on success, a negative error number otherwise.
> + * @write_init: Prepare the FPGA to receive configuration data.
> + * Return 0 on success, a negative error number otherwise.
> + * @write: Write count bytes of configuration data to the FPGA placed in @buf.
> + * Return 0 on success, a negative error number otherwise.
> + * @write_complete: Return FPGA to default state after writing is done.
> + * Return 0 on success, a negative error number otherwise.
> + * @fpga_remove: Set FPGA into a specific state during driver remove.
> + *
> + * fpga_manager_ops are the low level functions implemented by a specific
> + * fpga manager driver. Leaving any of these out that aren't needed is fine
> + * as they are all tested for NULL before being called.
> + */
> +struct fpga_manager_ops {
> + int (*status)(struct fpga_manager *mgr, char *buf);
> + int (*read_init)(struct fpga_manager *mgr);
> + int (*read)(struct fpga_manager *mgr, char *buf, ssize_t *count);
> + int (*read_complete)(struct fpga_manager *mgr);
> + int (*write_init)(struct fpga_manager *mgr);
> + int (*write)(struct fpga_manager *mgr, char *buf, ssize_t count);
> + int (*write_complete)(struct fpga_manager *mgr);
> + void (*fpga_remove)(struct fpga_manager *mgr);
> +};
> +
> +/* flag bits */
> +#define FPGA_MGR_DEV_BUSY 0
> +
> +/**
> + * struct fpga_manager - FPGA manager driver structure
> + *
> + * @name: Name of fpga manager
> + * @dev: Pointer to the device structure
> + * @mops: Pointer to the fpga manager operations
> + * @priv: Pointer to fpga manager private data
> + * @nr: Unique manager number in the system
> + * @flags: For saving temporary
> + * @fpga_read: The function to call for reading configuration data from
> + * the FPGA.
> + * @fpga_write: The function to call for writing configuration data to the FPGA.
> + */
> +struct fpga_manager {
> + char name[48];
Why this limit? It could just be char *, and allocate the string as
necessary.
Also, can this structure be made opaque, so that its definition is in
fpga_mgr.c, and callers only ever access it via the api?
> + struct device *dev;
> + struct fpga_manager_ops *mops;
> + void *priv;
> + unsigned int nr;
> + unsigned long flags;
> + int (*fpga_read)(struct fpga_manager *mgr, char *buf,
> + ssize_t *count);
> + int (*fpga_write)(struct fpga_manager *mgr, const char *fw_name);
> +};
> +
> +int fpga_mgr_register(struct platform_device *pdev,
> + struct fpga_manager_ops *mops, char *name, void *priv);
> +
> +int fpga_mgr_unregister(struct platform_device *pdev);
> +
> +struct fpga_manager *of_find_fpga_mgr_by_node(struct device_node *node);
> +struct fpga_manager *of_find_fpga_mgr_by_phandle(struct device_node *node,
> + const char *phandle_name);
> +
> +#endif /*_LINUX_FPGA_H */
> --
> 1.8.2.3
>
Hi Joe,
On 09/18/2013 06:11 PM, Joe Perches wrote:
> On Wed, 2013-09-18 at 17:56 +0200, Michal Simek wrote:
>> This new subsystem should unify all fpga drivers which
>> do the same things. Load configuration data to fpga
>> or another programmable logic through common interface.
>> It doesn't matter if it is MMIO device, gpio bitbanging,
>> etc. connection. The point is to have the same
>> inteface for these drivers.
>
> Is this really a "driver".
> Perhaps it's more of a core kernel function
> and belongs in kernel.
This is not a driver just kernel subsystem and real drivers
will register itself in this subsystem.
>
>> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
>
> The error messages are a little shouty with all
> the unnecessary "!" uses.
>
> []
Fixed.
>
>> +/**
>> + * fpga_mgr_read - Read data from fpga
>> + * @mgr: Pointer to the fpga manager structure
>> + * @buf: Pointer to the buffer location
>> + * @count: Pointer to the number of copied bytes
>> + *
>> + * Function reads fpga bitstream and copy them to output buffer.
>> + *
>> + * Returns the number of bytes copied to @buf, a negative error number otherwise
>> + */
>> +static int fpga_mgr_read(struct fpga_manager *mgr, char *buf, ssize_t *count)
>> +{
>> + int ret;
>> +
>> + if (test_and_set_bit_lock(FPGA_MGR_DEV_BUSY, &mgr->flags))
>> + return -EBUSY;
>> +
>> + if (!mgr->mops || !mgr->mops->read) {
>> + dev_err(mgr->dev,
>> + "Controller doesn't support read operations!\n");
>> + return -EPERM;
>> + }
>> +
>> + if (mgr->mops->read_init) {
>> + ret = mgr->mops->read_init(mgr);
>> + if (ret) {
>> + dev_err(mgr->dev, "Failed in read-init!\n");
>> + return ret;
>> + }
>> + }
>> +
>> + if (mgr->mops->read) {
>> + ret = mgr->mops->read(mgr, buf, count);
>> + if (ret) {
>> + dev_err(mgr->dev, "Failed to read firmware!\n");
>> + return ret;
>> + }
>> + }
>> +
>> + if (mgr->mops->read_complete) {
>> + ret = mgr->mops->read_complete(mgr);
>> + if (ret) {
>> + dev_err(mgr->dev, "Failed in read-complete!\n");
>> + return ret;
>> + }
>> + }
>> +
>> + clear_bit_unlock(FPGA_MGR_DEV_BUSY, &mgr->flags);
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * fpga_mgr_attr_read - Read data from fpga
>> + * @dev: Pointer to the device structure
>> + * @attr: Pointer to the device attribute structure
>> + * @buf: Pointer to the buffer location
>> + *
>> + * Function reads fpga bitstream and copy them to output buffer
>> + *
>> + * Returns the number of bytes copied to @buf, a negative error number otherwise
>> + */
>> +static ssize_t fpga_mgr_attr_read(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct fpga_manager *mgr = dev_get_drvdata(dev);
>> + ssize_t count;
>> + int ret;
>> +
>> + if (mgr && mgr->fpga_read)
>> + ret = mgr->fpga_read(mgr, buf, &count);
>> +
>> + return ret == 0 ? count : -EPERM;
>
> EPERM isn't the only error return from fpga_read.
Yeah I know. Will revisit all return codes.
>> +}
>> +
>> +/**
>> + * fpga_mgr_write - Write data to fpga
>> + * @mgr: Pointer to the fpga manager structure
>> + * @fw_name: Pointer to the buffer location with bistream firmware filename
>> + *
>> + * @buf contains firmware filename which is loading through firmware
>> + * interface and passed to the fpga driver.
>> + *
>> + * Returns string lenght added to @fw_name, a negative error number otherwise
>> + */
>> +static int fpga_mgr_write(struct fpga_manager *mgr, const char *fw_name)
>> +{
>> + int ret = 0;
>> + const struct firmware *fw;
>> +
>> + if (!fw_name || !strlen(fw_name)) {
>> + dev_err(mgr->dev, "Firmware name is not specified!\n");
>> + return -EINVAL;
>> + }
>> +
>> + if (!mgr->mops || !mgr->mops->write) {
>> + dev_err(mgr->dev,
>> + "Controller doesn't support write operations!\n");
>> + return -EPERM;
>
> I think you should revisit the return codes.
yap
>
>
>> +/**
>> + * fpga_mgr_attr_write - Write data to fpga
>> + * @dev: Pointer to the device structure
>> + * @attr: Pointer to the device attribute structure
>> + * @buf: Pointer to the buffer location with bistream firmware filename
>> + * @count: Number of characters in @buf
>> + *
>> + * @buf contains firmware filename which is loading through firmware
>> + * interface and passed to the fpga driver.
>> + *
>> + * Returns string lenght added to @buf, a negative error number otherwise
>> + */
>> +static ssize_t fpga_mgr_attr_write(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct fpga_manager *mgr = dev_get_drvdata(dev);
>> + int ret;
>> +
>> + if (mgr && mgr->fpga_write)
>> + ret = mgr->fpga_write(mgr, buf);
>> +
>> + return ret == 0 ? strlen(buf) : -EPERM;
>> +}
>
> Same -EPERM issue as read
yap
>
>> + mgr = devm_kzalloc(&pdev->dev, sizeof(*mgr), GFP_KERNEL);
>> + if (!mgr) {
>> + dev_err(&pdev->dev, "Failed to alloc mgr struct!\n");
>
> Unnecessary OOM message as there's a general dump_stack()
> already done on any OOM without GFP_NOWARN
>
>> diff --git a/include/linux/fpga.h b/include/linux/fpga.h
> []
>> +struct fpga_manager {
>> + char name[48];
>
> Maybe a #define instead of 48?
There is another comment to fix this by pointer which is sensible solution too.
Thanks for review,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
Hi!
> > + Jason Gunthorpe
>
> Thanks, looks interesting, we could possibly use this interface if it
> met our needs..
>
> > On Wed, Sep 18, 2013 at 05:56:39PM +0200, Michal Simek wrote:
> > > This new subsystem should unify all fpga drivers which
> > > do the same things. Load configuration data to fpga
> > > or another programmable logic through common interface.
> > > It doesn't matter if it is MMIO device, gpio bitbanging,
> > > etc. connection. The point is to have the same
> > > inteface for these drivers.
>
> So, we have many years of in-field experience with this and this API
> doesn't really match what we do.
>
> Here are the steps we perform, from userspace:
...
> - Ask kernel to place FPGA into reset and prepare for programming
> * Kernel can return an error (eg FPGA failed to erase, etc)
> * this is the PROG_N low -> DONE high, PROG_N high -> INIT_N high
> sequencing on Xilinx chips
> - Ask kernel to load a bitstream.
> * Userspace locats the bitstream file to load, and the mmaps it.
> * Userspace passes the entire file in a single write() call to the
> kernel which streams it over the configuration bus
> * The kernel can report an erro rhere (eg Xilinx can report CRC
> error)
> - Ask the kernel to verify that configuration is complete.
> * On Xilinx this wait for done to go high
> - Ask the kernel to release the configuration bus (tristate
> all drivers) (or sometimes we have to drive the bus low,
> it depends on the bitfile, user space knows what to do)
>
> It is very important that userspace know exactly which step fails
> because the resolution is different. We use this in a manufacturing
> setting, so failures are expected and need quick root cause
> determination.
>
> You could probably address that need by very clearly defining a
> variety of errno values for the various cases. However, it would be a
> disaster if every driver did something a little different :|
Well, exact steps are a bit hw-specific, too. So, if anything, I'd
suggest standartizing the errno values.
> Using request_firmware exclusively is not useful for us. We
> format the bitfile with a header that contains our internal tracking
> information. Sometimes we need to bitswap the bitfile. Our userspace
> handles all of this and can pass a bitfile in memory to write().
Take a look at
https://www.kernel.org/doc/Documentation/firmware_class/README . You
can do processing at
4), userspace:
- hotplug: cat appropriate_firmware_image > \
/sys/class/firmware/xxx/data
What is the specific reason request_firmware is unsuitable?
Thanks,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi!
> The firmware approach is interesting. It might be less flexible
> compared with my original code (see link to git below) that this is
On the other hand... that's the interface world wants, right? To most
users, fpga bitstream is just a firmware.
> Is there some way a per-device userspace helper can be added that can
> handle adding the headers? Such that different fpga types get different
> helpers?
https://www.kernel.org/doc/Documentation/firmware_class/README
4), userspace:
- hotplug: cat appropriate_firmware_image > \
/sys/class/firmware/xxx/data
I assume udev's firmware.sh could be modified to add headers.
But... if same bitstream is expected to work across multiple FPGAs (is
it?) maybe kernel should hide that difference and provide headers
itself.
Thanks,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi Jason,
On 09/18/2013 10:32 PM, Jason Gunthorpe wrote:
> On Wed, Sep 18, 2013 at 03:15:17PM -0400, Jason Cooper wrote:
>
>> + Jason Gunthorpe
>
> Thanks, looks interesting, we could possibly use this interface if it
> met our needs..
That will be great.
>
>> On Wed, Sep 18, 2013 at 05:56:39PM +0200, Michal Simek wrote:
>>> This new subsystem should unify all fpga drivers which
>>> do the same things. Load configuration data to fpga
>>> or another programmable logic through common interface.
>>> It doesn't matter if it is MMIO device, gpio bitbanging,
>>> etc. connection. The point is to have the same
>>> inteface for these drivers.
>
> So, we have many years of in-field experience with this and this API
> doesn't really match what we do.
then we can talk about it to also provide you enough support to achieve
what you want to achieve.
> Here are the steps we perform, from userspace:
> - Ask kernel to place FPGA into reset and prepare for programming
> * Kernel can return an error (eg FPGA failed to erase, etc)
> * this is the PROG_N low -> DONE high, PROG_N high -> INIT_N high
> sequencing on Xilinx chips
Isn't it enough to do it in your custom driver in read_init/write_init phase?
> - Ask kernel to load a bitstream.
> * Userspace locats the bitstream file to load, and the mmaps it.
> * Userspace passes the entire file in a single write() call to the
> kernel which streams it over the configuration bus
ok
> * The kernel can report an erro rhere (eg Xilinx can report CRC
> error)
yep - you can do it in read_init/write_init phase.
For these generic features like CRC we should probably create
xilinx.c, altera.c, etc files which will provide these generic algorithms.
> - Ask the kernel to verify that configuration is complete.
> * On Xilinx this wait for done to go high
that should be done by read_complete/write_complete.
> - Ask the kernel to release the configuration bus (tristate
> all drivers) (or sometimes we have to drive the bus low,
> it depends on the bitfile, user space knows what to do)
complete phase should be also suitable for that.
>
> It is very important that userspace know exactly which step fails
> because the resolution is different. We use this in a manufacturing
> setting, so failures are expected and need quick root cause
> determination.
>
> You could probably address that need by very clearly defining a
> variety of errno values for the various cases. However, it would be a
> disaster if every driver did something a little different :|
Sure I agree with you and others that we have to improve that error values
but in general we can just exactly identify correct error values
which driver should return. I can't see any problem in that.
>
> Using request_firmware exclusively is not useful for us. We
> format the bitfile with a header that contains our internal tracking
> information. Sometimes we need to bitswap the bitfile. Our userspace
> handles all of this and can pass a bitfile in memory to write().
>
> request_firmware would be horrible to use :)
>
> Our API uses a binary sysfs attribute to stream the FPGA data, you
> might want to consider that.
It depends where are you going to add this functionality but anyway
in origin Altera driver they use char device but I think that using
request firmware is also one way to go.
But I am not saying that we have to use it exlusively.
As you see the part of fpga_manager struct is just simple write
function which can be used there.
It means if you don't like that device attributes that you are passing
just firmware name we can change that.
I have no problem to use binary sysfs attribute just for reading/writing data
directly to the end driver instead of passing there just information
about firmware name itself.
It should just end up in very similar calling
sequence where it will call one function which will write data
to fpga though any interface.
But the point is that user access to all these devices is the same
and doesn't matter if this zynq pl, altera socfpga, fpga connected
through gpio, etc. These end drivers will be unique but interface
should be the same.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
Hi Alan,
On 09/18/2013 11:17 PM, Alan Tull wrote:
> On Wed, 2013-09-18 at 14:32 -0600, Jason Gunthorpe wrote:
>> On Wed, Sep 18, 2013 at 03:15:17PM -0400, Jason Cooper wrote:
>>
>>> + Jason Gunthorpe
>>
>> Thanks, looks interesting, we could possibly use this interface if it
>> met our needs..
>>
>>> On Wed, Sep 18, 2013 at 05:56:39PM +0200, Michal Simek wrote:
>>>> This new subsystem should unify all fpga drivers which
>>>> do the same things. Load configuration data to fpga
>>>> or another programmable logic through common interface.
>>>> It doesn't matter if it is MMIO device, gpio bitbanging,
>>>> etc. connection. The point is to have the same
>>>> inteface for these drivers.
>>
>> So, we have many years of in-field experience with this and this API
>> doesn't really match what we do.
>>
>> Here are the steps we perform, from userspace:
>> - Ask kernel to place FPGA into reset and prepare for programming
>> * Kernel can return an error (eg FPGA failed to erase, etc)
>> * this is the PROG_N low -> DONE high, PROG_N high -> INIT_N high
>> sequencing on Xilinx chips
>> - Ask kernel to load a bitstream.
>> * Userspace locats the bitstream file to load, and the mmaps it.
>> * Userspace passes the entire file in a single write() call to the
>> kernel which streams it over the configuration bus
>> * The kernel can report an erro rhere (eg Xilinx can report CRC
>> error)
>> - Ask the kernel to verify that configuration is complete.
>> * On Xilinx this wait for done to go high
>> - Ask the kernel to release the configuration bus (tristate
>> all drivers) (or sometimes we have to drive the bus low,
>> it depends on the bitfile, user space knows what to do)
>>
>> It is very important that userspace know exactly which step fails
>> because the resolution is different. We use this in a manufacturing
>> setting, so failures are expected and need quick root cause
>> determination.
>>
>> You could probably address that need by very clearly defining a
>> variety of errno values for the various cases. However, it would be a
>> disaster if every driver did something a little different :|
>>
>> Using request_firmware exclusively is not useful for us. We
>> format the bitfile with a header that contains our internal tracking
>> information. Sometimes we need to bitswap the bitfile. Our userspace
>> handles all of this and can pass a bitfile in memory to write().
>>
>> request_firmware would be horrible to use :)
>>
>> Our API uses a binary sysfs attribute to stream the FPGA data, you
>> might want to consider that.
>>
>> Regards,
>> Jason
>
> The firmware approach is interesting. It might be less flexible
> compared with my original code (see link to git below) that this is
> based on. The original code created a devnode like /dev/fpga0 and a raw
> bitstream could be loaded by doing 'cat bitstream > /dev/fpga0'. Or
> some other userspace app could write the /dev/fpga0 to handle any
> headers that needed to be added to the bitstream.
We are using char device driver for our devcfg device and hwicap too
but this firmware interface is not far from that.
As Jason mentioned we can use binary sysfs attributes and you should
get the same functionality for userspace.
> This code also creates a set of files under /sys for each separate fpga.
> I.e. checking status by looking at /sys/class/fpga/fpag0/status. It
> would be pretty small changes to control reseting the fpga by adding a
> 'reset' file there also (added first to the framework, and an interface
> into the low level fpga manager driver).
Status is just there and for my zynq devcfg driver I do export some status
bits.
root@petalinux:~# cat /sys/class/fpga/fpga0/status
partial_bitstream_status: 0
prog_done_status: 1
dbg_lock_status: 0
seu_lock_status: 0
aes_en_lock_status: 0
aes_status: 0
seu_status: 0
spniden_status: 1
spiden_status: 1
niden_status: 1
dbgen_status: 1
dap_en_status: 7
Originally these values are single device attribute but I need to confirm
exact usage for them. It means in this RFC I probably miss any standard
channel how to change end driver behaviour and probably there should be one more
hook for that.
> I am trying this out with my low level fpga manager driver. I'm very
> curious about your approach and I am wondering whether the firmware
> approach will work for us or not.
I believe so.
> Will this framework handle more than one fpga at a time?
I didn't tried that because I don't have any suitable hw for this on my desk
but I there shouldn't be any problem in that.
> Is there some way a per-device userspace helper can be added that can
> handle adding the headers? Such that different fpga types get different
> helpers?
What do you exactly mean by that? Any example what do you want to achieve?
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
On 09/19/2013 12:08 PM, Pavel Machek wrote:
> Hi!
>
>> The firmware approach is interesting. It might be less flexible
>> compared with my original code (see link to git below) that this is
>
> On the other hand... that's the interface world wants, right? To most
> users, fpga bitstream is just a firmware.
It is one nice way how to get data from one place to another and
it is easy to handle. Using different methods is also possible.
>> Is there some way a per-device userspace helper can be added that can
>> handle adding the headers? Such that different fpga types get different
>> helpers?
>
> https://www.kernel.org/doc/Documentation/firmware_class/README
>
> 4), userspace:
> - hotplug: cat appropriate_firmware_image > \
> /sys/class/firmware/xxx/data
>
> I assume udev's firmware.sh could be modified to add headers.
>
> But... if same bitstream is expected to work across multiple FPGAs (is
> it?) maybe kernel should hide that difference and provide headers
> itself.
There could be a configuration where you want to load one bitstream
to more fpgas with the same type. I can imagine this system and use cases.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
Hi!
> > This code also creates a set of files under /sys for each separate fpga.
> > I.e. checking status by looking at /sys/class/fpga/fpag0/status. It
> > would be pretty small changes to control reseting the fpga by adding a
> > 'reset' file there also (added first to the framework, and an interface
> > into the low level fpga manager driver).
>
> Status is just there and for my zynq devcfg driver I do export some status
> bits.
>
> root@petalinux:~# cat /sys/class/fpga/fpga0/status
> partial_bitstream_status: 0
> prog_done_status: 1
> dbg_lock_status: 0
> seu_lock_status: 0
> aes_en_lock_status: 0
> aes_status: 0
> seu_status: 0
> spniden_status: 1
> spiden_status: 1
> niden_status: 1
> dbgen_status: 1
> dap_en_status: 7
This is single file? If so, it needs to be changed. Greg is rather
clear about that.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On 09/19/2013 01:17 PM, Pavel Machek wrote:
> Hi!
>
>>> This code also creates a set of files under /sys for each separate fpga.
>>> I.e. checking status by looking at /sys/class/fpga/fpag0/status. It
>>> would be pretty small changes to control reseting the fpga by adding a
>>> 'reset' file there also (added first to the framework, and an interface
>>> into the low level fpga manager driver).
>>
>> Status is just there and for my zynq devcfg driver I do export some status
>> bits.
>>
>> root@petalinux:~# cat /sys/class/fpga/fpga0/status
>> partial_bitstream_status: 0
>> prog_done_status: 1
>> dbg_lock_status: 0
>> seu_lock_status: 0
>> aes_en_lock_status: 0
>> aes_status: 0
>> seu_status: 0
>> spniden_status: 1
>> spiden_status: 1
>> niden_status: 1
>> dbgen_status: 1
>> dap_en_status: 7
>
> This is single file? If so, it needs to be changed. Greg is rather
> clear about that.
Don't you have a link to these rules? I have seen any paragraph from Greg
about it but I forget where was it.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
Hi Ryan,
On 09/19/2013 01:45 AM, Ryan Mallon wrote:
> On 19/09/13 01:56, Michal Simek wrote:
>> This new subsystem should unify all fpga drivers which
>> do the same things. Load configuration data to fpga
>> or another programmable logic through common interface.
>> It doesn't matter if it is MMIO device, gpio bitbanging,
>> etc. connection. The point is to have the same
>> inteface for these drivers.
>>
>> Signed-off-by: Michal Simek <[email protected]>
>
> Hi Michal,
>
> Some comments below.
>
> ~Ryan
>
>>
>> ---
>> MAINTAINERS | 7 +
>> drivers/Kconfig | 2 +
>> drivers/Makefile | 1 +
>> drivers/fpga/Kconfig | 18 ++
>> drivers/fpga/Makefile | 5 +
>> drivers/fpga/fpga-mgr.c | 433 ++++++++++++++++++++++++++++++++++++++++++++++++
>> include/linux/fpga.h | 105 ++++++++++++
>> 7 files changed, 571 insertions(+)
>> create mode 100644 drivers/fpga/Kconfig
>> create mode 100644 drivers/fpga/Makefile
>> create mode 100644 drivers/fpga/fpga-mgr.c
>> create mode 100644 include/linux/fpga.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index e61c2e8..5c7597b 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -3427,6 +3427,13 @@ F: include/linux/fmc*.h
>> F: include/linux/ipmi-fru.h
>> K: fmc_d.*register
>>
>> +FPGA SUBSYSTEM
>> +M: Michal Simek <[email protected]>
>> +T: git git://git.xilinx.com/linux-xlnx.git
>> +S: Supported
>> +F: drivers/fpga/
>> +F: include/linux/fpga.h
>> +
>> FPU EMULATOR
>> M: Bill Metzenthen <[email protected]>
>> W: http://floatingpoint.sourceforge.net/emulator/index.html
>> diff --git a/drivers/Kconfig b/drivers/Kconfig
>> index aa43b91..17f3caa 100644
>> --- a/drivers/Kconfig
>> +++ b/drivers/Kconfig
>> @@ -166,4 +166,6 @@ source "drivers/reset/Kconfig"
>>
>> source "drivers/fmc/Kconfig"
>>
>> +source "drivers/fpga/Kconfig"
>> +
>> endmenu
>> diff --git a/drivers/Makefile b/drivers/Makefile
>> index ab93de8..2b5e73b 100644
>> --- a/drivers/Makefile
>> +++ b/drivers/Makefile
>> @@ -152,3 +152,4 @@ obj-$(CONFIG_VME_BUS) += vme/
>> obj-$(CONFIG_IPACK_BUS) += ipack/
>> obj-$(CONFIG_NTB) += ntb/
>> obj-$(CONFIG_FMC) += fmc/
>> +obj-$(CONFIG_FPGA) += fpga/
>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
>> new file mode 100644
>> index 0000000..5357645
>> --- /dev/null
>> +++ b/drivers/fpga/Kconfig
>> @@ -0,0 +1,18 @@
>> +#
>> +# FPGA framework configuration
>> +#
>> +
>> +menu "FPGA devices"
>> +
>> +config FPGA
>> + tristate "FPGA Framework"
>> + help
>> + Say Y here if you want support for configuring FPGAs from the
>> + kernel. The FPGA framework adds a FPGA manager class and FPGA
>> + manager drivers.
>> +
>> +if FPGA
>> +
>> +endif
>> +
>> +endmenu
>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
>> new file mode 100644
>> index 0000000..3c7f29b
>> --- /dev/null
>> +++ b/drivers/fpga/Makefile
>> @@ -0,0 +1,5 @@
>> +#
>> +# Makefile for the fpga framework and fpga manager drivers.
>> +#
>> +
>> +obj-$(CONFIG_FPGA) += fpga-mgr.o
>> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
>> new file mode 100644
>> index 0000000..7312efd
>> --- /dev/null
>> +++ b/drivers/fpga/fpga-mgr.c
>> @@ -0,0 +1,433 @@
>> +/*
>> + * FPGA Framework
>> + *
>> + * Copyright (C) 2013 Xilinx, Inc.
>> + *
>> + * based on origin code from
>> + * Copyright (C) 2013 Altera Corporation
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <linux/firmware.h>
>> +#include <linux/fpga.h>
>> +#include <linux/fs.h>
>> +#include <linux/idr.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/slab.h>
>> +
>> +static struct idr fpga_mgr_idr;
>> +static spinlock_t fpga_mgr_idr_lock;
>> +static struct class *fpga_mgr_class;
>
> Use the initialisers where available:
>
> static DEFINE_IDR(fpga_mgr_idr);
> static DEFINE_SPINLOCK(fpga_mgr_idr_lock);
done
>> +/**
>> + * fpga_mgr_name_show - Show fpga manager name
>> + * @dev: Pointer to the device structure
>> + * @attr: Pointer to the device attribute structure
>> + * @buf: Pointer to the buffer location
>> + *
>> + * Returns the number of bytes copied to @buf, a negative error number otherwise
>> + */
>> +static ssize_t fpga_mgr_name_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct fpga_manager *mgr = dev_get_drvdata(dev);
>> +
>> + if (!mgr)
>> + return -ENODEV;
>> +
>> + return snprintf(buf, sizeof(mgr->name), "%s\n", mgr->name);
>> +}
>> +
>> +/**
>> + * fpga_mgr_status_show - Show fpga manager status
>> + * @dev: Pointer to the device structure
>> + * @attr: Pointer to the device attribute structure
>> + * @buf: Pointer to the buffer location
>> + *
>> + * Returns the number of bytes copied to @buf, a negative error number otherwise
>> + */
>> +static ssize_t fpga_mgr_status_show(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct fpga_manager *mgr = dev_get_drvdata(dev);
>> +
>> + if (!mgr || !mgr->mops || !mgr->mops->status)
>> + return -ENODEV;
>> +
>> + return mgr->mops->status(mgr, buf);
>> +}
>> +
>> +/**
>> + * fpga_mgr_read - Read data from fpga
>> + * @mgr: Pointer to the fpga manager structure
>> + * @buf: Pointer to the buffer location
>> + * @count: Pointer to the number of copied bytes
>> + *
>> + * Function reads fpga bitstream and copy them to output buffer.
>> + *
>> + * Returns the number of bytes copied to @buf, a negative error number otherwise
>> + */
>> +static int fpga_mgr_read(struct fpga_manager *mgr, char *buf, ssize_t *count)
>> +{
>> + int ret;
>> +
>> + if (test_and_set_bit_lock(FPGA_MGR_DEV_BUSY, &mgr->flags))
>> + return -EBUSY;
>> +
>> + if (!mgr->mops || !mgr->mops->read) {
>> + dev_err(mgr->dev,
>> + "Controller doesn't support read operations!\n");
>> + return -EPERM;
>
> This error path leaves the FPGA_MGR_DEV_BUSY locked. Same on the error
> paths below.
fixed.
>
>> + }
>> +
>> + if (mgr->mops->read_init) {
>> + ret = mgr->mops->read_init(mgr);
>> + if (ret) {
>> + dev_err(mgr->dev, "Failed in read-init!\n");
>
> Error messages like this can be useful for debugging, but can also be
> used to spam the system log if the user can easily trigger the failure
> condition (e.g. by passing deliberately incorrect arguments). Consider
> dropping these, and other messages, to dev_dbg.
>
done.
>> + return ret;
>> + }
>> + }
>> +
>> + if (mgr->mops->read) {
>> + ret = mgr->mops->read(mgr, buf, count);
>> + if (ret) {
>> + dev_err(mgr->dev, "Failed to read firmware!\n");
>> + return ret;
>> + }
>> + }
>> +
>> + if (mgr->mops->read_complete) {
>> + ret = mgr->mops->read_complete(mgr);
>> + if (ret) {
>> + dev_err(mgr->dev, "Failed in read-complete!\n");
>> + return ret;
>> + }
>> + }
>> +
>> + clear_bit_unlock(FPGA_MGR_DEV_BUSY, &mgr->flags);
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * fpga_mgr_attr_read - Read data from fpga
>> + * @dev: Pointer to the device structure
>> + * @attr: Pointer to the device attribute structure
>> + * @buf: Pointer to the buffer location
>> + *
>> + * Function reads fpga bitstream and copy them to output buffer
>> + *
>> + * Returns the number of bytes copied to @buf, a negative error number otherwise
>> + */
>> +static ssize_t fpga_mgr_attr_read(struct device *dev,
>> + struct device_attribute *attr, char *buf)
>> +{
>> + struct fpga_manager *mgr = dev_get_drvdata(dev);
>> + ssize_t count;
>> + int ret;
>> +
>> + if (mgr && mgr->fpga_read)
>> + ret = mgr->fpga_read(mgr, buf, &count);
>> +
>> + return ret == 0 ? count : -EPERM;
>
> Why doesn't this return the value of ret from mgr->fpga_read if there is
> an error?
that's bug. Fixed.
>
>> +}
>> +
>> +/**
>> + * fpga_mgr_write - Write data to fpga
>> + * @mgr: Pointer to the fpga manager structure
>> + * @fw_name: Pointer to the buffer location with bistream firmware filename
>> + *
>> + * @buf contains firmware filename which is loading through firmware
>> + * interface and passed to the fpga driver.
>> + *
>> + * Returns string lenght added to @fw_name, a negative error number otherwise
>
> Typo: "length"
fixed globally.
>
>> + */
>> +static int fpga_mgr_write(struct fpga_manager *mgr, const char *fw_name)
>> +{
>> + int ret = 0;
>> + const struct firmware *fw;
>> +
>> + if (!fw_name || !strlen(fw_name)) {
>> + dev_err(mgr->dev, "Firmware name is not specified!\n");
>> + return -EINVAL;
>> + }
>> +
>> + if (!mgr->mops || !mgr->mops->write) {
>> + dev_err(mgr->dev,
>> + "Controller doesn't support write operations!\n");
>> + return -EPERM;
>> + }
>> +
>> + ret = request_firmware(&fw, fw_name, mgr->dev);
>> + if (ret) {
>> + dev_err(mgr->dev, "Failed to load firmware %s!\n", fw_name);
>> + return ret;
>> + }
>> +
>> + if (test_and_set_bit_lock(FPGA_MGR_DEV_BUSY, &mgr->flags))
>> + return -EBUSY;
>> +
>> + /* Init controller */
>> + if (mgr->mops->write_init) {
>> + ret = mgr->mops->write_init(mgr);
>> + if (ret) {
>> + dev_err(mgr->dev, "Failed to write_init!\n");
>> + return ret;
>
> Missing lock and firmware release. Same on error paths below.
fixed.
>
>> + }
>> + }
>> +
>> + /* Do write */
>> + ret = mgr->mops->write(mgr, (void *)fw->data, fw->size);
>
> It might be better to change the second argument of the write callback
> to take const u8 * so that you don't need to cast.
will look at it.
>> + if (ret) {
>> + dev_err(mgr->dev, "Failed to write data!\n");
>> + return ret;
>> + }
>> +
>> + if (mgr->mops->write_complete) {
>> + ret = mgr->mops->write_complete(mgr);
>> + if (ret) {
>> + dev_err(mgr->dev, "Failed to write_init!\n");
>> + return ret;
>> + }
>> + }
>> +
>> + release_firmware(fw);
>> +
>> + clear_bit_unlock(FPGA_MGR_DEV_BUSY, &mgr->flags);
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * fpga_mgr_attr_write - Write data to fpga
>> + * @dev: Pointer to the device structure
>> + * @attr: Pointer to the device attribute structure
>> + * @buf: Pointer to the buffer location with bistream firmware filename
>> + * @count: Number of characters in @buf
>> + *
>> + * @buf contains firmware filename which is loading through firmware
>> + * interface and passed to the fpga driver.
>> + *
>> + * Returns string lenght added to @buf, a negative error number otherwise
>
> Typo: "length". Check elsewhere.
done.
>
>> + */
>> +static ssize_t fpga_mgr_attr_write(struct device *dev,
>> + struct device_attribute *attr,
>> + const char *buf, size_t count)
>> +{
>> + struct fpga_manager *mgr = dev_get_drvdata(dev);
>> + int ret;
>> +
>> + if (mgr && mgr->fpga_write)
>> + ret = mgr->fpga_write(mgr, buf);
>> +
>> + return ret == 0 ? strlen(buf) : -EPERM;
>
> Again, this should return the error code from mgr->fpga_write.
yep as you see c&p. :-)
>
>> +}
>> +
>> +static struct device_attribute fpga_mgr_attrs[] = {
>> + __ATTR(name, S_IRUGO, fpga_mgr_name_show, NULL),
>> + __ATTR(status, S_IRUGO, fpga_mgr_status_show, NULL),
>> + __ATTR(fpga, S_IRUGO | S_IWUGO, fpga_mgr_attr_read,
>> + fpga_mgr_attr_write),
>> + __ATTR_NULL
>> +};
>> +
>> +/**
>> + * fpga_mgr_register: Register new fpga manager
>> + * @pdev: Pointer to the platform device of fpga manager
>> + * @mops: Pointer to the fpga manager operations
>> + * @name: Name of fpga manager
>> + * @priv: Pointer to the fpga manager private data
>> + *
>> + * Function register fpga manager with uniq ID and create device
>> + * for accessing the device.
>> + *
>> + * Returns 0 on success, a negative error number otherwise
>> + */
>> +int fpga_mgr_register(struct platform_device *pdev,
>> + struct fpga_manager_ops *mops, char *name, void *priv)
>> +{
>> + struct fpga_manager *mgr;
>> + int ret;
>> +
>> + if (!mops) {
>> + dev_err(&pdev->dev, "Register failed: NO fpga_manager_ops!\n");
>> + return -EINVAL;
>> + }
>> +
>> + if (!name || !strlen(name)) {
>> + dev_err(&pdev->dev, "Register failed: NO name specific!\n");
>> + return -EINVAL;
>> + }
>
> You could probably omit these checks, since this is an in-kernel
> interface. Callers are expected to follow the documentation correctly.
I added it just for sure.
>> +
>> + mgr = devm_kzalloc(&pdev->dev, sizeof(*mgr), GFP_KERNEL);
>> + if (!mgr) {
>> + dev_err(&pdev->dev, "Failed to alloc mgr struct!\n");
>
> kmalloc and friends already print a warning and stack trace if
> __GFP_NOWARN is not passed, so this error message is not required.
ok. Fixed. Joe also report this.
BTW: it means that a lot of drivers should be fixed because
some of them definitely have error message in this location.
>
>> + return -ENOMEM;
>> + }
>> +
>> + /* Get unique number */
>> + idr_preload(GFP_KERNEL);
>> + spin_lock(&fpga_mgr_idr_lock);
>> + ret = idr_alloc(&fpga_mgr_idr, mgr, 0, 0, GFP_NOWAIT);
>> + if (ret >= 0)
>> + mgr->nr = ret;
>> + spin_unlock(&fpga_mgr_idr_lock);
>> + idr_preload_end();
>> + if (ret < 0)
>> + return ret;
>> +
>> + /* Setup all necessary information */
>> + mgr->dev = &pdev->dev;
>> + mgr->mops = mops;
>> + mgr->priv = priv;
>> + mgr->fpga_read = fpga_mgr_read;
>> + mgr->fpga_write = fpga_mgr_write;
>> + strlcpy(mgr->name, name, sizeof(mgr->name));
>> +
>> + mgr->dev = device_create(fpga_mgr_class, get_device(&pdev->dev),
>> + MKDEV(0, 0), mgr, "fpga%d", mgr->nr);
>> + if (IS_ERR(mgr->dev)) {
>> + spin_lock(&fpga_mgr_idr_lock);
>> + idr_remove(&fpga_mgr_idr, mgr->nr);
>> + spin_unlock(&fpga_mgr_idr_lock);
>> +
>> + dev_err(&pdev->dev, "Failed to create device!\n");
>> + return PTR_ERR(mgr->dev);
>
> put_device or kfree to release the devm_kzalloc above?
done.
>
>> + }
>> +
>> + platform_set_drvdata(pdev, mgr);
>> +
>> + dev_info(&pdev->dev, "Fpga manager [%s] registered as minor %d\n",
>> + mgr->name, mgr->nr);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(fpga_mgr_register);
>> +
>> +/**
>> + * fpga_mgr_unregister: Remove fpga manager
>> + * @pdev: Pointer to the platform device of fpga manager
>> + *
>> + * Function unregister fpga manager and release all temporary structures
>> + *
>> + * Returns 0 for all cases
>> + */
>> +int fpga_mgr_unregister(struct platform_device *pdev)
>> +{
>> + struct fpga_manager *mgr = platform_get_drvdata(pdev);
>> +
>> + if (mgr && mgr->mops && mgr->mops->fpga_remove)
>> + mgr->mops->fpga_remove(mgr);
>> +
>> + device_unregister(mgr->dev);
>> +
>> + spin_lock(&fpga_mgr_idr_lock);
>> + idr_remove(&fpga_mgr_idr, mgr->nr);
>> + spin_unlock(&fpga_mgr_idr_lock);
>> +
>> + return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(fpga_mgr_unregister);
>> +
>> +/**
>> + * of_dev_node_match: Compare associated device tree node with
>> + * @dev: Pointer to the device
>> + * @data: Pointer to the device tree node
>> + *
>> + * Returns 1 on success
>> + */
>> +static int of_dev_node_match(struct device *dev, void *data)
>> +{
>> + return dev->of_node == data;
>> +}
>> +
>> +/**
>> + * of_find_fpga_mgr_by_node - Find the fpga_manager associated with a node
>> + * @node: Pointer to the device tree node
>> + *
>> + * Returns fpga_manager pointer, or NULL if not found
>> + */
>> +struct fpga_manager *of_find_fpga_mgr_by_node(struct device_node *node)
>> +{
>> + struct device *dev;
>> + struct fpga_manager *mgr;
>> +
>> + dev = bus_find_device(&platform_bus_type, NULL, node,
>> + of_dev_node_match);
>> + if (!dev)
>> + return NULL;
>> +
>> + mgr = dev_get_drvdata(dev);
>> +
>> + return mgr ? mgr : NULL;
>
> Umm,
>
> return dev_get_drvdata(dev);
>
> ?
OOps.
>
>> +}
>> +EXPORT_SYMBOL(of_find_fpga_mgr_by_node);
>> +
>> +/**
>> + * of_find_fpga_mgr_by_phandle - Find the fpga_manager associated with a node
>> + * @node: Pointer to the device tree node
>> + * @phandle_name: Pointer to the phandle_name
>> + *
>> + * Returns fpga_manager pointer, or NULL if not found
>> + */
>> +struct fpga_manager *of_find_fpga_mgr_by_phandle(struct device_node *node,
>> + const char *phandle_name)
>> +{
>> + struct device_node *fpga_node;
>> +
>> + fpga_node = of_parse_phandle(node, phandle_name, 0);
>> + if (!fpga_node) {
>> + pr_err("Phandle not found\n");
>> + return ERR_PTR(-ENODEV);
>> + }
>> +
>> + return of_find_fpga_mgr_by_node(fpga_node);
>> +}
>> +EXPORT_SYMBOL(of_find_fpga_mgr_by_phandle);
>> +
>> +/**
>> + * fpga_mgr_init: Create fpga class
>> + */
>> +static int __init fpga_mgr_init(void)
>> +{
>> + pr_info("FPGA Manager framework driver\n");
>> +
>> + fpga_mgr_class = class_create(THIS_MODULE, "fpga");
>
> Maybe "fpga_manager" since these are manager devices, rather than the
> fpga itself.
done.
>
>> + if (IS_ERR(fpga_mgr_class))
>> + return PTR_ERR(fpga_mgr_class);
>> +
>> + idr_init(&fpga_mgr_idr);
>> + spin_lock_init(&fpga_mgr_idr_lock);
>
> Remove. This can be done statically as suggested above.
done.
>
>> +
>> + fpga_mgr_class->dev_attrs = fpga_mgr_attrs;
>> +
>> + return 0;
>> +}
>> +subsys_initcall(fpga_mgr_init);
>> +
>> +/**
>> + * fpga_mgr_exit: Destroy fpga class
>> + */
>> +static void __exit fpga_mgr_exit(void)
>> +{
>> + class_destroy(fpga_mgr_class);
>> + idr_destroy(&fpga_mgr_idr);
>> +}
>> +module_exit(fpga_mgr_exit);
>> +
>> +MODULE_AUTHOR("Xilinx, Inc.");
>> +MODULE_DESCRIPTION("FPGA Manager framework driver");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/fpga.h b/include/linux/fpga.h
>> new file mode 100644
>> index 0000000..970c42e
>> --- /dev/null
>> +++ b/include/linux/fpga.h
>> @@ -0,0 +1,105 @@
>> +/*
>> + * FPGA Framework
>> + *
>> + * Copyright (C) 2013 Xilinx, Inc.
>> + *
>> + * based on origin code from
>> + * Copyright (C) 2013 Altera Corporation
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <linux/device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/of.h>
>> +
>> +#ifndef _LINUX_FPGA_H
>> +#define _LINUX_FPGA_H
>> +
>> +struct fpga_manager;
>> +
>> +/**
>> + * struct fpga_manager_ops - FPGA manager driver operations
>> + *
>> + * @status: The function to call for getting fpga manager status.
>> + * Returns number of characters written to the @buf and
>> + * a string of the FPGA's status in @bug.
>> + * @read_init: The function to call for preparing the FPGA for reading
>> + * its confuration data. Returns 0 on success, a negative error
>> + * number otherwise.
>> + * @read: Read configuration data from the FPGA. Return 0 on success,
>> + * a negative error number otherwise.
>> + * @read_complete: Return FPGA to a default state after reading is done.
>> + * Return 0 on success, a negative error number otherwise.
>> + * @write_init: Prepare the FPGA to receive configuration data.
>> + * Return 0 on success, a negative error number otherwise.
>> + * @write: Write count bytes of configuration data to the FPGA placed in @buf.
>> + * Return 0 on success, a negative error number otherwise.
>> + * @write_complete: Return FPGA to default state after writing is done.
>> + * Return 0 on success, a negative error number otherwise.
>> + * @fpga_remove: Set FPGA into a specific state during driver remove.
>> + *
>> + * fpga_manager_ops are the low level functions implemented by a specific
>> + * fpga manager driver. Leaving any of these out that aren't needed is fine
>> + * as they are all tested for NULL before being called.
>> + */
>> +struct fpga_manager_ops {
>> + int (*status)(struct fpga_manager *mgr, char *buf);
>> + int (*read_init)(struct fpga_manager *mgr);
>> + int (*read)(struct fpga_manager *mgr, char *buf, ssize_t *count);
>> + int (*read_complete)(struct fpga_manager *mgr);
>> + int (*write_init)(struct fpga_manager *mgr);
>> + int (*write)(struct fpga_manager *mgr, char *buf, ssize_t count);
>> + int (*write_complete)(struct fpga_manager *mgr);
>> + void (*fpga_remove)(struct fpga_manager *mgr);
>> +};
>> +
>> +/* flag bits */
>> +#define FPGA_MGR_DEV_BUSY 0
>> +
>> +/**
>> + * struct fpga_manager - FPGA manager driver structure
>> + *
>> + * @name: Name of fpga manager
>> + * @dev: Pointer to the device structure
>> + * @mops: Pointer to the fpga manager operations
>> + * @priv: Pointer to fpga manager private data
>> + * @nr: Unique manager number in the system
>> + * @flags: For saving temporary
>> + * @fpga_read: The function to call for reading configuration data from
>> + * the FPGA.
>> + * @fpga_write: The function to call for writing configuration data to the FPGA.
>> + */
>> +struct fpga_manager {
>> + char name[48];
>
> Why this limit? It could just be char *, and allocate the string as
> necessary.
I will look at this.
>
> Also, can this structure be made opaque, so that its definition is in
> fpga_mgr.c, and callers only ever access it via the api?
The question is if this will permit you to call this api functions
inside the kernel if you hide it.
End fpga driver doesn't need to work with this structure at all.
I am passing it there but in reality I need just to access private data.
Any example fo doing some experiments will be helpful.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
On 09/18/2013 09:02 PM, Dinh Nguyen wrote:
> Hi Michal,
>
> On Wed, 2013-09-18 at 17:56 +0200, Michal Simek wrote:
>> This new subsystem should unify all fpga drivers which
>> do the same things. Load configuration data to fpga
>> or another programmable logic through common interface.
>> It doesn't matter if it is MMIO device, gpio bitbanging,
>> etc. connection. The point is to have the same
>> inteface for these drivers.
>>
>> Signed-off-by: Michal Simek <[email protected]>
>
> [CC] Alan Tull <[email protected]>
> [CC] Yves Vandervennet <[email protected]>
>
> Thanks for doing this. Myself and Alan Tull had plans to send this
> upstream, but we wanted to make sure that this framework would also work
> on the Xilinx platform as well. Have you had a chance to test this on a
> SOCFGPA platform? If not, we can go do that.
Sorry. No. I don't have any socfpga platform to be able to test it.
I believe Alan will test it.
>>
>> ---
>> MAINTAINERS | 7 +
>> drivers/Kconfig | 2 +
>> drivers/Makefile | 1 +
>> drivers/fpga/Kconfig | 18 ++
>> drivers/fpga/Makefile | 5 +
>> drivers/fpga/fpga-mgr.c | 433 ++++++++++++++++++++++++++++++++++++++++++++++++
>> include/linux/fpga.h | 105 ++++++++++++
>> 7 files changed, 571 insertions(+)
>> create mode 100644 drivers/fpga/Kconfig
>> create mode 100644 drivers/fpga/Makefile
>> create mode 100644 drivers/fpga/fpga-mgr.c
>> create mode 100644 include/linux/fpga.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index e61c2e8..5c7597b 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -3427,6 +3427,13 @@ F: include/linux/fmc*.h
>> F: include/linux/ipmi-fru.h
>> K: fmc_d.*register
>>
>> +FPGA SUBSYSTEM
>> +M: Michal Simek <[email protected]>
>
> Since the basic framework of this driver was done by Alan Tull and
> myself, can you please add Alan and myself as the maintainer here?
If we end up with any reasonable solution which will suitable
for all of us I have no problem to add you here because every change
in this subsystem should be validated by all parties.
You should be definitely in a CC for all these emails which will
be ensured by adding your names here.
But still there will be just one repo which will store patches
for this subsystem.
>> +T: git git://git.xilinx.com/linux-xlnx.git
>> +S: Supported
>> +F: drivers/fpga/
>> +F: include/linux/fpga.h
>> +
>> FPU EMULATOR
>> M: Bill Metzenthen <[email protected]>
>> W: http://floatingpoint.sourceforge.net/emulator/index.html
>> diff --git a/drivers/Kconfig b/drivers/Kconfig
>> index aa43b91..17f3caa 100644
>> --- a/drivers/Kconfig
>> +++ b/drivers/Kconfig
>> @@ -166,4 +166,6 @@ source "drivers/reset/Kconfig"
>>
>> source "drivers/fmc/Kconfig"
>>
>> +source "drivers/fpga/Kconfig"
>> +
>> endmenu
>> diff --git a/drivers/Makefile b/drivers/Makefile
>> index ab93de8..2b5e73b 100644
>> --- a/drivers/Makefile
>> +++ b/drivers/Makefile
>> @@ -152,3 +152,4 @@ obj-$(CONFIG_VME_BUS) += vme/
>> obj-$(CONFIG_IPACK_BUS) += ipack/
>> obj-$(CONFIG_NTB) += ntb/
>> obj-$(CONFIG_FMC) += fmc/
>> +obj-$(CONFIG_FPGA) += fpga/
>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
>> new file mode 100644
>> index 0000000..5357645
>> --- /dev/null
>> +++ b/drivers/fpga/Kconfig
>> @@ -0,0 +1,18 @@
>> +#
>> +# FPGA framework configuration
>> +#
>> +
>> +menu "FPGA devices"
>> +
>> +config FPGA
>> + tristate "FPGA Framework"
>> + help
>> + Say Y here if you want support for configuring FPGAs from the
>> + kernel. The FPGA framework adds a FPGA manager class and FPGA
>> + manager drivers.
>> +
>> +if FPGA
>
> Not sure what this is for?
It is just for adding fpga end driver here. I have here
zynq devcfg driver.
It is just preparation for others end driver.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
On Thu 2013-09-19 13:22:00, Michal Simek wrote:
> On 09/19/2013 01:17 PM, Pavel Machek wrote:
> >> Status is just there and for my zynq devcfg driver I do export some status
> >> bits.
> >>
> >> root@petalinux:~# cat /sys/class/fpga/fpga0/status
> >> partial_bitstream_status: 0
> >> prog_done_status: 1
> >> dbg_lock_status: 0
> >> seu_lock_status: 0
> >> aes_en_lock_status: 0
> >> aes_status: 0
> >> seu_status: 0
> >> spniden_status: 1
> >> spiden_status: 1
> >> niden_status: 1
> >> dbgen_status: 1
> >> dap_en_status: 7
> >
> > This is single file? If so, it needs to be changed. Greg is rather
> > clear about that.
>
> Don't you have a link to these rules? I have seen any paragraph from Greg
> about it but I forget where was it.
"one value per file" and "there must be documentation in
Documentation/ for each file" are the rules, iirc.
There's Documentation/sysfs-rules.txt, but it does not seem too
relevant.
Thanks,
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Thu, Sep 19, 2013 at 02:52:37PM +0200, Pavel Machek wrote:
> On Thu 2013-09-19 13:22:00, Michal Simek wrote:
> > On 09/19/2013 01:17 PM, Pavel Machek wrote:
>
> > >> Status is just there and for my zynq devcfg driver I do export some status
> > >> bits.
> > >>
> > >> root@petalinux:~# cat /sys/class/fpga/fpga0/status
> > >> partial_bitstream_status: 0
> > >> prog_done_status: 1
> > >> dbg_lock_status: 0
> > >> seu_lock_status: 0
> > >> aes_en_lock_status: 0
> > >> aes_status: 0
> > >> seu_status: 0
> > >> spniden_status: 1
> > >> spiden_status: 1
> > >> niden_status: 1
> > >> dbgen_status: 1
> > >> dap_en_status: 7
> > >
> > > This is single file? If so, it needs to be changed. Greg is rather
> > > clear about that.
> >
> > Don't you have a link to these rules? I have seen any paragraph from Greg
> > about it but I forget where was it.
>
> "one value per file" and "there must be documentation in
> Documentation/ for each file" are the rules, iirc.
>
> There's Documentation/sysfs-rules.txt, but it does not seem too
> relevant.
Documentation/filesystems/sysfs.txt says it. It probably should be
added to sysfs-rules.txt as well, but the odds that anyone ever reads
the documentation is so low I doubt it's even worth it.
Also, all sysfs files have to be documented in Documentation/ABI/ which
this patch does not do :(
And yes, multiple values in a single sysfs file is not allowed at all.
thanks,
greg k-h
On 09/19/2013 04:06 PM, Greg KH wrote:
> On Thu, Sep 19, 2013 at 02:52:37PM +0200, Pavel Machek wrote:
>> On Thu 2013-09-19 13:22:00, Michal Simek wrote:
>>> On 09/19/2013 01:17 PM, Pavel Machek wrote:
>>
>>>>> Status is just there and for my zynq devcfg driver I do export some status
>>>>> bits.
>>>>>
>>>>> root@petalinux:~# cat /sys/class/fpga/fpga0/status
>>>>> partial_bitstream_status: 0
>>>>> prog_done_status: 1
>>>>> dbg_lock_status: 0
>>>>> seu_lock_status: 0
>>>>> aes_en_lock_status: 0
>>>>> aes_status: 0
>>>>> seu_status: 0
>>>>> spniden_status: 1
>>>>> spiden_status: 1
>>>>> niden_status: 1
>>>>> dbgen_status: 1
>>>>> dap_en_status: 7
>>>>
>>>> This is single file? If so, it needs to be changed. Greg is rather
>>>> clear about that.
>>>
>>> Don't you have a link to these rules? I have seen any paragraph from Greg
>>> about it but I forget where was it.
>>
>> "one value per file" and "there must be documentation in
>> Documentation/ for each file" are the rules, iirc.
>>
>> There's Documentation/sysfs-rules.txt, but it does not seem too
>> relevant.
>
> Documentation/filesystems/sysfs.txt says it. It probably should be
> added to sysfs-rules.txt as well, but the odds that anyone ever reads
> the documentation is so low I doubt it's even worth it.
>
> Also, all sysfs files have to be documented in Documentation/ABI/ which
> this patch does not do :(
>
> And yes, multiple values in a single sysfs file is not allowed at all.
>
ok. I will read that. This is output from my fpga end driver - nothing
what I have sent for review.
Can you please look at origin patch and tell me what you think about it?
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
On Thu, Sep 19, 2013 at 04:10:46PM +0200, Michal Simek wrote:
> On 09/19/2013 04:06 PM, Greg KH wrote:
> > On Thu, Sep 19, 2013 at 02:52:37PM +0200, Pavel Machek wrote:
> >> On Thu 2013-09-19 13:22:00, Michal Simek wrote:
> >>> On 09/19/2013 01:17 PM, Pavel Machek wrote:
> >>
> >>>>> Status is just there and for my zynq devcfg driver I do export some status
> >>>>> bits.
> >>>>>
> >>>>> root@petalinux:~# cat /sys/class/fpga/fpga0/status
> >>>>> partial_bitstream_status: 0
> >>>>> prog_done_status: 1
> >>>>> dbg_lock_status: 0
> >>>>> seu_lock_status: 0
> >>>>> aes_en_lock_status: 0
> >>>>> aes_status: 0
> >>>>> seu_status: 0
> >>>>> spniden_status: 1
> >>>>> spiden_status: 1
> >>>>> niden_status: 1
> >>>>> dbgen_status: 1
> >>>>> dap_en_status: 7
> >>>>
> >>>> This is single file? If so, it needs to be changed. Greg is rather
> >>>> clear about that.
> >>>
> >>> Don't you have a link to these rules? I have seen any paragraph from Greg
> >>> about it but I forget where was it.
> >>
> >> "one value per file" and "there must be documentation in
> >> Documentation/ for each file" are the rules, iirc.
> >>
> >> There's Documentation/sysfs-rules.txt, but it does not seem too
> >> relevant.
> >
> > Documentation/filesystems/sysfs.txt says it. It probably should be
> > added to sysfs-rules.txt as well, but the odds that anyone ever reads
> > the documentation is so low I doubt it's even worth it.
> >
> > Also, all sysfs files have to be documented in Documentation/ABI/ which
> > this patch does not do :(
> >
> > And yes, multiple values in a single sysfs file is not allowed at all.
> >
>
> ok. I will read that. This is output from my fpga end driver - nothing
> what I have sent for review.
> Can you please look at origin patch and tell me what you think about it?
When I return from LinuxCon/Plumbers and catch up on my pending patch
queue, I will. It seems that others have already provided good code
review, so I suggest sending out a new version with their changes
integrated and not wait for me.
thanks,
greg k-h
On Thu, Sep 19, 2013 at 04:10:46PM +0200, Michal Simek wrote:
> On 09/19/2013 04:06 PM, Greg KH wrote:
> > On Thu, Sep 19, 2013 at 02:52:37PM +0200, Pavel Machek wrote:
> >> On Thu 2013-09-19 13:22:00, Michal Simek wrote:
> >>> On 09/19/2013 01:17 PM, Pavel Machek wrote:
> >>
> >>>>> Status is just there and for my zynq devcfg driver I do export some status
> >>>>> bits.
> >>>>>
> >>>>> root@petalinux:~# cat /sys/class/fpga/fpga0/status
> >>>>> partial_bitstream_status: 0
> >>>>> prog_done_status: 1
> >>>>> dbg_lock_status: 0
> >>>>> seu_lock_status: 0
> >>>>> aes_en_lock_status: 0
> >>>>> aes_status: 0
> >>>>> seu_status: 0
> >>>>> spniden_status: 1
> >>>>> spiden_status: 1
> >>>>> niden_status: 1
> >>>>> dbgen_status: 1
> >>>>> dap_en_status: 7
> >>>>
> >>>> This is single file? If so, it needs to be changed. Greg is rather
> >>>> clear about that.
> >>>
> >>> Don't you have a link to these rules? I have seen any paragraph from Greg
> >>> about it but I forget where was it.
> >>
> >> "one value per file" and "there must be documentation in
> >> Documentation/ for each file" are the rules, iirc.
> >>
> >> There's Documentation/sysfs-rules.txt, but it does not seem too
> >> relevant.
> >
> > Documentation/filesystems/sysfs.txt says it. It probably should be
> > added to sysfs-rules.txt as well, but the odds that anyone ever reads
> > the documentation is so low I doubt it's even worth it.
> >
> > Also, all sysfs files have to be documented in Documentation/ABI/ which
> > this patch does not do :(
> >
> > And yes, multiple values in a single sysfs file is not allowed at all.
> >
>
> ok. I will read that. This is output from my fpga end driver - nothing
> what I have sent for review.
If this is useful to you (and others), you could use debugfs for this.
The rules there are intentionally a lot more lax.
thx,
Jason.
On Thu, Sep 19, 2013 at 10:20:48AM -0400, Jason Cooper wrote:
> On Thu, Sep 19, 2013 at 04:10:46PM +0200, Michal Simek wrote:
> > On 09/19/2013 04:06 PM, Greg KH wrote:
> > > On Thu, Sep 19, 2013 at 02:52:37PM +0200, Pavel Machek wrote:
> > >> On Thu 2013-09-19 13:22:00, Michal Simek wrote:
> > >>> On 09/19/2013 01:17 PM, Pavel Machek wrote:
> > >>
> > >>>>> Status is just there and for my zynq devcfg driver I do export some status
> > >>>>> bits.
> > >>>>>
> > >>>>> root@petalinux:~# cat /sys/class/fpga/fpga0/status
> > >>>>> partial_bitstream_status: 0
> > >>>>> prog_done_status: 1
> > >>>>> dbg_lock_status: 0
> > >>>>> seu_lock_status: 0
> > >>>>> aes_en_lock_status: 0
> > >>>>> aes_status: 0
> > >>>>> seu_status: 0
> > >>>>> spniden_status: 1
> > >>>>> spiden_status: 1
> > >>>>> niden_status: 1
> > >>>>> dbgen_status: 1
> > >>>>> dap_en_status: 7
> > >>>>
> > >>>> This is single file? If so, it needs to be changed. Greg is rather
> > >>>> clear about that.
> > >>>
> > >>> Don't you have a link to these rules? I have seen any paragraph from Greg
> > >>> about it but I forget where was it.
> > >>
> > >> "one value per file" and "there must be documentation in
> > >> Documentation/ for each file" are the rules, iirc.
> > >>
> > >> There's Documentation/sysfs-rules.txt, but it does not seem too
> > >> relevant.
> > >
> > > Documentation/filesystems/sysfs.txt says it. It probably should be
> > > added to sysfs-rules.txt as well, but the odds that anyone ever reads
> > > the documentation is so low I doubt it's even worth it.
> > >
> > > Also, all sysfs files have to be documented in Documentation/ABI/ which
> > > this patch does not do :(
> > >
> > > And yes, multiple values in a single sysfs file is not allowed at all.
> > >
> >
> > ok. I will read that. This is output from my fpga end driver - nothing
> > what I have sent for review.
>
> If this is useful to you (and others), you could use debugfs for this.
> The rules there are intentionally a lot more lax.
Yes, there is only one rule in debugfs, "There is no rules".
greg k-h
On 09/19/2013 04:42 PM, Yves Vandervennet wrote:
> HI Michal.,
>
>
> On Thu, Sep 19, 2013 at 5:55 AM, Michal Simek <[email protected]> wrote:
>
>>
>>
>>>> Will this framework handle more than one fpga at a time?
>>
>>> I didn't tried that because I don't have any suitable hw for this on my
>> desk
>>> but I there shouldn't be any problem in that.
>>
> Supporting multiple FPGA's at one time is key.
>
>>
>>>> Is there some way a per-device userspace helper can be added that can
>>>> handle adding the headers? Such that different fpga types get different
>>>> helpers?
>>
>>> What do you exactly mean by that? Any example what do you want to achieve?
>>
> Bit streams may not be in raw format, so using 'cat' is not possible.
> Xilinx and Altera have their own bit stream "richer" format, that can need
> to be processed before they are presented to the driver(s). So, a hotplug
> helper per manufacturer/FPGA is required. Assuming 'cat' will be used is
> too limited.
> Does it make sense to you?
Sure. We are handling this at the driver level but as I mentioned
previously these specific functions should be added to xilinx/altera.c file
(like crc checking mechanisms, etc).
Only one question remain which is if this driver should provide any hook
for this functions or these functions should be called in the end driver.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
On Thu, 2013-09-19 at 07:18 -0700, Greg KH wrote:
> On Thu, Sep 19, 2013 at 04:10:46PM +0200, Michal Simek wrote:
> > On 09/19/2013 04:06 PM, Greg KH wrote:
> > > On Thu, Sep 19, 2013 at 02:52:37PM +0200, Pavel Machek wrote:
> > >> On Thu 2013-09-19 13:22:00, Michal Simek wrote:
> > >>> On 09/19/2013 01:17 PM, Pavel Machek wrote:
> > >>
> > >>>>> Status is just there and for my zynq devcfg driver I do export some status
> > >>>>> bits.
> > >>>>>
> > >>>>> root@petalinux:~# cat /sys/class/fpga/fpga0/status
> > >>>>> partial_bitstream_status: 0
> > >>>>> prog_done_status: 1
> > >>>>> dbg_lock_status: 0
> > >>>>> seu_lock_status: 0
> > >>>>> aes_en_lock_status: 0
> > >>>>> aes_status: 0
> > >>>>> seu_status: 0
> > >>>>> spniden_status: 1
> > >>>>> spiden_status: 1
> > >>>>> niden_status: 1
> > >>>>> dbgen_status: 1
> > >>>>> dap_en_status: 7
> > >>>>
> > >>>> This is single file? If so, it needs to be changed. Greg is rather
> > >>>> clear about that.
> > >>>
> > >>> Don't you have a link to these rules? I have seen any paragraph from Greg
> > >>> about it but I forget where was it.
> > >>
> > >> "one value per file" and "there must be documentation in
> > >> Documentation/ for each file" are the rules, iirc.
> > >>
> > >> There's Documentation/sysfs-rules.txt, but it does not seem too
> > >> relevant.
> > >
> > > Documentation/filesystems/sysfs.txt says it. It probably should be
> > > added to sysfs-rules.txt as well, but the odds that anyone ever reads
> > > the documentation is so low I doubt it's even worth it.
> > >
> > > Also, all sysfs files have to be documented in Documentation/ABI/ which
> > > this patch does not do :(
> > >
> > > And yes, multiple values in a single sysfs file is not allowed at all.
> > >
> >
> > ok. I will read that. This is output from my fpga end driver - nothing
> > what I have sent for review.
> > Can you please look at origin patch and tell me what you think about it?
>
> When I return from LinuxCon/Plumbers and catch up on my pending patch
> queue, I will. It seems that others have already provided good code
> review, so I suggest sending out a new version with their changes
> integrated and not wait for me.
Michal,
I am currently porting my altera fpga driver to work with your changes.
Please go ahead and post your xilinx fpga driver patch, that would help
accelerate my testing and give us a chance to give more feedback quickly
and arrive a satisfactory solution that will work for more of the key
players here.
Thanks,
ALan
>
> thanks,
>
> greg k-h
>
On Thu, Sep 19, 2013 at 5:55 AM, Michal Simek <[email protected]> wrote:
>> Is there some way a per-device userspace helper can be added that can
>> handle adding the headers? Such that different fpga types get different
>> helpers?
>
> What do you exactly mean by that? Any example what do you want to achieve?
Bit streams may not be in raw format, so using 'cat' is not possible.
Xilinx and Altera have their own bit stream "richer" format, that need
to be processed before they are presented to the driver(s). So, a
hotplug helper per manufacturer/FPGA is required. Assuming 'cat' will
be used is too limiting.
Does it make sense to you?
Thanks
Yves
Altera Corp.
> >> +/**
> >> + * fpga_mgr_attr_read - Read data from fpga
> >> + * @dev: Pointer to the device structure
> >> + * @attr: Pointer to the device attribute structure
> >> + * @buf: Pointer to the buffer location
> >> + *
> >> + * Function reads fpga bitstream and copy them to output buffer
> >> + *
> >> + * Returns the number of bytes copied to @buf, a negative error number otherwise
> >> + */
> >> +static ssize_t fpga_mgr_attr_read(struct device *dev,
> >> + struct device_attribute *attr, char *buf)
> >> +{
> >> + struct fpga_manager *mgr = dev_get_drvdata(dev);
> >> + ssize_t count;
> >> + int ret;
> >> +
> >> + if (mgr && mgr->fpga_read)
> >> + ret = mgr->fpga_read(mgr, buf, &count);
> >> +
> >> + return ret == 0 ? count : -EPERM;
> >
> > EPERM isn't the only error return from fpga_read.
>
> Yeah I know. Will revisit all return codes.
Build warning:
/home/atull/repos/linux-socfpga/drivers/fpga/fpga-mgr.c:145:26: warning:
‘ret’ may be used uninitialized in this function [-Wmaybe-uninitialized]
>
> >
> >
> >> +/**
> >> + * fpga_mgr_attr_write - Write data to fpga
> >> + * @dev: Pointer to the device structure
> >> + * @attr: Pointer to the device attribute structure
> >> + * @buf: Pointer to the buffer location with bistream firmware filename
> >> + * @count: Number of characters in @buf
> >> + *
> >> + * @buf contains firmware filename which is loading through firmware
> >> + * interface and passed to the fpga driver.
> >> + *
> >> + * Returns string lenght added to @buf, a negative error number otherwise
> >> + */
> >> +static ssize_t fpga_mgr_attr_write(struct device *dev,
> >> + struct device_attribute *attr,
> >> + const char *buf, size_t count)
> >> +{
> >> + struct fpga_manager *mgr = dev_get_drvdata(dev);
> >> + int ret;
> >> +
> >> + if (mgr && mgr->fpga_write)
> >> + ret = mgr->fpga_write(mgr, buf);
> >> +
> >> + return ret == 0 ? strlen(buf) : -EPERM;
> >> +}
> >
> > Same -EPERM issue as read
>
> yap
Build warning:
/home/atull/repos/linux-socfpga/drivers/fpga/fpga-mgr.c:236:2: warning:
‘ret’ may be used uninitialized in this function [-Wmaybe-uninitialized]
Best Regards,
Alan
On Thu, Sep 19, 2013 at 10:18:03AM -0500, Yves Vandervennet wrote:
> On Thu, Sep 19, 2013 at 5:55 AM, Michal Simek <[email protected]> wrote:
>
> >> Is there some way a per-device userspace helper can be added that can
> >> handle adding the headers? Such that different fpga types get different
> >> helpers?
> >
> > What do you exactly mean by that? Any example what do you want to achieve?
> Bit streams may not be in raw format, so using 'cat' is not possible.
> Xilinx and Altera have their own bit stream "richer" format, that need
> to be processed before they are presented to the driver(s). So, a
> hotplug helper per manufacturer/FPGA is required. Assuming 'cat' will
> be used is too limiting.
> Does it make sense to you?
IMHO it doesn't make sense to use request firmware for this, in
general.
1) The driver doesn't know what firmware to request. It just knows
how to send it to a FPGA.
2) Telling the kernel a filename via sysfs and then having it go
around the long way via request_firwmare to get the data is silly.
Just give the kernel the actual data instead of a file name
3) It is hard to use, when userspace writes the file name the kernel
doesn't return until request_firmware completes, which means you
need multiple threads in user space to make this work.
4) Converting an input file into bitstreamable format should be done
in userspace. This doesn't require request_firmware. If you want
to make it automatic then a program invoked via udev is the way
to go.
Jason
On Thu 2013-09-19 07:37:15, Greg KH wrote:
> On Thu, Sep 19, 2013 at 10:20:48AM -0400, Jason Cooper wrote:
> > On Thu, Sep 19, 2013 at 04:10:46PM +0200, Michal Simek wrote:
> > > On 09/19/2013 04:06 PM, Greg KH wrote:
> > > > On Thu, Sep 19, 2013 at 02:52:37PM +0200, Pavel Machek wrote:
> > > >> On Thu 2013-09-19 13:22:00, Michal Simek wrote:
> > > >>> On 09/19/2013 01:17 PM, Pavel Machek wrote:
> > > >>
> > > >>>>> Status is just there and for my zynq devcfg driver I do export some status
> > > >>>>> bits.
> > > >>>>>
> > > >>>>> root@petalinux:~# cat /sys/class/fpga/fpga0/status
> > > >>>>> partial_bitstream_status: 0
> > > >>>>> prog_done_status: 1
> > > >>>>> dbg_lock_status: 0
> > > >>>>> seu_lock_status: 0
> > > >>>>> aes_en_lock_status: 0
> > > >>>>> aes_status: 0
> > > >>>>> seu_status: 0
> > > >>>>> spniden_status: 1
> > > >>>>> spiden_status: 1
> > > >>>>> niden_status: 1
> > > >>>>> dbgen_status: 1
> > > >>>>> dap_en_status: 7
> > > >>>>
> > > >>>> This is single file? If so, it needs to be changed. Greg is rather
> > > >>>> clear about that.
> > > >>>
> > > >>> Don't you have a link to these rules? I have seen any paragraph from Greg
> > > >>> about it but I forget where was it.
> > > >>
> > > >> "one value per file" and "there must be documentation in
> > > >> Documentation/ for each file" are the rules, iirc.
> > > >>
> > > >> There's Documentation/sysfs-rules.txt, but it does not seem too
> > > >> relevant.
> > > >
> > > > Documentation/filesystems/sysfs.txt says it. It probably should be
> > > > added to sysfs-rules.txt as well, but the odds that anyone ever reads
> > > > the documentation is so low I doubt it's even worth it.
> > > >
> > > > Also, all sysfs files have to be documented in Documentation/ABI/ which
> > > > this patch does not do :(
> > > >
> > > > And yes, multiple values in a single sysfs file is not allowed at all.
> > > >
> > >
> > > ok. I will read that. This is output from my fpga end driver - nothing
> > > what I have sent for review.
> >
> > If this is useful to you (and others), you could use debugfs for this.
> > The rules there are intentionally a lot more lax.
>
> Yes, there is only one rule in debugfs, "There is no rules".
Yeah. OTOH, depending on debugfs for normal operation would be
ugly. Don't do that (tm).
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Thu, 2013-09-19 at 13:02 +0200, Michal Simek wrote:
> On 09/19/2013 12:08 PM, Pavel Machek wrote:
> > Hi!
> >
> >> The firmware approach is interesting. It might be less flexible
> >> compared with my original code (see link to git below) that this is
> >
> > On the other hand... that's the interface world wants, right? To most
> > users, fpga bitstream is just a firmware.
>
> It is one nice way how to get data from one place to another and
> it is easy to handle. Using different methods is also possible.
>
> >> Is there some way a per-device userspace helper can be added that can
> >> handle adding the headers? Such that different fpga types get different
> >> helpers?
> >
> > https://www.kernel.org/doc/Documentation/firmware_class/README
> >
> > 4), userspace:
> > - hotplug: cat appropriate_firmware_image > \
> > /sys/class/firmware/xxx/data
> >
> > I assume udev's firmware.sh could be modified to add headers.
> >
> > But... if same bitstream is expected to work across multiple FPGAs (is
> > it?) maybe kernel should hide that difference and provide headers
> > itself.
>
> There could be a configuration where you want to load one bitstream
> to more fpgas with the same type. I can imagine this system and use cases.
>
> Thanks,
> Michal
>
>
Hi Michael,
I have ported the altera fpga manager driver to work with your version
of the fpga manager framework. It works fine if I use the
firmware_class.c's built-in support to load the firmware, but not with a
userspace helper.
I see my helper script get called, but I don't see 'loading' and 'data'
show up under /sys. I have CONFIG_FW_LOADER_USER_HELPER=y enabled and
have done enough printf debugging to see that there was no failure in
creating the attributes as far as firmware_class.c is concerned.
More details:
I'm taking cues from Documentation/firmware_class here.
This is with the 3.11 kernel.
I have CONFIG_FW_LOADER_USER_HELPER=y enabled.
I have this udev rule:
SUBSYSTEM=="firmware", ACTION=="add", RUN+="/lib/udev/hotplug-script"
My hotplug-script is linux/Documentation/firmware_class/hotplug-script
What I am seeing when I request 'image.rbf' is that my hotplug-script
gets run with this DEVPATH set:
DEVPATH == /devices/soc.0/ff706000.fpgamgr/fpga/fpga0/image.rbf
I added debug to my hotplug-script and it could not find 'loading' or
'data' appearing under /sys anywhere when it got called. According to
the firmware_class/README, these should appear under
/sys/class/firmware. However according to the
firmware_class/hotplug-script, they should be under /sys/$DEVPATH.
When I look at firmware_class.c, that code wants these attributes
to show up under the firmware class. Again, with printf debugging,
I don't see firmware_class.c being unhappy until it decides it has
timed out (which happens quickly).
I was wondering what behavior you were seeing with userspace helpers.
Alan
On 09/19/2013 05:18 PM, Yves Vandervennet wrote:
> On Thu, Sep 19, 2013 at 5:55 AM, Michal Simek <[email protected]> wrote:
>
>>> Is there some way a per-device userspace helper can be added that can
>>> handle adding the headers? Such that different fpga types get different
>>> helpers?
>>
>> What do you exactly mean by that? Any example what do you want to achieve?
> Bit streams may not be in raw format, so using 'cat' is not possible.
> Xilinx and Altera have their own bit stream "richer" format, that need
> to be processed before they are presented to the driver(s). So, a
> hotplug helper per manufacturer/FPGA is required. Assuming 'cat' will
> be used is too limiting.
> Does it make sense to you?
Definitely this make sense to me but the point is if this should be
the part of this patch or just separate altera.c/xilinx.c file which
is able to detect and check this format.
I am not saying that it can't be there but currently without
any code I can't see any reason why there should be hook for code
which doesn't exist.
M
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
On 09/19/2013 07:28 PM, Jason Gunthorpe wrote:
> On Thu, Sep 19, 2013 at 10:18:03AM -0500, Yves Vandervennet wrote:
>> On Thu, Sep 19, 2013 at 5:55 AM, Michal Simek <[email protected]> wrote:
>>
>>>> Is there some way a per-device userspace helper can be added that can
>>>> handle adding the headers? Such that different fpga types get different
>>>> helpers?
>>>
>>> What do you exactly mean by that? Any example what do you want to achieve?
>> Bit streams may not be in raw format, so using 'cat' is not possible.
>> Xilinx and Altera have their own bit stream "richer" format, that need
>> to be processed before they are presented to the driver(s). So, a
>> hotplug helper per manufacturer/FPGA is required. Assuming 'cat' will
>> be used is too limiting.
>> Does it make sense to you?
>
> IMHO it doesn't make sense to use request firmware for this, in
> general.
If this is not the best way for you than fine just don't use it.
As I wrote I see a value to have sysfs bin file for this and
you don't need to use this request firmware interface.
>
> 1) The driver doesn't know what firmware to request. It just knows
> how to send it to a FPGA.
But dts property in the manager driver which uses this as end driver
can know that.
> 2) Telling the kernel a filename via sysfs and then having it go
> around the long way via request_firwmare to get the data is silly.
> Just give the kernel the actual data instead of a file name
Firmware interface is valid way how to pass bitstream to the kernel.
If you don't like just don't use it. For example you can add firmware blob directly
to the kernel and load this at bootup phase without user-space access.
> 3) It is hard to use, when userspace writes the file name the kernel
> doesn't return until request_firmware completes, which means you
> need multiple threads in user space to make this work.
that's again and again.
> 4) Converting an input file into bitstreamable format should be done
> in userspace. This doesn't require request_firmware. If you want
> to make it automatic then a program invoked via udev is the way
> to go.
why are you restricting who should do this work? From my point of view
you are just pushing to have open way how to do it from user space
and there will be this option. But don't try to restrict others
if they want to do it just in the kernel in early phase.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
On Mon, Sep 23, 2013 at 03:10:11PM +0200, Michal Simek wrote:
> > 1) The driver doesn't know what firmware to request. It just knows
> > how to send it to a FPGA.
>
> But dts property in the manager driver which uses this as end driver
> can know that.
I think the device tree maintainers would push back on this since it
is not "describing the hardware"
> > 2) Telling the kernel a filename via sysfs and then having it go
> > around the long way via request_firwmare to get the data is silly.
> > Just give the kernel the actual data instead of a file name
>
> Firmware interface is valid way how to pass bitstream to the kernel.
> If you don't like just don't use it. For example you can add
> firmware blob directly to the kernel and load this at bootup phase
> without user-space access.
I'm not against using request firmware for what it is ment for: having
the kernel autonomously load firmware.
I am against the sysfs API in the core code where userspace writes a
file name that is then used to request_firwmare. That is a goofy API
for the reasons I outlined.
It is appropriate to use request firmware at the driver level where
the driver somehow knows what FPGA to request.
> and there will be this option. But don't try to restrict others
> if they want to do it just in the kernel in early phase.
Doing the load in kernel early phase doesn't involve the user space
sysfs interface.
Jason
> I have ported the altera fpga manager driver to work with your version
> of the fpga manager framework. It works fine if I use the
> firmware_class.c's built-in support to load the firmware, but not with a
> userspace helper.
>
Hi Michal,
I cleaned up my udev rules and now I see the userspace helper working.
Just adding one udev rule that points to the standard hotplug-script was
sufficient to see the userspace script working nicely.
All in all, the firmware interface seems pretty easy to use and it gives
us the flexibility to either use the kernel to load the firmware or to
use scripts, so it should work with a variety of use cases.
Since we are arriving at a solution that is suitable for the both of us,
I expect you will be able to add Dinh and myself as maintainers here,
right? I have no problem with there only being one repo to store
patches for this subsystem.
Alan
Hi,
On 09/24/2013 05:55 PM, Alan Tull wrote:
>
>> I have ported the altera fpga manager driver to work with your version
>> of the fpga manager framework. It works fine if I use the
>> firmware_class.c's built-in support to load the firmware, but not with a
>> userspace helper.
>>
>
>
> Hi Michal,
>
> I cleaned up my udev rules and now I see the userspace helper working.
> Just adding one udev rule that points to the standard hotplug-script was
> sufficient to see the userspace script working nicely.
>
> All in all, the firmware interface seems pretty easy to use and it gives
> us the flexibility to either use the kernel to load the firmware or to
> use scripts, so it should work with a variety of use cases.
>
> Since we are arriving at a solution that is suitable for the both of us,
> I expect you will be able to add Dinh and myself as maintainers here,
> right? I have no problem with there only being one repo to store
> patches for this subsystem.
Does it mean that you are able to see "loading" file there?
Can you share your script?
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
On Tue, 2013-09-24 at 17:58 +0200, Michal Simek wrote:
> Hi,
>
> On 09/24/2013 05:55 PM, Alan Tull wrote:
> >
> >> I have ported the altera fpga manager driver to work with your version
> >> of the fpga manager framework. It works fine if I use the
> >> firmware_class.c's built-in support to load the firmware, but not with a
> >> userspace helper.
> >>
> >
> >
> > Hi Michal,
> >
> > I cleaned up my udev rules and now I see the userspace helper working.
> > Just adding one udev rule that points to the standard hotplug-script was
> > sufficient to see the userspace script working nicely.
> >
> > All in all, the firmware interface seems pretty easy to use and it gives
> > us the flexibility to either use the kernel to load the firmware or to
> > use scripts, so it should work with a variety of use cases.
> >
> > Since we are arriving at a solution that is suitable for the both of us,
> > I expect you will be able to add Dinh and myself as maintainers here,
> > right? I have no problem with there only being one repo to store
> > patches for this subsystem.
>
> Does it mean that you are able to see "loading" file there?
>
> Can you share your script?
>
> Thanks,
> Michal
>
Hi Michal,
Yes, I could see /sys/class/fpga/fpga0/image.rbf/loading and 'data' and
a few others (If I was requesting to load 'image.rbf'). It's a nice
interface.
I just used the linux/Documentation/firmware_class/hotplug-script
without modifications.
To enable it:
* cp linux/Documentation/firmware_class/hotplug-script /lib/udev/
* chmod 755 /lib/udev/hotplug-script
* Add this udev rule:
SUBSYSTEM=="firmware", ACTION=="add", RUN+="/lib/udev/hotplug-script"
* Check that there aren't other 'firmware' udev rules to get in the
way.
* Add your firmware files to /usr/lib/hotplug/firmware/ or change that
path in hotplug-script to point to where your firmware files.
The default hotplug-script doesn't do anything special (that the kernel
couldn't do by itself). What's great is that it could call another
script that adds headers or does whatever other special un-gzipping or
other massaging that the firmware image needs before it gets loaded.
Regards,
Alan
>
>
On Tue, Sep 24, 2013 at 11:22:54AM -0500, Alan Tull wrote:
> Yes, I could see /sys/class/fpga/fpga0/image.rbf/loading and 'data' and
> a few others (If I was requesting to load 'image.rbf'). It's a nice
> interface.
>
> I just used the linux/Documentation/firmware_class/hotplug-script
> without modifications.
>
> To enable it:
>
> * cp linux/Documentation/firmware_class/hotplug-script /lib/udev/
>
> * chmod 755 /lib/udev/hotplug-script
>
> * Add this udev rule:
> SUBSYSTEM=="firmware", ACTION=="add", RUN+="/lib/udev/hotplug-script"
>
> * Check that there aren't other 'firmware' udev rules to get in the
> way.
Hm, don't do that, all "modern" distros will not do firmware loading
through udev anymore, so please don't try to add it back in. The kernel
handles the loading of the firmware directly, with no need to call any
usermodehelper program at all.
> * Add your firmware files to /usr/lib/hotplug/firmware/ or change that
> path in hotplug-script to point to where your firmware files.
>
> The default hotplug-script doesn't do anything special (that the kernel
> couldn't do by itself). What's great is that it could call another
> script that adds headers or does whatever other special un-gzipping or
> other massaging that the firmware image needs before it gets loaded.
Only if you need to do something "special" like this could it justify
not using the in-kernel firmware loader. Also note that I think future
versions of udev will have the udev firmware loader removed entirely, so
watch out for that.
thanks,
greg k-h
On 09/19/2013 03:08 AM, Pavel Machek wrote:
>
>> The firmware approach is interesting. It might be less flexible
>> compared with my original code (see link to git below) that this is
>
> On the other hand... that's the interface world wants, right? To most
> users, fpga bitstream is just a firmware.
>
No, not really.
The typical assumption with the firmware interface is that there is
exactly one possible firmware for each device (possibly modulated by
driver version, but still.) This is blatantly not true for an FPGA in
the most extreme way possible -- there are an almost infinite number of
ways one can load an FPGA.
However, I have to question the whole idea of an "FPGA subsystem" --
there is an almost infinite number of ways to program an FPGA or FPGA
programming device (which may even be a commodity flash with a
microcontroller or CPLD, and may be shared with other devices), and it
really doesn't make any inherent sense to lump them together.
-hpa
Hi,
On 09/25/2013 12:54 AM, H. Peter Anvin wrote:
> On 09/19/2013 03:08 AM, Pavel Machek wrote:
>>
>>> The firmware approach is interesting. It might be less flexible
>>> compared with my original code (see link to git below) that this is
>>
>> On the other hand... that's the interface world wants, right? To most
>> users, fpga bitstream is just a firmware.
>>
>
> No, not really.
>
> The typical assumption with the firmware interface is that there is
> exactly one possible firmware for each device (possibly modulated by
> driver version, but still.) This is blatantly not true for an FPGA in
> the most extreme way possible -- there are an almost infinite number of
> ways one can load an FPGA.
That's truth but on the other hand on target hw you probably have
well known bitstream which you want to load to the device and your
drivers exactly know based on board revision what firmware you want to load.
> However, I have to question the whole idea of an "FPGA subsystem" --
> there is an almost infinite number of ways to program an FPGA or FPGA
> programming device (which may even be a commodity flash with a
> microcontroller or CPLD, and may be shared with other devices), and it
> really doesn't make any inherent sense to lump them together.
That's exactly discussion what I would like to have about this concept
in general.
I can agree that there are number of ways how to program fpga/cpld and others.
I wouldn't say infinite number just because of users can choose really
custom options.
I would say there are some standard ways how to load bitstream to fpga/cpld.
The most of customers just use them and they can be described.
Currently I do more care about zynq devcfg driver for loading bitstream
to zynq PL and partially icap driver just because it is in the mainline.
But there are others way though gpio, etc. Altera has also their drivers
for doing this type of work.
If you look at current state in connection to the kernel then for example
hwicap just create char driver. zynq devcfg driver which we have in xilinx tree
just create another char driver. Altera (please guys correct me if I am wrong)
has one driver drivers/misc/altera-stapl in the kernel which is used by any pci
card which is working with firmware interface.
Then they have any fpga-bridge in their tree which create any new class
+ origin fpga-mgr which is I think for their socfpga programming.
In general all these drivers can just create whatever user interface
they like and just start to pushing these drivers to the mainline
to char or misc folders.
But IMHO all of them just do the same program fpga/cpld trough particular
interface but why not just to have fpga core driver just to unify
this user access.
When we have agreement on this another valid discussion is
if firmware interface is OK for this purpose or NOT. Or if make sense
to create different interface or have all of them.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
On 09/23/2013 07:10 PM, Jason Gunthorpe wrote:
> On Mon, Sep 23, 2013 at 03:10:11PM +0200, Michal Simek wrote:
>
>>> 1) The driver doesn't know what firmware to request. It just knows
>>> how to send it to a FPGA.
>>
>> But dts property in the manager driver which uses this as end driver
>> can know that.
>
> I think the device tree maintainers would push back on this since it
> is not "describing the hardware"
Yes. Then if you want to put it in this way firmware name doesn't need
to be specified in DTS and there can be default name in the driver
or you can read this information from i2c. There are options how to do
differently.
>>> 2) Telling the kernel a filename via sysfs and then having it go
>>> around the long way via request_firwmare to get the data is silly.
>>> Just give the kernel the actual data instead of a file name
>>
>> Firmware interface is valid way how to pass bitstream to the kernel.
>> If you don't like just don't use it. For example you can add
>> firmware blob directly to the kernel and load this at bootup phase
>> without user-space access.
>
> I'm not against using request firmware for what it is ment for: having
> the kernel autonomously load firmware.
>
> I am against the sysfs API in the core code where userspace writes a
> file name that is then used to request_firwmare. That is a goofy API
> for the reasons I outlined.
>
> It is appropriate to use request firmware at the driver level where
> the driver somehow knows what FPGA to request.
I don't have enough information if this can be a problem in connection
to have goofy API but I have no problem to keep firmware interface
just at driver level.
And instead sending firmware name which should be loaded just
send data instead.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
Hi!
> >> The firmware approach is interesting. It might be less flexible
> >> compared with my original code (see link to git below) that this is
> >
> > On the other hand... that's the interface world wants, right? To most
> > users, fpga bitstream is just a firmware.
> >
>
> No, not really.
>
> The typical assumption with the firmware interface is that there is
> exactly one possible firmware for each device (possibly modulated by
> driver version, but still.)
Actually, I have seen counterexample there, too. Wifi card had
different firmware for host and access points mode, probably because
internal RAM could not fit both at same time.
It is not really different; there's internal logic and you are
programming it to do something. You can program FPGA, CPLD, or
firmware for some embedded CPU.
> This is blatantly not true for an FPGA in
> the most extreme way possible -- there are an almost infinite number of
> ways one can load an FPGA.
Well, usually the FPGA has some function on the board (unless you are
using it for compute acceleration), and it also wants just one
version. If you have FPGA-based GSM board, you'll want GSM
firmware. If you have FPGA-based WIFI, you'll want WIFI firmware. If
you load GSM firmware there, it will very likely not work because it
needs different amplifiers etc.
Heck, you probably _could_ use your WIFI card for cryptographics
operations just by loading different firmware. People just don't do
that. I don't expect that to be too different in the FPGA case...
> However, I have to question the whole idea of an "FPGA subsystem" --
> there is an almost infinite number of ways to program an FPGA or FPGA
> programming device (which may even be a commodity flash with a
> microcontroller or CPLD, and may be shared with other devices), and it
> really doesn't make any inherent sense to lump them together.
Firmware loading methods are also different, yet it makes sense to
have uniform way of loading firmware. This is very similar situation.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Greg,
>> The default hotplug-script doesn't do anything special (that the kernel
>> couldn't do by itself). What's great is that it could call another
>> script that adds headers or does whatever other special un-gzipping or
>> other massaging that the firmware image needs before it gets loaded.
>
> Only if you need to do something "special" like this could it justify
> not using the in-kernel firmware loader. Also note that I think future
> versions of udev will have the udev firmware loader removed entirely, so
> watch out for that.
If the bit stream is in raw format, the kernel can deal with it.
however, there are "richer" formats for the FPGA bit streams that need
to be parsed in order to extract the raw data that an FPGA accepts. In
this case, user space must be involved.
Regards
Yves
>
> thanks,
>
> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On 09/25/2013 08:00 AM, Pavel Machek wrote:
> Hi!
>
>>>> The firmware approach is interesting. It might be less flexible
>>>> compared with my original code (see link to git below) that this is
>>>
>>> On the other hand... that's the interface world wants, right? To most
>>> users, fpga bitstream is just a firmware.
>>>
>>
>> No, not really.
>>
>> The typical assumption with the firmware interface is that there is
>> exactly one possible firmware for each device (possibly modulated by
>> driver version, but still.)
>
> Actually, I have seen counterexample there, too. Wifi card had
> different firmware for host and access points mode, probably because
> internal RAM could not fit both at same time.
And another counter example ...
For the Zynq based product I am working on, we encourage the end user to
create their own bitstreams to customize their application. So we need
an easy way for the user to load a bitstream. cat foo.bin > /dev/xdevcfg
works well for us.
Then we need to make sure that the interface supports partial
reconfiguration.
That said, having a sane user api that addresses the various fpga use
cases would be a win for use since we would not need to write custom
user space code for every platform created in the future. I'd like to
see this api work with things like PCI based FPGA cards and discrete
fpgas that are not integrated with an arm etc.
Phlip
>
> It is not really different; there's internal logic and you are
> programming it to do something. You can program FPGA, CPLD, or
> firmware for some embedded CPU.
>
>> This is blatantly not true for an FPGA in
>> the most extreme way possible -- there are an almost infinite number of
>> ways one can load an FPGA.
>
> Well, usually the FPGA has some function on the board (unless you are
> using it for compute acceleration), and it also wants just one
> version. If you have FPGA-based GSM board, you'll want GSM
> firmware. If you have FPGA-based WIFI, you'll want WIFI firmware. If
> you load GSM firmware there, it will very likely not work because it
> needs different amplifiers etc.
>
> Heck, you probably _could_ use your WIFI card for cryptographics
> operations just by loading different firmware. People just don't do
> that. I don't expect that to be too different in the FPGA case...
>
>> However, I have to question the whole idea of an "FPGA subsystem" --
>> there is an almost infinite number of ways to program an FPGA or FPGA
>> programming device (which may even be a commodity flash with a
>> microcontroller or CPLD, and may be shared with other devices), and it
>> really doesn't make any inherent sense to lump them together.
>
> Firmware loading methods are also different, yet it makes sense to
> have uniform way of loading firmware. This is very similar situation.
>
> Pavel
>
On 09/25/2013 04:27 PM, Philip Balister wrote:
> On 09/25/2013 08:00 AM, Pavel Machek wrote:
>> Hi!
>>
>>>>> The firmware approach is interesting. It might be less flexible
>>>>> compared with my original code (see link to git below) that this is
>>>>
>>>> On the other hand... that's the interface world wants, right? To most
>>>> users, fpga bitstream is just a firmware.
>>>>
>>>
>>> No, not really.
>>>
>>> The typical assumption with the firmware interface is that there is
>>> exactly one possible firmware for each device (possibly modulated by
>>> driver version, but still.)
>>
>> Actually, I have seen counterexample there, too. Wifi card had
>> different firmware for host and access points mode, probably because
>> internal RAM could not fit both at same time.
>
> And another counter example ...
>
> For the Zynq based product I am working on, we encourage the end user to
> create their own bitstreams to customize their application. So we need
> an easy way for the user to load a bitstream. cat foo.bin > /dev/xdevcfg
> works well for us.
You probably don't care if this will be
cat foo.bin > /sys/fpga/fpga0/<whatever>
(for zynq case you can also run)
cat foo.bit > /sys/fpga/fpga0/<whatever>
FYI: Current driver in xilinx repo supports bit format too.
> Then we need to make sure that the interface supports partial
> reconfiguration.
That will be next step.
> That said, having a sane user api that addresses the various fpga use
> cases would be a win for use since we would not need to write custom
> user space code for every platform created in the future. I'd like to
> see this api work with things like PCI based FPGA cards and discrete
> fpgas that are not integrated with an arm etc.
I don't have any PCI based FPGA card to have information how to work with
it but for discrete fpga I have one application note where jtag is emulated
by gpio interface and you can program another fpga from zynq (This can
be probably generic case for any combination in this 4 pin connection).
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
On 09/25/2013 03:55 PM, Yves Vandervennet wrote:
> Greg,
>
>>> The default hotplug-script doesn't do anything special (that the kernel
>>> couldn't do by itself). What's great is that it could call another
>>> script that adds headers or does whatever other special un-gzipping or
>>> other massaging that the firmware image needs before it gets loaded.
>>
>> Only if you need to do something "special" like this could it justify
>> not using the in-kernel firmware loader. Also note that I think future
>> versions of udev will have the udev firmware loader removed entirely, so
>> watch out for that.
> If the bit stream is in raw format, the kernel can deal with it.
> however, there are "richer" formats for the FPGA bit streams that need
> to be parsed in order to extract the raw data that an FPGA accepts. In
> this case, user space must be involved.
For xilinx there are two fpga formats which are commonly used. BIT and BIN.
BIT format is that richer one but there is no public documentation available
for it. IRC hwicap just support BIT format and because of any unknown decision
zynq can work just with BIN.
But getting BIN from BIT is easy and currently we have the support
in our driver for doing it.
Not sure what options there are for cplds.
But anyway AFAIK richer formats are not problem for us.
The same situation should be for partial bitstreams too.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
> >
> > * Add this udev rule:
> > SUBSYSTEM=="firmware", ACTION=="add", RUN+="/lib/udev/hotplug-script"
> >
> > * Check that there aren't other 'firmware' udev rules to get in the
> > way.
>
> Hm, don't do that, all "modern" distros will not do firmware loading
> through udev anymore, so please don't try to add it back in. The kernel
> handles the loading of the firmware directly, with no need to call any
> usermodehelper program at all.
>
> > * Add your firmware files to /usr/lib/hotplug/firmware/ or change that
> > path in hotplug-script to point to where your firmware files.
> >
> > The default hotplug-script doesn't do anything special (that the kernel
> > couldn't do by itself). What's great is that it could call another
> > script that adds headers or does whatever other special un-gzipping or
> > other massaging that the firmware image needs before it gets loaded.
>
> Only if you need to do something "special" like this could it justify
> not using the in-kernel firmware loader. Also note that I think future
> versions of udev will have the udev firmware loader removed entirely, so
> watch out for that.
>
Hi Greg,
Thanks for the heads-up on the direction the firmware class is taking.
Well, that's kind of disappointing. I was just getting excited about
how flexible the firmware class currently is. In the simpler fpga use
cases, using the in-kernel loader to load static raw images would be
great. But we are really excited about exploiting the possibilities
that fpga's give in terms of reconfiguration and partial reconfiguration
on the fly, so we will be needing to do 'special' things (unpacking
images, stripping headers, lots of stuff that userspace would better for
and that we wouldn't want to put into the kernel). I hope that we can
arrive at a framework with a unified interface that suites this.
Alan
> >
> > For the Zynq based product I am working on, we encourage the end user to
> > create their own bitstreams to customize their application. So we need
> > an easy way for the user to load a bitstream. cat foo.bin > /dev/xdevcfg
> > works well for us.
>
> You probably don't care if this will be
> cat foo.bin > /sys/fpga/fpga0/<whatever>
> (for zynq case you can also run)
> cat foo.bit > /sys/fpga/fpga0/<whatever>
>
> FYI: Current driver in xilinx repo supports bit format too.
>
Michal,
So your low level driver has a back door to avoid having to use the
firmware class? I thought the point of this exercise was to have a
framework with a unified interface.
My original framework supported the interface you mention here.
If we are going to have a framework, we should spec in in such a way
that vendors won't be required to add back doors to their drivers in
order to support the use cases they are interested in.
If the firmware interface isn't going to work all the fpga use cases,
then why upstream this patch? In that case we should go back to the
original implementation and have a unified interface that supports
cat'ing the bitstreams to /sys instead.
My current opinion about the firmware class is that we can use it (and
it is best to use existing interfaces), but it is not really suited for
all the fpga use cases.
The other thing the original fpga framework had was a transport layer.
The intent was to eventually support the same driver (such as a bitbang
driver) no matter what type of gpio lines it was attached to. It wasn't
really necessary for the fpga manager that is bolted onto a soc, but it
will be needed if you have an fpga that gets programmed from another
fpga that is connected to a soc. Otherwise you have to write separate
drivers depending on how the fpga is connected.
Alan
Hi Jason,
On 09/18/2013 10:32 PM, Jason Gunthorpe wrote:
> On Wed, Sep 18, 2013 at 03:15:17PM -0400, Jason Cooper wrote:
>
>> + Jason Gunthorpe
>
> Thanks, looks interesting, we could possibly use this interface if it
> met our needs..
>
>> On Wed, Sep 18, 2013 at 05:56:39PM +0200, Michal Simek wrote:
>>> This new subsystem should unify all fpga drivers which
>>> do the same things. Load configuration data to fpga
>>> or another programmable logic through common interface.
>>> It doesn't matter if it is MMIO device, gpio bitbanging,
>>> etc. connection. The point is to have the same
>>> inteface for these drivers.
>
> So, we have many years of in-field experience with this and this API
> doesn't really match what we do.
>
> Here are the steps we perform, from userspace:
> - Ask kernel to place FPGA into reset and prepare for programming
> * Kernel can return an error (eg FPGA failed to erase, etc)
> * this is the PROG_N low -> DONE high, PROG_N high -> INIT_N high
> sequencing on Xilinx chips
> - Ask kernel to load a bitstream.
> * Userspace locats the bitstream file to load, and the mmaps it.
> * Userspace passes the entire file in a single write() call to the
> kernel which streams it over the configuration bus
> * The kernel can report an erro rhere (eg Xilinx can report CRC
> error)
> - Ask the kernel to verify that configuration is complete.
> * On Xilinx this wait for done to go high
> - Ask the kernel to release the configuration bus (tristate
> all drivers) (or sometimes we have to drive the bus low,
> it depends on the bitfile, user space knows what to do)
>
> It is very important that userspace know exactly which step fails
> because the resolution is different. We use this in a manufacturing
> setting, so failures are expected and need quick root cause
> determination.
>
> You could probably address that need by very clearly defining a
> variety of errno values for the various cases. However, it would be a
> disaster if every driver did something a little different :|
>
> Using request_firmware exclusively is not useful for us. We
> format the bitfile with a header that contains our internal tracking
> information. Sometimes we need to bitswap the bitfile. Our userspace
> handles all of this and can pass a bitfile in memory to write().
>
> request_firmware would be horrible to use :)
>
> Our API uses a binary sysfs attribute to stream the FPGA data, you
> might want to consider that.
I have done some experiments with binary sysfs attribute and can
you please be more specific how you are working with it?
I expect that you are detecting/specifying in the driver which
fpga is connected and you also need to know size of this device.
Then your driver allocate buffer with this size in the kernel
and streming data to this buffer. When this is done you are
using another sysfs files to control device programming.
Can you please correct me if I am wrong?
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform
On Fri, Sep 27, 2013 at 03:31:53PM +0200, Michal Simek wrote:
> I expect that you are detecting/specifying in the driver which
> fpga is connected and you also need to know size of this device.
> Then your driver allocate buffer with this size in the kernel
> and streming data to this buffer. When this is done you are
> using another sysfs files to control device programming.
No, it just streams:
static ssize_t fpga_config_write(struct file *filp,struct kobject *kobj,
struct bin_attribute *attr,
char *buf, loff_t off, size_t len)
{
struct device *dev = container_of(kobj, struct device, kobj);
struct platform_device *pdev = to_platform_device(dev);
struct fpga_priv *priv = platform_get_drvdata(pdev);
uint8_t *cur = buf;
size_t total = len;
unsigned int bit;
for (; len != 0; len--, cur++) {
gpio_set_value(priv->gpio[GPIO_CCLK],0);
for (bit = 0; bit != 8; bit++)
gpio_set_value(priv->data_gpio[bit],
(*cur & (1<<bit)) != 0);
gpio_set_value(priv->gpio[GPIO_CCLK],1);
if (gpio_get_value(priv->gpio[GPIO_INIT_B]) == 0)
return -EIO;
}
return total;
}
static struct bin_attribute dev_attr_config_data = {
.attr = {
.name = "config_data",
.mode = 0600,
},
.size = 0,
.write = fpga_config_write,
};
User space does as many writes as necessary to send the entire
bitstream, the sysfs layer chunks things into PAGE_SIZE blocks, so it
acts much like a socket with O_NONBLOCK set.
We are controlling the other related GPIOs from userspace, but for
your purposes I would pair the data sysfs file with a control sysfs
file much like request firwmare does.
Here is a suggestion.
- Two files fpga_config_state, fpga_config_data
- fpga_config_state is a one value text string values are like
initializing, clearing, programming, operating, error_clear_failed,
error_bistream_crc
- Userspace writes to fpga_config_state which causes the kernel FSM
to move to that state. The normal progression would be initializing,
clearing, programming and finally operating
- The kernel can move to an error_* state if it detects a problem
- The programming state data from fpga_config_data to the
configuration bus and userspace writes 'operating' once the stream
is done to perform the post-configuration actions.
Jason
On 09/30/2013 07:12 PM, Jason Gunthorpe wrote:
> On Fri, Sep 27, 2013 at 03:31:53PM +0200, Michal Simek wrote:
>
>> I expect that you are detecting/specifying in the driver which
>> fpga is connected and you also need to know size of this device.
>> Then your driver allocate buffer with this size in the kernel
>> and streming data to this buffer. When this is done you are
>> using another sysfs files to control device programming.
>
> No, it just streams:
>
> static ssize_t fpga_config_write(struct file *filp,struct kobject *kobj,
> struct bin_attribute *attr,
> char *buf, loff_t off, size_t len)
> {
> struct device *dev = container_of(kobj, struct device, kobj);
> struct platform_device *pdev = to_platform_device(dev);
> struct fpga_priv *priv = platform_get_drvdata(pdev);
> uint8_t *cur = buf;
> size_t total = len;
> unsigned int bit;
>
> for (; len != 0; len--, cur++) {
> gpio_set_value(priv->gpio[GPIO_CCLK],0);
>
> for (bit = 0; bit != 8; bit++)
> gpio_set_value(priv->data_gpio[bit],
> (*cur & (1<<bit)) != 0);
>
> gpio_set_value(priv->gpio[GPIO_CCLK],1);
>
> if (gpio_get_value(priv->gpio[GPIO_INIT_B]) == 0)
> return -EIO;
> }
> return total;
> }
>
> static struct bin_attribute dev_attr_config_data = {
> .attr = {
> .name = "config_data",
> .mode = 0600,
> },
> .size = 0,
> .write = fpga_config_write,
> };
>
> User space does as many writes as necessary to send the entire
> bitstream, the sysfs layer chunks things into PAGE_SIZE blocks, so it
> acts much like a socket with O_NONBLOCK set.
>
> We are controlling the other related GPIOs from userspace, but for
> your purposes I would pair the data sysfs file with a control sysfs
> file much like request firwmare does.
>
> Here is a suggestion.
> - Two files fpga_config_state, fpga_config_data
> - fpga_config_state is a one value text string values are like
> initializing, clearing, programming, operating, error_clear_failed,
> error_bistream_crc
> - Userspace writes to fpga_config_state which causes the kernel FSM
> to move to that state. The normal progression would be initializing,
> clearing, programming and finally operating
> - The kernel can move to an error_* state if it detects a problem
> - The programming state data from fpga_config_data to the
> configuration bus and userspace writes 'operating' once the stream
> is done to perform the post-configuration actions.
yes, there is necessary to provide also any state to be able to control
the flow. For your case above with streams this is not necessary.
Thanks for this description I wanted to make sure that we are on the same page.
Thanks,
Michal
--
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: http://www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform