[RFC PATCH v2 0/1] FPGA subsystem core

[RFC PATCH v2 0/1] FPGA subsystem core

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 3734 bytes --]

Hi All,

this is the second attempt to introduce new Linux FPGA subsystem which
can help us to unify all fpga drivers which in general do the same
things.
Xilinx has hwicap in the kernel as char driver (drivers/char/xilinx_hwicap/)
and I would like to base Zynq devcfg driver based on this interface
because make no sense to push the Linux kernel another char driver
(I am testing this interface on this driver).

Based on my discussion at ELC with Greg KH the new driver
should support firmware interface for loading bitstream.

FPGA manager/driver just define set of functions and
call fpga_mgr_register().

struct fpga_manager_ops zynq_fpga_mgr_ops = {
	.read_init = zynq_init,
	.write_init = zynq_init,
	.read = zynq_read,
	.write = zynq_write,
	.read_complete = zynq_complete,
	.write_complete = zynq_complete,
};

fpga_mgr_register(pdev, &zynq_fpga_mgr_ops, "Zynq FPGA Manager", priv);

For unregistration it is enough to call:
fpga_mgr_unregister(pdev);

Here is the set of commands for writing bitstream to FPGA.

Through firmware interface:
cat /sys/class/fpga_manager/fpga0/name
echo -n fpga.bin > /sys/class/fpga_manager/fpga0/firmware

Through sysfs bin file:
cat /sys/class/fpga_manager/fpga0/fpga_config_state
echo -n write_init > /sys/class/fpga_manager/fpga0/fpga_config_state
cat /lib/firmware/fpga.bin > /sys/class/fpga_manager/fpga0/fpga_config_data
echo -n write_complete > /sys/class/fpga_manager/fpga0/fpga_config_state

Subsystem supports working with phandles for cases where you want to load
bitstreams for particular device through defined device.
For example:
mngr@0 {
	compatible = "whatever";
	fpga-mgr = <&ps7_dev_cfg_0>;
	...
} ;

With these lines you can get easily load bitstream to the device.

struct fpga_manager *mgr;
mgr = of_find_fpga_mgr_by_phandle(pdev->dev.of_node, "fpga-mgr");
if (mgr)
	mgr->fpga_firmware_write(mgr, "filename");

NOTE: I have added there of_find_fpga_mgr_by_node()
and of_find_fpga_mgr_by_phandle() but maybe they should be added
separately to drivers/of/of_fpga.c.

Alessandro: I haven't looked at your FMC cases but maybe this
could be also worth for your cases.

TODO:
- Probably make sense to create doc in Documentation folder too.
- When interface is fine also send zynq devcfg driver
- Properly test reading (we have problem with zynq devcfg driver now)
- Not sure if firmware interface also provide option to create
  files from kernel space.

Thanks for your comments,
Michal

Changes in v2:
- Remove ! from all error message not to be shouty
- Fix error codes
- Add sysfs-class-fpga description
- Use read/write helper functions with bit protection
- s/fpga_mgr_status_show/fpga_mgr_status_read/g
- Do not all end driver status just show core status
- Extract firmware support to specific sysfs firmware file
- Add support for sysfs bin attributes (fpga_config_state, fpga_config_data)
- Allocate space for name dynamically
- Introduce new flags bits (INIT_DONE, READ, WRITE)

Michal Simek (1):
  fpga: Introduce new fpga subsystem

 Documentation/ABI/testing/sysfs-class-fpga |  33 ++
 MAINTAINERS                                |   7 +
 drivers/Kconfig                            |   2 +
 drivers/Makefile                           |   1 +
 drivers/fpga/Kconfig                       |  18 +
 drivers/fpga/Makefile                      |   5 +
 drivers/fpga/fpga-mgr.c                    | 753 +++++++++++++++++++++++++++++
 include/linux/fpga.h                       | 110 +++++
 8 files changed, 929 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-fpga
 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

--
1.8.2.3


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[RFC PATCH v2] fpga: Introduce new fpga subsystem

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 28601 bytes --]

This new fpga subsystem core should unify all fpga drivers/managers 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
interface for these drivers.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>

---
Changes in v2:
- Remove ! from all error message not to be shouty
- Fix error codes
- Add sysfs-class-fpga description
- Use read/write helper functions with bit protection
- s/fpga_mgr_status_show/fpga_mgr_status_read/g
- Do not all end driver status just show core status
- Extract firmware support to specific sysfs firmware file
- Add support for sysfs bin attributes (fpga_config_state, fpga_config_data)
- Allocate space for name dynamically
- Introduce new flags bits (INIT_DONE, READ, WRITE)

 Documentation/ABI/testing/sysfs-class-fpga |  33 ++
 MAINTAINERS                                |   7 +
 drivers/Kconfig                            |   2 +
 drivers/Makefile                           |   1 +
 drivers/fpga/Kconfig                       |  18 +
 drivers/fpga/Makefile                      |   5 +
 drivers/fpga/fpga-mgr.c                    | 753 +++++++++++++++++++++++++++++
 include/linux/fpga.h                       | 110 +++++
 8 files changed, 929 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-fpga
 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/Documentation/ABI/testing/sysfs-class-fpga b/Documentation/ABI/testing/sysfs-class-fpga
new file mode 100644
index 0000000..3d66c71
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-fpga
@@ -0,0 +1,33 @@
+What:		/sys/class/fpga_manager/fpga<dev-id>/firmware
+Date:		October 2013
+KernelVersion:	3.12
+Contact:	Michal Simek <michal.simek@xilinx.com>
+Description:
+		Writing firmware name will invoke firmware interface
+		to request for a firmware which includes fpga bitstream.
+
+What:		/sys/class/fpga_manager/fpga<dev-id>/fpga_config_state
+Date:		October 2013
+KernelVersion:	3.12
+Contact:	Michal Simek <michal.simek@xilinx.com>
+Description:
+		By reading this file you will get current fpga manager state.
+		Flag bits are present in include/linux/fpga.h (FPGA_MGR_XX).
+		By writing to this file you can change fpga manager state.
+		Valid options are: write_init, write_complete, read_init,
+		read_complete.
+
+What:		/sys/class/fpga_manager/fpga<dev-id>/fpga_config_data
+Date:		October 2013
+KernelVersion:	3.12
+Contact:	Michal Simek <michal.simek@xilinx.com>
+Description:
+		For writing fpga bitstream to the device when device is
+		in proper state setup through fpga_config_state.
+
+What:		/sys/class/fpga_manager/fpga<dev-id>/name
+Date:		October 2013
+KernelVersion:	3.12
+Contact:	Michal Simek <michal.simek@xilinx.com>
+Description:
+		Show fpga manager name.
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 <michal.simek@xilinx.com>
+T:	git git://git.xilinx.com/linux-xlnx.git
+S:	Supported
+F:	drivers/fpga/
+F:	include/linux/fpga.h
+
 FPU EMULATOR
 M:	Bill Metzenthen <billm@melbpc.org.au>
 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..c8cf0d6
--- /dev/null
+++ b/drivers/fpga/fpga-mgr.c
@@ -0,0 +1,753 @@
+/*
+ * 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 DEFINE_IDR(fpga_mgr_idr);
+static DEFINE_SPINLOCK(fpga_mgr_idr_lock);
+static struct class *fpga_mgr_class;
+
+/**
+ * fpga_mgr_read_init - Perform read init function
+ * @mgr: Pointer to the fpga manager structure
+ *
+ * Returns 0 on success, a negative error number otherwise
+ */
+static inline int fpga_mgr_read_init(struct fpga_manager *mgr)
+{
+	int bit, ret;
+
+	dev_dbg(mgr->dev, "%s %lx\n", __func__, mgr->flags);
+
+	bit = test_bit(FPGA_MGR_WRITE, &mgr->flags);
+	if (bit) {
+		dev_dbg(mgr->dev, "Device initialized for write\n");
+		return -EBUSY;
+	}
+
+	bit = test_bit(FPGA_MGR_INIT_DONE, &mgr->flags);
+	if (bit) {
+		dev_dbg(mgr->dev, "Operations not completed properly\n");
+		return -EBUSY;
+	}
+
+	if (mgr->mops->read_init) {
+		ret = mgr->mops->read_init(mgr);
+		if (ret) {
+			dev_dbg(mgr->dev, "Failed to write_init\n");
+			return ret;
+		}
+	}
+
+	set_bit(FPGA_MGR_INIT_DONE, &mgr->flags);
+	set_bit(FPGA_MGR_READ, &mgr->flags);
+
+	dev_dbg(mgr->dev, "%s done %lx\n", __func__, mgr->flags);
+
+	return 0;
+}
+
+/**
+ * fpga_mgr_read - Perform read function
+ * @mgr: Pointer to the fpga manager structure
+ * @buf: Pointer to the buffer location
+ * @count: Pointer to the number of copied bytes
+ *
+ * Returns 0 on success, a negative error number otherwise
+ */
+static inline int fpga_mgr_read(struct fpga_manager *mgr, const u8 *buf,
+				 ssize_t *count)
+{
+	int bit, ret;
+
+	dev_dbg(mgr->dev, "%s %lx\n", __func__, mgr->flags);
+
+	/* FPGE init has to be done to be able to continue */
+	bit = test_bit(FPGA_MGR_INIT_DONE, &mgr->flags);
+	if (!bit) {
+		dev_dbg(mgr->dev, "Device not initialized properly\n");
+		return -EBUSY;
+	}
+
+	if (mgr->mops->write) {
+		ret = mgr->mops->read(mgr, buf, count);
+		if (ret) {
+			dev_dbg(mgr->dev, "Failed to write\n");
+			return ret;
+		}
+	}
+
+	dev_dbg(mgr->dev, "%s done %lx\n", __func__, mgr->flags);
+
+	return 0;
+}
+
+/**
+ * fpga_mgr_read_complete - Perform read complete function
+ * @mgr: Pointer to the fpga manager structure
+ *
+ * Returns 0 on success, a negative error number otherwise
+ */
+static inline int fpga_mgr_read_complete(struct fpga_manager *mgr)
+{
+	int bit, ret;
+
+	dev_dbg(mgr->dev, "%s %lx\n", __func__, mgr->flags);
+
+	/* FPGA init has to be done to be able to continue */
+	bit = test_bit(FPGA_MGR_INIT_DONE, &mgr->flags);
+	if (!bit) {
+		dev_dbg(mgr->dev, "Device not initialized properly\n");
+		return -EBUSY;
+	}
+
+	if (mgr->mops->write_complete) {
+		ret = mgr->mops->read_complete(mgr);
+		if (ret) {
+			dev_dbg(mgr->dev, "Failed to write_complete\n");
+			return ret;
+		}
+	}
+
+	clear_bit(FPGA_MGR_INIT_DONE, &mgr->flags);
+	clear_bit(FPGA_MGR_READ, &mgr->flags);
+
+	dev_dbg(mgr->dev, "%s done %lx\n", __func__, mgr->flags);
+
+	return 0;
+}
+
+/**
+ * fpga_mgr_write_init - Perform write init function
+ * @mgr: Pointer to the fpga manager structure
+ *
+ * Returns 0 on success, a negative error number otherwise
+ */
+static inline int fpga_mgr_write_init(struct fpga_manager *mgr)
+{
+	int bit, ret;
+
+	dev_dbg(mgr->dev, "%s %lx\n", __func__, mgr->flags);
+
+	bit = test_bit(FPGA_MGR_READ, &mgr->flags);
+	if (bit) {
+		dev_dbg(mgr->dev, "Device initialized for read\n");
+		return -EBUSY;
+	}
+
+	bit = test_bit(FPGA_MGR_INIT_DONE, &mgr->flags);
+	if (bit) {
+		dev_dbg(mgr->dev, "Operations not completed properly\n");
+		return -EBUSY;
+	}
+
+	if (mgr->mops->write_init) {
+		ret = mgr->mops->write_init(mgr);
+		if (ret) {
+			dev_dbg(mgr->dev, "Failed to write_init\n");
+			return ret;
+		}
+	}
+
+	set_bit(FPGA_MGR_INIT_DONE, &mgr->flags);
+	set_bit(FPGA_MGR_WRITE, &mgr->flags);
+
+	dev_dbg(mgr->dev, "%s done %lx\n", __func__, mgr->flags);
+
+	return 0;
+}
+
+/**
+ * fpga_mgr_write - Perform write function
+ * @mgr: Pointer to the fpga manager structure
+ * @buf: Pointer to the buffer location
+ * @count: Number of bytes to be copied
+ *
+ * Returns 0 on success, a negative error number otherwise
+ */
+static inline int fpga_mgr_write(struct fpga_manager *mgr, const u8 *buf,
+				 ssize_t count)
+{
+	int bit, ret;
+
+	dev_dbg(mgr->dev, "%s %lx\n", __func__, mgr->flags);
+
+	/* FPGE init has to be done to be able to continue */
+	bit = test_bit(FPGA_MGR_INIT_DONE, &mgr->flags);
+	if (!bit) {
+		dev_dbg(mgr->dev, "Device not initialized properly\n");
+		return -EBUSY;
+	}
+
+	if (mgr->mops->write) {
+		ret = mgr->mops->write(mgr, buf, count);
+		if (ret) {
+			dev_dbg(mgr->dev, "Failed to write\n");
+			return ret;
+		}
+	}
+
+	dev_dbg(mgr->dev, "%s done %lx\n", __func__, mgr->flags);
+
+	return 0;
+}
+
+/**
+ * fpga_mgr_write_complete - Perform write complete function
+ * @mgr: Pointer to the fpga manager structure
+ *
+ * Returns 0 on success, a negative error number otherwise
+ */
+static inline int fpga_mgr_write_complete(struct fpga_manager *mgr)
+{
+	int bit, ret;
+
+	dev_dbg(mgr->dev, "%s %lx\n", __func__, mgr->flags);
+
+	/* FPGA init has to be done to be able to continue */
+	bit = test_bit(FPGA_MGR_INIT_DONE, &mgr->flags);
+	if (!bit) {
+		dev_dbg(mgr->dev, "Device not initialized properly\n");
+		return -EBUSY;
+	}
+
+	if (mgr->mops->write_complete) {
+		ret = mgr->mops->write_complete(mgr);
+		if (ret) {
+			dev_dbg(mgr->dev, "Failed to write_complete\n");
+			return ret;
+		}
+	}
+
+	clear_bit(FPGA_MGR_INIT_DONE, &mgr->flags);
+	clear_bit(FPGA_MGR_WRITE, &mgr->flags);
+
+	dev_dbg(mgr->dev, "%s done %lx\n", __func__, mgr->flags);
+
+	return 0;
+}
+
+/**
+ * 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 sprintf(buf, "%s\n", mgr->name);
+}
+
+/**
+ * fpga_mgr_status_read - Read 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_read(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct fpga_manager *mgr = dev_get_drvdata(dev);
+
+	return sprintf(buf, "0x%lx\n", mgr->flags);
+}
+
+/**
+ * fpga_mgr_status_write - Write fpga manager status
+ * @dev: Pointer to the device structure
+ * @attr: Pointer to the device attribute structure
+ * @buf: Pointer to the buffer location
+ * @count: Number of characters in @buf
+ *
+ * Returns the number of bytes copied to @buf, a negative error number otherwise
+ */
+static ssize_t fpga_mgr_status_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 (test_and_set_bit_lock(FPGA_MGR_DEV_BUSY, &mgr->flags))
+		return -EBUSY;
+
+	ret = strcmp(buf, "write_init");
+	if (!ret) {
+		ret = fpga_mgr_write_init(mgr);
+		goto out;
+	}
+
+	ret = strcmp(buf, "write_complete");
+	if (!ret) {
+		ret = fpga_mgr_write_complete(mgr);
+		goto out;
+	}
+
+	ret = strcmp(buf, "read_init");
+	if (!ret) {
+		ret = fpga_mgr_read_init(mgr);
+		goto out;
+	}
+
+	ret = strcmp(buf, "read_complete");
+	if (!ret) {
+		ret = fpga_mgr_read_complete(mgr);
+		goto out;
+	}
+
+	ret = -EINVAL;
+out:
+	clear_bit_unlock(FPGA_MGR_DEV_BUSY, &mgr->flags);
+
+	return ret ? : count;
+}
+
+/**
+ * fpga_mgr_read_data - 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_data(struct fpga_manager *mgr, const u8 *buf,
+			      ssize_t *count)
+{
+	int ret;
+
+	if (!mgr->mops || !mgr->mops->read) {
+		dev_dbg(mgr->dev,
+			"Controller doesn't support read operations\n");
+		return -EPERM;
+	}
+
+	if (test_and_set_bit_lock(FPGA_MGR_DEV_BUSY, &mgr->flags))
+		return -EBUSY;
+
+	ret = fpga_mgr_read_init(mgr);
+	if (ret)
+		goto out;
+
+	/* Do read */
+	ret = fpga_mgr_read(mgr, buf, count);
+	if (ret)
+		goto out;
+
+	/* Do read complete */
+	ret = fpga_mgr_read_complete(mgr);
+out:
+	clear_bit_unlock(FPGA_MGR_DEV_BUSY, &mgr->flags);
+
+	return ret;
+}
+
+/**
+ * fpga_mgr_write_data - Write data to fpga
+ * @mgr: Pointer to the fpga manager structure
+ * @buf: Pointer to the buffer location with bistream data
+ * @count: Number of characters in @buf
+ *
+ * @buf contains data which is loading to the fpga.
+ *
+ * Returns string length added to @fw_name, a negative error number otherwise
+ */
+static int fpga_mgr_write_data(struct fpga_manager *mgr, const u8 *buf,
+			       ssize_t count)
+{
+	int ret = 0;
+
+	if (!mgr->mops || !mgr->mops->write) {
+		dev_dbg(mgr->dev,
+			"Controller doesn't support write operations\n");
+		return -EPERM;
+	}
+
+	if (test_and_set_bit_lock(FPGA_MGR_DEV_BUSY, &mgr->flags))
+		return -EBUSY;
+
+	/* Init controller */
+	ret = fpga_mgr_write_init(mgr);
+	if (ret)
+		goto out;
+
+	/* Do write */
+	ret = fpga_mgr_write(mgr, buf, count);
+	if (ret)
+		goto out;
+
+	/* Do write complete */
+	ret = fpga_mgr_write_complete(mgr);
+
+out:
+	clear_bit_unlock(FPGA_MGR_DEV_BUSY, &mgr->flags);
+
+	return ret;
+}
+
+/**
+ * fpga_mgr_firmware_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 length added to @fw_name, a negative error number otherwise
+ */
+static int fpga_mgr_firmware_write(struct fpga_manager *mgr,
+				   const char *fw_name)
+{
+	int ret = 0;
+	const struct firmware *fw;
+
+	if (!fw_name || !strlen(fw_name)) {
+		dev_dbg(mgr->dev, "Firmware name is not specified\n");
+		return -EINVAL;
+	}
+
+	ret = request_firmware(&fw, fw_name, mgr->dev);
+	if (ret) {
+		dev_dbg(mgr->dev, "Failed to load firmware %s\n", fw_name);
+		return ret;
+	}
+
+	/* Do write */
+	ret = fpga_mgr_write_data(mgr, fw->data, fw->size);
+	if (ret)
+		dev_dbg(mgr->dev, "Failed to write data\n");
+
+	release_firmware(fw);
+
+	return ret;
+}
+
+/**
+ * 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 length 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;
+
+	ret = fpga_mgr_firmware_write(mgr, buf);
+
+	return ret ? : count;
+}
+
+static struct device_attribute fpga_mgr_attrs[] = {
+	__ATTR(name, S_IRUGO, fpga_mgr_name_show, NULL),
+	__ATTR(firmware, S_IWUSR, NULL, fpga_mgr_attr_write),
+	__ATTR(fpga_config_state, S_IRUSR | S_IWUSR,
+	       fpga_mgr_status_read, fpga_mgr_status_write),
+	__ATTR_NULL
+};
+
+/**
+ * fpga_mgr_file_read - Perform read function through sysfs bin interface
+ * @filp: The file pointer to read from
+ * @kobj: Kernel object handle
+ * @attr: Pointer to the binary attribute structure
+ * @buf: Pointer to the buffer location with bistream data
+ * @off: File offset
+ * @count: Number of characters in @buf
+ *
+ * Returns string length added to @buf, a negative error number otherwise
+ */
+static ssize_t fpga_mgr_file_read(struct file *filp, struct kobject *kobj,
+		struct bin_attribute *attr,
+		char *buf, loff_t off, size_t count)
+{
+	int ret;
+	struct fpga_manager *mgr = dev_get_drvdata(
+					container_of(kobj, struct device,
+						     kobj));
+
+	if (test_and_set_bit_lock(FPGA_MGR_DEV_BUSY, &mgr->flags))
+		return -EBUSY;
+
+	ret = fpga_mgr_read(mgr, buf, &count);
+
+	clear_bit_unlock(FPGA_MGR_DEV_BUSY, &mgr->flags);
+
+	return ret ? : count;
+}
+
+/**
+ * fpga_mgr_write - Perform write function
+ * @filp: The file pointer to write from
+ * @kobj: Kernel object handle
+ * @attr: Pointer to the binary attribute structure
+ * @buf: Pointer to the buffer location with bistream data
+ * @off: File offset
+ * @count: Number of characters in @buf
+ *
+ * Returns string length added to @buf, a negative error number otherwise
+ */
+static ssize_t fpga_mgr_file_write(struct file *filp, struct kobject *kobj,
+		struct bin_attribute *attr,
+		char *buf, loff_t off, size_t count)
+{
+	int ret;
+	struct fpga_manager *mgr = dev_get_drvdata(
+					container_of(kobj, struct device,
+						     kobj));
+
+	if (test_and_set_bit_lock(FPGA_MGR_DEV_BUSY, &mgr->flags))
+		return -EBUSY;
+
+	/* Do write */
+	ret = fpga_mgr_write(mgr, buf, count);
+
+	clear_bit_unlock(FPGA_MGR_DEV_BUSY, &mgr->flags);
+
+	return ret ? : count;
+}
+
+static struct bin_attribute fpga_mgr_bin_attrs[] = {
+	{
+		.attr = {
+			.name = "fpga_config_data",
+			.mode = S_IRUSR | S_IWUSR
+		},
+		.read = fpga_mgr_file_read,
+		.write = fpga_mgr_file_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, const char *name,
+		      void *priv)
+{
+	struct fpga_manager *mgr;
+	int ret;
+
+	if (!mops) {
+		dev_dbg(&pdev->dev, "Register failed: NO fpga_manager_ops\n");
+		return -EINVAL;
+	}
+
+	if (!name || !strlen(name)) {
+		dev_dbg(&pdev->dev, "Register failed: NO name specific\n");
+		return -EINVAL;
+	}
+
+	mgr = devm_kzalloc(&pdev->dev, sizeof(*mgr), GFP_KERNEL);
+	if (!mgr)
+		return -ENOMEM;
+
+	mgr->name = devm_kzalloc(&pdev->dev, strlen(name) + 1, GFP_KERNEL);
+	if (!mgr->name)
+		return -ENOMEM;
+
+	strcpy(mgr->name, name);
+
+	/* 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_data;
+	mgr->fpga_write = fpga_mgr_write_data;
+	mgr->fpga_firmware_write = fpga_mgr_firmware_write;
+
+	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_dbg(&pdev->dev, "Failed to create device\n");
+		put_device(&pdev->dev);
+		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;
+
+	dev = bus_find_device(&platform_bus_type, NULL, node,
+			      of_dev_node_match);
+	if (!dev)
+		return NULL;
+
+	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_manager");
+	if (IS_ERR(fpga_mgr_class))
+		return PTR_ERR(fpga_mgr_class);
+
+	fpga_mgr_class->dev_attrs = fpga_mgr_attrs;
+	fpga_mgr_class->dev_bin_attrs = fpga_mgr_bin_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..7e73506
--- /dev/null
+++ b/include/linux/fpga.h
@@ -0,0 +1,110 @@
+/*
+ * 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
+ *
+ * @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 (*read_init)(struct fpga_manager *mgr);
+	int (*read)(struct fpga_manager *mgr, const u8 *buf, ssize_t *count);
+	int (*read_complete)(struct fpga_manager *mgr);
+	int (*write_init)(struct fpga_manager *mgr);
+	int (*write)(struct fpga_manager *mgr, const u8 *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
+#define FPGA_MGR_INIT_DONE	1
+#define FPGA_MGR_READ		2
+#define FPGA_MGR_WRITE		3
+
+/**
+ * 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.
+ * @fpga_firmware_write: The function to call for writing firmware name
+ *			 which contains configuration data for the FPGA.
+ */
+struct fpga_manager {
+	char *name;
+	struct device *dev;
+	struct fpga_manager_ops *mops;
+	void *priv;
+	unsigned int nr;
+	unsigned long flags;
+	int (*fpga_read)(struct fpga_manager *mgr, const u8 *buf,
+			 ssize_t *count);
+	int (*fpga_write)(struct fpga_manager *mgr, const u8 *buf,
+			 ssize_t count);
+	int (*fpga_firmware_write)(struct fpga_manager *mgr,
+				   const char *fw_name);
+};
+
+int fpga_mgr_register(struct platform_device *pdev,
+		      struct fpga_manager_ops *mops, const 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


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [RFC PATCH v2] fpga: Introduce new fpga subsystem

[Joe Perches]

On Wed, 2013-10-02 at 17:35 +0200, Michal Simek wrote:
> This new fpga subsystem core should unify all fpga drivers/managers 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
> interface for these drivers.

Does this interface support partial reprogramming/configuration
for FPGAs that can do that?

trivial notes:

There are a _lot_ of dev_dbg statements.

I hope some of these would be removed one day,
especially the function tracing style ones, because
there's already a generic kernel mechanism for that.

Maybe perf/trace support could be added eventually.

> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
[]
> +/**
> + * fpga_mgr_status_write - Write fpga manager status
> + * @dev: Pointer to the device structure
> + * @attr: Pointer to the device attribute structure
> + * @buf: Pointer to the buffer location
> + * @count: Number of characters in @buf
> + *
> + * Returns the number of bytes copied to @buf, a negative error number otherwise
> + */
> +static ssize_t fpga_mgr_status_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 (test_and_set_bit_lock(FPGA_MGR_DEV_BUSY, &mgr->flags))
> +		return -EBUSY;
> +
> +	ret = strcmp(buf, "write_init");
> +	if (!ret) {
> +		ret = fpga_mgr_write_init(mgr);
> +		goto out;
> +	}
> +
> +	ret = strcmp(buf, "write_complete");
> +	if (!ret) {
> +		ret = fpga_mgr_write_complete(mgr);
> +		goto out;
> +	}
> +
> +	ret = strcmp(buf, "read_init");
> +	if (!ret) {
> +		ret = fpga_mgr_read_init(mgr);
> +		goto out;
> +	}
> +
> +	ret = strcmp(buf, "read_complete");
> +	if (!ret) {
> +		ret = fpga_mgr_read_complete(mgr);
> +		goto out;
> +	}
> +
> +	ret = -EINVAL;
> +out:
> +	clear_bit_unlock(FPGA_MGR_DEV_BUSY, &mgr->flags);
> +
> +	return ret ? : count;
> +}

I think this style is awkward and this would be
better written as:

	if (!strcmp(buf, "write_init"))
		ret = fpga_mgr_write_init(mgr);
	else if (!strcmp(buf, "write_complete"))
		ret = fpga_mgr_write_complete(mgr);
	else if (!strcmp(buf, "read_init"))
		ret = fpga_mgr_read_init(mgr);
	else if (!strcmp(buf, "read_complete"))
		ret = fpga_mgr_read_complete(mgr);
	else
		ret = -EINVAL;

	clear_bit_unlock(FPGA_MGR_DEV_BUSY, &mgr->flags);

	if (ret)
		return ret;

	return count;
}

Maybe use (strcmp(...) == 0) if you prefer that.
Both styles are commonly used in linux.

Probably all of the "return ret ?: count;" uses
would be more easily understood on 3 lines.

Re: [RFC PATCH v2] fpga: Introduce new fpga subsystem

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 3930 bytes --]

Hi Joe,

On 10/02/2013 06:06 PM, Joe Perches wrote:
> On Wed, 2013-10-02 at 17:35 +0200, Michal Simek wrote:
>> This new fpga subsystem core should unify all fpga drivers/managers 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
>> interface for these drivers.
> 
> Does this interface support partial reprogramming/configuration
> for FPGAs that can do that?

Currently we have two interfaces and third one is around (char driver)
that's why I didn't look at this support.
But for Xilinx devices init and complete phase is different.

We can use one more flag parameter for init and complete functions.

It means we can simple extend config states with write_init_partial, etc
Or the second option is to create new sysfs file to indicate
that we will work with partial bitstreams.

> trivial notes:
> 
> There are a _lot_ of dev_dbg statements.
> 
> I hope some of these would be removed one day,
> especially the function tracing style ones, because
> there's already a generic kernel mechanism for that.
> 
> Maybe perf/trace support could be added eventually.

Are you talking about trace_printk?

>> diff --git a/drivers/fpga/fpga-mgr.c b/drivers/fpga/fpga-mgr.c
> []
>> +/**
>> + * fpga_mgr_status_write - Write fpga manager status
>> + * @dev: Pointer to the device structure
>> + * @attr: Pointer to the device attribute structure
>> + * @buf: Pointer to the buffer location
>> + * @count: Number of characters in @buf
>> + *
>> + * Returns the number of bytes copied to @buf, a negative error number otherwise
>> + */
>> +static ssize_t fpga_mgr_status_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 (test_and_set_bit_lock(FPGA_MGR_DEV_BUSY, &mgr->flags))
>> +		return -EBUSY;
>> +
>> +	ret = strcmp(buf, "write_init");
>> +	if (!ret) {
>> +		ret = fpga_mgr_write_init(mgr);
>> +		goto out;
>> +	}
>> +
>> +	ret = strcmp(buf, "write_complete");
>> +	if (!ret) {
>> +		ret = fpga_mgr_write_complete(mgr);
>> +		goto out;
>> +	}
>> +
>> +	ret = strcmp(buf, "read_init");
>> +	if (!ret) {
>> +		ret = fpga_mgr_read_init(mgr);
>> +		goto out;
>> +	}
>> +
>> +	ret = strcmp(buf, "read_complete");
>> +	if (!ret) {
>> +		ret = fpga_mgr_read_complete(mgr);
>> +		goto out;
>> +	}
>> +
>> +	ret = -EINVAL;
>> +out:
>> +	clear_bit_unlock(FPGA_MGR_DEV_BUSY, &mgr->flags);
>> +
>> +	return ret ? : count;
>> +}
> 
> I think this style is awkward and this would be
> better written as:
> 
> 	if (!strcmp(buf, "write_init"))
> 		ret = fpga_mgr_write_init(mgr);
> 	else if (!strcmp(buf, "write_complete"))
> 		ret = fpga_mgr_write_complete(mgr);
> 	else if (!strcmp(buf, "read_init"))
> 		ret = fpga_mgr_read_init(mgr);
> 	else if (!strcmp(buf, "read_complete"))
> 		ret = fpga_mgr_read_complete(mgr);
> 	else
> 		ret = -EINVAL;
> 
> 	clear_bit_unlock(FPGA_MGR_DEV_BUSY, &mgr->flags);
> 
> 	if (ret)
> 		return ret;
> 
> 	return count;
> }
> 
> Maybe use (strcmp(...) == 0) if you prefer that.
> Both styles are commonly used in linux.


sure.


> Probably all of the "return ret ?: count;" uses
> would be more easily understood on 3 lines.

This structure is also quite common in the kernel
git grep "? :"  | wc -l
415

git grep "?:"  | wc -l
541

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: 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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [RFC PATCH v2] fpga: Introduce new fpga subsystem

[Greg Kroah-Hartman]

On Fri, Oct 04, 2013 at 06:15:28PM +0200, Michal Simek wrote:
> > Probably all of the "return ret ?: count;" uses
> > would be more easily understood on 3 lines.
> 
> This structure is also quite common in the kernel
> git grep "? :"  | wc -l
> 415
> 
> git grep "?:"  | wc -l
> 541

And it should all be removed and fixed up properly to make it easier to
read and understand.  Please don't add new instances of it, I sure will
not take it.

greg k-h

Re: [RFC PATCH v2] fpga: Introduce new fpga subsystem

[Jason Gunthorpe]

On Wed, Oct 02, 2013 at 05:35:58PM +0200, Michal Simek wrote:

> +What:		/sys/class/fpga_manager/fpga<dev-id>/fpga_config_state
> +Date:		October 2013
> +KernelVersion:	3.12
> +Contact:	Michal Simek <michal.simek@xilinx.com>
> +Description:
> +		By reading this file you will get current fpga manager state.
> +		Flag bits are present in include/linux/fpga.h (FPGA_MGR_XX).
> +		By writing to this file you can change fpga manager state.
> +		Valid options are: write_init, write_complete, read_init,
> +		read_complete.

This shouldn't be asymmetric - read/write should be in the same
format.

I strongly encourage you to use text strings to indicate the state of
the configuration FSM, and I *really* think you should rework things
to have an explicit configuration FSM rather than trying to bodge one
together with a bunch of bit flags.

Plus error handling is missing, failures need to be reported back.

Noticed several typos:

> +
> +	if (mgr->mops->read_init) {
> +		ret = mgr->mops->read_init(mgr);
> +		if (ret) {
> +			dev_dbg(mgr->dev, "Failed to write_init\n");
                                                    ^^^^^^^^
read_init

> +	if (mgr->mops->write) {
                   ^^^^^^^^^^
read

> +		ret = mgr->mops->read(mgr, buf, count);
> +		if (ret) {
> +			dev_dbg(mgr->dev, "Failed to write\n");
                                             ^^^^^^^^^^^^^^^^^
read

> +
> +	if (mgr->mops->write_complete) {
                    ^^^^^^^^^^
read
> +		ret = mgr->mops->read_complete(mgr);
> +		if (ret) {
> +			dev_dbg(mgr->dev, "Failed to write_complete\n");
  					  	  ^^^^^^^^^^^^
read

> +static inline int fpga_mgr_write(struct fpga_manager *mgr, const u8 *buf,
> +				 ssize_t count)
> +{
> +	int bit, ret;
> +
> +	dev_dbg(mgr->dev, "%s %lx\n", __func__, mgr->flags);
> +
> +	/* FPGE init has to be done to be able to continue */
           ^^^^^^
FPGA

> +static struct device_attribute fpga_mgr_attrs[] = {
> +	__ATTR(name, S_IRUGO, fpga_mgr_name_show, NULL),
> +	__ATTR(firmware, S_IWUSR, NULL, fpga_mgr_attr_write),
> +	__ATTR(fpga_config_state, S_IRUSR | S_IWUSR,
> +	       fpga_mgr_status_read, fpga_mgr_status_write),
> +	__ATTR_NULL
> +};

AFAIK it is preferred to use DEVICE_ATTR_RO(), ATTRIBUTE_GROUPS()
and struct class.dev_groups

eg see this note in linux/device.h

struct class {
        struct device_attribute         *dev_attrs;     /* use dev_groups instead */
        const struct attribute_group    **dev_groups;
}

> +	struct fpga_manager *mgr;
> +	int ret;
> +
> +	if (!mops) {
> +		dev_dbg(&pdev->dev, "Register failed: NO fpga_manager_ops\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!name || !strlen(name)) {
> +		dev_dbg(&pdev->dev, "Register failed: NO name specific\n");
> +		return -EINVAL;
> +	}
> +
> +	mgr = devm_kzalloc(&pdev->dev, sizeof(*mgr), GFP_KERNEL);
> +	if (!mgr)
> +		return -ENOMEM;

I wonder if this is right, it seems like a strange way to make a class
subsystem, usually the struct fpga_manager would contain the 'struct
device' (not a pointer, so you can use container_of) and drvdata would
be reserved for something else.

This seems to create lifetime issues since the devm above will be
free'd when the platform driver is released, but the class device will
live on with the stray pointer. Better to allocate everything against
the class device below.

Plus you need to ensure the device is fully functionally before
device_register is called, otherwise you race with notifications to
userspace.

> +/**
> + * 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);

What happens when userspace is holding one of the sysfs files open and
you unload the module? Looks like bad things?

Regards,
Jason

Re: [RFC PATCH v2] fpga: Introduce new fpga subsystem

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 5829 bytes --]

On 10/02/2013 07:46 PM, Jason Gunthorpe wrote:
> On Wed, Oct 02, 2013 at 05:35:58PM +0200, Michal Simek wrote:
> 
>> +What:		/sys/class/fpga_manager/fpga<dev-id>/fpga_config_state
>> +Date:		October 2013
>> +KernelVersion:	3.12
>> +Contact:	Michal Simek <michal.simek@xilinx.com>
>> +Description:
>> +		By reading this file you will get current fpga manager state.
>> +		Flag bits are present in include/linux/fpga.h (FPGA_MGR_XX).
>> +		By writing to this file you can change fpga manager state.
>> +		Valid options are: write_init, write_complete, read_init,
>> +		read_complete.
> 
> This shouldn't be asymmetric - read/write should be in the same
> format.
> 
> I strongly encourage you to use text strings to indicate the state of
> the configuration FSM, and I *really* think you should rework things
> to have an explicit configuration FSM rather than trying to bodge one
> together with a bunch of bit flags.

Any favourite names for states?
Or ready, write_init, write_complete is enough for now?

Alan: This should be unified way for user to get proper states from the
driver. It means from my point of view will be better to have set of states
which this function can return instead of calling origin status function
where we can't control states for all these drivers.

> Plus error handling is missing, failures need to be reported back.

Will fix this.

> Noticed several typos:

Ah yeah c&p. :-(

> 
>> +
>> +	if (mgr->mops->read_init) {
>> +		ret = mgr->mops->read_init(mgr);
>> +		if (ret) {
>> +			dev_dbg(mgr->dev, "Failed to write_init\n");
>                                                     ^^^^^^^^
> read_init
> 
>> +	if (mgr->mops->write) {
>                    ^^^^^^^^^^
> read
> 
>> +		ret = mgr->mops->read(mgr, buf, count);
>> +		if (ret) {
>> +			dev_dbg(mgr->dev, "Failed to write\n");
>                                              ^^^^^^^^^^^^^^^^^
> read
> 
>> +
>> +	if (mgr->mops->write_complete) {
>                     ^^^^^^^^^^
> read
>> +		ret = mgr->mops->read_complete(mgr);
>> +		if (ret) {
>> +			dev_dbg(mgr->dev, "Failed to write_complete\n");
>   					  	  ^^^^^^^^^^^^
> read
> 
>> +static inline int fpga_mgr_write(struct fpga_manager *mgr, const u8 *buf,
>> +				 ssize_t count)
>> +{
>> +	int bit, ret;
>> +
>> +	dev_dbg(mgr->dev, "%s %lx\n", __func__, mgr->flags);
>> +
>> +	/* FPGE init has to be done to be able to continue */
>            ^^^^^^
> FPGA
> 
>> +static struct device_attribute fpga_mgr_attrs[] = {
>> +	__ATTR(name, S_IRUGO, fpga_mgr_name_show, NULL),
>> +	__ATTR(firmware, S_IWUSR, NULL, fpga_mgr_attr_write),
>> +	__ATTR(fpga_config_state, S_IRUSR | S_IWUSR,
>> +	       fpga_mgr_status_read, fpga_mgr_status_write),
>> +	__ATTR_NULL
>> +};
> 
> AFAIK it is preferred to use DEVICE_ATTR_RO(), ATTRIBUTE_GROUPS()
> and struct class.dev_groups
> 
> eg see this note in linux/device.h
> 
> struct class {
>         struct device_attribute         *dev_attrs;     /* use dev_groups instead */
>         const struct attribute_group    **dev_groups;
> }
> 

This will be fixed in v3. I have already changed this.

>> +	struct fpga_manager *mgr;
>> +	int ret;
>> +
>> +	if (!mops) {
>> +		dev_dbg(&pdev->dev, "Register failed: NO fpga_manager_ops\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (!name || !strlen(name)) {
>> +		dev_dbg(&pdev->dev, "Register failed: NO name specific\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	mgr = devm_kzalloc(&pdev->dev, sizeof(*mgr), GFP_KERNEL);
>> +	if (!mgr)
>> +		return -ENOMEM;
> 
> I wonder if this is right, it seems like a strange way to make a class
> subsystem, usually the struct fpga_manager would contain the 'struct
> device' (not a pointer, so you can use container_of) and drvdata would
> be reserved for something else.

I am not following you here. mrg structure is connected with the driver
it means when driver is removed then this structure is freed.


> 
> This seems to create lifetime issues since the devm above will be
> free'd when the platform driver is released, but the class device will
> live on with the stray pointer. Better to allocate everything against
> the class device below.

device in unregistered before this structure is freed.
fpga_mgr_unregister() is called in the platform driver in remove function.

> 
> Plus you need to ensure the device is fully functionally before
> device_register is called, otherwise you race with notifications to
> userspace.

fpga_mgr_register() should be called as the latest function in the probe
in platform_driver. At least it is what I have for zynq.

>> +/**
>> + * 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);
> 
> What happens when userspace is holding one of the sysfs files open and
> you unload the module? Looks like bad things?

I didn't test this but feel free to check it.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: 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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [RFC PATCH v2] fpga: Introduce new fpga subsystem

[Jason Gunthorpe]

On Fri, Oct 04, 2013 at 06:28:23PM +0200, Michal Simek wrote:
> > I strongly encourage you to use text strings to indicate the state of
> > the configuration FSM, and I *really* think you should rework things
> > to have an explicit configuration FSM rather than trying to bodge one
> > together with a bunch of bit flags.
> 
> Any favourite names for states?
> Or ready, write_init, write_complete is enough for now?

Doesnt really matter to me, don't forget error states. Transisionts to
ready, write_init and write_complete can all fail.

> > I wonder if this is right, it seems like a strange way to make a class
> > subsystem, usually the struct fpga_manager would contain the 'struct
> > device' (not a pointer, so you can use container_of) and drvdata would
> > be reserved for something else.
> 
> I am not following you here. mrg structure is connected with the driver
> it means when driver is removed then this structure is freed.

You've got your mgr structure and then later you allocate a struct
device as well, the mgr can be free'd before the struct device is
released, due to the way ref counting works. You are not doing
anything to compensate for that.

> > This seems to create lifetime issues since the devm above will be
> > free'd when the platform driver is released, but the class device will
> > live on with the stray pointer. Better to allocate everything against
> > the class device below.
> 
> device in unregistered before this structure is freed.
> fpga_mgr_unregister() is called in the platform driver in remove function.

Which is the problem, device_unregister dosen't delete 'mgr->dev', and it
doesn't mean the sysfs call backs are uncallable, so you have a free'd
dangling pointer in mgr->dev, and an object lifetime issue.

> > What happens when userspace is holding one of the sysfs files open and
> > you unload the module? Looks like bad things?
> 
> I didn't test this but feel free to check it.

You should fix these problems before your driver reaches Linus.

Jason

Re: [RFC PATCH v2] fpga: Introduce new fpga subsystem

[Alan Tull]

On Fri, 2013-10-04 at 18:28 +0200, Michal Simek wrote:
> On 10/02/2013 07:46 PM, Jason Gunthorpe wrote:
> > On Wed, Oct 02, 2013 at 05:35:58PM +0200, Michal Simek wrote:
> > 
> >> +What:		/sys/class/fpga_manager/fpga<dev-id>/fpga_config_state
> >> +Date:		October 2013
> >> +KernelVersion:	3.12
> >> +Contact:	Michal Simek <michal.simek@xilinx.com>
> >> +Description:
> >> +		By reading this file you will get current fpga manager state.
> >> +		Flag bits are present in include/linux/fpga.h (FPGA_MGR_XX).
> >> +		By writing to this file you can change fpga manager state.
> >> +		Valid options are: write_init, write_complete, read_init,
> >> +		read_complete.
> > 
> > This shouldn't be asymmetric - read/write should be in the same
> > format.
> > 
> > I strongly encourage you to use text strings to indicate the state of
> > the configuration FSM, and I *really* think you should rework things
> > to have an explicit configuration FSM rather than trying to bodge one
> > together with a bunch of bit flags.
> 
> Any favourite names for states?
> Or ready, write_init, write_complete is enough for now?

Currently the only state I want to attempt to *set* from sysfs would be
reset. 

For Altera fpga, the states I want to *report* are:

static const char *const altera_fpga_state_str[] = {
        [ALT_FPGAMGR_STAT_POWER_UP] = "power up phase",
        [ALT_FPGAMGR_STAT_RESET] = "reset phase",
        [ALT_FPGAMGR_STAT_CFG] = "configuration phase",
        [ALT_FPGAMGR_STAT_INIT] = "initialization phase",
        [ALT_FPGAMGR_STAT_USER_MODE] = "user mode",
        [ALT_FPGAMGR_STAT_UNKNOWN] = "undetermined",
        [ALT_FPGAMGR_STAT_POWER_OFF] = "powered off",
};

Setting write_init and write_complete explicitly is fine, but not needed
for me.  I'm fine with the driver knowing that if I request to write
data, it knows that it needs to write_init and write_complete.  Either
way works for me here.


> 
> Alan: This should be unified way for user to get proper states from the
> driver. It means from my point of view will be better to have set of states
> which this function can return instead of calling origin status function
> where we can't control states for all these drivers.

I agree with both you and Jason on this :)  I agree that it should be
strings, and also that it probably should come from the framework
instead of directly from the low level driver.  The framework should
query the low level driver for state.  Not all vendors' fpgas support
exactly the same set of states, so we can hash out a superset that we
can agree on that fits all our needs (mine are above). 

I think this is separate from your 'flags' implementation.  I like that
implementation for locking, but I want something that actually goes to
the low level driver to find out what state the fpga manager has to
report.

> 
> > Plus error handling is missing, failures need to be reported back.
> 
> Will fix this.
> 
> > Noticed several typos:
> 
> Ah yeah c&p. :-(
> 
> > 
> >> +
> >> +	if (mgr->mops->read_init) {
> >> +		ret = mgr->mops->read_init(mgr);
> >> +		if (ret) {
> >> +			dev_dbg(mgr->dev, "Failed to write_init\n");
> >                                                     ^^^^^^^^
> > read_init
> > 
> >> +	if (mgr->mops->write) {
> >                    ^^^^^^^^^^
> > read
> > 
> >> +		ret = mgr->mops->read(mgr, buf, count);
> >> +		if (ret) {
> >> +			dev_dbg(mgr->dev, "Failed to write\n");
> >                                              ^^^^^^^^^^^^^^^^^
> > read
> > 
> >> +
> >> +	if (mgr->mops->write_complete) {
> >                     ^^^^^^^^^^
> > read
> >> +		ret = mgr->mops->read_complete(mgr);
> >> +		if (ret) {
> >> +			dev_dbg(mgr->dev, "Failed to write_complete\n");
> >   					  	  ^^^^^^^^^^^^
> > read
> > 
> >> +static inline int fpga_mgr_write(struct fpga_manager *mgr, const u8 *buf,
> >> +				 ssize_t count)
> >> +{
> >> +	int bit, ret;
> >> +
> >> +	dev_dbg(mgr->dev, "%s %lx\n", __func__, mgr->flags);
> >> +
> >> +	/* FPGE init has to be done to be able to continue */
> >            ^^^^^^
> > FPGA
> > 
> >> +static struct device_attribute fpga_mgr_attrs[] = {
> >> +	__ATTR(name, S_IRUGO, fpga_mgr_name_show, NULL),
> >> +	__ATTR(firmware, S_IWUSR, NULL, fpga_mgr_attr_write),
> >> +	__ATTR(fpga_config_state, S_IRUSR | S_IWUSR,
> >> +	       fpga_mgr_status_read, fpga_mgr_status_write),
> >> +	__ATTR_NULL
> >> +};
> > 
> > AFAIK it is preferred to use DEVICE_ATTR_RO(), ATTRIBUTE_GROUPS()
> > and struct class.dev_groups
> > 
> > eg see this note in linux/device.h
> > 
> > struct class {
> >         struct device_attribute         *dev_attrs;     /* use dev_groups instead */
> >         const struct attribute_group    **dev_groups;
> > }
> > 
> 
> This will be fixed in v3. I have already changed this.
> 
> >> +	struct fpga_manager *mgr;
> >> +	int ret;
> >> +
> >> +	if (!mops) {
> >> +		dev_dbg(&pdev->dev, "Register failed: NO fpga_manager_ops\n");
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	if (!name || !strlen(name)) {
> >> +		dev_dbg(&pdev->dev, "Register failed: NO name specific\n");
> >> +		return -EINVAL;
> >> +	}
> >> +
> >> +	mgr = devm_kzalloc(&pdev->dev, sizeof(*mgr), GFP_KERNEL);
> >> +	if (!mgr)
> >> +		return -ENOMEM;
> > 
> > I wonder if this is right, it seems like a strange way to make a class
> > subsystem, usually the struct fpga_manager would contain the 'struct
> > device' (not a pointer, so you can use container_of) and drvdata would
> > be reserved for something else.
> 
> I am not following you here. mrg structure is connected with the driver
> it means when driver is removed then this structure is freed.
> 
> 
> > 
> > This seems to create lifetime issues since the devm above will be
> > free'd when the platform driver is released, but the class device will
> > live on with the stray pointer. Better to allocate everything against
> > the class device below.
> 
> device in unregistered before this structure is freed.
> fpga_mgr_unregister() is called in the platform driver in remove function.
> 
> > 
> > Plus you need to ensure the device is fully functionally before
> > device_register is called, otherwise you race with notifications to
> > userspace.
> 
> fpga_mgr_register() should be called as the latest function in the probe
> in platform_driver. At least it is what I have for zynq.
> 
> >> +/**
> >> + * 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);
> > 
> > What happens when userspace is holding one of the sysfs files open and
> > you unload the module? Looks like bad things?
> 
> I didn't test this but feel free to check it.
> 
> Thanks,
> Michal
> 

Re: [RFC PATCH v2 0/1] FPGA subsystem core

[H. Peter Anvin]

On 10/02/2013 08:35 AM, Michal Simek wrote:
> 
> Based on my discussion at ELC with Greg KH the new driver should
> support firmware interface for loading bitstream.
> 

As I have previously stated, I think this is a mistake simply because
the firmware interface is a bad mapping on requirements for an FPGA,
especially once you account for the vast number of ways an FPGA can
get loaded and you take partial reconfiguration into account.  I
happen to be at a face to face meeting with Greg today, so I'll pick
his brain a bit.

	-hpa

Re: [RFC PATCH v2 0/1] FPGA subsystem core

[Pavel Machek]

On Wed 2013-10-02 12:00:52, H. Peter Anvin wrote:
> On 10/02/2013 08:35 AM, Michal Simek wrote:
> > 
> > Based on my discussion at ELC with Greg KH the new driver should
> > support firmware interface for loading bitstream.
> > 
> 
> As I have previously stated, I think this is a mistake simply because
> the firmware interface is a bad mapping on requirements for an FPGA,
> especially once you account for the vast number of ways an FPGA can
> get loaded and you take partial reconfiguration into account.  I
> happen to be at a face to face meeting with Greg today, so I'll pick
> his brain a bit.

Umm. Could the discussion be kept on line?

Yes, more than one bitstream makes sense for fpga, but more than one
firmware makes sense for most other devices, too... 

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [RFC PATCH v2 0/1] FPGA subsystem core

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 1120 bytes --]

On 10/03/2013 08:49 AM, Pavel Machek wrote:
> On Wed 2013-10-02 12:00:52, H. Peter Anvin wrote:
>> On 10/02/2013 08:35 AM, Michal Simek wrote:
>>>
>>> Based on my discussion at ELC with Greg KH the new driver should
>>> support firmware interface for loading bitstream.
>>>
>>
>> As I have previously stated, I think this is a mistake simply because
>> the firmware interface is a bad mapping on requirements for an FPGA,
>> especially once you account for the vast number of ways an FPGA can
>> get loaded and you take partial reconfiguration into account.  I
>> happen to be at a face to face meeting with Greg today, so I'll pick
>> his brain a bit.
> 
> Umm. Could the discussion be kept on line?

I agree with you.
But anyway what was resolution from that meeting?

Thanks,
Michal


-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: 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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [RFC PATCH v2 0/1] FPGA subsystem core

[Greg Kroah-Hartman]

On Fri, Oct 04, 2013 at 03:57:57PM +0200, Michal Simek wrote:
> But anyway what was resolution from that meeting?

It never happened, we got distracted by lunch :)

Re: [RFC PATCH v2 0/1] FPGA subsystem core

[H. Peter Anvin]

Yes; I never got too corner Greg ;)

Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
>On Fri, Oct 04, 2013 at 03:57:57PM +0200, Michal Simek wrote:
>> But anyway what was resolution from that meeting?
>
>It never happened, we got distracted by lunch :)

-- 
Sent from my mobile phone.  Please pardon brevity and lack of formatting.

Re: [RFC PATCH v2 0/1] FPGA subsystem core

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 678 bytes --]

On 10/04/2013 04:21 PM, H. Peter Anvin wrote:
> Yes; I never got too corner Greg ;)
> 
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
>> On Fri, Oct 04, 2013 at 03:57:57PM +0200, Michal Simek wrote:
>>> But anyway what was resolution from that meeting?
>>
>> It never happened, we got distracted by lunch :)

Then why not to have it here?

M



-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: 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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [RFC PATCH v2 0/1] FPGA subsystem core

[H. Peter Anvin]

On 10/04/2013 07:28 AM, Michal Simek wrote:
> On 10/04/2013 04:21 PM, H. Peter Anvin wrote:
>> Yes; I never got too corner Greg ;)
>> 
>> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
>>> On Fri, Oct 04, 2013 at 03:57:57PM +0200, Michal Simek wrote:
>>>> But anyway what was resolution from that meeting?
>>> 
>>> It never happened, we got distracted by lunch :)
> 
> Then why not to have it here?
> 

The essential question is if the firmware interface really is
appropriate for FPGAs.  It definitely has a feel of a "square peg in a
round hole", especially when you consider the myriad ways FPGAs can be
configured (some persistent, some not, some which takes effect now,
some which come later, some which involve bytecode interpreters...)
and considering reconfiguration and partial reconfiguration.

	-hpa

Re: [RFC PATCH v2 0/1] FPGA subsystem core

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 2339 bytes --]

On 10/04/2013 06:46 PM, H. Peter Anvin wrote:
> On 10/04/2013 07:28 AM, Michal Simek wrote:
>> On 10/04/2013 04:21 PM, H. Peter Anvin wrote:
>>> Yes; I never got too corner Greg ;)
>>>
>>> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
>>>> On Fri, Oct 04, 2013 at 03:57:57PM +0200, Michal Simek wrote:
>>>>> But anyway what was resolution from that meeting?
>>>>
>>>> It never happened, we got distracted by lunch :)
>>
>> Then why not to have it here?
>>
> 
> The essential question is if the firmware interface really is
> appropriate for FPGAs.  It definitely has a feel of a "square peg in a
> round hole", especially when you consider the myriad ways FPGAs can be
> configured (some persistent, some not, some which takes effect now,
> some which come later, some which involve bytecode interpreters...)
> and considering reconfiguration and partial reconfiguration.

If you look at it in general I believe that there is wide range
of applications which just contain one bitstream per fpga and the bitstream is replaced
by newer version in upgrade. For them firmware interface should be pretty useful.
Just setup firmware name with bitstream and it will be automatically loaded in
startup phase.

Then there is another set of applications especially in connection to partial reconfiguration
where this can be done statically by pregenerated partial bitstreams
or automatically generated on target cpu. For doing everything on the target
firmware interface is not the best because everything can be handled by user application
and it is easier just to push this bitstream to do device and not to save it
to the fs.

I think the question here is if this subsystem could have several interfaces.
For example Alan is asking for adding char support.
Does it even make sense to have more interfaces with the same backend driver?
When this is answered then we can talk which one make sense to have.
In v2 is sysfs and firmware one. Adding char is also easy to do.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: 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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [RFC PATCH v2 0/1] FPGA subsystem core

[H. Peter Anvin]

On 10/04/2013 10:44 AM, Michal Simek wrote:
> 
> If you look at it in general I believe that there is wide range of 
> applications which just contain one bitstream per fpga and the 
> bitstream is replaced by newer version in upgrade. For them 
> firmware interface should be pretty useful. Just setup firmware 
> name with bitstream and it will be automatically loaded in startup 
> phase.
> 
> Then there is another set of applications especially in connection 
> to partial reconfiguration where this can be done statically by 
> pregenerated partial bitstreams or automatically generated on 
> target cpu. For doing everything on the target firmware interface 
> is not the best because everything can be handled by user 
> application and it is easier just to push this bitstream to do 
> device and not to save it to the fs.
> 
> I think the question here is if this subsystem could have several 
> interfaces. For example Alan is asking for adding char support. 
> Does it even make sense to have more interfaces with the same 
> backend driver? When this is answered then we can talk which one 
> make sense to have. In v2 is sysfs and firmware one. Adding char
> is also easy to do.
> 

Greg, what do you think?

I agree that the firmware interface makes sense when the use of the
FPGA is an implementation detail in a fixed hardware configuration,
but that is a fairly restricted use case all things considered.

	-hpa

Re: [RFC PATCH v2 0/1] FPGA subsystem core

[Greg Kroah-Hartman]

On Fri, Oct 04, 2013 at 11:12:13AM -0700, H. Peter Anvin wrote:
> On 10/04/2013 10:44 AM, Michal Simek wrote:
> > 
> > If you look at it in general I believe that there is wide range of 
> > applications which just contain one bitstream per fpga and the 
> > bitstream is replaced by newer version in upgrade. For them 
> > firmware interface should be pretty useful. Just setup firmware 
> > name with bitstream and it will be automatically loaded in startup 
> > phase.
> > 
> > Then there is another set of applications especially in connection 
> > to partial reconfiguration where this can be done statically by 
> > pregenerated partial bitstreams or automatically generated on 
> > target cpu. For doing everything on the target firmware interface 
> > is not the best because everything can be handled by user 
> > application and it is easier just to push this bitstream to do 
> > device and not to save it to the fs.
> > 
> > I think the question here is if this subsystem could have several 
> > interfaces. For example Alan is asking for adding char support. 
> > Does it even make sense to have more interfaces with the same 
> > backend driver? When this is answered then we can talk which one 
> > make sense to have. In v2 is sysfs and firmware one. Adding char
> > is also easy to do.
> > 
> 
> Greg, what do you think?
> 
> I agree that the firmware interface makes sense when the use of the
> FPGA is an implementation detail in a fixed hardware configuration,
> but that is a fairly restricted use case all things considered.

Ideally I thought this would be just like "firmware", you dump the file
to the FPGA, it validates it and away you go with a new image running in
the chip.

But, it sounds like this is much more complicated, so much so that
configfs might be the correct interface for it, as you can do lots of
things there, and it is very flexible (some say too flexible...)

A char device, with a zillion different custom ioctls is also a way to
do it, but one that I really want to avoid as that gets messy really
quickly.

thanks,

greg k-h

Re: [RFC PATCH v2 0/1] FPGA subsystem core

[Jason Gunthorpe]

On Fri, Oct 04, 2013 at 04:33:41PM -0700, Greg Kroah-Hartman wrote:

> > I agree that the firmware interface makes sense when the use of the
> > FPGA is an implementation detail in a fixed hardware configuration,
> > but that is a fairly restricted use case all things considered.
> 
> Ideally I thought this would be just like "firmware", you dump the file
> to the FPGA, it validates it and away you go with a new image running in
> the chip.

That is 99% of the use cases. The other stuff people are talking about
is fringe.

I've been doing FPGAs for > 10 years and I've never once used read back
via the config bus. In fact all my FPGAs turn that feature off once
they are loaded.

Partial reconfiguration is very specialized, and hard to use from a
FPGA design standpoint.

I also think it is sensible to focus this interface on simple SRAM
FPGAs, not FLASH based stuff, or whatever complex device required a
byte code interpreter (never heard of that before).

Jason

Re: [RFC PATCH v2 0/1] FPGA subsystem core

[H. Peter Anvin]

Every FPGA toolchain I know of has a way to emit JAM/STAPL bytecode files... and a fair number of programming scenarios need them.

Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:
>On Fri, Oct 04, 2013 at 04:33:41PM -0700, Greg Kroah-Hartman wrote:
>
>> > I agree that the firmware interface makes sense when the use of the
>> > FPGA is an implementation detail in a fixed hardware configuration,
>> > but that is a fairly restricted use case all things considered.
>> 
>> Ideally I thought this would be just like "firmware", you dump the
>file
>> to the FPGA, it validates it and away you go with a new image running
>in
>> the chip.
>
>That is 99% of the use cases. The other stuff people are talking about
>is fringe.
>
>I've been doing FPGAs for > 10 years and I've never once used read back
>via the config bus. In fact all my FPGAs turn that feature off once
>they are loaded.
>
>Partial reconfiguration is very specialized, and hard to use from a
>FPGA design standpoint.
>
>I also think it is sensible to focus this interface on simple SRAM
>FPGAs, not FLASH based stuff, or whatever complex device required a
>byte code interpreter (never heard of that before).
>
>Jason

-- 
Sent from my mobile phone.  Please pardon brevity and lack of formatting.

Re: [RFC PATCH v2 0/1] FPGA subsystem core

[Jason Gunthorpe]

On Fri, Oct 04, 2013 at 09:00:28PM -0700, H. Peter Anvin wrote:

> Every FPGA toolchain I know of has a way to emit JAM/STAPL bytecode
> files... and a fair number of programming scenarios need them.

Yes, but now you are talking about JTAG.

JTAG is a very different problem than configuring over the
configuration bus, I don't think it makes much sense to try and
combine those two things into a single subsystem.

The majority use of JAM/STAPL output is for manufacturing
automation. In system, In field programming of SRAM FPGAs via JTAG is
uncommon.

Jason

Re: [RFC PATCH v2 0/1] FPGA subsystem core

[H. Peter Anvin]

I do it all the time.

JAM/STAPL seems to me to be more used for exotic connections to serial flash for persistent programming.

Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:
>On Fri, Oct 04, 2013 at 09:00:28PM -0700, H. Peter Anvin wrote:
>
>> Every FPGA toolchain I know of has a way to emit JAM/STAPL bytecode
>> files... and a fair number of programming scenarios need them.
>
>Yes, but now you are talking about JTAG.
>
>JTAG is a very different problem than configuring over the
>configuration bus, I don't think it makes much sense to try and
>combine those two things into a single subsystem.
>
>The majority use of JAM/STAPL output is for manufacturing
>automation. In system, In field programming of SRAM FPGAs via JTAG is
>uncommon.
>
>Jason

-- 
Sent from my mobile phone.  Please pardon brevity and lack of formatting.

Re: [RFC PATCH v2 0/1] FPGA subsystem core

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 725 bytes --]

On 10/05/2013 07:34 AM, H. Peter Anvin wrote:
> I do it all the time.
> 
> JAM/STAPL seems to me to be more used for exotic connections to serial flash for persistent programming.

ok. I expect you have any code which you are using.
Why not to share it with us to see how you are using it?

I will try to find out some people at Xilinx to have generic overview
about all these methods.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: 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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [RFC PATCH v2 0/1] FPGA subsystem core

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 538 bytes --]

On 10/05/2013 08:56 AM, H. Peter Anvin wrote:
> I would, but in my case it was employer-owned and closed.

ok. But I believe general concept for this can be shared.
If you used char device, sysfs, etc.

Thanks,
Michal


-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: 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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [RFC PATCH v2 0/1] FPGA subsystem core

[H. Peter Anvin]

If I recall correctly we simply poked at the FPGA directly from userspace. Not ideal by any means and also meant we had to have a backup recovery mechanism as it meant that the FPGA had to be programmed already as the bus interface was in soft IP.

Michal Simek <monstr@monstr.eu> wrote:
>On 10/05/2013 08:56 AM, H. Peter Anvin wrote:
>> I would, but in my case it was employer-owned and closed.
>
>ok. But I believe general concept for this can be shared.
>If you used char device, sysfs, etc.
>
>Thanks,
>Michal

-- 
Sent from my mobile phone.  Please pardon brevity and lack of formatting.

Re: [RFC PATCH v2 0/1] FPGA subsystem core

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 779 bytes --]

On 10/07/2013 04:55 PM, H. Peter Anvin wrote:
> If I recall correctly we simply poked at the FPGA directly from userspace. 
Not ideal by any means and also meant we had to have a backup recovery mechanism as it meant
 that the FPGA had to be programmed already as the bus interface was in soft IP.

ok. How was that physical hardware connection to device you wanted to talk?
Was it any special IP with MMIO? Or gpio jtag emulation or similar?

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: 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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [RFC PATCH v2 0/1] FPGA subsystem core

[H. Peter Anvin]

Special soft IP presenting a PCI device to the host.

Michal Simek <monstr@monstr.eu> wrote:
>On 10/07/2013 04:55 PM, H. Peter Anvin wrote:
>> If I recall correctly we simply poked at the FPGA directly from
>userspace. 
>Not ideal by any means and also meant we had to have a backup recovery
>mechanism as it meant
>that the FPGA had to be programmed already as the bus interface was in
>soft IP.
>
>ok. How was that physical hardware connection to device you wanted to
>talk?
>Was it any special IP with MMIO? Or gpio jtag emulation or similar?
>
>Thanks,
>Michal

-- 
Sent from my mobile phone.  Please pardon brevity and lack of formatting.

Re: [RFC PATCH v2 0/1] FPGA subsystem core

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 1210 bytes --]

On 10/07/2013 05:07 PM, H. Peter Anvin wrote:
> Special soft IP presenting a PCI device to the host.

ok. It means that you should need just different backend for this device
which is able to communicate over PCI.

I still can't see why this case should be problematic for this fpga
manager.
As Jason pointed if this is just about JTAG emulation and your
data is in different format then you have to create your backend
which will support this configuration.
I will want to look at gpio jtag emulation to be able to program
different board. We have this support for u-boot and doing in Linux
should be also possible.

I think the question is if we can live with 2/3 user interfaces.
I tend to keep firmware one because it is covering a lot of common
use cases and it can be easily to use.
And then I don't have any preference if sysfs or char device
is better.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: 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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [RFC PATCH v2 0/1] FPGA subsystem core

[Alan Tull]

On Tue, 2013-10-08 at 15:00 +0200, Michal Simek wrote:
> On 10/07/2013 05:07 PM, H. Peter Anvin wrote:
> > Special soft IP presenting a PCI device to the host.
> 
> ok. It means that you should need just different backend for this device
> which is able to communicate over PCI.
> 
> I still can't see why this case should be problematic for this fpga
> manager.
> As Jason pointed if this is just about JTAG emulation and your
> data is in different format then you have to create your backend
> which will support this configuration.
> I will want to look at gpio jtag emulation to be able to program
> different board. We have this support for u-boot and doing in Linux
> should be also possible.
> 
> I think the question is if we can live with 2/3 user interfaces.
> I tend to keep firmware one because it is covering a lot of common
> use cases and it can be easily to use.
> And then I don't have any preference if sysfs or char device

The sysfs and char device interface are equal, except I don't think it
is right to write binary data to a sysfs attribute.

The difference between these 3 options is that firmware will work for
some fixed use cases, but either the sysfs or char interface will work
for all the use cases.

Alan


> is better.
> 
> Thanks,
> Michal
> 

Re: [RFC PATCH v2 0/1] FPGA subsystem core

[Greg Kroah-Hartman]

On Tue, Oct 08, 2013 at 11:49:46AM -0500, Alan Tull wrote:
> On Tue, 2013-10-08 at 15:00 +0200, Michal Simek wrote:
> > On 10/07/2013 05:07 PM, H. Peter Anvin wrote:
> > > Special soft IP presenting a PCI device to the host.
> > 
> > ok. It means that you should need just different backend for this device
> > which is able to communicate over PCI.
> > 
> > I still can't see why this case should be problematic for this fpga
> > manager.
> > As Jason pointed if this is just about JTAG emulation and your
> > data is in different format then you have to create your backend
> > which will support this configuration.
> > I will want to look at gpio jtag emulation to be able to program
> > different board. We have this support for u-boot and doing in Linux
> > should be also possible.
> > 
> > I think the question is if we can live with 2/3 user interfaces.
> > I tend to keep firmware one because it is covering a lot of common
> > use cases and it can be easily to use.
> > And then I don't have any preference if sysfs or char device
> 
> The sysfs and char device interface are equal, except I don't think it
> is right to write binary data to a sysfs attribute.

That's exactly what binary sysfs files are for :)

> The difference between these 3 options is that firmware will work for
> some fixed use cases, but either the sysfs or char interface will work
> for all the use cases.

I don't understand how one will work but the other will not, please
explain.

thanks,

greg k-h

Re: [RFC PATCH v2 0/1] FPGA subsystem core

[Jason Gunthorpe]

On Wed, Oct 09, 2013 at 01:37:05PM -0700, H. Peter Anvin wrote:

> A very common use case would be where a device contains an FPGA but is
> presented to the user as a product, often having its own device driver
> to drive the programmed device and/or additional logic.  From *that*
> point of view it would be nice if the FPGA subsystem had the capability
> for the *device driver* to trigger a firmware load request which is then
> fed to the FPGA subsystem for programming.  This would be an in-kernel
> interface, in other words.

That is sort of backwards though, how does the driver know it should
load and start fpga progamming?

The way we are working driver attach today is to program the FPGA,
under control of user space, and then do a PCI rescan, which discovers
the FPGA device and triggers driver binding of the PCI FPGA driver.

Please keep in mind that loading the wrong FPGA could permanently
destroy the system. This is why we have meta-data encoded with the
bitstream. The user space loader does some sanity checks :)

Jason

Re: [RFC PATCH v2 0/1] FPGA subsystem core

[H. Peter Anvin]

On 10/09/2013 02:07 PM, Jason Gunthorpe wrote:
> That is sort of backwards though, how does the driver know it should
> load and start fpga progamming?

A common way is for there to be a bitstream stored in flash which
presents an interface to download the data.  I think some FPGAs with
hard bus IP even has that built in.

Another variant -- common on USB -- is to use a simple USB interface
chip like an FTDI which can be used (sometimes in conjunction with a
CPLD) to (in effect) bitbang in a bitstream into the FPGA.  After
configuration, the programming pins are used for the USB interface.

	-hpa

Re: [RFC PATCH v2 0/1] FPGA subsystem core

[Jason Gunthorpe]

On Fri, Oct 04, 2013 at 10:34:10PM -0700, H. Peter Anvin wrote:
> I do it all the time.
> 
> JAM/STAPL seems to me to be more used for exotic connections to
> serial flash for persistent programming.

The FPGA tools write two kinds of SVF/JAM/STAPL files, one is ment to
be replayed the to FPGA itself (what I was talking about), and one is
ment to be replayed to the vendor's family of configuration PROMS (aka
funky serial FLASH)

Programming the PROM in-ciruit/in-field via JTAG is normal, while
programming the FPGA via JTAG is uncommon. This proposed FPGA
susbsytem is about programming SRAM FPGAs, not FLASH ...

Fortunately these PROMs are on their way out, modern FPGAs can be
directly connected to normal NOR FLASH and you can program the NOR
via drivers/mtd :)

Regards,
Jason

Re: [RFC PATCH v2 0/1] FPGA subsystem core

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 1656 bytes --]

On 10/05/2013 01:49 AM, Jason Gunthorpe wrote:
> On Fri, Oct 04, 2013 at 04:33:41PM -0700, Greg Kroah-Hartman wrote:
> 
>>> I agree that the firmware interface makes sense when the use of the
>>> FPGA is an implementation detail in a fixed hardware configuration,
>>> but that is a fairly restricted use case all things considered.
>>
>> Ideally I thought this would be just like "firmware", you dump the file
>> to the FPGA, it validates it and away you go with a new image running in
>> the chip.
> 
> That is 99% of the use cases. The other stuff people are talking about
> is fringe.

yep.

> I've been doing FPGAs for > 10 years and I've never once used read back
> via the config bus. In fact all my FPGAs turn that feature off once
> they are loaded.
> 
> Partial reconfiguration is very specialized, and hard to use from a
> FPGA design standpoint.

And also from software point of view in connection to drivers handling.
It means tools supports partial reconfiguration and even Xilinx produce
TRD design where partial reconfiguration is used but it is long way
to go to get this work nicely.

> I also think it is sensible to focus this interface on simple SRAM
> FPGAs, not FLASH based stuff, or whatever complex device required a
> byte code interpreter (never heard of that before).

Agree.

Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: 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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [RFC PATCH v2 0/1] FPGA subsystem core

[H. Peter Anvin]

On 10/04/2013 04:33 PM, Greg Kroah-Hartman wrote:
> 
> Ideally I thought this would be just like "firmware", you dump the file
> to the FPGA, it validates it and away you go with a new image running in
> the chip.
> 
> But, it sounds like this is much more complicated, so much so that
> configfs might be the correct interface for it, as you can do lots of
> things there, and it is very flexible (some say too flexible...)
> 
> A char device, with a zillion different custom ioctls is also a way to
> do it, but one that I really want to avoid as that gets messy really
> quickly.
> 

I'm not sure that a zillion custom ioctls are necessary, but I think we
really need to get a better understanding of the various usage cases
(and there are going to be ones where an "FPGA driver" simply makes no
sense at all.)

	-hpa

Re: [RFC PATCH v2 0/1] FPGA subsystem core

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 1306 bytes --]

On 10/05/2013 01:50 AM, H. Peter Anvin wrote:
> On 10/04/2013 04:33 PM, Greg Kroah-Hartman wrote:
>>
>> Ideally I thought this would be just like "firmware", you dump the file
>> to the FPGA, it validates it and away you go with a new image running in
>> the chip.
>>
>> But, it sounds like this is much more complicated, so much so that
>> configfs might be the correct interface for it, as you can do lots of
>> things there, and it is very flexible (some say too flexible...)
>>
>> A char device, with a zillion different custom ioctls is also a way to
>> do it, but one that I really want to avoid as that gets messy really
>> quickly.
>>
> 
> I'm not sure that a zillion custom ioctls are necessary, but I think we
> really need to get a better understanding of the various usage cases
> (and there are going to be ones where an "FPGA driver" simply makes no
> sense at all.)

Isn't this a reason to add this to staging area a see where this can go?

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: 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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [RFC PATCH v2 0/1] FPGA subsystem core

[Alan Tull]

On Fri, 2013-10-04 at 16:33 -0700, Greg Kroah-Hartman wrote:
> On Fri, Oct 04, 2013 at 11:12:13AM -0700, H. Peter Anvin wrote:
> > On 10/04/2013 10:44 AM, Michal Simek wrote:
> > > 
> > > If you look at it in general I believe that there is wide range of 
> > > applications which just contain one bitstream per fpga and the 
> > > bitstream is replaced by newer version in upgrade. For them 
> > > firmware interface should be pretty useful. Just setup firmware 
> > > name with bitstream and it will be automatically loaded in startup 
> > > phase.
> > > 
> > > Then there is another set of applications especially in connection 
> > > to partial reconfiguration where this can be done statically by 
> > > pregenerated partial bitstreams or automatically generated on 
> > > target cpu. For doing everything on the target firmware interface 
> > > is not the best because everything can be handled by user 
> > > application and it is easier just to push this bitstream to do 
> > > device and not to save it to the fs.
> > > 
> > > I think the question here is if this subsystem could have several 
> > > interfaces. For example Alan is asking for adding char support. 
> > > Does it even make sense to have more interfaces with the same 
> > > backend driver? When this is answered then we can talk which one 
> > > make sense to have. In v2 is sysfs and firmware one. Adding char
> > > is also easy to do.
> > > 
> > 
> > Greg, what do you think?
> > 
> > I agree that the firmware interface makes sense when the use of the
> > FPGA is an implementation detail in a fixed hardware configuration,
> > but that is a fairly restricted use case all things considered.
> 
> Ideally I thought this would be just like "firmware", you dump the file
> to the FPGA, it validates it and away you go with a new image running in
> the chip.
> 
> But, it sounds like this is much more complicated, so much so that
> configfs might be the correct interface for it, as you can do lots of
> things there, and it is very flexible (some say too flexible...)
> 
> A char device, with a zillion different custom ioctls is also a way to
> do it, but one that I really want to avoid as that gets messy really
> quickly.

Hi Greg,

We are discussing a char device that has very few interfaces:
 - a way of writing the image to fpga
 - a way of getting fpga manager status
 - a way of setting fpga manager state

This all looks like standard char driver interface to me.  Writing the
image could be writing to the devnode (cat image.bin > /dev/fpga0). The
status stuff would be sysfs attributes.  All normal stuff any char
driver in the kernel would do.  Why not just go with that?

> 
> thanks,
> 
> greg k-h
> 

Re: [RFC PATCH v2 0/1] FPGA subsystem core

[Greg Kroah-Hartman]

On Tue, Oct 08, 2013 at 12:00:14PM -0500, Alan Tull wrote:
> On Fri, 2013-10-04 at 16:33 -0700, Greg Kroah-Hartman wrote:
> > On Fri, Oct 04, 2013 at 11:12:13AM -0700, H. Peter Anvin wrote:
> > > On 10/04/2013 10:44 AM, Michal Simek wrote:
> > > > 
> > > > If you look at it in general I believe that there is wide range of 
> > > > applications which just contain one bitstream per fpga and the 
> > > > bitstream is replaced by newer version in upgrade. For them 
> > > > firmware interface should be pretty useful. Just setup firmware 
> > > > name with bitstream and it will be automatically loaded in startup 
> > > > phase.
> > > > 
> > > > Then there is another set of applications especially in connection 
> > > > to partial reconfiguration where this can be done statically by 
> > > > pregenerated partial bitstreams or automatically generated on 
> > > > target cpu. For doing everything on the target firmware interface 
> > > > is not the best because everything can be handled by user 
> > > > application and it is easier just to push this bitstream to do 
> > > > device and not to save it to the fs.
> > > > 
> > > > I think the question here is if this subsystem could have several 
> > > > interfaces. For example Alan is asking for adding char support. 
> > > > Does it even make sense to have more interfaces with the same 
> > > > backend driver? When this is answered then we can talk which one 
> > > > make sense to have. In v2 is sysfs and firmware one. Adding char
> > > > is also easy to do.
> > > > 
> > > 
> > > Greg, what do you think?
> > > 
> > > I agree that the firmware interface makes sense when the use of the
> > > FPGA is an implementation detail in a fixed hardware configuration,
> > > but that is a fairly restricted use case all things considered.
> > 
> > Ideally I thought this would be just like "firmware", you dump the file
> > to the FPGA, it validates it and away you go with a new image running in
> > the chip.
> > 
> > But, it sounds like this is much more complicated, so much so that
> > configfs might be the correct interface for it, as you can do lots of
> > things there, and it is very flexible (some say too flexible...)
> > 
> > A char device, with a zillion different custom ioctls is also a way to
> > do it, but one that I really want to avoid as that gets messy really
> > quickly.
> 
> Hi Greg,
> 
> We are discussing a char device that has very few interfaces:
>  - a way of writing the image to fpga
>  - a way of getting fpga manager status
>  - a way of setting fpga manager state
> 
> This all looks like standard char driver interface to me.  Writing the
> image could be writing to the devnode (cat image.bin > /dev/fpga0). The
> status stuff would be sysfs attributes.  All normal stuff any char
> driver in the kernel would do.  Why not just go with that?

Because we really hate to add new ioctls to the kernel if at all
possible.  Using sysfs (and it's one-value-per-file rule), makes
userspace tools easier, and managing the different devices in the system
easier (you know _exactly_ which device you are talking to, you don't
have to guess based on minor number).

thanks,

greg k-h

Re: [RFC PATCH v2 0/1] FPGA subsystem core

[delicious quinoa]

On Tue, Oct 8, 2013 at 4:44 PM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Tue, Oct 08, 2013 at 12:00:14PM -0500, Alan Tull wrote:
>> On Fri, 2013-10-04 at 16:33 -0700, Greg Kroah-Hartman wrote:
>> > On Fri, Oct 04, 2013 at 11:12:13AM -0700, H. Peter Anvin wrote:
>> > > On 10/04/2013 10:44 AM, Michal Simek wrote:
>> > > >
>> > > > If you look at it in general I believe that there is wide range of
>> > > > applications which just contain one bitstream per fpga and the
>> > > > bitstream is replaced by newer version in upgrade. For them
>> > > > firmware interface should be pretty useful. Just setup firmware
>> > > > name with bitstream and it will be automatically loaded in startup
>> > > > phase.
>> > > >
>> > > > Then there is another set of applications especially in connection
>> > > > to partial reconfiguration where this can be done statically by
>> > > > pregenerated partial bitstreams or automatically generated on
>> > > > target cpu. For doing everything on the target firmware interface
>> > > > is not the best because everything can be handled by user
>> > > > application and it is easier just to push this bitstream to do
>> > > > device and not to save it to the fs.
>> > > >
>> > > > I think the question here is if this subsystem could have several
>> > > > interfaces. For example Alan is asking for adding char support.
>> > > > Does it even make sense to have more interfaces with the same
>> > > > backend driver? When this is answered then we can talk which one
>> > > > make sense to have. In v2 is sysfs and firmware one. Adding char
>> > > > is also easy to do.
>> > > >
>> > >
>> > > Greg, what do you think?
>> > >
>> > > I agree that the firmware interface makes sense when the use of the
>> > > FPGA is an implementation detail in a fixed hardware configuration,
>> > > but that is a fairly restricted use case all things considered.
>> >
>> > Ideally I thought this would be just like "firmware", you dump the file
>> > to the FPGA, it validates it and away you go with a new image running in
>> > the chip.
>> >
>> > But, it sounds like this is much more complicated, so much so that
>> > configfs might be the correct interface for it, as you can do lots of
>> > things there, and it is very flexible (some say too flexible...)
>> >
>> > A char device, with a zillion different custom ioctls is also a way to
>> > do it, but one that I really want to avoid as that gets messy really
>> > quickly.
>>
>> Hi Greg,
>>
>> We are discussing a char device that has very few interfaces:
>>  - a way of writing the image to fpga
>>  - a way of getting fpga manager status
>>  - a way of setting fpga manager state
>>
>> This all looks like standard char driver interface to me.  Writing the
>> image could be writing to the devnode (cat image.bin > /dev/fpga0). The
>> status stuff would be sysfs attributes.  All normal stuff any char
>> driver in the kernel would do.  Why not just go with that?
>
> Because we really hate to add new ioctls to the kernel if at all
> possible.

I don't see any need for adding any ioctls.

> Using sysfs (and it's one-value-per-file rule), makes
> userspace tools easier, and managing the different devices in the system
> easier (you know _exactly_ which device you are talking to, you don't
> have to guess based on minor number).

That's cool.  The interface we could use is writing the raw fpga data
to /sys/class/fpga_manager/fpga0/fpga_config_data

Reading or setting the fpga state could be from
/sys/class/fpga_manager/fpga0/fpga_config_state

Or do I misunderstand?  Do you include sysfs attributes when you
are talking about ioctls?

Alan

>
> thanks,
>
> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [RFC PATCH v2 0/1] FPGA subsystem core

[Greg Kroah-Hartman]

On Tue, Oct 08, 2013 at 06:47:41PM -0500, delicious quinoa wrote:
> On Tue, Oct 8, 2013 at 4:44 PM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Tue, Oct 08, 2013 at 12:00:14PM -0500, Alan Tull wrote:
> >> On Fri, 2013-10-04 at 16:33 -0700, Greg Kroah-Hartman wrote:
> >> > On Fri, Oct 04, 2013 at 11:12:13AM -0700, H. Peter Anvin wrote:
> >> > > On 10/04/2013 10:44 AM, Michal Simek wrote:
> >> > > >
> >> > > > If you look at it in general I believe that there is wide range of
> >> > > > applications which just contain one bitstream per fpga and the
> >> > > > bitstream is replaced by newer version in upgrade. For them
> >> > > > firmware interface should be pretty useful. Just setup firmware
> >> > > > name with bitstream and it will be automatically loaded in startup
> >> > > > phase.
> >> > > >
> >> > > > Then there is another set of applications especially in connection
> >> > > > to partial reconfiguration where this can be done statically by
> >> > > > pregenerated partial bitstreams or automatically generated on
> >> > > > target cpu. For doing everything on the target firmware interface
> >> > > > is not the best because everything can be handled by user
> >> > > > application and it is easier just to push this bitstream to do
> >> > > > device and not to save it to the fs.
> >> > > >
> >> > > > I think the question here is if this subsystem could have several
> >> > > > interfaces. For example Alan is asking for adding char support.
> >> > > > Does it even make sense to have more interfaces with the same
> >> > > > backend driver? When this is answered then we can talk which one
> >> > > > make sense to have. In v2 is sysfs and firmware one. Adding char
> >> > > > is also easy to do.
> >> > > >
> >> > >
> >> > > Greg, what do you think?
> >> > >
> >> > > I agree that the firmware interface makes sense when the use of the
> >> > > FPGA is an implementation detail in a fixed hardware configuration,
> >> > > but that is a fairly restricted use case all things considered.
> >> >
> >> > Ideally I thought this would be just like "firmware", you dump the file
> >> > to the FPGA, it validates it and away you go with a new image running in
> >> > the chip.
> >> >
> >> > But, it sounds like this is much more complicated, so much so that
> >> > configfs might be the correct interface for it, as you can do lots of
> >> > things there, and it is very flexible (some say too flexible...)
> >> >
> >> > A char device, with a zillion different custom ioctls is also a way to
> >> > do it, but one that I really want to avoid as that gets messy really
> >> > quickly.
> >>
> >> Hi Greg,
> >>
> >> We are discussing a char device that has very few interfaces:
> >>  - a way of writing the image to fpga
> >>  - a way of getting fpga manager status
> >>  - a way of setting fpga manager state
> >>
> >> This all looks like standard char driver interface to me.  Writing the
> >> image could be writing to the devnode (cat image.bin > /dev/fpga0). The
> >> status stuff would be sysfs attributes.  All normal stuff any char
> >> driver in the kernel would do.  Why not just go with that?
> >
> > Because we really hate to add new ioctls to the kernel if at all
> > possible.
> 
> I don't see any need for adding any ioctls.
> 
> > Using sysfs (and it's one-value-per-file rule), makes
> > userspace tools easier, and managing the different devices in the system
> > easier (you know _exactly_ which device you are talking to, you don't
> > have to guess based on minor number).
> 
> That's cool.  The interface we could use is writing the raw fpga data
> to /sys/class/fpga_manager/fpga0/fpga_config_data
> 
> Reading or setting the fpga state could be from
> /sys/class/fpga_manager/fpga0/fpga_config_state

Ok, that's fine, I don't object to that, but you are giving up the
notification and loading ability of the kernel for the image files by
doing this, which will require you to use/write/maintain userspace
tools.  If you use the firmware interface, no userspace tool is needed
at all, which I can see some people really wanting, right?

> Or do I misunderstand?  Do you include sysfs attributes when you
> are talking about ioctls?

You can't do ioctls on sysfs files, so no.

greg k-h

Re: [RFC PATCH v2 0/1] FPGA subsystem core

[Alan Tull]

On Fri, 2013-10-04 at 19:44 +0200, Michal Simek wrote:
> On 10/04/2013 06:46 PM, H. Peter Anvin wrote:
> > On 10/04/2013 07:28 AM, Michal Simek wrote:
> >> On 10/04/2013 04:21 PM, H. Peter Anvin wrote:
> >>> Yes; I never got too corner Greg ;)
> >>>
> >>> Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> >>>> On Fri, Oct 04, 2013 at 03:57:57PM +0200, Michal Simek wrote:
> >>>>> But anyway what was resolution from that meeting?
> >>>>
> >>>> It never happened, we got distracted by lunch :)
> >>
> >> Then why not to have it here?
> >>
> > 
> > The essential question is if the firmware interface really is
> > appropriate for FPGAs.  It definitely has a feel of a "square peg in a
> > round hole", especially when you consider the myriad ways FPGAs can be
> > configured (some persistent, some not, some which takes effect now,
> > some which come later, some which involve bytecode interpreters...)
> > and considering reconfiguration and partial reconfiguration.
> 
> If you look at it in general I believe that there is wide range
> of applications which just contain one bitstream per fpga and the bitstream is replaced
> by newer version in upgrade. For them firmware interface should be pretty useful.
> Just setup firmware name with bitstream and it will be automatically loaded in
> startup phase.
> 
> Then there is another set of applications especially in connection to partial reconfiguration
> where this can be done statically by pregenerated partial bitstreams
> or automatically generated on target cpu. For doing everything on the target
> firmware interface is not the best because everything can be handled by user application
> and it is easier just to push this bitstream to do device and not to save it
> to the fs.
> 
> I think the question here is if this subsystem could have several interfaces.
> For example Alan is asking for adding char support.
> Does it even make sense to have more interfaces with the same backend driver?

Just for clarification, I'm asking for just one way to write the fpga
image data, not two or three.

I like being able to directly write the fpga image buffer from
userspace; that will support the superset of use cases.  v2 accepts the
binary image data from a sysfs attribute (cat image.bin
> /sys/class/fpga_manager/fpga0/fpga_config_data).  My original fpga
manager framework allowed writing the image data to the device node (cat
image.bin > /dev/fpga0) rather than sysfs. My point is that it that if
you are writing data, that goes to the file ops, not to sysfs
attributes.  sysfs is for text communication (Documentation/sysfs.txt:
"Attributes should be ASCII text files...")

> When this is answered then we can talk which one make sense to have.
> In v2 is sysfs and firmware one. Adding char is also easy to do.

Please, not three ways to do the same thing.  If you change from having
the fpga_config_data attribute to having a file_operations' write, that
would be what I think is more standard for char drivers in the kernel,
if I'm not mistaken.


> 
> Thanks,
> Michal
> 

Re: [RFC PATCH v2 0/1] FPGA subsystem core

[Alan Tull]

On Wed, 2013-10-02 at 17:35 +0200, Michal Simek wrote:

> 
> Through firmware interface:
> cat /sys/class/fpga_manager/fpga0/name
> echo -n fpga.bin > /sys/class/fpga_manager/fpga0/firmware
> 
> Through sysfs bin file:
> cat /sys/class/fpga_manager/fpga0/fpga_config_state
> echo -n write_init > /sys/class/fpga_manager/fpga0/fpga_config_state
> cat /lib/firmware/fpga.bin > /sys/class/fpga_manager/fpga0/fpga_config_data
> echo -n write_complete > /sys/class/fpga_manager/fpga0/fpga_config_state
> 

Hi Michal,

I have v2 working for me with Altera socfpga and had some feedback.

Add me and Dinh as maintainers.

This driver now has two interfaces for programming the image.
I don't think things in the kernel usually have multiple interfaces.

Does the fpga community in general find that the firmware class is
suitable for all our use cases?  I think it only supports the most simple
use cases.

My original fpga framework that you started with supported writing the
fpga device through the devnode, i.e.
cat fpga.bin > /dev/fpga0
I think we should get back to that basic char driver interface like that.
It seems like if you have a char driver, you would open and write to the
devnode instead of adding an attribute under /sys.

The 'flags' implementation is a nice way to do some locking.  But it doesn't
replace the status op to get fpga manager status which vanished in v2.
So please add that back.  Its interface was that catting the 'status'
attribute got a status description from the low level driver such as
'power up phase' or 'reset phase'.  Too useful to just get rid of.

Alan



> Subsystem supports working with phandles for cases where you want to load
> bitstreams for particular device through defined device.
> For example:
> mngr@0 {
> 	compatible = "whatever";
> 	fpga-mgr = <&ps7_dev_cfg_0>;
> 	...
> } ;
> 
> With these lines you can get easily load bitstream to the device.
> 
> struct fpga_manager *mgr;
> mgr = of_find_fpga_mgr_by_phandle(pdev->dev.of_node, "fpga-mgr");
> if (mgr)
> 	mgr->fpga_firmware_write(mgr, "filename");
> 
> NOTE: I have added there of_find_fpga_mgr_by_node()
> and of_find_fpga_mgr_by_phandle() but maybe they should be added
> separately to drivers/of/of_fpga.c.
> 
> Alessandro: I haven't looked at your FMC cases but maybe this
> could be also worth for your cases.
> 
> TODO:
> - Probably make sense to create doc in Documentation folder too.
> - When interface is fine also send zynq devcfg driver
> - Properly test reading (we have problem with zynq devcfg driver now)
> - Not sure if firmware interface also provide option to create
>   files from kernel space.
> 
> Thanks for your comments,
> Michal
> 
> Changes in v2:
> - Remove ! from all error message not to be shouty
> - Fix error codes
> - Add sysfs-class-fpga description
> - Use read/write helper functions with bit protection
> - s/fpga_mgr_status_show/fpga_mgr_status_read/g
> - Do not all end driver status just show core status
> - Extract firmware support to specific sysfs firmware file
> - Add support for sysfs bin attributes (fpga_config_state, fpga_config_data)
> - Allocate space for name dynamically
> - Introduce new flags bits (INIT_DONE, READ, WRITE)
> 
> Michal Simek (1):
>   fpga: Introduce new fpga subsystem
> 
>  Documentation/ABI/testing/sysfs-class-fpga |  33 ++
>  MAINTAINERS                                |   7 +
>  drivers/Kconfig                            |   2 +
>  drivers/Makefile                           |   1 +
>  drivers/fpga/Kconfig                       |  18 +
>  drivers/fpga/Makefile                      |   5 +
>  drivers/fpga/fpga-mgr.c                    | 753 +++++++++++++++++++++++++++++
>  include/linux/fpga.h                       | 110 +++++
>  8 files changed, 929 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-fpga
>  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
> 
> --
> 1.8.2.3
> 

Re: [RFC PATCH v2 0/1] FPGA subsystem core

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 2661 bytes --]

Hi,

On 10/03/2013 11:46 PM, Alan Tull wrote:
> On Wed, 2013-10-02 at 17:35 +0200, Michal Simek wrote:
> 
>>
>> Through firmware interface:
>> cat /sys/class/fpga_manager/fpga0/name
>> echo -n fpga.bin > /sys/class/fpga_manager/fpga0/firmware
>>
>> Through sysfs bin file:
>> cat /sys/class/fpga_manager/fpga0/fpga_config_state
>> echo -n write_init > /sys/class/fpga_manager/fpga0/fpga_config_state
>> cat /lib/firmware/fpga.bin > /sys/class/fpga_manager/fpga0/fpga_config_data
>> echo -n write_complete > /sys/class/fpga_manager/fpga0/fpga_config_state
>>
> 
> Hi Michal,
> 
> I have v2 working for me with Altera socfpga and had some feedback.
> 
> Add me and Dinh as maintainers.

why not just one? What about you?

> 
> This driver now has two interfaces for programming the image.
> I don't think things in the kernel usually have multiple interfaces.

The question here is if this is a problem. i2c create char devices
and also provide sysfs access too. It is done through notification.

> Does the fpga community in general find that the firmware class is
> suitable for all our use cases?  I think it only supports the most simple
> use cases.

Let's continue with this on that second thread and we will see what happen.


> My original fpga framework that you started with supported writing the
> fpga device through the devnode, i.e.
> cat fpga.bin > /dev/fpga0
> I think we should get back to that basic char driver interface like that.
> It seems like if you have a char driver, you would open and write to the
> devnode instead of adding an attribute under /sys.

It is the same as above. As you know we can simple add support for char
device with the current set of functions without changing logic in the driver.

> 
> The 'flags' implementation is a nice way to do some locking.  But it doesn't
> replace the status op to get fpga manager status which vanished in v2.
> So please add that back.  Its interface was that catting the 'status'
> attribute got a status description from the low level driver such as
> 'power up phase' or 'reset phase'.  Too useful to just get rid of.

No problem to add it back but it means that core will loose control
about values which can be returned back to the user. It is probably better
to create set of return values.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: 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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]

Re: [RFC PATCH v2 0/1] FPGA subsystem core

[Alan Tull]

On Fri, 2013-10-04 at 17:27 +0200, Michal Simek wrote:
> Hi,
> 
> On 10/03/2013 11:46 PM, Alan Tull wrote:
> > On Wed, 2013-10-02 at 17:35 +0200, Michal Simek wrote:
> > 
> >>
> >> Through firmware interface:
> >> cat /sys/class/fpga_manager/fpga0/name
> >> echo -n fpga.bin > /sys/class/fpga_manager/fpga0/firmware
> >>
> >> Through sysfs bin file:
> >> cat /sys/class/fpga_manager/fpga0/fpga_config_state
> >> echo -n write_init > /sys/class/fpga_manager/fpga0/fpga_config_state
> >> cat /lib/firmware/fpga.bin > /sys/class/fpga_manager/fpga0/fpga_config_data
> >> echo -n write_complete > /sys/class/fpga_manager/fpga0/fpga_config_state
> >>
> > 
> > Hi Michal,
> > 
> > I have v2 working for me with Altera socfpga and had some feedback.
> > 
> > Add me and Dinh as maintainers.
> 
> why not just one? What about you?

That's fine.  Just add "Alan Tull <atull@altera.com>" So we can expect
that in v3, right?

> > 
> > This driver now has two interfaces for programming the image.
> > I don't think things in the kernel usually have multiple interfaces.
> 
> The question here is if this is a problem. i2c create char devices
> and also provide sysfs access too. It is done through notification.

i2c_master_send() is called as the fops write or from an i2c client, but
not by writing data to some attribute under sysfs.  So there aren't two
direct paths from userspace to the i2c core.  I am advocating against
multiple ways for userspace to request/send fpga images for programming.

> 
> > Does the fpga community in general find that the firmware class is
> > suitable for all our use cases?  I think it only supports the most simple
> > use cases.
> 
> Let's continue with this on that second thread and we will see what happen.

Yes, let's continue this particular discussion on the other thread.

> 
> 
> > My original fpga framework that you started with supported writing the
> > fpga device through the devnode, i.e.
> > cat fpga.bin > /dev/fpga0
> > I think we should get back to that basic char driver interface like that.
> > It seems like if you have a char driver, you would open and write to the
> > devnode instead of adding an attribute under /sys.
> 
> It is the same as above. As you know we can simple add support for char
> device with the current set of functions without changing logic in the driver.
> 
> > 
> > The 'flags' implementation is a nice way to do some locking.  But it doesn't
> > replace the status op to get fpga manager status which vanished in v2.
> > So please add that back.  Its interface was that catting the 'status'
> > attribute got a status description from the low level driver such as
> > 'power up phase' or 'reset phase'.  Too useful to just get rid of.
> 
> No problem to add it back but it means that core will loose control
> about values which can be returned back to the user. It is probably better
> to create set of return values.
> 
> Thanks,
> Michal
>