LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v8 0/4] FPGA Manager Framework
@ 2015-01-06 20:13 atull
  2015-01-06 20:13 ` [PATCH v8 1/4] doc: add bindings document for altera fpga manager atull
                   ` (4 more replies)
  0 siblings, 5 replies; 64+ messages in thread
From: atull @ 2015-01-06 20:13 UTC (permalink / raw)
  To: gregkh, jgunthorpe, hpa, monstr, michal.simek, rdunlap
  Cc: linux-kernel, devicetree, pantelis.antoniou, robh+dt,
	grant.likely, iws, linux-doc, pavel, broonie, philip, rubini,
	s.trumtrar, jason, kyle.teske, nico, balbi, m.chehab, davidb,
	rob, davem, cesarb, sameo, akpm, linus.walleij, pawel.moll,
	mark.rutland, ijc+devicetree, galak, devel, delicious.quinoa,
	dinguyen, yvanderv, Alan Tull

From: Alan Tull <atull@opensource.altera.com>

v8 changes the compatible string for SOCFPGA FPGA managers
to be more chip specific.

"altr,fpga-mgr" becomes "altr,socfpga-fpga-mgr"

Thanks,
Alan

Alan Tull (4):
  doc: add bindings document for altera fpga manager
  fpga manager: add sysfs interface document
  staging: fpga manager: framework core
  staging: fpga manager: add driver for socfpga fpga manager

 drivers/staging/Kconfig                            |    2 +
 drivers/staging/Makefile                           |    1 +
 .../Documentation/ABI/sysfs-class-fpga-manager     |   38 ++
 .../bindings/altera-socfpga-fpga-mgr.txt           |   17 +
 drivers/staging/fpga/Kconfig                       |   29 +
 drivers/staging/fpga/Makefile                      |    9 +
 drivers/staging/fpga/fpga-mgr.c                    |  551 ++++++++++++++++
 drivers/staging/fpga/socfpga.c                     |  694 ++++++++++++++++++++
 include/linux/fpga/fpga-mgr.h                      |  124 ++++
 9 files changed, 1465 insertions(+)
 create mode 100644 drivers/staging/fpga/Documentation/ABI/sysfs-class-fpga-manager
 create mode 100644 drivers/staging/fpga/Documentation/bindings/altera-socfpga-fpga-mgr.txt
 create mode 100644 drivers/staging/fpga/Kconfig
 create mode 100644 drivers/staging/fpga/Makefile
 create mode 100644 drivers/staging/fpga/fpga-mgr.c
 create mode 100644 drivers/staging/fpga/socfpga.c
 create mode 100644 include/linux/fpga/fpga-mgr.h

-- 
1.7.9.5


^ permalink raw reply	[flat|nested] 64+ messages in thread

* [PATCH v8 1/4] doc: add bindings document for altera fpga manager
  2015-01-06 20:13 [PATCH v8 0/4] FPGA Manager Framework atull
@ 2015-01-06 20:13 ` atull
  2015-01-06 22:05   ` Rob Herring
  2015-01-06 20:13 ` [PATCH v8 2/4] fpga manager: add sysfs interface document atull
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 64+ messages in thread
From: atull @ 2015-01-06 20:13 UTC (permalink / raw)
  To: gregkh, jgunthorpe, hpa, monstr, michal.simek, rdunlap
  Cc: linux-kernel, devicetree, pantelis.antoniou, robh+dt,
	grant.likely, iws, linux-doc, pavel, broonie, philip, rubini,
	s.trumtrar, jason, kyle.teske, nico, balbi, m.chehab, davidb,
	rob, davem, cesarb, sameo, akpm, linus.walleij, pawel.moll,
	mark.rutland, ijc+devicetree, galak, devel, delicious.quinoa,
	dinguyen, yvanderv, Alan Tull

From: Alan Tull <atull@opensource.altera.com>

New bindings document for Altera fpga manager.

Signed-off-by: Alan Tull <atull@opensource.altera.com>
---
v5 : Move bindings to drivers/staging/fpga/Documentation/bindings

v6 : No change in this patch for v6 of the patch set

v7 : No change in this patch for v7 of the patch set

v8 : Make compatible string and name of bindings doc more chip
     specific
---
 .../bindings/altera-socfpga-fpga-mgr.txt           |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)
 create mode 100644 drivers/staging/fpga/Documentation/bindings/altera-socfpga-fpga-mgr.txt

diff --git a/drivers/staging/fpga/Documentation/bindings/altera-socfpga-fpga-mgr.txt b/drivers/staging/fpga/Documentation/bindings/altera-socfpga-fpga-mgr.txt
new file mode 100644
index 0000000..9b027a6
--- /dev/null
+++ b/drivers/staging/fpga/Documentation/bindings/altera-socfpga-fpga-mgr.txt
@@ -0,0 +1,17 @@
+Altera SOCFPGA FPGA Manager
+
+Required properties:
+- compatible : should contain "altr,socfpga-fpga-mgr"
+- reg        : base address and size for memory mapped io.
+               - The first index is for FPGA manager register access.
+               - The second index is for writing FPGA configuration data.
+- interrupts : interrupt for the FPGA Manager device.
+
+Example:
+
+	hps_0_fpgamgr: fpgamgr@0xff706000 {
+		compatible = "altr,socfpga-fpga-mgr";
+		reg = <0xFF706000 0x1000
+		       0xFFB90000 0x1000>;
+		interrupts = <0 175 4>;
+	};
-- 
1.7.9.5


^ permalink raw reply	[flat|nested] 64+ messages in thread

* [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-06 20:13 [PATCH v8 0/4] FPGA Manager Framework atull
  2015-01-06 20:13 ` [PATCH v8 1/4] doc: add bindings document for altera fpga manager atull
@ 2015-01-06 20:13 ` atull
  2015-01-07  8:48   ` Pavel Machek
  2015-01-06 20:13 ` [PATCH v8 3/4] staging: fpga manager: framework core atull
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 64+ messages in thread
From: atull @ 2015-01-06 20:13 UTC (permalink / raw)
  To: gregkh, jgunthorpe, hpa, monstr, michal.simek, rdunlap
  Cc: linux-kernel, devicetree, pantelis.antoniou, robh+dt,
	grant.likely, iws, linux-doc, pavel, broonie, philip, rubini,
	s.trumtrar, jason, kyle.teske, nico, balbi, m.chehab, davidb,
	rob, davem, cesarb, sameo, akpm, linus.walleij, pawel.moll,
	mark.rutland, ijc+devicetree, galak, devel, delicious.quinoa,
	dinguyen, yvanderv, Alan Tull

From: Alan Tull <atull@opensource.altera.com>

Add documentation under drivers/staging for new fpga manager's
sysfs interface.

Signed-off-by: Alan Tull <atull@opensource.altera.com>
---
v5  : (actually second version, but keeping version numbers
      aligned with rest of patch series)
      Move document to drivers/staging/fpga/Documentation/ABI

v6  : No change in this patch for v6 of the patch set

v7  : No change in this patch for v7 of the patch set

v8  : No change in this patch for v8 of the patch set
---
 .../Documentation/ABI/sysfs-class-fpga-manager     |   38 ++++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 drivers/staging/fpga/Documentation/ABI/sysfs-class-fpga-manager

diff --git a/drivers/staging/fpga/Documentation/ABI/sysfs-class-fpga-manager b/drivers/staging/fpga/Documentation/ABI/sysfs-class-fpga-manager
new file mode 100644
index 0000000..eb600f2
--- /dev/null
+++ b/drivers/staging/fpga/Documentation/ABI/sysfs-class-fpga-manager
@@ -0,0 +1,38 @@
+What:		/sys/class/fpga_manager/<fpga>/name
+Date:		October 2014
+KernelVersion:	3.18
+Contact:	Alan Tull <atull@opensource.altera.com>
+Description:	Name of low level driver.
+
+What:		/sys/class/fpga_manager/<fpga>/firmware
+Date:		October 2014
+KernelVersion:	3.18
+Contact:	Alan Tull <atull@opensource.altera.com>
+Description:	Name of the FPGA image file to load using firmware class.
+
+What:		/sys/class/fpga_manager/<fpga>/reset
+Date:		October 2014
+KernelVersion:	3.18
+Contact:	Alan Tull <atull@opensource.altera.com>
+Description:	Write 1 to reset the FPGA
+
+What:		/sys/class/fpga_manager/<fpga>/state
+Date:		October 2014
+KernelVersion:	3.18
+Contact:	Alan Tull <atull@opensource.altera.com>
+Description:	Read state of fpga framework state machine as a string.
+		Valid states may vary by manufacturer; superset is:
+		* unknown		= can't determine state
+		* power_off		= FPGA power is off
+		* power_up		= FPGA reports power is up
+		* reset			= FPGA held in reset state
+		* firmware_request	= firmware class request in progress
+		* firmware_request_err	= firmware request failed
+		* write_init		= FPGA being prepared for programming
+		* write_init_err	= Error while preparing FPGA for
+					  programming
+		* write			= FPGA ready to receive image data
+		* write_err		= Error while programming
+		* write_complete	= Doing post programming steps
+		* write_complete_err	= Error while doing post programming
+		* operating		= FPGA is programmed and operating
-- 
1.7.9.5


^ permalink raw reply	[flat|nested] 64+ messages in thread

* [PATCH v8 3/4] staging: fpga manager: framework core
  2015-01-06 20:13 [PATCH v8 0/4] FPGA Manager Framework atull
  2015-01-06 20:13 ` [PATCH v8 1/4] doc: add bindings document for altera fpga manager atull
  2015-01-06 20:13 ` [PATCH v8 2/4] fpga manager: add sysfs interface document atull
@ 2015-01-06 20:13 ` atull
  2015-01-06 20:13 ` [PATCH v8 4/4] staging: fpga manager: add driver for socfpga fpga manager atull
  2015-01-10 18:11 ` [PATCH v8 0/4] FPGA Manager Framework Konrad Zapalowicz
  4 siblings, 0 replies; 64+ messages in thread
From: atull @ 2015-01-06 20:13 UTC (permalink / raw)
  To: gregkh, jgunthorpe, hpa, monstr, michal.simek, rdunlap
  Cc: linux-kernel, devicetree, pantelis.antoniou, robh+dt,
	grant.likely, iws, linux-doc, pavel, broonie, philip, rubini,
	s.trumtrar, jason, kyle.teske, nico, balbi, m.chehab, davidb,
	rob, davem, cesarb, sameo, akpm, linus.walleij, pawel.moll,
	mark.rutland, ijc+devicetree, galak, devel, delicious.quinoa,
	dinguyen, yvanderv, Alan Tull

From: Alan Tull <atull@opensource.altera.com>

Supports standard ops for low level FPGA drivers.
Various manufacturors' FPGAs can be supported by adding low
level drivers.  Each driver needs to register its ops
using fpga_mgr_register().

Exports methods of doing operations to program  FPGAs. These
should be sufficient for individual drivers to request FPGA
programming directly if desired.

Adds a sysfs interface.  The sysfs interface can be compiled out
where desired in production builds.

Resume is supported by calling low level driver's resume
function, then reloading the firmware image.

The following are exported as GPL:
* fpga_mgr_reset
   Reset the FGPA.

* fpga_mgr_write
   Write a image (in buffer) to the FPGA.

* fpga_mgr_firmware_write
   Request firmware by file name and write it to the FPGA.

* fpga_mgr_name
   Get name of FPGA manager.

* fpga_mgr_state
   Get a state of framework as a string.

* fpga_mgr_register and fpga_mgr_remove
   Register/unregister low level fpga manager driver.

The following sysfs files are created:
* /sys/class/fpga_manager/<fpga>/name
  Name of low level driver.

* /sys/class/fpga_manager/<fpga>/firmware
  Name of FPGA image file to load using firmware class.
  $ echo image.rbf > /sys/class/fpga_manager/<fpga>/firmware

  read: read back name of image file previous loaded
  $ cat /sys/class/fpga_manager/<fpga>/firmware

* /sys/class/fpga_manager/<fpga>/reset
  reset the FPGA
  $ echo 1 > /sys/class/fpga_manager/<fpga>/reset

* /sys/class/fpga_manager/<fpga>/state
  State of fpga framework state machine

Signed-off-by: Alan Tull <atull@opensource.altera.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
v2: s/mangager/manager/
    check for invalid request_nr
    add fpga reset interface
    rework the state code
    remove FPGA_MGR_FAIL flag
    add _ERR states to fpga manager states enum
    low level state op now returns a state enum value
    initialize framework state from driver state op
    remove unused fpga read stuff
    merge sysfs.c into fpga-mgr.c
    move suspend/resume from bus.c to fpga-mgr.c

v3: Add struct device to fpga_manager (not as a pointer)
    Add to_fpga_manager
    Don't get irq in fpga-mgr.c (let low level driver do it)
    remove from struct fpga_manager: nr, np, parent
    get rid of fpga_mgr_get_new_minor()
    simplify fpga_manager_register:
      reorder parameters
      use dev instead of pdev
    get rid of code that used to make more sense when this
      was a char driver, don't alloc_chrdev_region
    use a mutex instead of flags

v4: Move to drivers/staging

v5: Make some things be static
    Kconfig: add 'if FPGA'
    Makefile: s/fpga-mgr-core.o/fpga-mgr.o/
    clean up includes
    use enum fpga_mgr_states instead of int
    static const char *state_str
    use DEVICE_ATTR_RO/RW/WO and ATTRIBUTE_GROUPS
    return -EINVAL instead of BUG_ON
    move ida_simple_get after kzalloc
    clean up fpga_mgr_remove
    fpga-mgr.h: remove '#if IS_ENABLED(CONFIG_FPGA)'
    add kernel-doc documentation
    Move header to new include/linux/fpga folder
    static const struct fpga_mgr_ops
    dev_info msg simplified

v6: no statically allocated string for image_name
    kernel doc fixes
    changes regarding enabling SYSFS for fpga mgr
    Makefile cleanup

v7: no change in this patch for v7 of the patchset

v8: no change in this patch for v8 of the patchset
---
 drivers/staging/Kconfig         |    2 +
 drivers/staging/Makefile        |    1 +
 drivers/staging/fpga/Kconfig    |   24 ++
 drivers/staging/fpga/Makefile   |    8 +
 drivers/staging/fpga/fpga-mgr.c |  551 +++++++++++++++++++++++++++++++++++++++
 include/linux/fpga/fpga-mgr.h   |  124 +++++++++
 6 files changed, 710 insertions(+)
 create mode 100644 drivers/staging/fpga/Kconfig
 create mode 100644 drivers/staging/fpga/Makefile
 create mode 100644 drivers/staging/fpga/fpga-mgr.c
 create mode 100644 include/linux/fpga/fpga-mgr.h

diff --git a/drivers/staging/Kconfig b/drivers/staging/Kconfig
index 815de37..6380ae2 100644
--- a/drivers/staging/Kconfig
+++ b/drivers/staging/Kconfig
@@ -106,4 +106,6 @@ source "drivers/staging/unisys/Kconfig"
 
 source "drivers/staging/clocking-wizard/Kconfig"
 
+source "drivers/staging/fpga/Kconfig"
+
 endif # STAGING
diff --git a/drivers/staging/Makefile b/drivers/staging/Makefile
index 33c640b..7ea49c0 100644
--- a/drivers/staging/Makefile
+++ b/drivers/staging/Makefile
@@ -45,3 +45,4 @@ obj-$(CONFIG_GS_FPGABOOT)	+= gs_fpgaboot/
 obj-$(CONFIG_CRYPTO_SKEIN)	+= skein/
 obj-$(CONFIG_UNISYSSPAR)	+= unisys/
 obj-$(CONFIG_COMMON_CLK_XLNX_CLKWZRD)	+= clocking-wizard/
+obj-$(CONFIG_FPGA)		+= fpga/
diff --git a/drivers/staging/fpga/Kconfig b/drivers/staging/fpga/Kconfig
new file mode 100644
index 0000000..89ebafc
--- /dev/null
+++ b/drivers/staging/fpga/Kconfig
@@ -0,0 +1,24 @@
+#
+# 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
+
+config FPGA_MGR_SYSFS
+	bool "FPGA Manager SysFS Interface"
+	depends on SYSFS
+	help
+	  FPGA Manager SysFS interface.
+
+endif # FPGA
+
+endmenu
diff --git a/drivers/staging/fpga/Makefile b/drivers/staging/fpga/Makefile
new file mode 100644
index 0000000..3313c52
--- /dev/null
+++ b/drivers/staging/fpga/Makefile
@@ -0,0 +1,8 @@
+#
+# Makefile for the fpga framework and fpga manager drivers.
+#
+
+# Core FPGA Manager Framework
+obj-$(CONFIG_FPGA)			+= fpga-mgr.o
+
+# FPGA Manager Drivers
diff --git a/drivers/staging/fpga/fpga-mgr.c b/drivers/staging/fpga/fpga-mgr.c
new file mode 100644
index 0000000..daf28b5
--- /dev/null
+++ b/drivers/staging/fpga/fpga-mgr.c
@@ -0,0 +1,551 @@
+/*
+ * FPGA Manager Core
+ *
+ *  Copyright (C) 2013-2014 Altera Corporation
+ *
+ * With code from the mailing list:
+ * Copyright (C) 2013 Xilinx, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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/delay.h>
+#include <linux/firmware.h>
+#include <linux/fpga/fpga-mgr.h>
+#include <linux/idr.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/pm.h>
+#include <linux/slab.h>
+
+static DEFINE_MUTEX(fpga_mgr_mutex);
+static DEFINE_IDA(fpga_mgr_ida);
+static struct class *fpga_mgr_class;
+
+static LIST_HEAD(fpga_manager_list);
+
+/**
+ * fpga_mgr_low_level_state - get FPGA state from low level driver
+ * @mgr: fpga manager
+ *
+ * This will be used to initialize framework state
+ *
+ * Return: an enum value for state.
+ */
+static enum fpga_mgr_states fpga_mgr_low_level_state(struct fpga_manager *mgr)
+{
+	if (!mgr || !mgr->mops || !mgr->mops->state)
+		return FPGA_MGR_STATE_UNKNOWN;
+
+	return mgr->mops->state(mgr);
+}
+
+/**
+ * __fpga_mgr_reset - unlocked version of fpga_mgr_reset
+ * @mgr: fpga manager
+ *
+ * Return: 0 on success, error code otherwise.
+ */
+static int __fpga_mgr_reset(struct fpga_manager *mgr)
+{
+	int ret;
+
+	if (!mgr->mops->reset)
+		return -EINVAL;
+
+	ret = mgr->mops->reset(mgr);
+
+	mgr->state = fpga_mgr_low_level_state(mgr);
+	kfree(mgr->image_name);
+	mgr->image_name = NULL;
+
+	return ret;
+}
+
+/**
+ * fpga_mgr_reset - reset the fpga
+ * @mgr: fpga manager
+ *
+ * Return: 0 on success, error code otherwise.
+ */
+int fpga_mgr_reset(struct fpga_manager *mgr)
+{
+	int ret;
+
+	if (!mutex_trylock(&mgr->lock))
+		return -EBUSY;
+
+	ret = __fpga_mgr_reset(mgr);
+
+	mutex_unlock(&mgr->lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(fpga_mgr_reset);
+
+/**
+ * __fpga_mgr_stage_init - prepare fpga for configuration
+ * @mgr: fpga manager
+ *
+ * Return: 0 on success, error code otherwise.
+ */
+static int __fpga_mgr_stage_write_init(struct fpga_manager *mgr)
+{
+	int ret;
+
+	if (mgr->mops->write_init) {
+		mgr->state = FPGA_MGR_STATE_WRITE_INIT;
+		ret = mgr->mops->write_init(mgr);
+		if (ret) {
+			mgr->state = FPGA_MGR_STATE_WRITE_INIT_ERR;
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+/**
+ * __fpga_mgr_stage_write - write buffer to fpga
+ * @mgr: fpga manager
+ * @buf: buffer contain fpga image
+ * @count: byte count of buf
+ *
+ * Return: 0 on success, error code otherwise.
+ */
+static int __fpga_mgr_stage_write(struct fpga_manager *mgr, const char *buf,
+				  size_t count)
+{
+	int ret;
+
+	mgr->state = FPGA_MGR_STATE_WRITE;
+	ret = mgr->mops->write(mgr, buf, count);
+	if (ret) {
+		mgr->state = FPGA_MGR_STATE_WRITE_ERR;
+		return ret;
+	}
+
+	return 0;
+}
+
+/**
+ * __fpga_mgr_stage_complete - after writing, place fpga in operating state
+ * @mgr: fpga manager
+ *
+ * Return: 0 on success, error code otherwise.
+ */
+static int __fpga_mgr_stage_write_complete(struct fpga_manager *mgr)
+{
+	int ret;
+
+	if (mgr->mops->write_complete) {
+		mgr->state = FPGA_MGR_STATE_WRITE_COMPLETE;
+		ret = mgr->mops->write_complete(mgr);
+		if (ret) {
+			mgr->state = FPGA_MGR_STATE_WRITE_COMPLETE_ERR;
+			return ret;
+		}
+	}
+
+	mgr->state = fpga_mgr_low_level_state(mgr);
+
+	return 0;
+}
+
+/**
+ * __fpga_mgr_write - whole fpga image write cycle
+ * @mgr: fpga manager
+ * @buf: buffer contain fpga image
+ * @count: byte count of buf
+ *
+ * Return: 0 on success, error code otherwise.
+ */
+static int __fpga_mgr_write(struct fpga_manager *mgr, const char *buf,
+			    size_t count)
+{
+	int ret;
+
+	ret = __fpga_mgr_stage_write_init(mgr);
+	if (ret)
+		return ret;
+
+	ret = __fpga_mgr_stage_write(mgr, buf, count);
+	if (ret)
+		return ret;
+
+	return __fpga_mgr_stage_write_complete(mgr);
+}
+
+/**
+ * fpga_mgr_write - do complete fpga image write cycle
+ * @mgr: fpga manager
+ * @buf: buffer contain fpga image
+ * @count: byte count of buf
+ *
+ * Return: 0 on success, error code otherwise.
+ */
+int fpga_mgr_write(struct fpga_manager *mgr, const char *buf, size_t count)
+{
+	int ret;
+
+	if (!mutex_trylock(&mgr->lock))
+		return -EBUSY;
+
+	dev_info(&mgr->dev, "writing buffer to %s\n", mgr->name);
+
+	ret = __fpga_mgr_write(mgr, buf, count);
+	mutex_unlock(&mgr->lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(fpga_mgr_write);
+
+/**
+ * fpga_mgr_firmware_write - request firmware and write to fpga
+ * @mgr: fpga manager
+ * @image_name: name of image file on the firmware search path
+ *
+ * Grab lock, request firmware, and write out to the FPGA.
+ * Update the state before each step to provide info on what step
+ * failed if there is a failure.
+ *
+ * Return: 0 on success, error code otherwise.
+ */
+int fpga_mgr_firmware_write(struct fpga_manager *mgr, const char *image_name)
+{
+	const struct firmware *fw;
+	int ret;
+
+	if (!mutex_trylock(&mgr->lock))
+		return -EBUSY;
+
+	dev_info(&mgr->dev, "writing %s to %s\n", image_name, mgr->name);
+
+	mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ;
+	ret = request_firmware(&fw, image_name, &mgr->dev);
+	if (ret) {
+		mgr->state = FPGA_MGR_STATE_FIRMWARE_REQ_ERR;
+		goto fw_wr_exit;
+	}
+
+	ret = __fpga_mgr_write(mgr, fw->data, fw->size);
+	if (ret)
+		goto fw_wr_exit;
+
+	kfree(mgr->image_name);
+	mgr->image_name = kstrdup(image_name, GFP_KERNEL);
+
+fw_wr_exit:
+	mutex_unlock(&mgr->lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(fpga_mgr_firmware_write);
+
+/**
+ * fpga_mgr_name - returns the fpga manager name
+ * @mgr: fpga manager
+ * @buf: buffer to receive the name
+ *
+ * Return: number of chars in buf excluding null byte on success;
+ * error code otherwise.
+ */
+int fpga_mgr_name(struct fpga_manager *mgr, char *buf)
+{
+	if (!mgr)
+		return -ENODEV;
+
+	return sprintf(buf, "%s\n", mgr->name);
+}
+EXPORT_SYMBOL_GPL(fpga_mgr_name);
+
+static const char * const state_str[] = {
+	[FPGA_MGR_STATE_UNKNOWN] =		"unknown",
+	[FPGA_MGR_STATE_POWER_OFF] =		"power_off",
+	[FPGA_MGR_STATE_POWER_UP] =		"power_up",
+	[FPGA_MGR_STATE_RESET] =		"reset",
+
+	/* write sequence */
+	[FPGA_MGR_STATE_FIRMWARE_REQ] =		"firmware_request",
+	[FPGA_MGR_STATE_FIRMWARE_REQ_ERR] =	"firmware_request_err",
+	[FPGA_MGR_STATE_WRITE_INIT] =		"write_init",
+	[FPGA_MGR_STATE_WRITE_INIT_ERR] =	"write_init_err",
+	[FPGA_MGR_STATE_WRITE] =		"write",
+	[FPGA_MGR_STATE_WRITE_ERR] =		"write_err",
+	[FPGA_MGR_STATE_WRITE_COMPLETE] =	"write_complete",
+	[FPGA_MGR_STATE_WRITE_COMPLETE_ERR] =	"write_complete_err",
+
+	[FPGA_MGR_STATE_OPERATING] =		"operating",
+};
+
+/*
+ * class attributes
+ */
+static ssize_t name_show(struct device *dev,
+			 struct device_attribute *attr, char *buf)
+{
+	struct fpga_manager *mgr = to_fpga_manager(dev);
+
+	return fpga_mgr_name(mgr, buf);
+}
+
+static ssize_t state_show(struct device *dev,
+			  struct device_attribute *attr, char *buf)
+{
+	struct fpga_manager *mgr = to_fpga_manager(dev);
+
+	return sprintf(buf, "%s\n", state_str[mgr->state]);
+}
+
+static ssize_t firmware_show(struct device *dev,
+			     struct device_attribute *attr, char *buf)
+{
+	struct fpga_manager *mgr = to_fpga_manager(dev);
+
+	if (!mgr->image_name)
+		return 0;
+
+	return sprintf(buf, "%s\n", mgr->image_name);
+}
+
+static ssize_t firmware_store(struct device *dev,
+			      struct device_attribute *attr,
+			      const char *buf, size_t count)
+{
+	struct fpga_manager *mgr = to_fpga_manager(dev);
+	unsigned int len;
+	char image_name[NAME_MAX];
+	int ret;
+
+	/* lose terminating \n */
+	strcpy(image_name, buf);
+	len = strlen(image_name);
+	if (image_name[len - 1] == '\n')
+		image_name[len - 1] = 0;
+
+	ret = fpga_mgr_firmware_write(mgr, image_name);
+	if (ret)
+		return ret;
+
+	return count;
+}
+
+static ssize_t reset_store(struct device *dev,
+			   struct device_attribute *attr,
+			   const char *buf, size_t count)
+{
+	struct fpga_manager *mgr = to_fpga_manager(dev);
+	unsigned long val;
+	int ret;
+
+	ret = kstrtoul(buf, 0, &val);
+	if (ret)
+		return ret;
+
+	if (val == 1) {
+		ret = fpga_mgr_reset(mgr);
+		if (ret)
+			return ret;
+	} else {
+		return -EINVAL;
+	}
+
+	return count;
+}
+
+static DEVICE_ATTR_RO(name);
+static DEVICE_ATTR_RO(state);
+static DEVICE_ATTR_RW(firmware);
+static DEVICE_ATTR_WO(reset);
+
+static struct attribute *fpga_mgr_attrs[] = {
+	&dev_attr_name.attr,
+	&dev_attr_state.attr,
+	&dev_attr_firmware.attr,
+	&dev_attr_reset.attr,
+	NULL,
+};
+ATTRIBUTE_GROUPS(fpga_mgr);
+
+static int fpga_mgr_suspend(struct device *dev)
+{
+	struct fpga_manager *mgr = to_fpga_manager(dev);
+
+	if (!mgr)
+		return -ENODEV;
+
+	if (mgr->mops->suspend)
+		return mgr->mops->suspend(mgr);
+
+	return 0;
+}
+
+static int fpga_mgr_resume(struct device *dev)
+{
+	struct fpga_manager *mgr = to_fpga_manager(dev);
+	int ret = 0;
+
+	if (!mgr)
+		return -ENODEV;
+
+	if (mgr->mops->resume) {
+		ret = mgr->mops->resume(mgr);
+		if (ret)
+			return ret;
+	}
+
+	if (strlen(mgr->image_name) != 0)
+		fpga_mgr_firmware_write(mgr, mgr->image_name);
+
+	return 0;
+}
+
+static const struct dev_pm_ops fpga_mgr_dev_pm_ops = {
+	.suspend	= fpga_mgr_suspend,
+	.resume		= fpga_mgr_resume,
+};
+
+static void fpga_mgr_dev_release(struct device *dev)
+{
+	struct fpga_manager *mgr = to_fpga_manager(dev);
+
+	dev_dbg(dev, "releasing '%s'\n", mgr->name);
+
+	if (mgr->mops->fpga_remove)
+		mgr->mops->fpga_remove(mgr);
+
+	mgr->mops = NULL;
+
+	mutex_lock(&fpga_mgr_mutex);
+	list_del(&mgr->list);
+	mutex_unlock(&fpga_mgr_mutex);
+
+	ida_simple_remove(&fpga_mgr_ida, mgr->dev.id);
+	kfree(mgr->image_name);
+	kfree(mgr);
+}
+
+/**
+ * fpga_mgr_register - register a low level fpga manager driver
+ * @dev: fpga manager device
+ * @name: fpga manager name
+ * @mops: pointer to structure of fpga manager ops
+ * @priv: fpga manager private data
+ *
+ * Return: 0 on success, error code otherwise.
+ */
+int fpga_mgr_register(struct device *dev, const char *name,
+		      const struct fpga_manager_ops *mops,
+		      void *priv)
+{
+	struct fpga_manager *mgr;
+	int id, ret;
+
+	if (!mops || !name || !strlen(name))
+		return -EINVAL;
+
+	mgr = kzalloc(sizeof(*mgr), GFP_KERNEL);
+	if (!mgr)
+		return -ENOMEM;
+
+	id = ida_simple_get(&fpga_mgr_ida, 0, 0, GFP_KERNEL);
+	if (id < 0)
+		return id;
+
+	mutex_init(&mgr->lock);
+
+	mgr->name = name;
+	mgr->mops = mops;
+	mgr->priv = priv;
+
+	/*
+	 * Initialize framework state by requesting low level driver read state
+	 * from device.  FPGA may be in reset mode or may have been programmed
+	 * by bootloader or EEPROM.
+	 */
+	mgr->state = fpga_mgr_low_level_state(mgr);
+
+	INIT_LIST_HEAD(&mgr->list);
+	mutex_lock(&fpga_mgr_mutex);
+	list_add(&mgr->list, &fpga_manager_list);
+	mutex_unlock(&fpga_mgr_mutex);
+
+	device_initialize(&mgr->dev);
+	mgr->dev.class = fpga_mgr_class;
+	mgr->dev.parent = dev;
+	mgr->dev.of_node = dev->of_node;
+	mgr->dev.release = fpga_mgr_dev_release;
+	mgr->dev.id = id;
+	dev_set_name(&mgr->dev, "%d", id);
+	ret = device_add(&mgr->dev);
+	if (ret)
+		goto error_device;
+
+	dev_info(&mgr->dev, "%s registered\n", mgr->name);
+
+	return 0;
+
+error_device:
+	ida_simple_remove(&fpga_mgr_ida, id);
+	kfree(mgr);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(fpga_mgr_register);
+
+/**
+ * fpga_mgr_remove - remove a low level fpga manager driver
+ * @pdev: fpga manager device
+ */
+void fpga_mgr_remove(struct platform_device *pdev)
+{
+	struct list_head *tmp;
+	struct fpga_manager *mgr = NULL;
+
+	list_for_each(tmp, &fpga_manager_list) {
+		mgr = list_entry(tmp, struct fpga_manager, list);
+		if (mgr->dev.parent == &pdev->dev) {
+			device_unregister(&mgr->dev);
+			break;
+		}
+	}
+}
+EXPORT_SYMBOL_GPL(fpga_mgr_remove);
+
+static int __init fpga_mgr_dev_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);
+
+	if (IS_ENABLED(CONFIG_FPGA_MGR_SYSFS))
+		fpga_mgr_class->dev_groups = fpga_mgr_groups;
+
+	fpga_mgr_class->pm = &fpga_mgr_dev_pm_ops;
+
+	return 0;
+}
+
+static void __exit fpga_mgr_dev_exit(void)
+{
+	class_destroy(fpga_mgr_class);
+	ida_destroy(&fpga_mgr_ida);
+}
+
+MODULE_AUTHOR("Alan Tull <atull@opensource.altera.com>");
+MODULE_DESCRIPTION("FPGA Manager framework driver");
+MODULE_LICENSE("GPL v2");
+
+subsys_initcall(fpga_mgr_dev_init);
+module_exit(fpga_mgr_dev_exit);
diff --git a/include/linux/fpga/fpga-mgr.h b/include/linux/fpga/fpga-mgr.h
new file mode 100644
index 0000000..dcf2c68
--- /dev/null
+++ b/include/linux/fpga/fpga-mgr.h
@@ -0,0 +1,124 @@
+/*
+ * FPGA Framework
+ *
+ *  Copyright (C) 2013-2014 Altera Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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/interrupt.h>
+#include <linux/limits.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+#ifndef _LINUX_FPGA_MGR_H
+#define _LINUX_FPGA_MGR_H
+
+struct fpga_manager;
+
+/**
+ * struct fpga_manager_ops - ops for low level fpga manager drivers
+ * @state: returns an enum value of the FPGA's state
+ * @reset: put FPGA into reset state
+ * @write_init: prepare the FPGA to receive confuration data
+ * @write: write count bytes of configuration data to the FPGA
+ * @write_complete: set FPGA to operating state after writing is done
+ * @fpga_remove: optional: Set FPGA into a specific state during driver remove
+ * @suspend: optional: low level fpga suspend
+ * @resume: optional: low level fpga resume
+ *
+ * fpga_manager_ops are the low level functions implemented by a specific
+ * fpga manager driver.  The optional ones are tested for NULL before being
+ * called, so leaving them out is fine.
+ */
+struct fpga_manager_ops {
+	enum fpga_mgr_states (*state)(struct fpga_manager *mgr);
+	int (*reset)(struct fpga_manager *mgr);
+	int (*write_init)(struct fpga_manager *mgr);
+	int (*write)(struct fpga_manager *mgr, const char *buf, size_t count);
+	int (*write_complete)(struct fpga_manager *mgr);
+	void (*fpga_remove)(struct fpga_manager *mgr);
+	int (*suspend)(struct fpga_manager *mgr);
+	int (*resume)(struct fpga_manager *mgr);
+};
+
+/**
+ * enum fpga_mgr_states - fpga framework states
+ * @FPGA_MGR_STATE_UNKNOWN: can't determine state
+ * @FPGA_MGR_STATE_POWER_OFF: FPGA power is off
+ * @FPGA_MGR_STATE_POWER_UP: FPGA reports power is up
+ * @FPGA_MGR_STATE_RESET: FPGA in reset state
+ * @FPGA_MGR_STATE_FIRMWARE_REQ: firmware request in progress
+ * @FPGA_MGR_STATE_FIRMWARE_REQ_ERR: firmware request failed
+ * @FPGA_MGR_STATE_WRITE_INIT: preparing FPGA for programming
+ * @FPGA_MGR_STATE_WRITE_INIT_ERR: Error during WRITE_INIT stage
+ * @FPGA_MGR_STATE_WRITE: writing image to FPGA
+ * @FPGA_MGR_STATE_WRITE_ERR: Error while writing FPGA
+ * @FPGA_MGR_STATE_WRITE_COMPLETE: Doing post programming steps
+ * @FPGA_MGR_STATE_WRITE_COMPLETE_ERR: Error during WRITE_COMPLETE
+ * @FPGA_MGR_STATE_OPERATING: FPGA is programmed and operating
+ */
+enum fpga_mgr_states {
+	FPGA_MGR_STATE_UNKNOWN,
+	FPGA_MGR_STATE_POWER_OFF,
+	FPGA_MGR_STATE_POWER_UP,
+	FPGA_MGR_STATE_RESET,
+
+	/* write sequence */
+	FPGA_MGR_STATE_FIRMWARE_REQ,
+	FPGA_MGR_STATE_FIRMWARE_REQ_ERR,
+	FPGA_MGR_STATE_WRITE_INIT,
+	FPGA_MGR_STATE_WRITE_INIT_ERR,
+	FPGA_MGR_STATE_WRITE,
+	FPGA_MGR_STATE_WRITE_ERR,
+	FPGA_MGR_STATE_WRITE_COMPLETE,
+	FPGA_MGR_STATE_WRITE_COMPLETE_ERR,
+
+	FPGA_MGR_STATE_OPERATING,
+};
+
+/**
+ * struct fpga_manager - fpga manager structure
+ * @name: name of low level fpga manager
+ * @dev: fpga manager device
+ * @list: entry in list of all fpga managers
+ * @lock: lock on calls to fpga manager ops
+ * @state: state of fpga manager
+ * @image_name: name of fpga image file if any
+ * @mops: pointer to struct of fpga manager ops
+ * @priv: low level driver private date
+ */
+struct fpga_manager {
+	const char *name;
+	struct device dev;
+	struct list_head list;
+	struct mutex lock;	/* lock on calls to ops */
+	enum fpga_mgr_states state;
+	char *image_name;
+
+	const struct fpga_manager_ops *mops;
+	void *priv;
+};
+
+#define to_fpga_manager(d) container_of(d, struct fpga_manager, dev)
+
+int fpga_mgr_firmware_write(struct fpga_manager *mgr, const char *image_name);
+int fpga_mgr_write(struct fpga_manager *mgr, const char *buf, size_t count);
+int fpga_mgr_name(struct fpga_manager *mgr, char *buf);
+int fpga_mgr_reset(struct fpga_manager *mgr);
+int fpga_mgr_register(struct device *pdev, const char *name,
+		      const struct fpga_manager_ops *mops, void *priv);
+void fpga_mgr_remove(struct platform_device *pdev);
+
+#endif /*_LINUX_FPGA_MGR_H */
-- 
1.7.9.5


^ permalink raw reply	[flat|nested] 64+ messages in thread

* [PATCH v8 4/4] staging: fpga manager: add driver for socfpga fpga manager
  2015-01-06 20:13 [PATCH v8 0/4] FPGA Manager Framework atull
                   ` (2 preceding siblings ...)
  2015-01-06 20:13 ` [PATCH v8 3/4] staging: fpga manager: framework core atull
@ 2015-01-06 20:13 ` atull
  2015-01-10 18:11 ` [PATCH v8 0/4] FPGA Manager Framework Konrad Zapalowicz
  4 siblings, 0 replies; 64+ messages in thread
From: atull @ 2015-01-06 20:13 UTC (permalink / raw)
  To: gregkh, jgunthorpe, hpa, monstr, michal.simek, rdunlap
  Cc: linux-kernel, devicetree, pantelis.antoniou, robh+dt,
	grant.likely, iws, linux-doc, pavel, broonie, philip, rubini,
	s.trumtrar, jason, kyle.teske, nico, balbi, m.chehab, davidb,
	rob, davem, cesarb, sameo, akpm, linus.walleij, pawel.moll,
	mark.rutland, ijc+devicetree, galak, devel, delicious.quinoa,
	dinguyen, yvanderv, Alan Tull

From: Alan Tull <atull@opensource.altera.com>

Add driver to fpga manager framework to allow configuration
of FPGA in Altera SoCFPGA parts.

Signed-off-by: Alan Tull <atull@opensource.altera.com>
Acked-by: Michal Simek <michal.simek@xilinx.com>
---
v2: fpga_manager struct now contains struct device
    fpga_manager_register parameters now take device

v3: skip a version to align versions

v4: move to drivers/staging

v5: fix array_size.cocci warnings
    fix platform_no_drv_owner.cocci warnings
    Remove .owner = THIS_MODULE
    include asm/irq.h
    clean up list of includes
    use altera_fpga_reset for ops
    use enum fpga_mgr_states or u32 as needed
    use devm_request_irq
    check irq <= 0 instead of == NO_IRQ
    Use ARRAY_SIZE
    rename altera -> socfpga
    static const socfpga_fpga_ops
    header moved to linux/fpga/ folder
    remove ifdef'ed code
    use platform_get_resource and platform_get_irq
    move .probe and .remove lines adjacent
    use module_platform_driver
    use __maybe_unused
    only need to 'if (IS_ENABLED(CONFIG_REGULATOR))' in one fn
    fix "unsigned 'mode' is never < 0"

v6: return error for (unused) default of case statement

v7: PTR_ERR should access the value just tested by IS_ERR

v8: change compatible string to be more chip specific
---
 drivers/staging/fpga/Kconfig   |    5 +
 drivers/staging/fpga/Makefile  |    1 +
 drivers/staging/fpga/socfpga.c |  694 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 700 insertions(+)
 create mode 100644 drivers/staging/fpga/socfpga.c

diff --git a/drivers/staging/fpga/Kconfig b/drivers/staging/fpga/Kconfig
index 89ebafc..ce17342 100644
--- a/drivers/staging/fpga/Kconfig
+++ b/drivers/staging/fpga/Kconfig
@@ -19,6 +19,11 @@ config FPGA_MGR_SYSFS
 	help
 	  FPGA Manager SysFS interface.
 
+config FPGA_MGR_SOCFPGA
+	tristate "Altera SOCFPGA"
+	help
+	  FPGA manager driver support for Altera SOCFPGA.
+
 endif # FPGA
 
 endmenu
diff --git a/drivers/staging/fpga/Makefile b/drivers/staging/fpga/Makefile
index 3313c52..ba6c5c5 100644
--- a/drivers/staging/fpga/Makefile
+++ b/drivers/staging/fpga/Makefile
@@ -6,3 +6,4 @@
 obj-$(CONFIG_FPGA)			+= fpga-mgr.o
 
 # FPGA Manager Drivers
+obj-$(CONFIG_FPGA_MGR_SOCFPGA)		+= socfpga.o
diff --git a/drivers/staging/fpga/socfpga.c b/drivers/staging/fpga/socfpga.c
new file mode 100644
index 0000000..56f75e0
--- /dev/null
+++ b/drivers/staging/fpga/socfpga.c
@@ -0,0 +1,694 @@
+/*
+ * FPGA Manager Driver for Altera SOCFPGA
+ *
+ *  Copyright (C) 2013-2014 Altera Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope 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/completion.h>
+#include <linux/delay.h>
+#include <linux/fpga/fpga-mgr.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/pm.h>
+#include <linux/string.h>
+#include <linux/regulator/consumer.h>
+
+/* Register offsets */
+#define SOCFPGA_FPGMGR_STAT_OFST				0x0
+#define SOCFPGA_FPGMGR_CTL_OFST					0x4
+#define SOCFPGA_FPGMGR_DCLKCNT_OFST				0x8
+#define SOCFPGA_FPGMGR_DCLKSTAT_OFST				0xc
+#define SOCFPGA_FPGMGR_GPIO_INTEN_OFST				0x830
+#define SOCFPGA_FPGMGR_GPIO_INTMSK_OFST				0x834
+#define SOCFPGA_FPGMGR_GPIO_INTTYPE_LEVEL_OFST			0x838
+#define SOCFPGA_FPGMGR_GPIO_INT_POL_OFST			0x83c
+#define SOCFPGA_FPGMGR_GPIO_INTSTAT_OFST			0x840
+#define SOCFPGA_FPGMGR_GPIO_RAW_INTSTAT_OFST			0x844
+#define SOCFPGA_FPGMGR_GPIO_PORTA_EOI_OFST			0x84c
+#define SOCFPGA_FPGMGR_GPIO_EXT_PORTA_OFST			0x850
+
+/* Register bit defines */
+/* SOCFPGA_FPGMGR_STAT register mode field values */
+#define SOCFPGA_FPGMGR_STAT_POWER_UP				0x0 /*ramping*/
+#define SOCFPGA_FPGMGR_STAT_RESET				0x1
+#define SOCFPGA_FPGMGR_STAT_CFG					0x2
+#define SOCFPGA_FPGMGR_STAT_INIT				0x3
+#define SOCFPGA_FPGMGR_STAT_USER_MODE				0x4
+#define SOCFPGA_FPGMGR_STAT_UNKNOWN				0x5
+#define SOCFPGA_FPGMGR_STAT_STATE_MASK				0x7
+/* This is a flag value that doesn't really happen in this register field */
+#define SOCFPGA_FPGMGR_STAT_POWER_OFF				0x0
+
+#define MSEL_PP16_FAST_NOAES_NODC				0x0
+#define MSEL_PP16_FAST_AES_NODC					0x1
+#define MSEL_PP16_FAST_AESOPT_DC				0x2
+#define MSEL_PP16_SLOW_NOAES_NODC				0x4
+#define MSEL_PP16_SLOW_AES_NODC					0x5
+#define MSEL_PP16_SLOW_AESOPT_DC				0x6
+#define MSEL_PP32_FAST_NOAES_NODC				0x8
+#define MSEL_PP32_FAST_AES_NODC					0x9
+#define MSEL_PP32_FAST_AESOPT_DC				0xa
+#define MSEL_PP32_SLOW_NOAES_NODC				0xc
+#define MSEL_PP32_SLOW_AES_NODC					0xd
+#define MSEL_PP32_SLOW_AESOPT_DC				0xe
+#define SOCFPGA_FPGMGR_STAT_MSEL_MASK				0x000000f8
+#define SOCFPGA_FPGMGR_STAT_MSEL_SHIFT				3
+
+/* SOCFPGA_FPGMGR_CTL register */
+#define SOCFPGA_FPGMGR_CTL_EN					0x00000001
+#define SOCFPGA_FPGMGR_CTL_NCE					0x00000002
+#define SOCFPGA_FPGMGR_CTL_NCFGPULL				0x00000004
+
+#define CDRATIO_X1						0x00000000
+#define CDRATIO_X2						0x00000040
+#define CDRATIO_X4						0x00000080
+#define CDRATIO_X8						0x000000c0
+#define SOCFPGA_FPGMGR_CTL_CDRATIO_MASK				0x000000c0
+
+#define SOCFPGA_FPGMGR_CTL_AXICFGEN				0x00000100
+
+#define CFGWDTH_16						0x00000000
+#define CFGWDTH_32						0x00000200
+#define SOCFPGA_FPGMGR_CTL_CFGWDTH_MASK				0x00000200
+
+/* SOCFPGA_FPGMGR_DCLKSTAT register */
+#define SOCFPGA_FPGMGR_DCLKSTAT_DCNTDONE_E_DONE			0x1
+
+/* SOCFPGA_FPGMGR_GPIO_* registers share the same bit positions */
+#define SOCFPGA_FPGMGR_MON_NSTATUS				0x0001
+#define SOCFPGA_FPGMGR_MON_CONF_DONE				0x0002
+#define SOCFPGA_FPGMGR_MON_INIT_DONE				0x0004
+#define SOCFPGA_FPGMGR_MON_CRC_ERROR				0x0008
+#define SOCFPGA_FPGMGR_MON_CVP_CONF_DONE			0x0010
+#define SOCFPGA_FPGMGR_MON_PR_READY				0x0020
+#define SOCFPGA_FPGMGR_MON_PR_ERROR				0x0040
+#define SOCFPGA_FPGMGR_MON_PR_DONE				0x0080
+#define SOCFPGA_FPGMGR_MON_NCONFIG_PIN				0x0100
+#define SOCFPGA_FPGMGR_MON_NSTATUS_PIN				0x0200
+#define SOCFPGA_FPGMGR_MON_CONF_DONE_PIN			0x0400
+#define SOCFPGA_FPGMGR_MON_FPGA_POWER_ON			0x0800
+#define SOCFPGA_FPGMGR_MON_STATUS_MASK				0x0fff
+
+#define SOCFPGA_FPGMGR_NUM_SUPPLIES 3
+#define SOCFPGA_RESUME_TIMEOUT 3
+
+/* In power-up order. Reverse for power-down. */
+static const char *supply_names[SOCFPGA_FPGMGR_NUM_SUPPLIES] __maybe_unused = {
+	"FPGA-1.5V",
+	"FPGA-1.1V",
+	"FPGA-2.5V",
+};
+
+struct socfpga_fpga_priv {
+	void __iomem *fpga_base_addr;
+	void __iomem *fpga_data_addr;
+	struct completion status_complete;
+	int irq;
+	struct regulator *fpga_supplies[SOCFPGA_FPGMGR_NUM_SUPPLIES];
+};
+
+struct cfgmgr_mode {
+	/* Values to set in the CTRL register */
+	u32 ctrl;
+
+	/* flag that this table entry is a valid mode */
+	bool valid;
+};
+
+/* For SOCFPGA_FPGMGR_STAT_MSEL field */
+static struct cfgmgr_mode cfgmgr_modes[] = {
+	[MSEL_PP16_FAST_NOAES_NODC] = { CFGWDTH_16 | CDRATIO_X1, 1 },
+	[MSEL_PP16_FAST_AES_NODC] =   { CFGWDTH_16 | CDRATIO_X2, 1 },
+	[MSEL_PP16_FAST_AESOPT_DC] =  { CFGWDTH_16 | CDRATIO_X4, 1 },
+	[MSEL_PP16_SLOW_NOAES_NODC] = { CFGWDTH_16 | CDRATIO_X1, 1 },
+	[MSEL_PP16_SLOW_AES_NODC] =   { CFGWDTH_16 | CDRATIO_X2, 1 },
+	[MSEL_PP16_SLOW_AESOPT_DC] =  { CFGWDTH_16 | CDRATIO_X4, 1 },
+	[MSEL_PP32_FAST_NOAES_NODC] = { CFGWDTH_32 | CDRATIO_X1, 1 },
+	[MSEL_PP32_FAST_AES_NODC] =   { CFGWDTH_32 | CDRATIO_X4, 1 },
+	[MSEL_PP32_FAST_AESOPT_DC] =  { CFGWDTH_32 | CDRATIO_X8, 1 },
+	[MSEL_PP32_SLOW_NOAES_NODC] = { CFGWDTH_32 | CDRATIO_X1, 1 },
+	[MSEL_PP32_SLOW_AES_NODC] =   { CFGWDTH_32 | CDRATIO_X4, 1 },
+	[MSEL_PP32_SLOW_AESOPT_DC] =  { CFGWDTH_32 | CDRATIO_X8, 1 },
+};
+
+static u32 socfpga_fpga_readl(struct socfpga_fpga_priv *priv, u32 reg_offset)
+{
+	return readl(priv->fpga_base_addr + reg_offset);
+}
+
+static void socfpga_fpga_writel(struct socfpga_fpga_priv *priv, u32 reg_offset,
+				u32 value)
+{
+	writel(value, priv->fpga_base_addr + reg_offset);
+}
+
+static u32 socfpga_fpga_raw_readl(struct socfpga_fpga_priv *priv,
+				  u32 reg_offset)
+{
+	return __raw_readl(priv->fpga_base_addr + reg_offset);
+}
+
+static void socfpga_fpga_raw_writel(struct socfpga_fpga_priv *priv,
+				    u32 reg_offset, u32 value)
+{
+	__raw_writel(value, priv->fpga_base_addr + reg_offset);
+}
+
+static void socfpga_fpga_data_writel(struct socfpga_fpga_priv *priv, u32 value)
+{
+	writel(value, priv->fpga_data_addr);
+}
+
+static inline void socfpga_fpga_set_bitsl(struct socfpga_fpga_priv *priv,
+					  u32 offset, u32 bits)
+{
+	u32 val;
+
+	val = socfpga_fpga_readl(priv, offset);
+	val |= bits;
+	socfpga_fpga_writel(priv, offset, val);
+}
+
+static inline void socfpga_fpga_clr_bitsl(struct socfpga_fpga_priv *priv,
+					  u32 offset, u32 bits)
+{
+	u32 val;
+
+	val = socfpga_fpga_readl(priv, offset);
+	val &= ~bits;
+	socfpga_fpga_writel(priv, offset, val);
+}
+
+static u32 socfpga_fpga_mon_status_get(struct socfpga_fpga_priv *priv)
+{
+	return socfpga_fpga_readl(priv, SOCFPGA_FPGMGR_GPIO_EXT_PORTA_OFST) &
+		SOCFPGA_FPGMGR_MON_STATUS_MASK;
+}
+
+static u32 socfpga_fpga_state_get(struct socfpga_fpga_priv *priv)
+{
+	u32 status = socfpga_fpga_mon_status_get(priv);
+
+	if ((status & SOCFPGA_FPGMGR_MON_FPGA_POWER_ON) == 0)
+		return SOCFPGA_FPGMGR_STAT_POWER_OFF;
+
+	return socfpga_fpga_readl(priv, SOCFPGA_FPGMGR_STAT_OFST) &
+		SOCFPGA_FPGMGR_STAT_STATE_MASK;
+}
+
+static void socfpga_fpga_clear_done_status(struct socfpga_fpga_priv *priv)
+{
+	socfpga_fpga_writel(priv, SOCFPGA_FPGMGR_DCLKSTAT_OFST,
+			    SOCFPGA_FPGMGR_DCLKSTAT_DCNTDONE_E_DONE);
+}
+
+/*
+ * Set the DCLKCNT, wait for DCLKSTAT to report the count completed, and clear
+ * the complete status.
+ */
+static int socfpga_fpga_dclk_set_and_wait_clear(struct socfpga_fpga_priv *priv,
+						u32 count)
+{
+	int timeout = 2;
+	u32 done;
+
+	/* Clear any existing DONE status. */
+	if (socfpga_fpga_readl(priv, SOCFPGA_FPGMGR_DCLKSTAT_OFST))
+		socfpga_fpga_clear_done_status(priv);
+
+	/* Issue the DCLK count. */
+	socfpga_fpga_writel(priv, SOCFPGA_FPGMGR_DCLKCNT_OFST, count);
+
+	/* Poll DCLKSTAT to see if it completed in the timeout period. */
+	do {
+		done = socfpga_fpga_readl(priv, SOCFPGA_FPGMGR_DCLKSTAT_OFST);
+		if (done == SOCFPGA_FPGMGR_DCLKSTAT_DCNTDONE_E_DONE) {
+			socfpga_fpga_clear_done_status(priv);
+			return 0;
+		}
+		udelay(1);
+	} while (timeout--);
+
+	return -ETIMEDOUT;
+}
+
+static int socfpga_fpga_wait_for_state(struct socfpga_fpga_priv *priv,
+				       u32 state)
+{
+	int timeout = 2;
+
+	/*
+	 * HW doesn't support an interrupt for changes in state, so poll to see
+	 * if it matches the requested state within the timeout period.
+	 */
+	do {
+		if ((socfpga_fpga_state_get(priv) & state) != 0)
+			return 0;
+		msleep(20);
+	} while (timeout--);
+
+	return -ETIMEDOUT;
+}
+
+static void socfpga_fpga_enable_irqs(struct socfpga_fpga_priv *priv, u32 irqs)
+{
+	/* set irqs to level sensitive */
+	socfpga_fpga_writel(priv, SOCFPGA_FPGMGR_GPIO_INTTYPE_LEVEL_OFST, 0);
+
+	/* set interrupt polarity */
+	socfpga_fpga_writel(priv, SOCFPGA_FPGMGR_GPIO_INT_POL_OFST, irqs);
+
+	/* clear irqs */
+	socfpga_fpga_writel(priv, SOCFPGA_FPGMGR_GPIO_PORTA_EOI_OFST, irqs);
+
+	/* unmask interrupts */
+	socfpga_fpga_writel(priv, SOCFPGA_FPGMGR_GPIO_INTMSK_OFST, 0);
+
+	/* enable interrupts */
+	socfpga_fpga_writel(priv, SOCFPGA_FPGMGR_GPIO_INTEN_OFST, irqs);
+}
+
+static void socfpga_fpga_disable_irqs(struct socfpga_fpga_priv *priv)
+{
+	socfpga_fpga_writel(priv, SOCFPGA_FPGMGR_GPIO_INTEN_OFST, 0);
+}
+
+static irqreturn_t socfpga_fpga_isr(int irq, void *dev_id)
+{
+	struct socfpga_fpga_priv *priv = dev_id;
+	u32 irqs, st;
+	bool conf_done, nstatus;
+
+	/* clear irqs */
+	irqs = socfpga_fpga_raw_readl(priv, SOCFPGA_FPGMGR_GPIO_INTSTAT_OFST);
+
+	socfpga_fpga_raw_writel(priv, SOCFPGA_FPGMGR_GPIO_PORTA_EOI_OFST, irqs);
+
+	st = socfpga_fpga_raw_readl(priv, SOCFPGA_FPGMGR_GPIO_EXT_PORTA_OFST);
+	conf_done = (st & SOCFPGA_FPGMGR_MON_CONF_DONE) != 0;
+	nstatus = (st & SOCFPGA_FPGMGR_MON_NSTATUS) != 0;
+
+	/* success */
+	if (conf_done && nstatus) {
+		/* disable irqs */
+		socfpga_fpga_raw_writel(priv,
+					SOCFPGA_FPGMGR_GPIO_INTEN_OFST, 0);
+		complete(&priv->status_complete);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int socfpga_fpga_wait_for_config_done(struct socfpga_fpga_priv *priv)
+{
+	int timeout, ret = 0;
+
+	socfpga_fpga_disable_irqs(priv);
+	init_completion(&priv->status_complete);
+	socfpga_fpga_enable_irqs(priv, SOCFPGA_FPGMGR_MON_CONF_DONE);
+
+	timeout = wait_for_completion_interruptible_timeout(
+						&priv->status_complete,
+						msecs_to_jiffies(10));
+	if (timeout == 0)
+		ret = -ETIMEDOUT;
+
+	socfpga_fpga_disable_irqs(priv);
+	return ret;
+}
+
+static int socfpga_fpga_cfg_mode_get(struct socfpga_fpga_priv *priv)
+{
+	u32 msel;
+
+	msel = socfpga_fpga_readl(priv, SOCFPGA_FPGMGR_STAT_OFST);
+	msel &= SOCFPGA_FPGMGR_STAT_MSEL_MASK;
+	msel >>= SOCFPGA_FPGMGR_STAT_MSEL_SHIFT;
+
+	/* Check that this MSEL setting is supported */
+	if ((msel >= ARRAY_SIZE(cfgmgr_modes)) || !cfgmgr_modes[msel].valid)
+		return -EINVAL;
+
+	return msel;
+}
+
+static int socfpga_fpga_cfg_mode_set(struct socfpga_fpga_priv *priv)
+{
+	u32 ctrl_reg;
+	int mode;
+
+	/* get value from MSEL pins */
+	mode = socfpga_fpga_cfg_mode_get(priv);
+	if (mode < 0)
+		return mode;
+
+	/* Adjust CTRL for the CDRATIO */
+	ctrl_reg = socfpga_fpga_readl(priv, SOCFPGA_FPGMGR_CTL_OFST);
+	ctrl_reg &= ~SOCFPGA_FPGMGR_CTL_CDRATIO_MASK;
+	ctrl_reg &= ~SOCFPGA_FPGMGR_CTL_CFGWDTH_MASK;
+	ctrl_reg |= cfgmgr_modes[mode].ctrl;
+
+	/* Set NCE to 0. */
+	ctrl_reg &= ~SOCFPGA_FPGMGR_CTL_NCE;
+	socfpga_fpga_writel(priv, SOCFPGA_FPGMGR_CTL_OFST, ctrl_reg);
+
+	return 0;
+}
+
+static int socfpga_fpga_reset(struct fpga_manager *mgr)
+{
+	struct socfpga_fpga_priv *priv = mgr->priv;
+	u32 ctrl_reg, status;
+	int ret;
+
+	/*
+	 * Step 1:
+	 *  - Set CTRL.CFGWDTH, CTRL.CDRATIO to match cfg mode
+	 *  - Set CTRL.NCE to 0
+	 */
+	ret = socfpga_fpga_cfg_mode_set(priv);
+	if (ret)
+		return ret;
+
+	/* Step 2: Set CTRL.EN to 1 */
+	socfpga_fpga_set_bitsl(priv, SOCFPGA_FPGMGR_CTL_OFST,
+			       SOCFPGA_FPGMGR_CTL_EN);
+
+	/* Step 3: Set CTRL.NCONFIGPULL to 1 to put FPGA in reset */
+	ctrl_reg = socfpga_fpga_readl(priv, SOCFPGA_FPGMGR_CTL_OFST);
+	ctrl_reg |= SOCFPGA_FPGMGR_CTL_NCFGPULL;
+	socfpga_fpga_writel(priv, SOCFPGA_FPGMGR_CTL_OFST, ctrl_reg);
+
+	/* Step 4: Wait for STATUS.MODE to report FPGA is in reset phase */
+	status = socfpga_fpga_wait_for_state(priv, SOCFPGA_FPGMGR_STAT_RESET);
+
+	/* Step 5: Set CONTROL.NCONFIGPULL to 0 to release FPGA from reset */
+	ctrl_reg &= ~SOCFPGA_FPGMGR_CTL_NCFGPULL;
+	socfpga_fpga_writel(priv, SOCFPGA_FPGMGR_CTL_OFST, ctrl_reg);
+
+	/* Timeout waiting for reset */
+	if (status)
+		return -ETIMEDOUT;
+
+	return 0;
+}
+
+/*
+ * Prepare the FPGA to receive the configuration data.
+ */
+static int socfpga_fpga_ops_configure_init(struct fpga_manager *mgr)
+{
+	struct socfpga_fpga_priv *priv = mgr->priv;
+	int ret;
+
+	/* Steps 1 - 5: Reset the FPGA */
+	ret = socfpga_fpga_reset(mgr);
+	if (ret)
+		return ret;
+
+	/* Step 6: Wait for FPGA to enter configuration phase */
+	if (socfpga_fpga_wait_for_state(priv, SOCFPGA_FPGMGR_STAT_CFG))
+		return -ETIMEDOUT;
+
+	/* Step 7: Clear nSTATUS interrupt */
+	socfpga_fpga_writel(priv, SOCFPGA_FPGMGR_GPIO_PORTA_EOI_OFST,
+			    SOCFPGA_FPGMGR_MON_NSTATUS);
+
+	/* Step 8: Set CTRL.AXICFGEN to 1 to enable transfer of config data */
+	socfpga_fpga_set_bitsl(priv, SOCFPGA_FPGMGR_CTL_OFST,
+			       SOCFPGA_FPGMGR_CTL_AXICFGEN);
+
+	return 0;
+}
+
+/*
+ * Step 9: write data to the FPGA data register
+ */
+static int socfpga_fpga_ops_configure_write(struct fpga_manager *mgr,
+					    const char *buf, size_t count)
+{
+	struct socfpga_fpga_priv *priv = mgr->priv;
+	u32 *buffer_32 = (u32 *)buf;
+	size_t i = 0;
+
+	if (count <= 0)
+		return -EINVAL;
+
+	/* Write out the complete 32-bit chunks. */
+	while (count >= sizeof(u32)) {
+		socfpga_fpga_data_writel(priv, buffer_32[i++]);
+		count -= sizeof(u32);
+	}
+
+	/* Write out remaining non 32-bit chunks. */
+	switch (count) {
+	case 3:
+		socfpga_fpga_data_writel(priv, buffer_32[i++] & 0x00ffffff);
+		break;
+	case 2:
+		socfpga_fpga_data_writel(priv, buffer_32[i++] & 0x0000ffff);
+		break;
+	case 1:
+		socfpga_fpga_data_writel(priv, buffer_32[i++] & 0x000000ff);
+		break;
+	case 0:
+		break;
+	default:
+		/* This will never happen. */
+		return -EFAULT;
+	}
+
+	return 0;
+}
+
+static int socfpga_fpga_ops_configure_complete(struct fpga_manager *mgr)
+{
+	struct socfpga_fpga_priv *priv = mgr->priv;
+	u32 status;
+
+	/*
+	 * Step 10:
+	 *  - Observe CONF_DONE and nSTATUS (active low)
+	 *  - if CONF_DONE = 1 and nSTATUS = 1, configuration was successful
+	 *  - if CONF_DONE = 0 and nSTATUS = 0, configuration failed
+	 */
+	status = socfpga_fpga_wait_for_config_done(priv);
+	if (status)
+		return status;
+
+	/* Step 11: Clear CTRL.AXICFGEN to disable transfer of config data */
+	socfpga_fpga_clr_bitsl(priv, SOCFPGA_FPGMGR_CTL_OFST,
+			       SOCFPGA_FPGMGR_CTL_AXICFGEN);
+
+	/*
+	 * Step 12:
+	 *  - Write 4 to DCLKCNT
+	 *  - Wait for STATUS.DCNTDONE = 1
+	 *  - Clear W1C bit in STATUS.DCNTDONE
+	 */
+	if (socfpga_fpga_dclk_set_and_wait_clear(priv, 4))
+		return -ETIMEDOUT;
+
+	/* Step 13: Wait for STATUS.MODE to report USER MODE */
+	if (socfpga_fpga_wait_for_state(priv, SOCFPGA_FPGMGR_STAT_USER_MODE))
+		return -ETIMEDOUT;
+
+	/* Step 14: Set CTRL.EN to 0 */
+	socfpga_fpga_clr_bitsl(priv, SOCFPGA_FPGMGR_CTL_OFST,
+			       SOCFPGA_FPGMGR_CTL_EN);
+
+	return 0;
+}
+
+/* Translate state register values to FPGA framework state */
+static const enum fpga_mgr_states socfpga_state_to_framework_state[] = {
+	[SOCFPGA_FPGMGR_STAT_POWER_OFF] = FPGA_MGR_STATE_POWER_OFF,
+	[SOCFPGA_FPGMGR_STAT_RESET] = FPGA_MGR_STATE_RESET,
+	[SOCFPGA_FPGMGR_STAT_CFG] = FPGA_MGR_STATE_WRITE_INIT,
+	[SOCFPGA_FPGMGR_STAT_INIT] = FPGA_MGR_STATE_WRITE_INIT,
+	[SOCFPGA_FPGMGR_STAT_USER_MODE] = FPGA_MGR_STATE_OPERATING,
+	[SOCFPGA_FPGMGR_STAT_UNKNOWN] = FPGA_MGR_STATE_UNKNOWN,
+};
+
+static enum fpga_mgr_states socfpga_fpga_ops_state(struct fpga_manager *mgr)
+{
+	struct socfpga_fpga_priv *priv = mgr->priv;
+	enum fpga_mgr_states ret;
+	u32 state;
+
+	state = socfpga_fpga_state_get(priv);
+
+	if (state < ARRAY_SIZE(socfpga_state_to_framework_state))
+		ret = socfpga_state_to_framework_state[state];
+	else
+		ret = FPGA_MGR_STATE_UNKNOWN;
+
+	return ret;
+}
+
+static int socfpga_fpga_regulators_on(struct socfpga_fpga_priv *priv)
+{
+	int i, ret;
+
+	for (i = 0; i < SOCFPGA_FPGMGR_NUM_SUPPLIES; i++) {
+		ret = regulator_enable(priv->fpga_supplies[i]);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static void socfpga_fpga_regulators_power_off(struct socfpga_fpga_priv *priv)
+{
+	int i;
+
+	for (i = SOCFPGA_FPGMGR_NUM_SUPPLIES - 1; i >= 0; i--)
+		regulator_disable(priv->fpga_supplies[i]);
+}
+
+static int socfpga_fpga_regulator_probe(struct platform_device *pdev,
+					struct socfpga_fpga_priv *priv)
+{
+	struct regulator *supply;
+	unsigned int i;
+
+	if (!IS_ENABLED(CONFIG_REGULATOR))
+		return 0;
+
+	for (i = 0; i < SOCFPGA_FPGMGR_NUM_SUPPLIES; i++) {
+		supply = devm_regulator_get_optional(&pdev->dev,
+						     supply_names[i]);
+		if (IS_ERR(supply)) {
+			dev_err(&pdev->dev, "could not get regulators");
+			return -EPROBE_DEFER;
+		}
+		priv->fpga_supplies[i] = supply;
+	}
+
+	return socfpga_fpga_regulators_on(priv);
+}
+
+static int socfpga_fpga_suspend(struct fpga_manager *mgr)
+{
+	struct socfpga_fpga_priv *priv = mgr->priv;
+
+	socfpga_fpga_regulators_power_off(priv);
+
+	return 0;
+}
+
+static int socfpga_fpga_resume(struct fpga_manager *mgr)
+{
+	struct socfpga_fpga_priv *priv = mgr->priv;
+	u32 state;
+	unsigned int timeout;
+	int ret;
+
+	ret = socfpga_fpga_regulators_on(priv);
+	if (ret)
+		return ret;
+
+	for (timeout = 0; timeout < SOCFPGA_RESUME_TIMEOUT; timeout++) {
+		state = socfpga_fpga_state_get(priv);
+		if (state != SOCFPGA_FPGMGR_STAT_POWER_OFF)
+			break;
+		msleep(20);
+	}
+	if (state == SOCFPGA_FPGMGR_STAT_POWER_OFF)
+		return -ETIMEDOUT;
+
+	return ret;
+}
+
+static const struct fpga_manager_ops socfpga_fpga_ops = {
+	.reset = socfpga_fpga_reset,
+	.state = socfpga_fpga_ops_state,
+	.write_init = socfpga_fpga_ops_configure_init,
+	.write = socfpga_fpga_ops_configure_write,
+	.write_complete = socfpga_fpga_ops_configure_complete,
+	.suspend = socfpga_fpga_suspend,
+	.resume = socfpga_fpga_resume,
+};
+
+static int socfpga_fpga_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct socfpga_fpga_priv *priv;
+	struct resource *res;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	ret = socfpga_fpga_regulator_probe(pdev, priv);
+	if (ret)
+		return ret;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->fpga_base_addr = devm_ioremap_resource(dev, res);
+	if (IS_ERR(priv->fpga_base_addr))
+		return PTR_ERR(priv->fpga_base_addr);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	priv->fpga_data_addr = devm_ioremap_resource(dev, res);
+	if (IS_ERR(priv->fpga_data_addr))
+		return PTR_ERR(priv->fpga_data_addr);
+
+	priv->irq = platform_get_irq(pdev, 0);
+	if (priv->irq < 0)
+		return priv->irq;
+
+	ret = devm_request_irq(dev, priv->irq, socfpga_fpga_isr, 0,
+			       dev_name(dev), priv);
+	if (IS_ERR_VALUE(ret))
+		return ret;
+
+	return fpga_mgr_register(dev, "Altera SOCFPGA FPGA Manager",
+				 &socfpga_fpga_ops, priv);
+}
+
+static int socfpga_fpga_remove(struct platform_device *pdev)
+{
+	fpga_mgr_remove(pdev);
+
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id socfpga_fpga_of_match[] = {
+	{ .compatible = "altr,socfpga-fpga-mgr", },
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, socfpga_fpga_of_match);
+#endif
+
+static struct platform_driver socfpga_fpga_driver = {
+	.probe = socfpga_fpga_probe,
+	.remove = socfpga_fpga_remove,
+	.driver = {
+		.name	= "socfpga_fpga_manager",
+		.of_match_table = of_match_ptr(socfpga_fpga_of_match),
+	},
+};
+
+module_platform_driver(socfpga_fpga_driver);
+
+MODULE_AUTHOR("Alan Tull <atull@opensource.altera.com>");
+MODULE_DESCRIPTION("Altera SOCFPGA FPGA Manager");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5


^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 1/4] doc: add bindings document for altera fpga manager
  2015-01-06 20:13 ` [PATCH v8 1/4] doc: add bindings document for altera fpga manager atull
@ 2015-01-06 22:05   ` Rob Herring
  2015-01-06 22:34     ` atull
  0 siblings, 1 reply; 64+ messages in thread
From: Rob Herring @ 2015-01-06 22:05 UTC (permalink / raw)
  To: Alan Tull
  Cc: Greg Kroah-Hartman, Jason Gunthorpe, H. Peter Anvin,
	Michal Simek, Michal Simek, Randy Dunlap, linux-kernel,
	devicetree, Pantelis Antoniou, Rob Herring, Grant Likely, iws,
	linux-doc, Pavel Machek, Mark Brown, Philip Balister,
	Alessandro Rubini, Steffen Trumtrar, Jason Cooper, Kyle Teske,
	Nicolas Pitre, Felipe Balbi, Mauro Carvalho Chehab, David Brown,
	Rob Landley, David Miller, Cesar Eduardo Barros, Samuel Ortiz,
	Andrew Morton, Linus Walleij, Pawel Moll, Mark Rutland,
	Ian Campbell, Kumar Gala, devel, Alan Tull, Dinh Nguyen,
	yvanderv

On Tue, Jan 6, 2015 at 2:13 PM,  <atull@opensource.altera.com> wrote:
> From: Alan Tull <atull@opensource.altera.com>
>
> New bindings document for Altera fpga manager.
>
> Signed-off-by: Alan Tull <atull@opensource.altera.com>

Acked-by: Rob Herring <robh@kernel.org>

Like I said, this binding doesn't look like it will change other than
perhaps additional optional properties, so it can go into
Documentation. Perhaps .../bindings/fpga/. I'm happy to merge this
thru DT tree if you want.

Rob

> ---
> v5 : Move bindings to drivers/staging/fpga/Documentation/bindings
>
> v6 : No change in this patch for v6 of the patch set
>
> v7 : No change in this patch for v7 of the patch set
>
> v8 : Make compatible string and name of bindings doc more chip
>      specific
> ---
>  .../bindings/altera-socfpga-fpga-mgr.txt           |   17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>  create mode 100644 drivers/staging/fpga/Documentation/bindings/altera-socfpga-fpga-mgr.txt
>
> diff --git a/drivers/staging/fpga/Documentation/bindings/altera-socfpga-fpga-mgr.txt b/drivers/staging/fpga/Documentation/bindings/altera-socfpga-fpga-mgr.txt
> new file mode 100644
> index 0000000..9b027a6
> --- /dev/null
> +++ b/drivers/staging/fpga/Documentation/bindings/altera-socfpga-fpga-mgr.txt
> @@ -0,0 +1,17 @@
> +Altera SOCFPGA FPGA Manager
> +
> +Required properties:
> +- compatible : should contain "altr,socfpga-fpga-mgr"
> +- reg        : base address and size for memory mapped io.
> +               - The first index is for FPGA manager register access.
> +               - The second index is for writing FPGA configuration data.
> +- interrupts : interrupt for the FPGA Manager device.
> +
> +Example:
> +
> +       hps_0_fpgamgr: fpgamgr@0xff706000 {
> +               compatible = "altr,socfpga-fpga-mgr";
> +               reg = <0xFF706000 0x1000
> +                      0xFFB90000 0x1000>;
> +               interrupts = <0 175 4>;
> +       };
> --
> 1.7.9.5
>

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 1/4] doc: add bindings document for altera fpga manager
  2015-01-06 22:05   ` Rob Herring
@ 2015-01-06 22:34     ` atull
  2015-01-09 15:50       ` Rob Herring
  0 siblings, 1 reply; 64+ messages in thread
From: atull @ 2015-01-06 22:34 UTC (permalink / raw)
  To: Rob Herring
  Cc: Greg Kroah-Hartman, Jason Gunthorpe, H. Peter Anvin,
	Michal Simek, Michal Simek, Randy Dunlap, linux-kernel,
	devicetree, Pantelis Antoniou, Rob Herring, Grant Likely, iws,
	linux-doc, Pavel Machek, Mark Brown, Philip Balister,
	Alessandro Rubini, Steffen Trumtrar, Jason Cooper, Kyle Teske,
	Nicolas Pitre, Felipe Balbi, Mauro Carvalho Chehab, David Brown,
	Rob Landley, David Miller, Cesar Eduardo Barros, Samuel Ortiz,
	Andrew Morton, Linus Walleij, Pawel Moll, Mark Rutland,
	Ian Campbell, Kumar Gala, devel, Alan Tull, Dinh Nguyen,
	yvanderv

On Tue, 6 Jan 2015, Rob Herring wrote:

> On Tue, Jan 6, 2015 at 2:13 PM,  <atull@opensource.altera.com> wrote:
> > From: Alan Tull <atull@opensource.altera.com>
> >
> > New bindings document for Altera fpga manager.
> >
> > Signed-off-by: Alan Tull <atull@opensource.altera.com>
> 
> Acked-by: Rob Herring <robh@kernel.org>
> 
> Like I said, this binding doesn't look like it will change other than
> perhaps additional optional properties, so it can go into
> Documentation. Perhaps .../bindings/fpga/. I'm happy to merge this
> thru DT tree if you want.
> 
> Rob

Thanks, that would be great.  Yes ../bindings/fpga sounds good.

Do you want me to send you a patch with the updated path?

Alan

> 
> > ---
> > v5 : Move bindings to drivers/staging/fpga/Documentation/bindings
> >
> > v6 : No change in this patch for v6 of the patch set
> >
> > v7 : No change in this patch for v7 of the patch set
> >
> > v8 : Make compatible string and name of bindings doc more chip
> >      specific
> > ---
> >  .../bindings/altera-socfpga-fpga-mgr.txt           |   17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> >  create mode 100644 drivers/staging/fpga/Documentation/bindings/altera-socfpga-fpga-mgr.txt
> >
> > diff --git a/drivers/staging/fpga/Documentation/bindings/altera-socfpga-fpga-mgr.txt b/drivers/staging/fpga/Documentation/bindings/altera-socfpga-fpga-mgr.txt
> > new file mode 100644
> > index 0000000..9b027a6
> > --- /dev/null
> > +++ b/drivers/staging/fpga/Documentation/bindings/altera-socfpga-fpga-mgr.txt
> > @@ -0,0 +1,17 @@
> > +Altera SOCFPGA FPGA Manager
> > +
> > +Required properties:
> > +- compatible : should contain "altr,socfpga-fpga-mgr"
> > +- reg        : base address and size for memory mapped io.
> > +               - The first index is for FPGA manager register access.
> > +               - The second index is for writing FPGA configuration data.
> > +- interrupts : interrupt for the FPGA Manager device.
> > +
> > +Example:
> > +
> > +       hps_0_fpgamgr: fpgamgr@0xff706000 {
> > +               compatible = "altr,socfpga-fpga-mgr";
> > +               reg = <0xFF706000 0x1000
> > +                      0xFFB90000 0x1000>;
> > +               interrupts = <0 175 4>;
> > +       };
> > --
> > 1.7.9.5
> >
> 

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-06 20:13 ` [PATCH v8 2/4] fpga manager: add sysfs interface document atull
@ 2015-01-07  8:48   ` Pavel Machek
  2015-01-09 19:14     ` atull
  0 siblings, 1 reply; 64+ messages in thread
From: Pavel Machek @ 2015-01-07  8:48 UTC (permalink / raw)
  To: atull
  Cc: gregkh, jgunthorpe, hpa, monstr, michal.simek, rdunlap,
	linux-kernel, devicetree, pantelis.antoniou, robh+dt,
	grant.likely, iws, linux-doc, broonie, philip, rubini,
	s.trumtrar, jason, kyle.teske, nico, balbi, m.chehab, davidb,
	rob, davem, cesarb, sameo, akpm, linus.walleij, pawel.moll,
	mark.rutland, ijc+devicetree, galak, devel, delicious.quinoa,
	dinguyen, yvanderv

On Tue 2015-01-06 14:13:37, atull@opensource.altera.com wrote:
> From: Alan Tull <atull@opensource.altera.com>
> 
> Add documentation under drivers/staging for new fpga manager's
> sysfs interface.
> 
> Signed-off-by: Alan Tull <atull@opensource.altera.com>
> ---
> v5  : (actually second version, but keeping version numbers
>       aligned with rest of patch series)
>       Move document to drivers/staging/fpga/Documentation/ABI
> 
> v6  : No change in this patch for v6 of the patch set
> 
> v7  : No change in this patch for v7 of the patch set
> 
> v8  : No change in this patch for v8 of the patch set
> ---
>  .../Documentation/ABI/sysfs-class-fpga-manager     |   38 ++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>  create mode 100644 drivers/staging/fpga/Documentation/ABI/sysfs-class-fpga-manager
> 
> diff --git a/drivers/staging/fpga/Documentation/ABI/sysfs-class-fpga-manager b/drivers/staging/fpga/Documentation/ABI/sysfs-class-fpga-manager
> new file mode 100644
> index 0000000..eb600f2
> --- /dev/null
> +++ b/drivers/staging/fpga/Documentation/ABI/sysfs-class-fpga-manager
> @@ -0,0 +1,38 @@
> +What:		/sys/class/fpga_manager/<fpga>/name
> +Date:		October 2014
> +KernelVersion:	3.18
> +Contact:	Alan Tull <atull@opensource.altera.com>
> +Description:	Name of low level driver.
> +
> +What:		/sys/class/fpga_manager/<fpga>/firmware
> +Date:		October 2014
> +KernelVersion:	3.18
> +Contact:	Alan Tull <atull@opensource.altera.com>
> +Description:	Name of the FPGA image file to load using firmware
> class.

This one is ugly: it unneccessarily passes firmware name through the
kernel. Just make interface and code simpler by always passing
"socfpga-fpga-image" or something like that.

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

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 1/4] doc: add bindings document for altera fpga manager
  2015-01-06 22:34     ` atull
@ 2015-01-09 15:50       ` Rob Herring
  2015-01-09 18:58         ` atull
  0 siblings, 1 reply; 64+ messages in thread
From: Rob Herring @ 2015-01-09 15:50 UTC (permalink / raw)
  To: atull
  Cc: Greg Kroah-Hartman, Jason Gunthorpe, H. Peter Anvin,
	Michal Simek, Michal Simek, Randy Dunlap, linux-kernel,
	devicetree, Pantelis Antoniou, Rob Herring, Grant Likely, iws,
	linux-doc, Pavel Machek, Mark Brown, Philip Balister,
	Alessandro Rubini, Steffen Trumtrar, Jason Cooper, Kyle Teske,
	Nicolas Pitre, Felipe Balbi, Mauro Carvalho Chehab, David Brown,
	Rob Landley, David Miller, Cesar Eduardo Barros, Samuel Ortiz,
	Andrew Morton, Linus Walleij, Pawel Moll, Mark Rutland,
	Ian Campbell, Kumar Gala, devel, Alan Tull, Dinh Nguyen,
	yvanderv

On Tue, Jan 6, 2015 at 4:34 PM, atull <atull@opensource.altera.com> wrote:
> On Tue, 6 Jan 2015, Rob Herring wrote:
>
>> On Tue, Jan 6, 2015 at 2:13 PM,  <atull@opensource.altera.com> wrote:
>> > From: Alan Tull <atull@opensource.altera.com>
>> >
>> > New bindings document for Altera fpga manager.
>> >
>> > Signed-off-by: Alan Tull <atull@opensource.altera.com>
>>
>> Acked-by: Rob Herring <robh@kernel.org>
>>
>> Like I said, this binding doesn't look like it will change other than
>> perhaps additional optional properties, so it can go into
>> Documentation. Perhaps .../bindings/fpga/. I'm happy to merge this
>> thru DT tree if you want.
>>
>> Rob
>
> Thanks, that would be great.  Yes ../bindings/fpga sounds good.
>
> Do you want me to send you a patch with the updated path?

Yes, please.

Rob

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 1/4] doc: add bindings document for altera fpga manager
  2015-01-09 15:50       ` Rob Herring
@ 2015-01-09 18:58         ` atull
  0 siblings, 0 replies; 64+ messages in thread
From: atull @ 2015-01-09 18:58 UTC (permalink / raw)
  To: Rob Herring
  Cc: Greg Kroah-Hartman, Jason Gunthorpe, H. Peter Anvin,
	Michal Simek, Michal Simek, Randy Dunlap, linux-kernel,
	devicetree, Pantelis Antoniou, Rob Herring, Grant Likely, iws,
	linux-doc, Pavel Machek, Mark Brown, Philip Balister,
	Alessandro Rubini, Steffen Trumtrar, Jason Cooper, Kyle Teske,
	Nicolas Pitre, Felipe Balbi, Mauro Carvalho Chehab, David Brown,
	Rob Landley, David Miller, Cesar Eduardo Barros, Samuel Ortiz,
	Andrew Morton, Linus Walleij, Pawel Moll, Mark Rutland,
	Ian Campbell, Kumar Gala, devel, Alan Tull, Dinh Nguyen,
	yvanderv

On Fri, 9 Jan 2015, Rob Herring wrote:

> On Tue, Jan 6, 2015 at 4:34 PM, atull <atull@opensource.altera.com> wrote:
> > On Tue, 6 Jan 2015, Rob Herring wrote:
> >
> >> On Tue, Jan 6, 2015 at 2:13 PM,  <atull@opensource.altera.com> wrote:
> >> > From: Alan Tull <atull@opensource.altera.com>
> >> >
> >> > New bindings document for Altera fpga manager.
> >> >
> >> > Signed-off-by: Alan Tull <atull@opensource.altera.com>
> >>
> >> Acked-by: Rob Herring <robh@kernel.org>
> >>
> >> Like I said, this binding doesn't look like it will change other than
> >> perhaps additional optional properties, so it can go into
> >> Documentation. Perhaps .../bindings/fpga/. I'm happy to merge this
> >> thru DT tree if you want.
> >>
> >> Rob
> >
> > Thanks, that would be great.  Yes ../bindings/fpga sounds good.
> >
> > Do you want me to send you a patch with the updated path?
> 
> Yes, please.
> 
> Rob
> 

OK I'll split that patch out and just send it separately to you and the DT 
list.

Alan

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-07  8:48   ` Pavel Machek
@ 2015-01-09 19:14     ` atull
  2015-01-09 20:56       ` Pavel Machek
  0 siblings, 1 reply; 64+ messages in thread
From: atull @ 2015-01-09 19:14 UTC (permalink / raw)
  To: Pavel Machek
  Cc: gregkh, jgunthorpe, hpa, monstr, michal.simek, rdunlap,
	linux-kernel, devicetree, pantelis.antoniou, robh+dt,
	grant.likely, iws, linux-doc, broonie, philip, rubini,
	s.trumtrar, jason, kyle.teske, nico, balbi, m.chehab, davidb,
	rob, davem, cesarb, sameo, akpm, linus.walleij, pawel.moll,
	mark.rutland, ijc+devicetree, galak, devel, delicious.quinoa,
	dinguyen, yvanderv

On Wed, 7 Jan 2015, Pavel Machek wrote:

> On Tue 2015-01-06 14:13:37, atull@opensource.altera.com wrote:
> > +
> > +What:		/sys/class/fpga_manager/<fpga>/firmware
> > +Date:		October 2014
> > +KernelVersion:	3.18
> > +Contact:	Alan Tull <atull@opensource.altera.com>
> > +Description:	Name of the FPGA image file to load using firmware
> > class.
> 
> This one is ugly: it unneccessarily passes firmware name through the
> kernel. Just make interface and code simpler by always passing
> "socfpga-fpga-image" or something like that.
> 
> Thanks,
> 									Pavel

Hi Pavel,

It might be ugly.  It's not unnecessary.  Some uses of FPGAs involve
switching out the FPGA images dynamically under control of the userspace.

Alan

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-09 19:14     ` atull
@ 2015-01-09 20:56       ` Pavel Machek
  2015-01-10  8:10         ` Pantelis Antoniou
  2015-01-12 21:01         ` One Thousand Gnomes
  0 siblings, 2 replies; 64+ messages in thread
From: Pavel Machek @ 2015-01-09 20:56 UTC (permalink / raw)
  To: atull
  Cc: gregkh, jgunthorpe, hpa, monstr, michal.simek, rdunlap,
	linux-kernel, devicetree, pantelis.antoniou, robh+dt,
	grant.likely, iws, linux-doc, broonie, philip, rubini,
	s.trumtrar, jason, kyle.teske, nico, balbi, m.chehab, davidb,
	rob, davem, cesarb, sameo, akpm, linus.walleij, pawel.moll,
	mark.rutland, ijc+devicetree, galak, devel, delicious.quinoa,
	dinguyen, yvanderv

On Fri 2015-01-09 13:14:24, atull wrote:
> On Wed, 7 Jan 2015, Pavel Machek wrote:
> 
> > On Tue 2015-01-06 14:13:37, atull@opensource.altera.com wrote:
> > > +
> > > +What:		/sys/class/fpga_manager/<fpga>/firmware
> > > +Date:		October 2014
> > > +KernelVersion:	3.18
> > > +Contact:	Alan Tull <atull@opensource.altera.com>
> > > +Description:	Name of the FPGA image file to load using firmware
> > > class.
> > 
> > This one is ugly: it unneccessarily passes firmware name through the
> > kernel. Just make interface and code simpler by always passing
> > "socfpga-fpga-image" or something like that.
> > 
> > Thanks,
> > 									Pavel
> 
> Hi Pavel,
> 
> It might be ugly.  It's not unnecessary.  Some uses of FPGAs involve
> switching out the FPGA images dynamically under control of the userspace.

Then configure udev to load right firmware for you, or ln -s
image-i-want-now socfpga-fpga-image to select the one to read...?

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

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-09 20:56       ` Pavel Machek
@ 2015-01-10  8:10         ` Pantelis Antoniou
  2015-01-10 15:11           ` Pavel Machek
  2015-01-12 21:01         ` One Thousand Gnomes
  1 sibling, 1 reply; 64+ messages in thread
From: Pantelis Antoniou @ 2015-01-10  8:10 UTC (permalink / raw)
  To: Pavel Machek
  Cc: atull, Greg Kroah-Hartman, jgunthorpe, hpa, Michal Simek,
	Michal Simek, rdunlap, Linux Kernel Mailing List, devicetree,
	robh+dt, Grant Likely, iws, linux-doc, Mark Brown, philip,
	rubini, Steffen Trumtrar, jason, kyle.teske, nico, Felipe Balbi,
	m.chehab, davidb, Rob Landley, davem, cesarb, sameo, akpm,
	Linus Walleij, pawel.moll, mark.rutland, ijc+devicetree, galak,
	devel, Alan Tull, dinguyen, yvanderv

Hi Pavel,

> On Jan 9, 2015, at 22:56 , Pavel Machek <pavel@denx.de> wrote:
> 
> On Fri 2015-01-09 13:14:24, atull wrote:
>> On Wed, 7 Jan 2015, Pavel Machek wrote:
>> 
>>> On Tue 2015-01-06 14:13:37, atull@opensource.altera.com wrote:
>>>> +
>>>> +What:		/sys/class/fpga_manager/<fpga>/firmware
>>>> +Date:		October 2014
>>>> +KernelVersion:	3.18
>>>> +Contact:	Alan Tull <atull@opensource.altera.com>
>>>> +Description:	Name of the FPGA image file to load using firmware
>>>> class.
>>> 
>>> This one is ugly: it unneccessarily passes firmware name through the
>>> kernel. Just make interface and code simpler by always passing
>>> "socfpga-fpga-image" or something like that.
>>> 
>>> Thanks,
>>> 									Pavel
>> 
>> Hi Pavel,
>> 
>> It might be ugly.  It's not unnecessary.  Some uses of FPGAs involve
>> switching out the FPGA images dynamically under control of the userspace.
> 
> Then configure udev to load right firmware for you, or ln -s
> image-i-want-now socfpga-fpga-image to select the one to read…?
> 

Who said that udev is going to be available in the kind of system this is going to end in?

Doing ln -s tricks to load a different image? Really?

I say the interface is fine as it is.

Regards

— Pantelis

> 
> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-10  8:10         ` Pantelis Antoniou
@ 2015-01-10 15:11           ` Pavel Machek
  2015-01-11 16:29             ` atull
  0 siblings, 1 reply; 64+ messages in thread
From: Pavel Machek @ 2015-01-10 15:11 UTC (permalink / raw)
  To: Pantelis Antoniou
  Cc: atull, Greg Kroah-Hartman, jgunthorpe, hpa, Michal Simek,
	Michal Simek, rdunlap, Linux Kernel Mailing List, devicetree,
	robh+dt, Grant Likely, iws, linux-doc, Mark Brown, philip,
	rubini, Steffen Trumtrar, jason, kyle.teske, nico, Felipe Balbi,
	m.chehab, davidb, Rob Landley, davem, cesarb, sameo, akpm,
	Linus Walleij, pawel.moll, mark.rutland, ijc+devicetree, galak,
	devel, Alan Tull, dinguyen, yvanderv

On Sat 2015-01-10 10:10:51, Pantelis Antoniou wrote:
> Hi Pavel,
> 
> > On Jan 9, 2015, at 22:56 , Pavel Machek <pavel@denx.de> wrote:
> > 
> > On Fri 2015-01-09 13:14:24, atull wrote:
> >> On Wed, 7 Jan 2015, Pavel Machek wrote:
> >> 
> >>> On Tue 2015-01-06 14:13:37, atull@opensource.altera.com wrote:
> >>>> +
> >>>> +What:		/sys/class/fpga_manager/<fpga>/firmware
> >>>> +Date:		October 2014
> >>>> +KernelVersion:	3.18
> >>>> +Contact:	Alan Tull <atull@opensource.altera.com>
> >>>> +Description:	Name of the FPGA image file to load using firmware
> >>>> class.
> >>> 
> >>> This one is ugly: it unneccessarily passes firmware name through the
> >>> kernel. Just make interface and code simpler by always passing
> >>> "socfpga-fpga-image" or something like that.
> >>> 
> >>> Thanks,
> >>> 									Pavel
> >> 
> >> Hi Pavel,
> >> 
> >> It might be ugly.  It's not unnecessary.  Some uses of FPGAs involve
> >> switching out the FPGA images dynamically under control of the userspace.
> > 
> > Then configure udev to load right firmware for you, or ln -s
> > image-i-want-now socfpga-fpga-image to select the one to read…?
> > 
> 
> Who said that udev is going to be available in the kind of system this is going to end in?

Noone.

> Doing ln -s tricks to load a different image? Really?

But if you don't have udev, you can do ln -s. It is better than
open-coding symlink functionality into
/sys/class/fpga_manager/<fpga>/firmware ... cause that's what this is.

Actually, clean implementation of "firmware" would be symlink in
sysfs; but I'd say that would be overdoing it.

> I say the interface is fine as it is.

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

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 0/4] FPGA Manager Framework
  2015-01-06 20:13 [PATCH v8 0/4] FPGA Manager Framework atull
                   ` (3 preceding siblings ...)
  2015-01-06 20:13 ` [PATCH v8 4/4] staging: fpga manager: add driver for socfpga fpga manager atull
@ 2015-01-10 18:11 ` Konrad Zapalowicz
  2015-01-11 16:08   ` atull
  4 siblings, 1 reply; 64+ messages in thread
From: Konrad Zapalowicz @ 2015-01-10 18:11 UTC (permalink / raw)
  To: atull
  Cc: gregkh, jgunthorpe, hpa, monstr, michal.simek, rdunlap,
	mark.rutland, linux-doc, rubini, pantelis.antoniou, s.trumtrar,
	devel, sameo, nico, ijc+devicetree, kyle.teske, grant.likely,
	davidb, linus.walleij, cesarb, devicetree, jason, pawel.moll,
	iws, broonie, philip, dinguyen, pavel, yvanderv, linux-kernel,
	balbi, delicious.quinoa, robh+dt, rob, galak, akpm, davem,
	m.chehab

On 01/06, atull@opensource.altera.com wrote:
> From: Alan Tull <atull@opensource.altera.com>

Alan, there is something wrong with your email client configuration
and you need to fix.

thanks,
konrad
 
> v8 changes the compatible string for SOCFPGA FPGA managers
> to be more chip specific.
> 
> "altr,fpga-mgr" becomes "altr,socfpga-fpga-mgr"
> 
> Thanks,
> Alan
> 
> Alan Tull (4):
>   doc: add bindings document for altera fpga manager
>   fpga manager: add sysfs interface document
>   staging: fpga manager: framework core
>   staging: fpga manager: add driver for socfpga fpga manager
> 
>  drivers/staging/Kconfig                            |    2 +
>  drivers/staging/Makefile                           |    1 +
>  .../Documentation/ABI/sysfs-class-fpga-manager     |   38 ++
>  .../bindings/altera-socfpga-fpga-mgr.txt           |   17 +
>  drivers/staging/fpga/Kconfig                       |   29 +
>  drivers/staging/fpga/Makefile                      |    9 +
>  drivers/staging/fpga/fpga-mgr.c                    |  551 ++++++++++++++++
>  drivers/staging/fpga/socfpga.c                     |  694 ++++++++++++++++++++
>  include/linux/fpga/fpga-mgr.h                      |  124 ++++
>  9 files changed, 1465 insertions(+)
>  create mode 100644 drivers/staging/fpga/Documentation/ABI/sysfs-class-fpga-manager
>  create mode 100644 drivers/staging/fpga/Documentation/bindings/altera-socfpga-fpga-mgr.txt
>  create mode 100644 drivers/staging/fpga/Kconfig
>  create mode 100644 drivers/staging/fpga/Makefile
>  create mode 100644 drivers/staging/fpga/fpga-mgr.c
>  create mode 100644 drivers/staging/fpga/socfpga.c
>  create mode 100644 include/linux/fpga/fpga-mgr.h
> 
> -- 
> 1.7.9.5
> 
> _______________________________________________
> devel mailing list
> devel@linuxdriverproject.org
> http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 0/4] FPGA Manager Framework
  2015-01-10 18:11 ` [PATCH v8 0/4] FPGA Manager Framework Konrad Zapalowicz
@ 2015-01-11 16:08   ` atull
  2015-01-11 16:24     ` Konrad Zapalowicz
  0 siblings, 1 reply; 64+ messages in thread
From: atull @ 2015-01-11 16:08 UTC (permalink / raw)
  To: Konrad Zapalowicz
  Cc: gregkh, jgunthorpe, hpa, monstr, michal.simek, rdunlap,
	mark.rutland, linux-doc, rubini, pantelis.antoniou, s.trumtrar,
	devel, sameo, nico, ijc+devicetree, kyle.teske, grant.likely,
	davidb, linus.walleij, cesarb, devicetree, jason, pawel.moll,
	iws, broonie, philip, dinguyen, pavel, yvanderv, linux-kernel,
	balbi, delicious.quinoa, robh+dt, rob, galak, akpm, davem,
	m.chehab

On Sat, 10 Jan 2015, Konrad Zapalowicz wrote:

> On 01/06, atull@opensource.altera.com wrote:
> > From: Alan Tull <atull@opensource.altera.com>
> 
> Alan, there is something wrong with your email client configuration
> and you need to fix.
> 
> thanks,
> konrad

Hi Konrad,

Can you be more specific?  What problem are you seeing?  Is it a problem 
with the original patch  post or when I reply?  When I a post, I'm using
'git send-email' with sendmail and when I am replying, I am using alpine.

Alan

>  
> > v8 changes the compatible string for SOCFPGA FPGA managers
> > to be more chip specific.
> > 
> > "altr,fpga-mgr" becomes "altr,socfpga-fpga-mgr"
> > 
> > Thanks,
> > Alan
> > 
> > Alan Tull (4):
> >   doc: add bindings document for altera fpga manager
> >   fpga manager: add sysfs interface document
> >   staging: fpga manager: framework core
> >   staging: fpga manager: add driver for socfpga fpga manager
> > 
> >  drivers/staging/Kconfig                            |    2 +
> >  drivers/staging/Makefile                           |    1 +
> >  .../Documentation/ABI/sysfs-class-fpga-manager     |   38 ++
> >  .../bindings/altera-socfpga-fpga-mgr.txt           |   17 +
> >  drivers/staging/fpga/Kconfig                       |   29 +
> >  drivers/staging/fpga/Makefile                      |    9 +
> >  drivers/staging/fpga/fpga-mgr.c                    |  551 ++++++++++++++++
> >  drivers/staging/fpga/socfpga.c                     |  694 ++++++++++++++++++++
> >  include/linux/fpga/fpga-mgr.h                      |  124 ++++
> >  9 files changed, 1465 insertions(+)
> >  create mode 100644 drivers/staging/fpga/Documentation/ABI/sysfs-class-fpga-manager
> >  create mode 100644 drivers/staging/fpga/Documentation/bindings/altera-socfpga-fpga-mgr.txt
> >  create mode 100644 drivers/staging/fpga/Kconfig
> >  create mode 100644 drivers/staging/fpga/Makefile
> >  create mode 100644 drivers/staging/fpga/fpga-mgr.c
> >  create mode 100644 drivers/staging/fpga/socfpga.c
> >  create mode 100644 include/linux/fpga/fpga-mgr.h
> > 
> > -- 
> > 1.7.9.5
> > 
> > _______________________________________________
> > devel mailing list
> > devel@linuxdriverproject.org
> > http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
> 

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 0/4] FPGA Manager Framework
  2015-01-11 16:08   ` atull
@ 2015-01-11 16:24     ` Konrad Zapalowicz
  2015-01-11 19:52       ` Pavel Machek
  0 siblings, 1 reply; 64+ messages in thread
From: Konrad Zapalowicz @ 2015-01-11 16:24 UTC (permalink / raw)
  To: atull
  Cc: gregkh, jgunthorpe, hpa, monstr, michal.simek, rdunlap,
	mark.rutland, linux-doc, rubini, pantelis.antoniou, s.trumtrar,
	devel, sameo, nico, ijc+devicetree, kyle.teske, grant.likely,
	davidb, linus.walleij, cesarb, devicetree, jason, pawel.moll,
	iws, broonie, philip, dinguyen, pavel, yvanderv, linux-kernel,
	balbi, delicious.quinoa, robh+dt, rob, galak, akpm, davem,
	m.chehab

On 01/11, atull wrote:
> On Sat, 10 Jan 2015, Konrad Zapalowicz wrote:
> 
> > On 01/06, atull@opensource.altera.com wrote:
> > > From: Alan Tull <atull@opensource.altera.com>
> > 
> > Alan, there is something wrong with your email client configuration
> > and you need to fix.
> > 
> > thanks,
> > konrad
> 
> Hi Konrad,
> 
> Can you be more specific?  What problem are you seeing?  Is it a problem 
> with the original patch  post or when I reply?  When I a post, I'm using
> 'git send-email' with sendmail and when I am replying, I am using alpine.

I think that because you email client configuration sets the 'From'
header to "atull@..." and at the same time you sign-off messages with
"Alan Tull atull@..." the "From" is added at the beginning of the
message to indicate the original author.

This should be fixed as the "from" is not welcomed in the body of the
commit message.

hope this helps,
konrad
 
> Alan
> 
> >  
> > > v8 changes the compatible string for SOCFPGA FPGA managers
> > > to be more chip specific.
> > > 
> > > "altr,fpga-mgr" becomes "altr,socfpga-fpga-mgr"
> > > 
> > > Thanks,
> > > Alan
> > > 
> > > Alan Tull (4):
> > >   doc: add bindings document for altera fpga manager
> > >   fpga manager: add sysfs interface document
> > >   staging: fpga manager: framework core
> > >   staging: fpga manager: add driver for socfpga fpga manager
> > > 
> > >  drivers/staging/Kconfig                            |    2 +
> > >  drivers/staging/Makefile                           |    1 +
> > >  .../Documentation/ABI/sysfs-class-fpga-manager     |   38 ++
> > >  .../bindings/altera-socfpga-fpga-mgr.txt           |   17 +
> > >  drivers/staging/fpga/Kconfig                       |   29 +
> > >  drivers/staging/fpga/Makefile                      |    9 +
> > >  drivers/staging/fpga/fpga-mgr.c                    |  551 ++++++++++++++++
> > >  drivers/staging/fpga/socfpga.c                     |  694 ++++++++++++++++++++
> > >  include/linux/fpga/fpga-mgr.h                      |  124 ++++
> > >  9 files changed, 1465 insertions(+)
> > >  create mode 100644 drivers/staging/fpga/Documentation/ABI/sysfs-class-fpga-manager
> > >  create mode 100644 drivers/staging/fpga/Documentation/bindings/altera-socfpga-fpga-mgr.txt
> > >  create mode 100644 drivers/staging/fpga/Kconfig
> > >  create mode 100644 drivers/staging/fpga/Makefile
> > >  create mode 100644 drivers/staging/fpga/fpga-mgr.c
> > >  create mode 100644 drivers/staging/fpga/socfpga.c
> > >  create mode 100644 include/linux/fpga/fpga-mgr.h
> > > 
> > > -- 
> > > 1.7.9.5
> > > 
> > > _______________________________________________
> > > devel mailing list
> > > devel@linuxdriverproject.org
> > > http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
> > 

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-10 15:11           ` Pavel Machek
@ 2015-01-11 16:29             ` atull
  2015-01-12  8:45               ` Pavel Machek
                                 ` (2 more replies)
  0 siblings, 3 replies; 64+ messages in thread
From: atull @ 2015-01-11 16:29 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Pantelis Antoniou, Greg Kroah-Hartman, jgunthorpe, hpa,
	Michal Simek, Michal Simek, rdunlap, Linux Kernel Mailing List,
	devicetree, robh+dt, Grant Likely, iws, linux-doc, Mark Brown,
	philip, rubini, Steffen Trumtrar, jason, kyle.teske, nico,
	Felipe Balbi, m.chehab, davidb, Rob Landley, davem, cesarb,
	sameo, akpm, Linus Walleij, pawel.moll, mark.rutland,
	ijc+devicetree, galak, devel, Alan Tull, dinguyen, yvanderv

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

On Sat, 10 Jan 2015, Pavel Machek wrote:

> On Sat 2015-01-10 10:10:51, Pantelis Antoniou wrote:
> > Hi Pavel,
> > 
> > > On Jan 9, 2015, at 22:56 , Pavel Machek <pavel@denx.de> wrote:
> > > 
> > > On Fri 2015-01-09 13:14:24, atull wrote:
> > >> On Wed, 7 Jan 2015, Pavel Machek wrote:
> > >> 
> > >>> On Tue 2015-01-06 14:13:37, atull@opensource.altera.com wrote:
> > >>>> +
> > >>>> +What:		/sys/class/fpga_manager/<fpga>/firmware
> > >>>> +Date:		October 2014
> > >>>> +KernelVersion:	3.18
> > >>>> +Contact:	Alan Tull <atull@opensource.altera.com>
> > >>>> +Description:	Name of the FPGA image file to load using firmware
> > >>>> class.
> > >>> 
> > >>> This one is ugly: it unneccessarily passes firmware name through the
> > >>> kernel. Just make interface and code simpler by always passing
> > >>> "socfpga-fpga-image" or something like that.
> > >>> 
> > >>> Thanks,
> > >>> 									Pavel
> > >> 
> > >> Hi Pavel,
> > >> 
> > >> It might be ugly.  It's not unnecessary.  Some uses of FPGAs involve
> > >> switching out the FPGA images dynamically under control of the userspace.
> > > 
> > > Then configure udev to load right firmware for you, or ln -s
> > > image-i-want-now socfpga-fpga-image to select the one to read…?
> > > 
> > 
> > Who said that udev is going to be available in the kind of system this is going to end in?
> 
> Noone.
> 
> > Doing ln -s tricks to load a different image? Really?
> 
> But if you don't have udev, you can do ln -s. It is better than
> open-coding symlink functionality into
> /sys/class/fpga_manager/<fpga>/firmware ... cause that's what this is.
> 
> Actually, clean implementation of "firmware" would be symlink in
> sysfs; but I'd say that would be overdoing it.
> 
> > I say the interface is fine as it is.
> 
> I say it is not.
> 									Pavel

Hi Pavel,

I see that we could do it that way and it would eliminate one of the
sysfs files (../fpga_manager/<fpga>/firmware).  Either way we are assuming
that there are fpga images in the filesystem in the firmware search path,
so I don't see why adding a piece of indirection (symlink) makes things
better.

We want to be able to switch out the FPGA image
under control of userspace.  So we don't want an interface that makes
it more cumbersome or tries to pretend that there's only one image.

Previous uses of the firmware layer has been to use it to load once after
bootup; this is different since some use cases will want to switch out
the FPGA image.  If someone wants there to be only one FPGA image on
the FGPA forever, they will probably not be using this framework; their
FPGA will probably be loaded before Linux boots up.

Alan


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

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 0/4] FPGA Manager Framework
  2015-01-11 16:24     ` Konrad Zapalowicz
@ 2015-01-11 19:52       ` Pavel Machek
  2015-01-11 20:58         ` Konrad Zapalowicz
  2015-01-12 14:06         ` Dan Carpenter
  0 siblings, 2 replies; 64+ messages in thread
From: Pavel Machek @ 2015-01-11 19:52 UTC (permalink / raw)
  To: Konrad Zapalowicz
  Cc: atull, gregkh, jgunthorpe, hpa, monstr, michal.simek, rdunlap,
	mark.rutland, linux-doc, rubini, pantelis.antoniou, s.trumtrar,
	devel, sameo, nico, ijc+devicetree, kyle.teske, grant.likely,
	davidb, linus.walleij, cesarb, devicetree, jason, pawel.moll,
	iws, broonie, philip, dinguyen, yvanderv, linux-kernel, balbi,
	delicious.quinoa, robh+dt, rob, galak, akpm, davem, m.chehab

On Sun 2015-01-11 17:24:26, Konrad Zapalowicz wrote:
> On 01/11, atull wrote:
> > On Sat, 10 Jan 2015, Konrad Zapalowicz wrote:
> > 
> > > On 01/06, atull@opensource.altera.com wrote:
> > > > From: Alan Tull <atull@opensource.altera.com>
> > > 
> > > Alan, there is something wrong with your email client configuration
> > > and you need to fix.
> > > 
> > > thanks,
> > > konrad
> > 
> > Hi Konrad,
> > 
> > Can you be more specific?  What problem are you seeing?  Is it a problem 
> > with the original patch  post or when I reply?  When I a post, I'm using
> > 'git send-email' with sendmail and when I am replying, I am using alpine.
> 
> I think that because you email client configuration sets the 'From'
> header to "atull@..." and at the same time you sign-off messages with
> "Alan Tull atull@..." the "From" is added at the beginning of the
> message to indicate the original author.
> 
> This should be fixed as the "from" is not welcomed in the body of the
> commit message.

Actually, why not? Tools should know to look at From: in the
commit. AFAICT, Alan has it set up correctly.
								Pavel
								
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 0/4] FPGA Manager Framework
  2015-01-11 19:52       ` Pavel Machek
@ 2015-01-11 20:58         ` Konrad Zapalowicz
  2015-01-11 21:31           ` Pavel Machek
  2015-01-12 14:06         ` Dan Carpenter
  1 sibling, 1 reply; 64+ messages in thread
From: Konrad Zapalowicz @ 2015-01-11 20:58 UTC (permalink / raw)
  To: Pavel Machek
  Cc: atull, gregkh, jgunthorpe, hpa, monstr, michal.simek, rdunlap,
	mark.rutland, linux-doc, rubini, pantelis.antoniou, s.trumtrar,
	devel, sameo, nico, ijc+devicetree, kyle.teske, grant.likely,
	davidb, linus.walleij, cesarb, devicetree, jason, pawel.moll,
	iws, broonie, philip, dinguyen, yvanderv, linux-kernel, balbi,
	delicious.quinoa, robh+dt, rob, galak, akpm, davem, m.chehab

On 01/11, Pavel Machek wrote:
> On Sun 2015-01-11 17:24:26, Konrad Zapalowicz wrote:
> > On 01/11, atull wrote:
> > > On Sat, 10 Jan 2015, Konrad Zapalowicz wrote:
> > > 
> > > > On 01/06, atull@opensource.altera.com wrote:
> > > > > From: Alan Tull <atull@opensource.altera.com>
> > > > 
> > > > Alan, there is something wrong with your email client configuration
> > > > and you need to fix.
> > > > 
> > > > thanks,
> > > > konrad
> > > 
> > > Hi Konrad,
> > > 
> > > Can you be more specific?  What problem are you seeing?  Is it a problem 
> > > with the original patch  post or when I reply?  When I a post, I'm using
> > > 'git send-email' with sendmail and when I am replying, I am using alpine.
> > 
> > I think that because you email client configuration sets the 'From'
> > header to "atull@..." and at the same time you sign-off messages with
> > "Alan Tull atull@..." the "From" is added at the beginning of the
> > message to indicate the original author.
> > 
> > This should be fixed as the "from" is not welcomed in the body of the
> > commit message.
> 
> Actually, why not? Tools should know to look at From: in the
> commit. AFAICT, Alan has it set up correctly.

I think he has not. I suspect that the sendmail configuration in the git
config might be wrong. The 'from' must contain the same data as the git
is configured for the user.name and user.email options. Otherwise the
send-email is adding the "From: ..." to the message body.

cheers,
konrad

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

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 0/4] FPGA Manager Framework
  2015-01-11 20:58         ` Konrad Zapalowicz
@ 2015-01-11 21:31           ` Pavel Machek
  2015-01-12 13:50             ` Michal Simek
  0 siblings, 1 reply; 64+ messages in thread
From: Pavel Machek @ 2015-01-11 21:31 UTC (permalink / raw)
  To: Konrad Zapalowicz
  Cc: atull, gregkh, jgunthorpe, hpa, monstr, michal.simek, rdunlap,
	mark.rutland, linux-doc, rubini, pantelis.antoniou, s.trumtrar,
	devel, sameo, nico, ijc+devicetree, kyle.teske, grant.likely,
	davidb, linus.walleij, cesarb, devicetree, jason, pawel.moll,
	iws, broonie, philip, dinguyen, yvanderv, linux-kernel, balbi,
	delicious.quinoa, robh+dt, rob, galak, akpm, davem, m.chehab

On Sun 2015-01-11 21:58:00, Konrad Zapalowicz wrote:
> On 01/11, Pavel Machek wrote:
> > On Sun 2015-01-11 17:24:26, Konrad Zapalowicz wrote:
> > > On 01/11, atull wrote:
> > > > On Sat, 10 Jan 2015, Konrad Zapalowicz wrote:
> > > > 
> > > > > On 01/06, atull@opensource.altera.com wrote:
> > > > > > From: Alan Tull <atull@opensource.altera.com>
> > > > > 
> > > > > Alan, there is something wrong with your email client configuration
> > > > > and you need to fix.
> > > > > 
> > > > > thanks,
> > > > > konrad
> > > > 
> > > > Hi Konrad,
> > > > 
> > > > Can you be more specific?  What problem are you seeing?  Is it a problem 
> > > > with the original patch  post or when I reply?  When I a post, I'm using
> > > > 'git send-email' with sendmail and when I am replying, I am using alpine.
> > > 
> > > I think that because you email client configuration sets the 'From'
> > > header to "atull@..." and at the same time you sign-off messages with
> > > "Alan Tull atull@..." the "From" is added at the beginning of the
> > > message to indicate the original author.
> > > 
> > > This should be fixed as the "from" is not welcomed in the body of the
> > > commit message.
> > 
> > Actually, why not? Tools should know to look at From: in the
> > commit. AFAICT, Alan has it set up correctly.
> 
> I think he has not. I suspect that the sendmail configuration in the git
> config might be wrong. The 'from' must contain the same data as the git
> is configured for the user.name and user.email options. Otherwise the
> send-email is adding the "From: ..." to the message body.

And what is the problem with From: in the message body? Tools should
handle that ok.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-11 16:29             ` atull
@ 2015-01-12  8:45               ` Pavel Machek
  2015-01-12 13:48                 ` Michal Simek
  2015-01-12 16:05               ` Rob Herring
  2015-01-12 18:06               ` Jason Gunthorpe
  2 siblings, 1 reply; 64+ messages in thread
From: Pavel Machek @ 2015-01-12  8:45 UTC (permalink / raw)
  To: atull
  Cc: Pantelis Antoniou, Greg Kroah-Hartman, jgunthorpe, hpa,
	Michal Simek, Michal Simek, rdunlap, Linux Kernel Mailing List,
	devicetree, robh+dt, Grant Likely, iws, linux-doc, Mark Brown,
	philip, rubini, Steffen Trumtrar, jason, kyle.teske, nico,
	Felipe Balbi, m.chehab, davidb, Rob Landley, davem, cesarb,
	sameo, akpm, Linus Walleij, pawel.moll, mark.rutland,
	ijc+devicetree, galak, devel, Alan Tull, dinguyen, yvanderv

On Sun 2015-01-11 10:29:00, atull wrote:
> On Sat, 10 Jan 2015, Pavel Machek wrote:
> 
> > On Sat 2015-01-10 10:10:51, Pantelis Antoniou wrote:
> > > Hi Pavel,
> > > 
> > > > On Jan 9, 2015, at 22:56 , Pavel Machek <pavel@denx.de> wrote:
> > > > 
> > > > On Fri 2015-01-09 13:14:24, atull wrote:
> > > >> On Wed, 7 Jan 2015, Pavel Machek wrote:
> > > >> 
> > > >>> On Tue 2015-01-06 14:13:37, atull@opensource.altera.com wrote:
> > > >>>> +
> > > >>>> +What:		/sys/class/fpga_manager/<fpga>/firmware
> > > >>>> +Date:		October 2014
> > > >>>> +KernelVersion:	3.18
> > > >>>> +Contact:	Alan Tull <atull@opensource.altera.com>
> > > >>>> +Description:	Name of the FPGA image file to load using firmware
> > > >>>> class.
> > > >>> 
> > > >>> This one is ugly: it unneccessarily passes firmware name through the
> > > >>> kernel. Just make interface and code simpler by always passing
> > > >>> "socfpga-fpga-image" or something like that.
> > > >>> 
> > > >>> Thanks,
> > > >>> 									Pavel
> > > >> 
> > > >> Hi Pavel,
> > > >> 
> > > >> It might be ugly.  It's not unnecessary.  Some uses of FPGAs involve
> > > >> switching out the FPGA images dynamically under control of the userspace.
> > > > 
> > > > Then configure udev to load right firmware for you, or ln -s
> > > > image-i-want-now socfpga-fpga-image to select the one to read…?
> > > > 
> > > 
> > > Who said that udev is going to be available in the kind of system this is going to end in?
> > 
> > Noone.
> > 
> > > Doing ln -s tricks to load a different image? Really?
> > 
> > But if you don't have udev, you can do ln -s. It is better than
> > open-coding symlink functionality into
> > /sys/class/fpga_manager/<fpga>/firmware ... cause that's what this is.
> > 
> > Actually, clean implementation of "firmware" would be symlink in
> > sysfs; but I'd say that would be overdoing it.
> > 
> > > I say the interface is fine as it is.
> > 
> > I say it is not.
> > 									Pavel
> 
> Hi Pavel,
> 
> I see that we could do it that way and it would eliminate one of the
> sysfs files (../fpga_manager/<fpga>/firmware).  Either way we are assuming
> that there are fpga images in the filesystem in the firmware search path,
> so I don't see why adding a piece of indirection (symlink) makes things
> better.

Well.. you are basically re-implementing symlink with
../fpga_manager/<fpga>/firmware "file" in sysfs, and doing it
badly. It has no advantages over real symlink.

> We want to be able to switch out the FPGA image
> under control of userspace.  So we don't want an interface that makes
> it more cumbersome or tries to pretend that there's only one image.

What is cumbersome about symlink? Why is "fake" symlink in sysfs better?

> Previous uses of the firmware layer has been to use it to load once after
> bootup; this is different since some use cases will want to switch out
> the FPGA image.  If someone wants there to be only one FPGA image on
> the FGPA forever, they will probably not be using this framework; their
> FPGA will probably be loaded before Linux boots up.

Why? I have just one image on the fpga, and would prefer to load it
from Linux.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-12  8:45               ` Pavel Machek
@ 2015-01-12 13:48                 ` Michal Simek
  2015-01-13  7:28                   ` Pavel Machek
  0 siblings, 1 reply; 64+ messages in thread
From: Michal Simek @ 2015-01-12 13:48 UTC (permalink / raw)
  To: Pavel Machek, atull
  Cc: Pantelis Antoniou, Greg Kroah-Hartman, jgunthorpe, hpa,
	Michal Simek, Michal Simek, rdunlap, Linux Kernel Mailing List,
	devicetree, robh+dt, Grant Likely, iws, linux-doc, Mark Brown,
	philip, rubini, Steffen Trumtrar, jason, kyle.teske, nico,
	Felipe Balbi, m.chehab, davidb, Rob Landley, davem, cesarb,
	sameo, akpm, Linus Walleij, pawel.moll, mark.rutland,
	ijc+devicetree, galak, devel, Alan Tull, dinguyen, yvanderv

On 01/12/2015 09:45 AM, Pavel Machek wrote:
> On Sun 2015-01-11 10:29:00, atull wrote:
>> On Sat, 10 Jan 2015, Pavel Machek wrote:
>>
>>> On Sat 2015-01-10 10:10:51, Pantelis Antoniou wrote:
>>>> Hi Pavel,
>>>>
>>>>> On Jan 9, 2015, at 22:56 , Pavel Machek <pavel@denx.de> wrote:
>>>>>
>>>>> On Fri 2015-01-09 13:14:24, atull wrote:
>>>>>> On Wed, 7 Jan 2015, Pavel Machek wrote:
>>>>>>
>>>>>>> On Tue 2015-01-06 14:13:37, atull@opensource.altera.com wrote:
>>>>>>>> +
>>>>>>>> +What:		/sys/class/fpga_manager/<fpga>/firmware
>>>>>>>> +Date:		October 2014
>>>>>>>> +KernelVersion:	3.18
>>>>>>>> +Contact:	Alan Tull <atull@opensource.altera.com>
>>>>>>>> +Description:	Name of the FPGA image file to load using firmware
>>>>>>>> class.
>>>>>>>
>>>>>>> This one is ugly: it unneccessarily passes firmware name through the
>>>>>>> kernel. Just make interface and code simpler by always passing
>>>>>>> "socfpga-fpga-image" or something like that.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> 									Pavel
>>>>>>
>>>>>> Hi Pavel,
>>>>>>
>>>>>> It might be ugly.  It's not unnecessary.  Some uses of FPGAs involve
>>>>>> switching out the FPGA images dynamically under control of the userspace.
>>>>>
>>>>> Then configure udev to load right firmware for you, or ln -s
>>>>> image-i-want-now socfpga-fpga-image to select the one to read…?
>>>>>
>>>>
>>>> Who said that udev is going to be available in the kind of system this is going to end in?
>>>
>>> Noone.
>>>
>>>> Doing ln -s tricks to load a different image? Really?
>>>
>>> But if you don't have udev, you can do ln -s. It is better than
>>> open-coding symlink functionality into
>>> /sys/class/fpga_manager/<fpga>/firmware ... cause that's what this is.
>>>
>>> Actually, clean implementation of "firmware" would be symlink in
>>> sysfs; but I'd say that would be overdoing it.
>>>
>>>> I say the interface is fine as it is.
>>>
>>> I say it is not.
>>> 									Pavel
>>
>> Hi Pavel,
>>
>> I see that we could do it that way and it would eliminate one of the
>> sysfs files (../fpga_manager/<fpga>/firmware).  Either way we are assuming
>> that there are fpga images in the filesystem in the firmware search path,
>> so I don't see why adding a piece of indirection (symlink) makes things
>> better.
> 
> Well.. you are basically re-implementing symlink with
> ../fpga_manager/<fpga>/firmware "file" in sysfs, and doing it
> badly. It has no advantages over real symlink.
> 
>> We want to be able to switch out the FPGA image
>> under control of userspace.  So we don't want an interface that makes
>> it more cumbersome or tries to pretend that there's only one image.
> 
> What is cumbersome about symlink? Why is "fake" symlink in sysfs better?
> 
>> Previous uses of the firmware layer has been to use it to load once after
>> bootup; this is different since some use cases will want to switch out
>> the FPGA image.  If someone wants there to be only one FPGA image on
>> the FGPA forever, they will probably not be using this framework; their
>> FPGA will probably be loaded before Linux boots up.
> 
> Why? I have just one image on the fpga, and would prefer to load it
> from Linux.

Pavel: These patches target staging and sysfs interface doesn't need to be stable
at this time. I would prefer to add these patches to staging for 3.20
and feel free to send the patch which fix this.
With your code will be exactly clear how you want to use it and we can
talk about it.

Thanks,
Michal




^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 0/4] FPGA Manager Framework
  2015-01-11 21:31           ` Pavel Machek
@ 2015-01-12 13:50             ` Michal Simek
  0 siblings, 0 replies; 64+ messages in thread
From: Michal Simek @ 2015-01-12 13:50 UTC (permalink / raw)
  To: Pavel Machek, Konrad Zapalowicz
  Cc: atull, gregkh, jgunthorpe, hpa, monstr, michal.simek, rdunlap,
	mark.rutland, linux-doc, rubini, pantelis.antoniou, s.trumtrar,
	devel, sameo, nico, ijc+devicetree, kyle.teske, grant.likely,
	davidb, linus.walleij, cesarb, devicetree, jason, pawel.moll,
	iws, broonie, philip, dinguyen, yvanderv, linux-kernel, balbi,
	delicious.quinoa, robh+dt, rob, galak, akpm, davem, m.chehab

On 01/11/2015 10:31 PM, Pavel Machek wrote:
> On Sun 2015-01-11 21:58:00, Konrad Zapalowicz wrote:
>> On 01/11, Pavel Machek wrote:
>>> On Sun 2015-01-11 17:24:26, Konrad Zapalowicz wrote:
>>>> On 01/11, atull wrote:
>>>>> On Sat, 10 Jan 2015, Konrad Zapalowicz wrote:
>>>>>
>>>>>> On 01/06, atull@opensource.altera.com wrote:
>>>>>>> From: Alan Tull <atull@opensource.altera.com>
>>>>>>
>>>>>> Alan, there is something wrong with your email client configuration
>>>>>> and you need to fix.
>>>>>>
>>>>>> thanks,
>>>>>> konrad
>>>>>
>>>>> Hi Konrad,
>>>>>
>>>>> Can you be more specific?  What problem are you seeing?  Is it a problem 
>>>>> with the original patch  post or when I reply?  When I a post, I'm using
>>>>> 'git send-email' with sendmail and when I am replying, I am using alpine.
>>>>
>>>> I think that because you email client configuration sets the 'From'
>>>> header to "atull@..." and at the same time you sign-off messages with
>>>> "Alan Tull atull@..." the "From" is added at the beginning of the
>>>> message to indicate the original author.
>>>>
>>>> This should be fixed as the "from" is not welcomed in the body of the
>>>> commit message.
>>>
>>> Actually, why not? Tools should know to look at From: in the
>>> commit. AFAICT, Alan has it set up correctly.
>>
>> I think he has not. I suspect that the sendmail configuration in the git
>> config might be wrong. The 'from' must contain the same data as the git
>> is configured for the user.name and user.email options. Otherwise the
>> send-email is adding the "From: ..." to the message body.
> 
> And what is the problem with From: in the message body? Tools should
> handle that ok.

Konrad: Problem is probably just on your end. I have also no problem to handle
these patches.
Alan: From my point of view you don't need to change anything.

Thanks,
Michal



^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 0/4] FPGA Manager Framework
  2015-01-11 19:52       ` Pavel Machek
  2015-01-11 20:58         ` Konrad Zapalowicz
@ 2015-01-12 14:06         ` Dan Carpenter
  1 sibling, 0 replies; 64+ messages in thread
From: Dan Carpenter @ 2015-01-12 14:06 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Konrad Zapalowicz, mark.rutland, linux-doc, rubini,
	pantelis.antoniou, hpa, s.trumtrar, devel, sameo, nico, iws,
	michal.simek, kyle.teske, jgunthorpe, grant.likely, davidb,
	linus.walleij, cesarb, devicetree, jason, pawel.moll,
	ijc+devicetree, atull, broonie, philip, akpm, monstr, gregkh,
	yvanderv, linux-kernel, balbi, davem, robh+dt, rob, galak,
	dinguyen, delicious.quinoa, m.chehab

On Sun, Jan 11, 2015 at 08:52:19PM +0100, Pavel Machek wrote:
> 
> Actually, why not? Tools should know to look at From: in the
> commit. AFAICT, Alan has it set up correctly.
> 								Pavel

Normally, the Extra From header should only be used if the patches were
sent on behalf of someone else.  It's just a bit of a pain to verify
that the Extra From matches the email headers.

Also sometimes people can't get their corporate email configured
properly for patches and so they use gmail but specify an Extra From so
their employer gets credit...  That's ok.

The patches apply fine so it's not a "redo the patch" type thing, it's
just a "In the long term, please, fix your email From header so the
Extra From isn't needed."

regards,
dan carpenter


^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-11 16:29             ` atull
  2015-01-12  8:45               ` Pavel Machek
@ 2015-01-12 16:05               ` Rob Herring
  2015-01-12 16:26                 ` Mark Brown
  2015-01-12 18:06               ` Jason Gunthorpe
  2 siblings, 1 reply; 64+ messages in thread
From: Rob Herring @ 2015-01-12 16:05 UTC (permalink / raw)
  To: atull
  Cc: Pavel Machek, Pantelis Antoniou, Greg Kroah-Hartman,
	Jason Gunthorpe, H. Peter Anvin, Michal Simek, Michal Simek,
	Randy Dunlap, Linux Kernel Mailing List, devicetree, Rob Herring,
	Grant Likely, iws, linux-doc, Mark Brown, Philip Balister,
	Alessandro Rubini, Steffen Trumtrar, Jason Cooper, Kyle Teske,
	Nicolas Pitre, Felipe Balbi, Mauro Carvalho Chehab, David Brown,
	Rob Landley, David Miller, Cesar Eduardo Barros, Samuel Ortiz,
	Andrew Morton, Linus Walleij, Pawel Moll, Mark Rutland,
	Ian Campbell, Kumar Gala, devel, Alan Tull, Dinh Nguyen,
	yvanderv

On Sun, Jan 11, 2015 at 10:29 AM, atull <atull@opensource.altera.com> wrote:
> On Sat, 10 Jan 2015, Pavel Machek wrote:
>
>> On Sat 2015-01-10 10:10:51, Pantelis Antoniou wrote:
>> > Hi Pavel,
>> >
>> > > On Jan 9, 2015, at 22:56 , Pavel Machek <pavel@denx.de> wrote:
>> > >
>> > > On Fri 2015-01-09 13:14:24, atull wrote:
>> > >> On Wed, 7 Jan 2015, Pavel Machek wrote:
>> > >>
>> > >>> On Tue 2015-01-06 14:13:37, atull@opensource.altera.com wrote:
>> > >>>> +
>> > >>>> +What:         /sys/class/fpga_manager/<fpga>/firmware
>> > >>>> +Date:         October 2014
>> > >>>> +KernelVersion:        3.18
>> > >>>> +Contact:      Alan Tull <atull@opensource.altera.com>
>> > >>>> +Description:  Name of the FPGA image file to load using firmware
>> > >>>> class.
>> > >>>
>> > >>> This one is ugly: it unneccessarily passes firmware name through the
>> > >>> kernel. Just make interface and code simpler by always passing
>> > >>> "socfpga-fpga-image" or something like that.
>> > >>>
>> > >>> Thanks,
>> > >>>                                                                         Pavel
>> > >>
>> > >> Hi Pavel,
>> > >>
>> > >> It might be ugly.  It's not unnecessary.  Some uses of FPGAs involve
>> > >> switching out the FPGA images dynamically under control of the userspace.
>> > >
>> > > Then configure udev to load right firmware for you, or ln -s
>> > > image-i-want-now socfpga-fpga-image to select the one to read…?
>> > >
>> >
>> > Who said that udev is going to be available in the kind of system this is going to end in?
>>
>> Noone.
>>
>> > Doing ln -s tricks to load a different image? Really?
>>
>> But if you don't have udev, you can do ln -s. It is better than
>> open-coding symlink functionality into
>> /sys/class/fpga_manager/<fpga>/firmware ... cause that's what this is.
>>
>> Actually, clean implementation of "firmware" would be symlink in
>> sysfs; but I'd say that would be overdoing it.
>>
>> > I say the interface is fine as it is.
>>
>> I say it is not.
>>                                                                       Pavel
>
> Hi Pavel,
>
> I see that we could do it that way and it would eliminate one of the
> sysfs files (../fpga_manager/<fpga>/firmware).  Either way we are assuming
> that there are fpga images in the filesystem in the firmware search path,
> so I don't see why adding a piece of indirection (symlink) makes things
> better.
>
> We want to be able to switch out the FPGA image
> under control of userspace.  So we don't want an interface that makes
> it more cumbersome or tries to pretend that there's only one image.
>
> Previous uses of the firmware layer has been to use it to load once after
> bootup; this is different since some use cases will want to switch out
> the FPGA image.  If someone wants there to be only one FPGA image on
> the FGPA forever, they will probably not be using this framework; their
> FPGA will probably be loaded before Linux boots up.

That's not really true. Some WiFi devices have to load different
firmware for client and AP modes and switching is a common usecase in
phones.

There may be other instances of drivers wanting configurable firmware
names already. A common interface here would be nice. It could also be
useful to expose the firmware names such that you could generate a
list of all firmware files needed for a machine (that may already be
possible with uevents?).

Rob

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-12 16:05               ` Rob Herring
@ 2015-01-12 16:26                 ` Mark Brown
  0 siblings, 0 replies; 64+ messages in thread
From: Mark Brown @ 2015-01-12 16:26 UTC (permalink / raw)
  To: Rob Herring
  Cc: atull, Pavel Machek, Pantelis Antoniou, Greg Kroah-Hartman,
	Jason Gunthorpe, H. Peter Anvin, Michal Simek, Michal Simek,
	Randy Dunlap, Linux Kernel Mailing List, devicetree, Rob Herring,
	Grant Likely, iws, linux-doc, Philip Balister, Alessandro Rubini,
	Steffen Trumtrar, Jason Cooper, Kyle Teske, Nicolas Pitre,
	Felipe Balbi, Mauro Carvalho Chehab, David Brown, Rob Landley,
	David Miller, Cesar Eduardo Barros, Samuel Ortiz, Andrew Morton,
	Linus Walleij, Pawel Moll, Mark Rutland, Ian Campbell,
	Kumar Gala, devel, Alan Tull, Dinh Nguyen, yvanderv

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

On Mon, Jan 12, 2015 at 10:05:49AM -0600, Rob Herring wrote:
> On Sun, Jan 11, 2015 at 10:29 AM, atull <atull@opensource.altera.com> wrote:

> > Previous uses of the firmware layer has been to use it to load once after
> > bootup; this is different since some use cases will want to switch out
> > the FPGA image.  If someone wants there to be only one FPGA image on
> > the FGPA forever, they will probably not be using this framework; their
> > FPGA will probably be loaded before Linux boots up.

> That's not really true. Some WiFi devices have to load different
> firmware for client and AP modes and switching is a common usecase in
> phones.

Right, this is also very common for audio DSPs - the DSPs are frequently
much smaller than the total set of firmware they might want to run so
they need to switch at runtime.

> There may be other instances of drivers wanting configurable firmware
> names already. A common interface here would be nice. It could also be
> useful to expose the firmware names such that you could generate a
> list of all firmware files needed for a machine (that may already be
> possible with uevents?).

Some standard mechanism for enumerating the possible firmwares and
switching between them would definitely make life easier on the audio
side.  It'd also be good for userspace to be able to say it's got new
firmware to add to the list, but that's suggesting to me that the
enumeration part of things might be better handled in userspace.

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

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-11 16:29             ` atull
  2015-01-12  8:45               ` Pavel Machek
  2015-01-12 16:05               ` Rob Herring
@ 2015-01-12 18:06               ` Jason Gunthorpe
  2015-01-13 16:21                 ` One Thousand Gnomes
  2 siblings, 1 reply; 64+ messages in thread
From: Jason Gunthorpe @ 2015-01-12 18:06 UTC (permalink / raw)
  To: atull
  Cc: Pavel Machek, Pantelis Antoniou, Greg Kroah-Hartman, hpa,
	Michal Simek, Michal Simek, rdunlap, Linux Kernel Mailing List,
	devicetree, robh+dt, Grant Likely, iws, linux-doc, Mark Brown,
	philip, rubini, Steffen Trumtrar, jason, kyle.teske, nico,
	Felipe Balbi, m.chehab, davidb, Rob Landley, davem, cesarb,
	sameo, akpm, Linus Walleij, pawel.moll, mark.rutland,
	ijc+devicetree, galak, devel, Alan Tull, dinguyen, yvanderv

On Sun, Jan 11, 2015 at 10:29:00AM -0600, atull wrote:
> the FPGA image.  If someone wants there to be only one FPGA image on
> the FGPA forever, they will probably not be using this framework; their
> FPGA will probably be loaded before Linux boots up.

Nonsense, loading the FPGA through Linux is much better, it avoids
having to deal with this complexity in the boot loader and means that
Linux can be used to locate the huge image in some kind of sensible
filesystem, log failures, do any FPGA startup sequence/etc.

With hotplug and DT I find this this works very well. The FPGA devices
simply are not registered with the kernel until userspace gives the OK
(in future a DT overload can handle this)

If you keep with the notion that the DT overload specifies the
matching FPGA firmware then it makes alot more sense to me to use DT
overlays as the API to change the file name - not sysfs.

To reload a FPGA, unload the DT overlay (this would have to disconnect
all the drivers), de-program the FPGA, the load a new DT overlay,
which reprograms and re-binds the new drivers. All those steps have to
be done anyhow, you can't just swap the HW while drivers are attached.

.. and if there are no drivers then Alan is right, this is the wrong
interface for the 'FPGA as a user space co-processor' model.

I would also re-iterate my earlier comments: requiring the whole FPGA
image to be held in ram makes this useless for any of my applications.

Jason

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-09 20:56       ` Pavel Machek
  2015-01-10  8:10         ` Pantelis Antoniou
@ 2015-01-12 21:01         ` One Thousand Gnomes
  2015-01-12 21:43           ` Jason Gunthorpe
  1 sibling, 1 reply; 64+ messages in thread
From: One Thousand Gnomes @ 2015-01-12 21:01 UTC (permalink / raw)
  To: Pavel Machek
  Cc: atull, gregkh, jgunthorpe, hpa, monstr, michal.simek, rdunlap,
	linux-kernel, devicetree, pantelis.antoniou, robh+dt,
	grant.likely, iws, linux-doc, broonie, philip, rubini,
	s.trumtrar, jason, kyle.teske, nico, balbi, m.chehab, davidb,
	rob, davem, cesarb, sameo, akpm, linus.walleij, pawel.moll,
	mark.rutland, ijc+devicetree, galak, devel, delicious.quinoa,
	dinguyen, yvanderv

> Then configure udev to load right firmware for you, or ln -s
> image-i-want-now socfpga-fpga-image to select the one to read...?

Your conceptual model is wrong. FPGA firmware is dynamic. There are
already people who lazy reload FPGA firmware on taskswitches. This
proposed fpga manager is broken but it's broken in exactly the reverse
direction to the one you are arguing for.

There are plenty of people today who treat the FPGA as an entirely
dynamic resource. It's not like flashing a controller, its near
immediate.

The udev/sysfs model is broken because
- it's too slow for dynamic switching
- it doesn't address permissions
- it doesn't address namespaces
- it doesn't address dynamic management of resources (open/use/close)
  with dyanmic resource recovery on the final close as is the Unix way.

not because FPGA's are some static boot time resource.

If you look at all the academic work on this you see the same kind
of dynamic usage, even more so.

The library API you actually need is much closer to

	/* Get my firmware */
	fw = fpga_openfirmware("foo.fpga");
	/* Get me a suitable FPGA for it */
	fd = fpga_alloc(&w);
	/* Load it */
	fgpa_load(fd, "foo.fpga");

	do_shit(fd);

	fpga_close(fd);

You want to be able to have things like your game just load up an audio
physics engine, lob it into a random fpga, play it, drop it. Likewise
lots of other FPGA apps - video processing for example, crypto, format
convertors, gesture analysers and so on. Think a world where there are
gimp filters that want to just grab an fpga for 3 seconds.

Its completely dynamic and it will get more so as we switch from the
painful world of VHDL and friends to high level parallel aware language
compilers for FPGAs and everyone will be knocking up quick FPGA hacks.

Alan

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-12 21:01         ` One Thousand Gnomes
@ 2015-01-12 21:43           ` Jason Gunthorpe
  2015-01-13 16:28             ` One Thousand Gnomes
  0 siblings, 1 reply; 64+ messages in thread
From: Jason Gunthorpe @ 2015-01-12 21:43 UTC (permalink / raw)
  To: One Thousand Gnomes
  Cc: Pavel Machek, atull, gregkh, hpa, monstr, michal.simek, rdunlap,
	linux-kernel, devicetree, pantelis.antoniou, robh+dt,
	grant.likely, iws, linux-doc, broonie, philip, rubini,
	s.trumtrar, jason, kyle.teske, nico, balbi, m.chehab, davidb,
	rob, davem, cesarb, sameo, akpm, linus.walleij, pawel.moll,
	mark.rutland, ijc+devicetree, galak, devel, delicious.quinoa,
	dinguyen, yvanderv

On Mon, Jan 12, 2015 at 09:01:34PM +0000, One Thousand Gnomes wrote:
> There are plenty of people today who treat the FPGA as an entirely
> dynamic resource. It's not like flashing a controller, its near
> immediate.

But this is a completely different use case. Remember, there are
*megabytes* of internal state in a FPGA, and it isn't really feasible
to dump/restore that state.

It is one thing to context switch a maths algorithm that is built to
be stateless, it is quite another to context switch between, say an
ethernet core with an operating Linux Net driver doing DMA and a maths
algorithm.

A DT overlay approach where the overlay has to be unloaded to 'free'
the FPGA makes alot of sense to me for the stateful kernel driver
environment, and open/close/etc makes alot of sense for the stateless
switchable userspace environment - other than sharing configuration
code, is there any overlap between these use cases????

> Its completely dynamic and it will get more so as we switch from the
> painful world of VHDL and friends to high level parallel aware language
> compilers for FPGAs and everyone will be knocking up quick FPGA hacks.

Only for some users. In my world FPGAs are filled with bus interface
logic, ethernet controllers, memory controllers, packet processing
engines, etc. This is all incredibly stateful - both in the FPGA
itself, and in the Linux side w/ drviers. It certainly will not ever
work in the model you are talking about.

Even if the digital state could somehow be frozen, dumped and
restored, all the FPGA external interface logic has *ANALOG* state
that cannot ever be dump/restored. It just isn't feasible for that
class of application.

Jason

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-12 13:48                 ` Michal Simek
@ 2015-01-13  7:28                   ` Pavel Machek
  2015-01-13  7:40                     ` Pantelis Antoniou
  0 siblings, 1 reply; 64+ messages in thread
From: Pavel Machek @ 2015-01-13  7:28 UTC (permalink / raw)
  To: Michal Simek
  Cc: atull, Pantelis Antoniou, Greg Kroah-Hartman, jgunthorpe, hpa,
	Michal Simek, rdunlap, Linux Kernel Mailing List, devicetree,
	robh+dt, Grant Likely, iws, linux-doc, Mark Brown, philip,
	rubini, Steffen Trumtrar, jason, kyle.teske, nico, Felipe Balbi,
	m.chehab, davidb, Rob Landley, davem, cesarb, sameo, akpm,
	Linus Walleij, pawel.moll, mark.rutland, ijc+devicetree, galak,
	devel, Alan Tull, dinguyen, yvanderv

Hi!

> >>>>>>>> +What:		/sys/class/fpga_manager/<fpga>/firmware
> >>>>>>>> +Date:		October 2014
> >>>>>>>> +KernelVersion:	3.18
> >>>>>>>> +Contact:	Alan Tull <atull@opensource.altera.com>
> >>>>>>>> +Description:	Name of the FPGA image file to load using firmware
> >>>>>>>> class.
> >>>>>>>
> >>>>>>> This one is ugly: it unneccessarily passes firmware name through the
> >>>>>>> kernel. Just make interface and code simpler by always passing
> >>>>>>> "socfpga-fpga-image" or something like that.
...
 
> > What is cumbersome about symlink? Why is "fake" symlink in sysfs better?
> > 
> >> Previous uses of the firmware layer has been to use it to load once after
> >> bootup; this is different since some use cases will want to switch out
> >> the FPGA image.  If someone wants there to be only one FPGA image on
> >> the FGPA forever, they will probably not be using this framework; their
> >> FPGA will probably be loaded before Linux boots up.
> > 
> > Why? I have just one image on the fpga, and would prefer to load it
> > from Linux.
> 
> Pavel: These patches target staging and sysfs interface doesn't need to be stable
> at this time. I would prefer to add these patches to staging for 3.20
> and feel free to send the patch which fix this.

Interesting way to address patch review. "We'll merge it, and you can
fix it up later".

> With your code will be exactly clear how you want to use it and we can
> talk about it.

I'm pretty sure Alan knows what I want at this point, he just does not
want to do it.

For the record, I want to drop "firmware" file, use fixed firmware
name, and deal with multiple firmwares in userspace (using symlink or
udev magic).

I believe this is simplest solution, should be adequate, and is
certainly less ugly than implementing fake symlink in
/sys/.../firmware. And I have yet to hear what is wrong with that
suggestion.

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

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-13  7:28                   ` Pavel Machek
@ 2015-01-13  7:40                     ` Pantelis Antoniou
  2015-01-13  7:56                       ` Pavel Machek
  0 siblings, 1 reply; 64+ messages in thread
From: Pantelis Antoniou @ 2015-01-13  7:40 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Michal Simek, atull, Greg Kroah-Hartman, jgunthorpe, hpa,
	Michal Simek, rdunlap, Linux Kernel Mailing List, devicetree,
	robh+dt, Grant Likely, iws, linux-doc, Mark Brown, philip,
	rubini, Steffen Trumtrar, jason, kyle.teske, nico, Felipe Balbi,
	m.chehab, davidb, Rob Landley, davem, cesarb, sameo, akpm,
	Linus Walleij, pawel.moll, mark.rutland, ijc+devicetree, galak,
	devel, Alan Tull, dinguyen, yvanderv

Hi Pavel,

> On Jan 13, 2015, at 09:28 , Pavel Machek <pavel@denx.de> wrote:
> 
> Hi!
> 
>>>>>>>>>> +What:		/sys/class/fpga_manager/<fpga>/firmware
>>>>>>>>>> +Date:		October 2014
>>>>>>>>>> +KernelVersion:	3.18
>>>>>>>>>> +Contact:	Alan Tull <atull@opensource.altera.com>
>>>>>>>>>> +Description:	Name of the FPGA image file to load using firmware
>>>>>>>>>> class.
>>>>>>>>> 
>>>>>>>>> This one is ugly: it unneccessarily passes firmware name through the
>>>>>>>>> kernel. Just make interface and code simpler by always passing
>>>>>>>>> "socfpga-fpga-image" or something like that.
> ...
> 
>>> What is cumbersome about symlink? Why is "fake" symlink in sysfs better?
>>> 
>>>> Previous uses of the firmware layer has been to use it to load once after
>>>> bootup; this is different since some use cases will want to switch out
>>>> the FPGA image.  If someone wants there to be only one FPGA image on
>>>> the FGPA forever, they will probably not be using this framework; their
>>>> FPGA will probably be loaded before Linux boots up.
>>> 
>>> Why? I have just one image on the fpga, and would prefer to load it
>>> from Linux.
>> 
>> Pavel: These patches target staging and sysfs interface doesn't need to be stable
>> at this time. I would prefer to add these patches to staging for 3.20
>> and feel free to send the patch which fix this.
> 
> Interesting way to address patch review. "We'll merge it, and you can
> fix it up later".
> 
>> With your code will be exactly clear how you want to use it and we can
>> talk about it.
> 
> I'm pretty sure Alan knows what I want at this point, he just does not
> want to do it.
> 
> For the record, I want to drop "firmware" file, use fixed firmware
> name, and deal with multiple firmwares in userspace (using symlink or
> udev magic).
> 

That’s completely bogus. Using a fixed firmware file does not respond to
real world usage, and there are no words to describe the hacks using
symlinks and udev in an embedded product.

> I believe this is simplest solution, should be adequate, and is
> certainly less ugly than implementing fake symlink in
> /sys/.../firmware. And I have yet to hear what is wrong with that
> suggestion.
> 

Everything.

> Thanks,									

Regards

— Pantelis

> 									Pavel
> -- 
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-13  7:40                     ` Pantelis Antoniou
@ 2015-01-13  7:56                       ` Pavel Machek
  2015-01-13 17:27                         ` atull
  0 siblings, 1 reply; 64+ messages in thread
From: Pavel Machek @ 2015-01-13  7:56 UTC (permalink / raw)
  To: Pantelis Antoniou
  Cc: Michal Simek, atull, Greg Kroah-Hartman, jgunthorpe, hpa,
	Michal Simek, rdunlap, Linux Kernel Mailing List, devicetree,
	robh+dt, Grant Likely, iws, linux-doc, Mark Brown, philip,
	rubini, Steffen Trumtrar, jason, kyle.teske, nico, Felipe Balbi,
	m.chehab, davidb, Rob Landley, davem, cesarb, sameo, akpm,
	Linus Walleij, pawel.moll, mark.rutland, ijc+devicetree, galak,
	devel, Alan Tull, dinguyen, yvanderv

On Tue 2015-01-13 09:40:18, Pantelis Antoniou wrote:
> Hi Pavel,
> 
> > On Jan 13, 2015, at 09:28 , Pavel Machek <pavel@denx.de> wrote:
> > 
> > Hi!
> > 
> >>>>>>>>>> +What:		/sys/class/fpga_manager/<fpga>/firmware
> >>>>>>>>>> +Date:		October 2014
> >>>>>>>>>> +KernelVersion:	3.18
> >>>>>>>>>> +Contact:	Alan Tull <atull@opensource.altera.com>
> >>>>>>>>>> +Description:	Name of the FPGA image file to load using firmware
> >>>>>>>>>> class.
> >>>>>>>>> 
> >>>>>>>>> This one is ugly: it unneccessarily passes firmware name through the
> >>>>>>>>> kernel. Just make interface and code simpler by always passing
> >>>>>>>>> "socfpga-fpga-image" or something like that.
> > ...
> > 
> >>> What is cumbersome about symlink? Why is "fake" symlink in sysfs better?
> >>> 
> >>>> Previous uses of the firmware layer has been to use it to load once after
> >>>> bootup; this is different since some use cases will want to switch out
> >>>> the FPGA image.  If someone wants there to be only one FPGA image on
> >>>> the FGPA forever, they will probably not be using this framework; their
> >>>> FPGA will probably be loaded before Linux boots up.
> >>> 
> >>> Why? I have just one image on the fpga, and would prefer to load it
> >>> from Linux.
> >> 
> >> Pavel: These patches target staging and sysfs interface doesn't need to be stable
> >> at this time. I would prefer to add these patches to staging for 3.20
> >> and feel free to send the patch which fix this.
> > 
> > Interesting way to address patch review. "We'll merge it, and you can
> > fix it up later".
> > 
> >> With your code will be exactly clear how you want to use it and we can
> >> talk about it.
> > 
> > I'm pretty sure Alan knows what I want at this point, he just does not
> > want to do it.
> > 
> > For the record, I want to drop "firmware" file, use fixed firmware
> > name, and deal with multiple firmwares in userspace (using symlink or
> > udev magic).
> 
> That’s completely bogus. Using a fixed firmware file does not respond to
> real world usage, and there are no words to describe the hacks using
> symlinks and udev in an embedded product.

> > I believe this is simplest solution, should be adequate, and is
> > certainly less ugly than implementing fake symlink in
> > /sys/.../firmware. And I have yet to hear what is wrong with that
> > suggestion.
> 
> Everything.

Can you be more specific? Current solution already has a fake symlink,
implemented badly.

Just remove it, and use real symlink (and then you can optionally
think of something better...)


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

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-12 18:06               ` Jason Gunthorpe
@ 2015-01-13 16:21                 ` One Thousand Gnomes
  2015-01-15 21:52                   ` Pavel Machek
  0 siblings, 1 reply; 64+ messages in thread
From: One Thousand Gnomes @ 2015-01-13 16:21 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: atull, Pavel Machek, Pantelis Antoniou, Greg Kroah-Hartman, hpa,
	Michal Simek, Michal Simek, rdunlap, Linux Kernel Mailing List,
	devicetree, robh+dt, Grant Likely, iws, linux-doc, Mark Brown,
	philip, rubini, Steffen Trumtrar, jason, kyle.teske, nico,
	Felipe Balbi, m.chehab, davidb, Rob Landley, davem, cesarb,
	sameo, akpm, Linus Walleij, pawel.moll, mark.rutland,
	ijc+devicetree, galak, devel, Alan Tull, dinguyen, yvanderv

On Mon, 12 Jan 2015 11:06:08 -0700
Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:

> On Sun, Jan 11, 2015 at 10:29:00AM -0600, atull wrote:
> > the FPGA image.  If someone wants there to be only one FPGA image on
> > the FGPA forever, they will probably not be using this framework; their
> > FPGA will probably be loaded before Linux boots up.
> 
> Nonsense, loading the FPGA through Linux is much better

You need both. There are platforms where the FPGA must be loaded to even
boot the OS (for example Linux running an FPGA soft 486 can't run until
the FPGA has the CPU loaded into it.


^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-12 21:43           ` Jason Gunthorpe
@ 2015-01-13 16:28             ` One Thousand Gnomes
  2015-01-13 17:26               ` Pantelis Antoniou
  2015-01-13 20:00               ` Jason Gunthorpe
  0 siblings, 2 replies; 64+ messages in thread
From: One Thousand Gnomes @ 2015-01-13 16:28 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Pavel Machek, atull, gregkh, hpa, monstr, michal.simek, rdunlap,
	linux-kernel, devicetree, pantelis.antoniou, robh+dt,
	grant.likely, iws, linux-doc, broonie, philip, rubini,
	s.trumtrar, jason, kyle.teske, nico, balbi, m.chehab, davidb,
	rob, davem, cesarb, sameo, akpm, linus.walleij, pawel.moll,
	mark.rutland, ijc+devicetree, galak, devel, delicious.quinoa,
	dinguyen, yvanderv

On Mon, 12 Jan 2015 14:43:14 -0700
Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:

> On Mon, Jan 12, 2015 at 09:01:34PM +0000, One Thousand Gnomes wrote:
> > There are plenty of people today who treat the FPGA as an entirely
> > dynamic resource. It's not like flashing a controller, its near
> > immediate.
> 
> But this is a completely different use case. Remember, there are
> *megabytes* of internal state in a FPGA, and it isn't really feasible
> to dump/restore that state.

People already do it with cases where it makes sense

> It is one thing to context switch a maths algorithm that is built to
> be stateless, it is quite another to context switch between, say an
> ethernet core with an operating Linux Net driver doing DMA and a maths
> algorithm.

And people already do it with thins like maths algorithms.
 
> A DT overlay approach where the overlay has to be unloaded to 'free'
> the FPGA makes alot of sense to me for the stateful kernel driver
> environment, and open/close/etc makes alot of sense for the stateless
> switchable userspace environment - other than sharing configuration
> code, is there any overlap between these use cases????

There is a lot of code overlap in things like loading the bitstreams,
there is also some overlap because you want to be able to assign FPGA
resources. For example if you have four FPGAs and you want to use one for
OS stuff (say video) you want the other three to be open/close
accessible, but if you've not got a video driver loaded and are running
the same board headless you'd like all four to be handed out as normal
resources.

So IMHO it's no different to say kmalloc. We don't pre-empt kernel memory
and give it to users but we don't reserve memory for kernel and not use
it.

> > Its completely dynamic and it will get more so as we switch from the
> > painful world of VHDL and friends to high level parallel aware language
> > compilers for FPGAs and everyone will be knocking up quick FPGA hacks.
> 
> Only for some users. In my world FPGAs are filled with bus interface
> logic, ethernet controllers, memory controllers, packet processing
> engines, etc. This is all incredibly stateful - both in the FPGA
> itself, and in the Linux side w/ drviers. It certainly will not ever
> work in the model you are talking about.

For those cases I mostly agree (the state in the FPGA isn't always a
problem as you can program an FPGA to DMA its state out).

> Even if the digital state could somehow be frozen, dumped and
> restored, all the FPGA external interface logic has *ANALOG* state
> that cannot ever be dump/restored. It just isn't feasible for that
> class of application.

Yes, however as we are starting to get things like OpenCL for FPGA they
become extremely attractive for a wide range of purposes that don't
involve glueing them to electronica in quite the same way. All the 'GPU
as CPU' type uses and more begin to apply.

Alan

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-13 16:28             ` One Thousand Gnomes
@ 2015-01-13 17:26               ` Pantelis Antoniou
  2015-01-13 19:44                 ` atull
  2015-01-14 15:58                 ` One Thousand Gnomes
  2015-01-13 20:00               ` Jason Gunthorpe
  1 sibling, 2 replies; 64+ messages in thread
From: Pantelis Antoniou @ 2015-01-13 17:26 UTC (permalink / raw)
  To: One Thousand Gnomes
  Cc: Jason Gunthorpe, Pavel Machek, atull, Greg Kroah-Hartman, hpa,
	Michal Simek, Michal Simek, rdunlap, Linux Kernel Mailing List,
	devicetree, robh+dt, Grant Likely, iws, linux-doc, Mark Brown,
	philip, rubini, Steffen Trumtrar, jason, kyle.teske, nico,
	Felipe Balbi, m.chehab, davidb, Rob Landley, davem, cesarb,
	sameo, akpm, Linus Walleij, pawel.moll, mark.rutland,
	ijc+devicetree, galak, devel, Alan Tull, dinguyen, yvanderv

Hi Alan,

> On Jan 13, 2015, at 18:28 , One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> wrote:
> 
> On Mon, 12 Jan 2015 14:43:14 -0700
> Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:
> 
>> On Mon, Jan 12, 2015 at 09:01:34PM +0000, One Thousand Gnomes wrote:
>>> There are plenty of people today who treat the FPGA as an entirely
>>> dynamic resource. It's not like flashing a controller, its near
>>> immediate.
>> 
>> But this is a completely different use case. Remember, there are
>> *megabytes* of internal state in a FPGA, and it isn't really feasible
>> to dump/restore that state.
> 
> People already do it with cases where it makes sense
> 

Those people have failed to show up and provide input and/or code.

>> It is one thing to context switch a maths algorithm that is built to
>> be stateless, it is quite another to context switch between, say an
>> ethernet core with an operating Linux Net driver doing DMA and a maths
>> algorithm.
> 
> And people already do it with thins like maths algorithms.
> 

Again those people have failed to show up and provide input and/or code.

>> A DT overlay approach where the overlay has to be unloaded to 'free'
>> the FPGA makes alot of sense to me for the stateful kernel driver
>> environment, and open/close/etc makes alot of sense for the stateless
>> switchable userspace environment - other than sharing configuration
>> code, is there any overlap between these use cases????
> 
> There is a lot of code overlap in things like loading the bitstreams,
> there is also some overlap because you want to be able to assign FPGA
> resources. For example if you have four FPGAs and you want to use one for
> OS stuff (say video) you want the other three to be open/close
> accessible, but if you've not got a video driver loaded and are running
> the same board headless you'd like all four to be handed out as normal
> resources.
> 
> So IMHO it's no different to say kmalloc. We don't pre-empt kernel memory
> and give it to users but we don't reserve memory for kernel and not use
> it.
> 
>>> Its completely dynamic and it will get more so as we switch from the
>>> painful world of VHDL and friends to high level parallel aware language
>>> compilers for FPGAs and everyone will be knocking up quick FPGA hacks.
>> 
>> Only for some users. In my world FPGAs are filled with bus interface
>> logic, ethernet controllers, memory controllers, packet processing
>> engines, etc. This is all incredibly stateful - both in the FPGA
>> itself, and in the Linux side w/ drviers. It certainly will not ever
>> work in the model you are talking about.
> 
> For those cases I mostly agree (the state in the FPGA isn't always a
> problem as you can program an FPGA to DMA its state out).
> 
>> Even if the digital state could somehow be frozen, dumped and
>> restored, all the FPGA external interface logic has *ANALOG* state
>> that cannot ever be dump/restored. It just isn't feasible for that
>> class of application.
> 
> Yes, however as we are starting to get things like OpenCL for FPGA they
> become extremely attractive for a wide range of purposes that don't
> involve glueing them to electronica in quite the same way. All the 'GPU
> as CPU' type uses and more begin to apply.
> 

Your points are valid, but they are mostly academic right now.

We have a lot of well known use cases that this patchset addresses
satisfactorily. The possible users of the cases you point out are 
welcome to provide input and working code to address their needs.

At this point in time, no-one has showed up; is there a reason for
this needed capability to be delayed in order to address something
that no-one has expressed interest for?

> Alan
> —

Regards

— Pantelis


^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-13  7:56                       ` Pavel Machek
@ 2015-01-13 17:27                         ` atull
  0 siblings, 0 replies; 64+ messages in thread
From: atull @ 2015-01-13 17:27 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Pantelis Antoniou, Michal Simek, Greg Kroah-Hartman, jgunthorpe,
	hpa, Michal Simek, rdunlap, Linux Kernel Mailing List,
	devicetree, robh+dt, Grant Likely, iws, linux-doc, Mark Brown,
	philip, rubini, Steffen Trumtrar, jason, kyle.teske, nico,
	Felipe Balbi, m.chehab, davidb, Rob Landley, davem, cesarb,
	sameo, akpm, Linus Walleij, pawel.moll, mark.rutland,
	ijc+devicetree, galak, devel, Alan Tull, dinguyen, yvanderv

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

On Tue, 13 Jan 2015, Pavel Machek wrote:

> On Tue 2015-01-13 09:40:18, Pantelis Antoniou wrote:
> > Hi Pavel,
> > 
> > > On Jan 13, 2015, at 09:28 , Pavel Machek <pavel@denx.de> wrote:
> > > 
> > > Hi!
> > > 
> > >>>>>>>>>> +What:		/sys/class/fpga_manager/<fpga>/firmware
> > >>>>>>>>>> +Date:		October 2014
> > >>>>>>>>>> +KernelVersion:	3.18
> > >>>>>>>>>> +Contact:	Alan Tull <atull@opensource.altera.com>
> > >>>>>>>>>> +Description:	Name of the FPGA image file to load using firmware
> > >>>>>>>>>> class.
> > >>>>>>>>> 
> > >>>>>>>>> This one is ugly: it unneccessarily passes firmware name through the
> > >>>>>>>>> kernel. Just make interface and code simpler by always passing
> > >>>>>>>>> "socfpga-fpga-image" or something like that.
> > > ...
> > > 
> > >>> What is cumbersome about symlink? Why is "fake" symlink in sysfs better?
> > >>> 
> > >>>> Previous uses of the firmware layer has been to use it to load once after
> > >>>> bootup; this is different since some use cases will want to switch out
> > >>>> the FPGA image.  If someone wants there to be only one FPGA image on
> > >>>> the FGPA forever, they will probably not be using this framework; their
> > >>>> FPGA will probably be loaded before Linux boots up.
> > >>> 
> > >>> Why? I have just one image on the fpga, and would prefer to load it
> > >>> from Linux.
> > >> 
> > >> Pavel: These patches target staging and sysfs interface doesn't need to be stable
> > >> at this time. I would prefer to add these patches to staging for 3.20
> > >> and feel free to send the patch which fix this.
> > > 
> > > Interesting way to address patch review. "We'll merge it, and you can
> > > fix it up later".
> > > 
> > >> With your code will be exactly clear how you want to use it and we can
> > >> talk about it.
> > > 
> > > I'm pretty sure Alan knows what I want at this point, he just does not
> > > want to do it.
> > > 
> > > For the record, I want to drop "firmware" file, use fixed firmware
> > > name, and deal with multiple firmwares in userspace (using symlink or
> > > udev magic).
> > 
> > That’s completely bogus. Using a fixed firmware file does not respond to
> > real world usage, and there are no words to describe the hacks using
> > symlinks and udev in an embedded product.
> 
> > > I believe this is simplest solution, should be adequate, and is
> > > certainly less ugly than implementing fake symlink in
> > > /sys/.../firmware. And I have yet to hear what is wrong with that
> > > suggestion.
> > 
> > Everything.
> 
> Can you be more specific? Current solution already has a fake symlink,
> implemented badly.
> 
> Just remove it, and use real symlink (and then you can optionally
> think of something better...)
> 

Hi Pavel,

I still don't get what you are saying.  It's not a symlink.  I'm not
changing it.  

Alan


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

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-13 17:26               ` Pantelis Antoniou
@ 2015-01-13 19:44                 ` atull
  2015-01-14 15:58                 ` One Thousand Gnomes
  1 sibling, 0 replies; 64+ messages in thread
From: atull @ 2015-01-13 19:44 UTC (permalink / raw)
  To: Pantelis Antoniou
  Cc: One Thousand Gnomes, Jason Gunthorpe, Pavel Machek,
	Greg Kroah-Hartman, hpa, Michal Simek, Michal Simek, rdunlap,
	Linux Kernel Mailing List, devicetree, robh+dt, Grant Likely,
	iws, linux-doc, Mark Brown, philip, rubini, Steffen Trumtrar,
	jason, kyle.teske, nico, Felipe Balbi, m.chehab, davidb,
	Rob Landley, davem, cesarb, sameo, akpm, Linus Walleij,
	pawel.moll, mark.rutland, ijc+devicetree, galak, devel,
	Alan Tull, dinguyen, yvanderv

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

On Tue, 13 Jan 2015, Pantelis Antoniou wrote:

> Hi Alan,
> 
> > On Jan 13, 2015, at 18:28 , One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> wrote:
> > 
> > On Mon, 12 Jan 2015 14:43:14 -0700
> > Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:
> > 
> >> On Mon, Jan 12, 2015 at 09:01:34PM +0000, One Thousand Gnomes wrote:
> >>> There are plenty of people today who treat the FPGA as an entirely
> >>> dynamic resource. It's not like flashing a controller, its near
> >>> immediate.
> >> 
> >> But this is a completely different use case. Remember, there are
> >> *megabytes* of internal state in a FPGA, and it isn't really feasible
> >> to dump/restore that state.
> > 
> > People already do it with cases where it makes sense
> > 
> 
> Those people have failed to show up and provide input and/or code.
> 
> >> It is one thing to context switch a maths algorithm that is built to
> >> be stateless, it is quite another to context switch between, say an
> >> ethernet core with an operating Linux Net driver doing DMA and a maths
> >> algorithm.
> > 
> > And people already do it with thins like maths algorithms.
> > 
> 
> Again those people have failed to show up and provide input and/or code.
> 
> >> A DT overlay approach where the overlay has to be unloaded to 'free'
> >> the FPGA makes alot of sense to me for the stateful kernel driver
> >> environment, and open/close/etc makes alot of sense for the stateless
> >> switchable userspace environment - other than sharing configuration
> >> code, is there any overlap between these use cases????
> > 
> > There is a lot of code overlap in things like loading the bitstreams,
> > there is also some overlap because you want to be able to assign FPGA
> > resources. For example if you have four FPGAs and you want to use one for
> > OS stuff (say video) you want the other three to be open/close
> > accessible, but if you've not got a video driver loaded and are running
> > the same board headless you'd like all four to be handed out as normal
> > resources.
> > 
> > So IMHO it's no different to say kmalloc. We don't pre-empt kernel memory
> > and give it to users but we don't reserve memory for kernel and not use
> > it.
> > 
> >>> Its completely dynamic and it will get more so as we switch from the
> >>> painful world of VHDL and friends to high level parallel aware language
> >>> compilers for FPGAs and everyone will be knocking up quick FPGA hacks.
> >> 
> >> Only for some users. In my world FPGAs are filled with bus interface
> >> logic, ethernet controllers, memory controllers, packet processing
> >> engines, etc. This is all incredibly stateful - both in the FPGA
> >> itself, and in the Linux side w/ drviers. It certainly will not ever
> >> work in the model you are talking about.
> > 
> > For those cases I mostly agree (the state in the FPGA isn't always a
> > problem as you can program an FPGA to DMA its state out).
> > 
> >> Even if the digital state could somehow be frozen, dumped and
> >> restored, all the FPGA external interface logic has *ANALOG* state
> >> that cannot ever be dump/restored. It just isn't feasible for that
> >> class of application.
> > 
> > Yes, however as we are starting to get things like OpenCL for FPGA they
> > become extremely attractive for a wide range of purposes that don't
> > involve glueing them to electronica in quite the same way. All the 'GPU
> > as CPU' type uses and more begin to apply.
> > 
> 
> Your points are valid, but they are mostly academic right now.
> 
> We have a lot of well known use cases that this patchset addresses
> satisfactorily. The possible users of the cases you point out are 
> welcome to provide input and working code to address their needs.
> 
> At this point in time, no-one has showed up; is there a reason for
> this needed capability to be delayed in order to address something
> that no-one has expressed interest for?
> 
> > Alan
> > —
> 
> Regards
> 
> — Pantelis
> 
> 

There has been a lot of discussion in the past.  It is possible that
some of the use models are conflicting, but that doesn't really mean
that one model is more valid and needs to be the
winner that determines the only valid way to use FPGAs under Linux.

I like the dynamic usage model and the DT overlays model both.  It
is likely that both will be added and used.

This set of patches gets something started.  The sysfs interface
is built on a set of functions that can be used to build other
interfaces.  An earlier set of patches included DT overlay support,
but I've taken that out for the moment.  It's going into
drivers/staging so that we can use it and grow it.

Alan

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-13 16:28             ` One Thousand Gnomes
  2015-01-13 17:26               ` Pantelis Antoniou
@ 2015-01-13 20:00               ` Jason Gunthorpe
  2015-01-13 21:37                 ` atull
  1 sibling, 1 reply; 64+ messages in thread
From: Jason Gunthorpe @ 2015-01-13 20:00 UTC (permalink / raw)
  To: One Thousand Gnomes
  Cc: Pavel Machek, atull, gregkh, hpa, monstr, michal.simek, rdunlap,
	linux-kernel, devicetree, pantelis.antoniou, robh+dt,
	grant.likely, iws, linux-doc, broonie, philip, rubini,
	s.trumtrar, jason, kyle.teske, nico, balbi, m.chehab, davidb,
	rob, davem, cesarb, sameo, akpm, linus.walleij, pawel.moll,
	mark.rutland, ijc+devicetree, galak, devel, delicious.quinoa,
	dinguyen, yvanderv

On Tue, Jan 13, 2015 at 04:28:47PM +0000, One Thousand Gnomes wrote:

> There is a lot of code overlap in things like loading the bitstreams,
> there is also some overlap because you want to be able to assign FPGA
> resources. For example if you have four FPGAs and you want to use one for
> OS stuff (say video) you want the other three to be open/close
> accessible, but if you've not got a video driver loaded and are running
> the same board headless you'd like all four to be handed out as normal
> resources.
>
> So IMHO it's no different to say kmalloc. We don't pre-empt kernel memory
> and give it to users but we don't reserve memory for kernel and not use
> it.

I do agree with this, and I think this is where this patch set goes so
wrong.

Just exposing all sorts of controls to userspace and having a way for
the kernel to load/unload a bitstream compleletely misses the point that
the two main ways to use FPGAs *require* some kind of kernel side
driver suppport on the FPGA.

Even if it is just a maths FPGA kernel side has to be involved in some
way to restrict DMAs, MMIO, by the user process. Obviously a FPGA that
uses kernel drivers needs kernel side help to bind those drivers.

Plonking /sys/class/fpga_manager/<fpga>/reset for either case is more
likely than not to completely crash the system.

I would look at this problem from a completely different angle - the
90% case is a single FPGA that is loaded to provide HW that Linux
drivers bind to, these days the majority use DT.

So..

soc
{
    zynq_cfg: ps7-dev-cfg@f8007000 { compatible = "xlnx,zynq-devcfg-1.0"; }
    
    fpga@pl {
       compatible = "..";
       fpga,api = "gpio-api1";
       fpga,controller = &zynq_cfg;

       gp0_axi: axi@40000000 {
         gpio3: klina_gpio@80010 {
                 #gpio-cells = <2>;
                 compatible = "linux,basic-mmio-gpio";
                 gpio-controller;
                 reg-names = "dat", "set", "dirin";
                 reg = <0x80010 4>,
                       <0x80014 4>,
                       <0x80018 4>;
         };
       }
    }  
}

Make it so linux can boot, automatically fetch the gpio-api1 bit file
and load it into the chip and then bind the GPIO driver to the FPGA
resource. All without userspace involvment, all potentially done
before calling INIT.

Make it so via sysfs we can reverse this process and reboot the FPGA.

Make it so userspace can't do something to the FPGA without first
unloading the drivers.

Then worry about how to change the FPGA, or providing more user space
control over the process (DT overlays are a natural answer).

Providing a roll-your-own approach via a bunch of sysfs controls
satisfies the 'make the HW work' need without actually providing the
overall architecture someone needs to *make use* of this mechanism.

And no, this isn't just a 'theory', I have 4 shipping products today
that work pretty much exactly like this, including a Zynq
design. Binding kernel drivers to a FPGA once it is loaded is a big
functionality gap, and an annoying problem. Programming the actual
FPGA? That is the *EASIEST* part of this whole thing!

Honestly, I'd much rather see the above use case solved properly
without sysfs support and then go from there to build a replacable
FPGA scheme on the same concepts.

Providing a 'build your own' solution with sysfs controls just seems
to completely miss the mark to me.

> So IMHO it's no different to say kmalloc. We don't pre-empt kernel memory
> and give it to users but we don't reserve memory for kernel and not use
> it.

I think we see a kernel side with more structure, that knows when the
FPGA is in use or not, there is a pretty clear path to later add a
/dev/ scheme for user space use that can properly interlock.

A /dev/ scheme doesn't make much sense to bind kernel drivers...

Jason

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-13 20:00               ` Jason Gunthorpe
@ 2015-01-13 21:37                 ` atull
  2015-01-13 22:24                   ` Jason Gunthorpe
  0 siblings, 1 reply; 64+ messages in thread
From: atull @ 2015-01-13 21:37 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: One Thousand Gnomes, Pavel Machek, gregkh, hpa, monstr,
	michal.simek, rdunlap, linux-kernel, devicetree,
	pantelis.antoniou, robh+dt, grant.likely, iws, linux-doc,
	broonie, philip, rubini, s.trumtrar, jason, kyle.teske, nico,
	balbi, m.chehab, davidb, rob, davem, cesarb, sameo, akpm,
	linus.walleij, pawel.moll, mark.rutland, ijc+devicetree, galak,
	devel, delicious.quinoa, dinguyen, yvanderv

On Tue, 13 Jan 2015, Jason Gunthorpe wrote:

> On Tue, Jan 13, 2015 at 04:28:47PM +0000, One Thousand Gnomes wrote:
> 
> > There is a lot of code overlap in things like loading the bitstreams,
> > there is also some overlap because you want to be able to assign FPGA
> > resources. For example if you have four FPGAs and you want to use one for
> > OS stuff (say video) you want the other three to be open/close
> > accessible, but if you've not got a video driver loaded and are running
> > the same board headless you'd like all four to be handed out as normal
> > resources.
> >
> > So IMHO it's no different to say kmalloc. We don't pre-empt kernel memory
> > and give it to users but we don't reserve memory for kernel and not use
> > it.
> 
> I do agree with this, and I think this is where this patch set goes so
> wrong.
> 
> Just exposing all sorts of controls to userspace and having a way for
> the kernel to load/unload a bitstream compleletely misses the point that
> the two main ways to use FPGAs *require* some kind of kernel side
> driver suppport on the FPGA.

Hi Jason,

I may be misunderstanding.  I thought you wanted lots of user control
a couple years ago.

https://lkml.org/lkml/2013/9/18/490

Your feedback carried a lot of weight as I've been developing this patchset
although I didn't do absolutely everything you asked for.  Back then you
were asking for lots of user control including being able to reset the FPGA
and being able to know from userspace what configuration steps failed (if any).

I'm glad you like DT support for FPGA images + drivers loading since I've
been pushing that since April and have submitted several earlier versions
of this patchset which included it.   I think your arguements against the
current patchset are a bit over the top here.  These patches aren't going to
keep DT support from happening.

> 
> Even if it is just a maths FPGA kernel side has to be involved in some
> way to restrict DMAs, MMIO, by the user process. Obviously a FPGA that
> uses kernel drivers needs kernel side help to bind those drivers.
> 
> Plonking /sys/class/fpga_manager/<fpga>/reset for either case is more
> likely than not to completely crash the system.
> 
> I would look at this problem from a completely different angle - the
> 90% case is a single FPGA that is loaded to provide HW that Linux
> drivers bind to, these days the majority use DT.
> 
> So..
> 
> soc
> {
>     zynq_cfg: ps7-dev-cfg@f8007000 { compatible = "xlnx,zynq-devcfg-1.0"; }
>     
>     fpga@pl {
>        compatible = "..";
>        fpga,api = "gpio-api1";
>        fpga,controller = &zynq_cfg;
> 
>        gp0_axi: axi@40000000 {
>          gpio3: klina_gpio@80010 {
>                  #gpio-cells = <2>;
>                  compatible = "linux,basic-mmio-gpio";
>                  gpio-controller;
>                  reg-names = "dat", "set", "dirin";
>                  reg = <0x80010 4>,
>                        <0x80014 4>,
>                        <0x80018 4>;
>          };
>        }
>     }  
> }
> 
> Make it so linux can boot, automatically fetch the gpio-api1 bit file
> and load it into the chip and then bind the GPIO driver to the FPGA
> resource. All without userspace involvment, all potentially done
> before calling INIT.

Where would the fpga image be fetch from?  What is the mechanism for
doing that?  Do we need to reqire everybody to do that or can
firmware be one of the available mechanisms?

> 
> Make it so via sysfs we can reverse this process and reboot the FPGA.

Better to use the device tree's configfs interface if you are going
to add/delete a DT overlay.  Adding an overlay could load the fpga,
enable bridges, and load driver(s).  Some fpga images may have several
hardware blocks so several drivers to load.

> 
> Make it so userspace can't do something to the FPGA without first
> unloading the drivers.

If we do this using DT overlays, deleting the overlay would result
in the driver getting unloaded first.

> 
> Then worry about how to change the FPGA, or providing more user space
> control over the process (DT overlays are a natural answer).
> 
> Providing a roll-your-own approach via a bunch of sysfs controls
> satisfies the 'make the HW work' need without actually providing the
> overall architecture someone needs to *make use* of this mechanism.
> 
> And no, this isn't just a 'theory', I have 4 shipping products today
> that work pretty much exactly like this, including a Zynq
> design. Binding kernel drivers to a FPGA once it is loaded is a big
> functionality gap, and an annoying problem. Programming the actual
> FPGA? That is the *EASIEST* part of this whole thing!
> 

That's why I'm trying to get programming the fpga out of the way here.

> Honestly, I'd much rather see the above use case solved properly
> without sysfs support and then go from there to build a replacable
> FPGA scheme on the same concepts.
> 
> Providing a 'build your own' solution with sysfs controls just seems
> to completely miss the mark to me.
> 
> > So IMHO it's no different to say kmalloc. We don't pre-empt kernel memory
> > and give it to users but we don't reserve memory for kernel and not use
> > it.
> 
> I think we see a kernel side with more structure, that knows when the
> FPGA is in use or not, there is a pretty clear path to later add a
> /dev/ scheme for user space use that can properly interlock.
> 
> A /dev/ scheme doesn't make much sense to bind kernel drivers...
> 
> Jason
> --
> 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/
> 

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-13 21:37                 ` atull
@ 2015-01-13 22:24                   ` Jason Gunthorpe
  2015-01-14 16:06                     ` One Thousand Gnomes
  2015-01-15 16:34                     ` atull
  0 siblings, 2 replies; 64+ messages in thread
From: Jason Gunthorpe @ 2015-01-13 22:24 UTC (permalink / raw)
  To: atull
  Cc: One Thousand Gnomes, Pavel Machek, gregkh, hpa, monstr,
	michal.simek, rdunlap, linux-kernel, devicetree,
	pantelis.antoniou, robh+dt, grant.likely, iws, linux-doc,
	broonie, philip, rubini, s.trumtrar, jason, kyle.teske, nico,
	balbi, m.chehab, davidb, rob, davem, cesarb, sameo, akpm,
	linus.walleij, pawel.moll, mark.rutland, ijc+devicetree, galak,
	devel, delicious.quinoa, dinguyen, yvanderv

On Tue, Jan 13, 2015 at 03:37:14PM -0600, atull wrote:

> > I do agree with this, and I think this is where this patch set goes so
> > wrong.
> > 
> > Just exposing all sorts of controls to userspace and having a way for
> > the kernel to load/unload a bitstream compleletely misses the point that
> > the two main ways to use FPGAs *require* some kind of kernel side
> > driver suppport on the FPGA.
> 
> Hi Jason,
> 
> I may be misunderstanding.  I thought you wanted lots of user control
> a couple years ago.
> 
> https://lkml.org/lkml/2013/9/18/490

Yes, that is absolutely true, that is certainly where my projects have
been historically, and in the context of requiring user space to
manage the whole process I think that is completely right.

But - I like Alan's point - this should be a higher level thing, the
kernel should be able to load the FPGA, and handle binding the drivers
so user space doesn't need to be involved, and it makes sense that
should be the place to start the API design.

> Your feedback carried a lot of weight as I've been developing this patchset
> although I didn't do absolutely everything you asked for.  Back then you
> were asking for lots of user control including being able to reset the FPGA
> and being able to know from userspace what configuration steps failed (if any).

Yes, this is all true. Since my FPGAs all seem to require a software
mediated startup sequence the dream of having a DT scheme like I
described is farther away for me, but I think that is not the 90%
case. Almost all vendor IP designs do not require a SW startup and
auto-start after programming. 90% of designs can program and then
immediately connect drivers.

> I'm glad you like DT support for FPGA images + drivers loading since I've
> been pushing that since April and have submitted several earlier versions
> of this patchset which included it.

Yes, I do, thank you for working on this!

> I think your arguements against the current patchset are a bit over
> the top here.

Perhaps, and I apologize. But as you know I've never liked the sysfs
interface, and I think Pavel is right. catting a file name just so you
can call request_firmware on that file in the kernel is a bad design.

The request_firmware interface should be for the DT overlay path, and
other schemes shouldn't use it. The name should come from the DT and
no place else.

> > Make it so linux can boot, automatically fetch the gpio-api1 bit file
> > and load it into the chip and then bind the GPIO driver to the FPGA
> > resource. All without userspace involvment, all potentially done
> > before calling INIT.
> 
> Where would the fpga image be fetch from?  What is the mechanism for
> doing that?  Do we need to reqire everybody to do that or can
> firmware be one of the available mechanisms?

I see there are three options:
 1) The bootloader programs the FPGA and passes a DT to the kernel
    that completely describes the hard and soft logic, the kernel
    doesn't need FPGA bitfile management code or other special
    support.

    Obviously things like reprogram on resume have to be done by the
    bootloader, there is no handoff here where the kernel takes over
    control of the bitfile programming. I think that is fine, #2/#3
    are better choices for most people anyhow.
 2) The bootloader starts the kernel and passes a DT that describes
    the hard logic and soft logic using a scheme like I outlined. The
    kernel is responsible to request_firmware the bitfile and start
    the FPGA and connect the drivers.

    DT purists will rightly point out that this isn't great, but it
    is a simple and pragmatic solution.

    This probably falls out automatically if #3 is implemented..
 3) The bootloader starts the kernel with a DT that only describes the
    hard logic.

    Userspace must load a DT overlay early on. Otherwise proceeds like #2.

    In both 2 and 3 the FPGA can be reprogrammed on resume by re-doing
    request_firmware.

And for folks that need more control, expose all the knobs explicitly
to user space:
  4) There is a /dev/ node for the fpga that allows
     - ioctl to program via mmap'd file (no request_firmware)

       The ioctl has an option for the kernel to hang on to the mmap
       for the repogram on resume case.
     - ioctl to get status/error reporting/etc
     - ioctl to load/bind a DT overlay to the FPGA instance
       This provides a gap where userspace can boot and configure the
       FPGA internals before binding the kernel drivers.
       (end result is the same as #3 case, but no request_firmware)
     - (future) ioctl to load a swappable FPGA (what Alan is talking
       about)

The key thing is that we have a FPGA object that can be associated
with some DT element, and the kernel can then keep track if the FPGA
is is in use or not. Exactly like you said in your reply.

Otherwise, for the /dev/ case the FPGA becomes unuse once the FD is
closed if it wasn't attached to an overlay. This is at least the start
of addressing the space Alan is talking about.

If this sort of world is the goal, it is hard for me to see how the
proposed sysfs interface fits into that long term.

The /dev/ interface is probably a better place to start talking about
partial reconfiguration as well. The fixed part might be bound to a
kernel driver (eg the PCI-E interface) and the reconfigurable part
might be a maths algorithm - for instance.

> > Make it so via sysfs we can reverse this process and reboot the FPGA.
> 
> Better to use the device tree's configfs interface if you are going
> to add/delete a DT overlay.  Adding an overlay could load the fpga,
> enable bridges, and load driver(s).  Some fpga images may have several
> hardware blocks so several drivers to load.

Yes, absolutely!

> > And no, this isn't just a 'theory', I have 4 shipping products today
> > that work pretty much exactly like this, including a Zynq
> > design. Binding kernel drivers to a FPGA once it is loaded is a big
> > functionality gap, and an annoying problem. Programming the actual
> > FPGA? That is the *EASIEST* part of this whole thing!

> That's why I'm trying to get programming the fpga out of the way here.

I have no problem with the in kernel stuff, and I'd have no problem if
the sysfs controls were in debugfs for testing purposes. It just
doesn't make sense to me to expose that kind of interface as a
permanent API...
 
Jason

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-13 17:26               ` Pantelis Antoniou
  2015-01-13 19:44                 ` atull
@ 2015-01-14 15:58                 ` One Thousand Gnomes
  1 sibling, 0 replies; 64+ messages in thread
From: One Thousand Gnomes @ 2015-01-14 15:58 UTC (permalink / raw)
  To: Pantelis Antoniou
  Cc: Jason Gunthorpe, Pavel Machek, atull, Greg Kroah-Hartman, hpa,
	Michal Simek, Michal Simek, rdunlap, Linux Kernel Mailing List,
	devicetree, robh+dt, Grant Likely, iws, linux-doc, Mark Brown,
	philip, rubini, Steffen Trumtrar, jason, kyle.teske, nico,
	Felipe Balbi, m.chehab, davidb, Rob Landley, davem, cesarb,
	sameo, akpm, Linus Walleij, pawel.moll, mark.rutland,
	ijc+devicetree, galak, devel, Alan Tull, dinguyen, yvanderv

> Those people have failed to show up and provide input and/or code.

That doesn't excuse failing to design the code properly.

> 
> >> It is one thing to context switch a maths algorithm that is built to
> >> be stateless, it is quite another to context switch between, say an
> >> ethernet core with an operating Linux Net driver doing DMA and a maths
> >> algorithm.
> > 
> > And people already do it with thins like maths algorithms.
> > 
> 
> Again those people have failed to show up and provide input and/or code.

Actually there is a whole academic world out there, lots of papers and
research OS code including Linux based bits. 

> We have a lot of well known use cases that this patchset addresses
> satisfactorily. The possible users of the cases you point out are 
> welcome to provide input and working code to address their needs.

So you want to shove crap in the kernel with a broken ABI someone (no
doubt not you) will have to keep maintained forever ?
 
> At this point in time, no-one has showed up; is there a reason for
> this needed capability to be delayed in order to address something
> that no-one has expressed interest for?

I'm not suggesting it's delayed - I'm suggesting its designed properly in
the first place.

We know the kind of use cases that are coming. Designing in complete
ignorance of them is just stupidity.

Atul wants to put something in staging, and grow it from there. That's
not delaying it, but trying to get it done right.

Alan

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-13 22:24                   ` Jason Gunthorpe
@ 2015-01-14 16:06                     ` One Thousand Gnomes
  2015-01-14 18:12                       ` Jason Gunthorpe
  2015-01-15 16:34                     ` atull
  1 sibling, 1 reply; 64+ messages in thread
From: One Thousand Gnomes @ 2015-01-14 16:06 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: atull, Pavel Machek, gregkh, hpa, monstr, michal.simek, rdunlap,
	linux-kernel, devicetree, pantelis.antoniou, robh+dt,
	grant.likely, iws, linux-doc, broonie, philip, rubini,
	s.trumtrar, jason, kyle.teske, nico, balbi, m.chehab, davidb,
	rob, davem, cesarb, sameo, akpm, linus.walleij, pawel.moll,
	mark.rutland, ijc+devicetree, galak, devel, delicious.quinoa,
	dinguyen, yvanderv

> The request_firmware interface should be for the DT overlay path, and
> other schemes shouldn't use it. The name should come from the DT and
> no place else.

For the static bindings agreed (or ACPI but that's a detail) or other
dynamic discovery post boot.

>  2) The bootloader starts the kernel and passes a DT that describes
>     the hard logic and soft logic using a scheme like I outlined. The
>     kernel is responsible to request_firmware the bitfile and start
>     the FPGA and connect the drivers.
> 
>     DT purists will rightly point out that this isn't great, but it
>     is a simple and pragmatic solution.
> 
>     This probably falls out automatically if #3 is implemented..
>  3) The bootloader starts the kernel with a DT that only describes the
>     hard logic.
> 
>     Userspace must load a DT overlay early on. Otherwise proceeds like #2.
> 
>     In both 2 and 3 the FPGA can be reprogrammed on resume by re-doing
>     request_firmware.

and I think you effectively have the user usage covered here for such
things. It much like GPIO pins - we can describe them but we can also
declare they are not visible to the user.

> And for folks that need more control, expose all the knobs explicitly
> to user space:
>   4) There is a /dev/ node for the fpga that allows
>      - ioctl to program via mmap'd file (no request_firmware)
> 
>        The ioctl has an option for the kernel to hang on to the mmap
>        for the repogram on resume case.
>      - ioctl to get status/error reporting/etc
>      - ioctl to load/bind a DT overlay to the FPGA instance
>        This provides a gap where userspace can boot and configure the
>        FPGA internals before binding the kernel drivers.
>        (end result is the same as #3 case, but no request_firmware)
>      - (future) ioctl to load a swappable FPGA (what Alan is talking
>        about)

The swappable case mostly comes out of the /dev node. Once you have the
dev node it becomes a detail of the OS not the FPGA driver as to who may
open it, and how it is handed about. It might be an FPU manager daemon it
might not.

> The key thing is that we have a FPGA object that can be associated
> with some DT element, and the kernel can then keep track if the FPGA
> is is in use or not. Exactly like you said in your reply.

I think this is right. You have a set of FPGA objects initially coming
from DT and system enumerations and later maybe even showing up via
hotplug.

You have a set of bindings which describes what to do with those that
have fixed bindings.

The fpga /dev entries are then a subset of the enumerated list which has
not been marked as OS in use (or they return -EBUSY and can't be opened)

At that point it works like pretty much everything else.

> I have no problem with the in kernel stuff, and I'd have no problem if
> the sysfs controls were in debugfs for testing purposes. It just
> doesn't make sense to me to expose that kind of interface as a
> permanent API...

That's my big concern too. We need an FPGA manager in kernel clearly. We
need kernel ability to program FPGA bitstreams (if only because there are
both boot time and suspend/resume cases that turn into complete insanity
otherwise).

Alan

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-14 16:06                     ` One Thousand Gnomes
@ 2015-01-14 18:12                       ` Jason Gunthorpe
  2015-01-14 19:01                         ` Pantelis Antoniou
  2015-01-15 11:36                         ` One Thousand Gnomes
  0 siblings, 2 replies; 64+ messages in thread
From: Jason Gunthorpe @ 2015-01-14 18:12 UTC (permalink / raw)
  To: One Thousand Gnomes
  Cc: atull, Pavel Machek, gregkh, hpa, monstr, michal.simek, rdunlap,
	linux-kernel, devicetree, pantelis.antoniou, robh+dt,
	grant.likely, iws, linux-doc, broonie, philip, rubini,
	s.trumtrar, jason, kyle.teske, nico, balbi, m.chehab, davidb,
	rob, davem, cesarb, sameo, akpm, linus.walleij, pawel.moll,
	mark.rutland, ijc+devicetree, galak, devel, delicious.quinoa,
	dinguyen, yvanderv

On Wed, Jan 14, 2015 at 04:06:17PM +0000, One Thousand Gnomes wrote:

> and I think you effectively have the user usage covered here for such
> things. It much like GPIO pins - we can describe them but we can also
> declare they are not visible to the user.

A missing element in mainline is a kind of VFIO scheme to let user
space access the FPGA registers designated for user space use.

We have been using these patches since 2.6.16 to allow user space to
access the FPGA register resources via a PCI like /sys/../resource0
mmapable:

    https://github.com/jgunthorpe/linux/commit/59d5d13ddeffa8980ccc6248ebb5f1678ccb23f4
    https://github.com/jgunthorpe/linux/commit/7c29c4344627be8a3906d64d32db533bc131df86
    https://github.com/jgunthorpe/linux/commit/e41bb4a197368a9d505d66b627aee82f2d2b8895

We deliberately split up the FPGA registers and the assign user space
permissions to the resource0 files in a way that makes sense for our
app. Typically there are 10-20 FPGA register regions. User space does
not access register regions that control DMA.

> The swappable case mostly comes out of the /dev node. Once you have the
> dev node it becomes a detail of the OS not the FPGA driver as to who may
> open it, and how it is handed about. It might be an FPU manager daemon it
> might not.

Right, but the thing that scares me about the swappable case is the
kernel side support.. The FPGA has to connect to the CPU in some way,
it must have some address assigned, etc. Swapping needs to take care
of all those details in some way.

A fixed bus interface block and dynamic reconfiguration for the
remainder is probably the way to manage that. But, that implies that
even a family of swappable FPGAs will have a DT overlay associated
with it.

Ideally, I could see wanting to have a file format that combines the
overlay and FPGA bitfile. A loader tool would use the /dev/ interface
to setup both elements. That would be much more robust than the
current scheme I see (eg Xilinx) using where the bitfile and DT bit
live in completely different places and have to be perfectly matched.

Jason

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-14 18:12                       ` Jason Gunthorpe
@ 2015-01-14 19:01                         ` Pantelis Antoniou
  2015-01-15 11:36                         ` One Thousand Gnomes
  1 sibling, 0 replies; 64+ messages in thread
From: Pantelis Antoniou @ 2015-01-14 19:01 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: One Thousand Gnomes, atull, Pavel Machek, Greg Kroah-Hartman,
	hpa, Michal Simek, Michal Simek, rdunlap,
	Linux Kernel Mailing List, devicetree, robh+dt, Grant Likely,
	iws, linux-doc, Mark Brown, philip, rubini, Steffen Trumtrar,
	jason, kyle.teske, nico, Felipe Balbi, m.chehab, davidb,
	Rob Landley, davem, cesarb, sameo, akpm, Linus Walleij,
	pawel.moll, mark.rutland, ijc+devicetree, galak, devel,
	Alan Tull, dinguyen, yvanderv

Hi Jason,

> On Jan 14, 2015, at 20:12 , Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:
> 
> On Wed, Jan 14, 2015 at 04:06:17PM +0000, One Thousand Gnomes wrote:
> 
>> and I think you effectively have the user usage covered here for such
>> things. It much like GPIO pins - we can describe them but we can also
>> declare they are not visible to the user.
> 
> A missing element in mainline is a kind of VFIO scheme to let user
> space access the FPGA registers designated for user space use.
> 
> We have been using these patches since 2.6.16 to allow user space to
> access the FPGA register resources via a PCI like /sys/../resource0
> mmapable:
> 
>    https://github.com/jgunthorpe/linux/commit/59d5d13ddeffa8980ccc6248ebb5f1678ccb23f4
>    https://github.com/jgunthorpe/linux/commit/7c29c4344627be8a3906d64d32db533bc131df86
>    https://github.com/jgunthorpe/linux/commit/e41bb4a197368a9d505d66b627aee82f2d2b8895
> 
> We deliberately split up the FPGA registers and the assign user space
> permissions to the resource0 files in a way that makes sense for our
> app. Typically there are 10-20 FPGA register regions. User space does
> not access register regions that control DMA.
> 

Interesting.

>> The swappable case mostly comes out of the /dev node. Once you have the
>> dev node it becomes a detail of the OS not the FPGA driver as to who may
>> open it, and how it is handed about. It might be an FPU manager daemon it
>> might not.
> 
> Right, but the thing that scares me about the swappable case is the
> kernel side support.. The FPGA has to connect to the CPU in some way,
> it must have some address assigned, etc. Swapping needs to take care
> of all those details in some way.
> 
> A fixed bus interface block and dynamic reconfiguration for the
> remainder is probably the way to manage that. But, that implies that
> even a family of swappable FPGAs will have a DT overlay associated
> with it.
> 
> Ideally, I could see wanting to have a file format that combines the
> overlay and FPGA bitfile. A loader tool would use the /dev/ interface
> to setup both elements. That would be much more robust than the
> current scheme I see (eg Xilinx) using where the bitfile and DT bit
> live in completely different places and have to be perfectly matched.
> 

A single DT property can be 4 gigs of binary data (cheating a bit, the whole blob can be
4 gigs). You can conceivably include the whole bitfile in the DT blob.

Whether this is a perversion or not it’s left to the reader. The bitfile does
describe the hardware though.

> Jason

Regards

— Pantelis


^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-14 18:12                       ` Jason Gunthorpe
  2015-01-14 19:01                         ` Pantelis Antoniou
@ 2015-01-15 11:36                         ` One Thousand Gnomes
  2015-01-15 11:44                           ` Mark Brown
  1 sibling, 1 reply; 64+ messages in thread
From: One Thousand Gnomes @ 2015-01-15 11:36 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: atull, Pavel Machek, gregkh, hpa, monstr, michal.simek, rdunlap,
	linux-kernel, devicetree, pantelis.antoniou, robh+dt,
	grant.likely, iws, linux-doc, broonie, philip, rubini,
	s.trumtrar, jason, kyle.teske, nico, balbi, m.chehab, davidb,
	rob, davem, cesarb, sameo, akpm, linus.walleij, pawel.moll,
	mark.rutland, ijc+devicetree, galak, devel, delicious.quinoa,
	dinguyen, yvanderv

On Wed, 14 Jan 2015 11:12:58 -0700
Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:

> On Wed, Jan 14, 2015 at 04:06:17PM +0000, One Thousand Gnomes wrote:
> 
> > and I think you effectively have the user usage covered here for such
> > things. It much like GPIO pins - we can describe them but we can also
> > declare they are not visible to the user.
> 
> A missing element in mainline is a kind of VFIO scheme to let user
> space access the FPGA registers designated for user space use.

Agreed entirely.

> A fixed bus interface block and dynamic reconfiguration for the
> remainder is probably the way to manage that. But, that implies that
> even a family of swappable FPGAs will have a DT overlay associated
> with it.

Or some other resource, it could be described by PCI but something is
going to need to describe it when we get to that state and something
will need to set up the IOMMU for it and potentially manage virtualising
it or assigning it to things like docker instances.
 
> Ideally, I could see wanting to have a file format that combines the
> overlay and FPGA bitfile. A loader tool would use the /dev/ interface
> to setup both elements. That would be much more robust than the
> current scheme I see (eg Xilinx) using where the bitfile and DT bit
> live in completely different places and have to be perfectly matched.

yes - there is a model for this in Linux already. Some of the audio
subsystems have "firmware" files distributed which are actually a
structured file that userspace parses to get a real set of firmware for
the controller and a set of descriptions for things like mixer controls.

They have the same basic problem - when you change firmware your control
interfaces may change so you need both descriptions together.

Alan

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-15 11:36                         ` One Thousand Gnomes
@ 2015-01-15 11:44                           ` Mark Brown
  0 siblings, 0 replies; 64+ messages in thread
From: Mark Brown @ 2015-01-15 11:44 UTC (permalink / raw)
  To: One Thousand Gnomes
  Cc: Jason Gunthorpe, atull, Pavel Machek, gregkh, hpa, monstr,
	michal.simek, rdunlap, linux-kernel, devicetree,
	pantelis.antoniou, robh+dt, grant.likely, iws, linux-doc, philip,
	rubini, s.trumtrar, jason, kyle.teske, nico, balbi, m.chehab,
	davidb, rob, davem, cesarb, sameo, akpm, linus.walleij,
	pawel.moll, mark.rutland, ijc+devicetree, galak, devel,
	delicious.quinoa, dinguyen, yvanderv

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

On Thu, Jan 15, 2015 at 11:36:17AM +0000, One Thousand Gnomes wrote:

> yes - there is a model for this in Linux already. Some of the audio
> subsystems have "firmware" files distributed which are actually a
> structured file that userspace parses to get a real set of firmware for
> the controller and a set of descriptions for things like mixer controls.

We didn't actually merge that yet (but we do want to).  It's likely that
the parsing of the files will end up in the kernel rather than userspace,
or that anything userspace is doing is mostly about translating vendor
formats into a standard Linux one anyway.

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

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-13 22:24                   ` Jason Gunthorpe
  2015-01-14 16:06                     ` One Thousand Gnomes
@ 2015-01-15 16:34                     ` atull
  2015-01-15 18:47                       ` Jason Gunthorpe
  2015-01-17 21:11                       ` Pavel Machek
  1 sibling, 2 replies; 64+ messages in thread
From: atull @ 2015-01-15 16:34 UTC (permalink / raw)
  To: Jason Gunthorpe, gregkh
  Cc: One Thousand Gnomes, Pavel Machek, hpa, monstr, michal.simek,
	rdunlap, linux-kernel, devicetree, pantelis.antoniou, robh+dt,
	grant.likely, iws, linux-doc, broonie, philip, rubini,
	s.trumtrar, jason, kyle.teske, nico, balbi, m.chehab, davidb,
	rob, davem, cesarb, sameo, akpm, linus.walleij, pawel.moll,
	mark.rutland, ijc+devicetree, galak, devel, delicious.quinoa,
	dinguyen, yvanderv

On Tue, 13 Jan 2015, Jason Gunthorpe wrote:

> On Tue, Jan 13, 2015 at 03:37:14PM -0600, atull wrote:
> 
> > > I do agree with this, and I think this is where this patch set goes so
> > > wrong.
> > > 
> > > Just exposing all sorts of controls to userspace and having a way for
> > > the kernel to load/unload a bitstream compleletely misses the point that
> > > the two main ways to use FPGAs *require* some kind of kernel side
> > > driver suppport on the FPGA.
> > 
> > Hi Jason,
> > 
> > I may be misunderstanding.  I thought you wanted lots of user control
> > a couple years ago.
> > 
> > https://lkml.org/lkml/2013/9/18/490
> 
> Yes, that is absolutely true, that is certainly where my projects have
> been historically, and in the context of requiring user space to
> manage the whole process I think that is completely right.
> 
> But - I like Alan's point - this should be a higher level thing, the
> kernel should be able to load the FPGA, and handle binding the drivers
> so user space doesn't need to be involved, and it makes sense that
> should be the place to start the API design.
> 
> > Your feedback carried a lot of weight as I've been developing this patchset
> > although I didn't do absolutely everything you asked for.  Back then you
> > were asking for lots of user control including being able to reset the FPGA
> > and being able to know from userspace what configuration steps failed (if any).
> 
> Yes, this is all true. Since my FPGAs all seem to require a software
> mediated startup sequence the dream of having a DT scheme like I
> described is farther away for me, but I think that is not the 90%
> case. Almost all vendor IP designs do not require a SW startup and
> auto-start after programming. 90% of designs can program and then
> immediately connect drivers.
> 
> > I'm glad you like DT support for FPGA images + drivers loading since I've
> > been pushing that since April and have submitted several earlier versions
> > of this patchset which included it.
> 
> Yes, I do, thank you for working on this!
> 
> > I think your arguements against the current patchset are a bit over
> > the top here.
> 
> Perhaps, and I apologize. But as you know I've never liked the sysfs
> interface, and I think Pavel is right. catting a file name just so you
> can call request_firmware on that file in the kernel is a bad design.
> 
> The request_firmware interface should be for the DT overlay path, and
> other schemes shouldn't use it. The name should come from the DT and
> no place else.
> 
> > > Make it so linux can boot, automatically fetch the gpio-api1 bit file
> > > and load it into the chip and then bind the GPIO driver to the FPGA
> > > resource. All without userspace involvment, all potentially done
> > > before calling INIT.
> > 
> > Where would the fpga image be fetch from?  What is the mechanism for
> > doing that?  Do we need to reqire everybody to do that or can
> > firmware be one of the available mechanisms?
> 
> I see there are three options:
>  1) The bootloader programs the FPGA and passes a DT to the kernel
>     that completely describes the hard and soft logic, the kernel
>     doesn't need FPGA bitfile management code or other special
>     support.
> 
>     Obviously things like reprogram on resume have to be done by the
>     bootloader, there is no handoff here where the kernel takes over
>     control of the bitfile programming. I think that is fine, #2/#3
>     are better choices for most people anyhow.
>  2) The bootloader starts the kernel and passes a DT that describes
>     the hard logic and soft logic using a scheme like I outlined. The
>     kernel is responsible to request_firmware the bitfile and start
>     the FPGA and connect the drivers.
> 
>     DT purists will rightly point out that this isn't great, but it
>     is a simple and pragmatic solution.
> 
>     This probably falls out automatically if #3 is implemented..
>  3) The bootloader starts the kernel with a DT that only describes the
>     hard logic.
> 
>     Userspace must load a DT overlay early on. Otherwise proceeds like #2.
> 
>     In both 2 and 3 the FPGA can be reprogrammed on resume by re-doing
>     request_firmware.
> 

This is great!  The way I had it working was using Pantelis' devicetree
configfs interface.

# To load a fpga image and a driver or drivers under userspace control:
mkdir /config/device-tree/overlays/1
echo socfpga_overlay.dtbo > /config/device-tree/overlays/1/path

# To remove the driver(s) and fpga image:
rmdir /config/device-tree/overlays/1

The DT fragment described the FPGA logic and included a filename
for firmware to load. In another branch of this thread, I see discussion
starting on what the overlay should look like and whether it could somehow
contain the DT itself.

> And for folks that need more control, expose all the knobs explicitly
> to user space:
>   4) There is a /dev/ node for the fpga that allows
>      - ioctl to program via mmap'd file (no request_firmware)
> 
>        The ioctl has an option for the kernel to hang on to the mmap
>        for the repogram on resume case.
>      - ioctl to get status/error reporting/etc
>      - ioctl to load/bind a DT overlay to the FPGA instance
>        This provides a gap where userspace can boot and configure the
>        FPGA internals before binding the kernel drivers.
>        (end result is the same as #3 case, but no request_firmware)
>      - (future) ioctl to load a swappable FPGA (what Alan is talking
>        about)
> 

This sounds good to me.  To do that I will need to export more a few more
base functions to get at a finer grain of control that we can add a /dev
interface for.

Long ago this driver started out with a /dev interface.  It didn't have
an ioctl yet at that point, but programming the fpga was by opening
the devnode and writing to it.  Greg KH preferred sysfs or configfs
over adding another ioctl:

https://lkml.org/lkml/2013/10/8/677

snippet
 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).
end snippet

I'm ok with this becoming an /dev interface.  We lose the easy interface
of sysfs or configfs but gain a permissions scheme.  It would be
good to have concensus before I do to much coding in that direction.

> The key thing is that we have a FPGA object that can be associated
> with some DT element, and the kernel can then keep track if the FPGA
> is is in use or not. Exactly like you said in your reply.
> 
> Otherwise, for the /dev/ case the FPGA becomes unuse once the FD is
> closed if it wasn't attached to an overlay. This is at least the start
> of addressing the space Alan is talking about.
> 
> If this sort of world is the goal, it is hard for me to see how the
> proposed sysfs interface fits into that long term.
> 
> The /dev/ interface is probably a better place to start talking about
> partial reconfiguration as well. The fixed part might be bound to a
> kernel driver (eg the PCI-E interface) and the reconfigurable part
> might be a maths algorithm - for instance.
> 
> > > Make it so via sysfs we can reverse this process and reboot the FPGA.
> > 
> > Better to use the device tree's configfs interface if you are going
> > to add/delete a DT overlay.  Adding an overlay could load the fpga,
> > enable bridges, and load driver(s).  Some fpga images may have several
> > hardware blocks so several drivers to load.
> 
> Yes, absolutely!
> 
> > > And no, this isn't just a 'theory', I have 4 shipping products today
> > > that work pretty much exactly like this, including a Zynq
> > > design. Binding kernel drivers to a FPGA once it is loaded is a big
> > > functionality gap, and an annoying problem. Programming the actual
> > > FPGA? That is the *EASIEST* part of this whole thing!
> 
> > That's why I'm trying to get programming the fpga out of the way here.
> 
> I have no problem with the in kernel stuff, and I'd have no problem if
> the sysfs controls were in debugfs for testing purposes. It just
> doesn't make sense to me to expose that kind of interface as a
> permanent API...
>  
> Jason
> 

I appreciate all of you and Alan's input on this.  I'm glad we are starting
to see ways of supporting both DT and fpga resources.

I'm looking into moving the sysfs interface to debugfs.  Doesn't look too
hard and you and Alan are making lots of sense about this.

Alan Tull

aka atull
aka delicious quinoa

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-15 16:34                     ` atull
@ 2015-01-15 18:47                       ` Jason Gunthorpe
  2015-01-15 20:45                         ` One Thousand Gnomes
  2015-01-17 21:11                       ` Pavel Machek
  1 sibling, 1 reply; 64+ messages in thread
From: Jason Gunthorpe @ 2015-01-15 18:47 UTC (permalink / raw)
  To: atull
  Cc: gregkh, One Thousand Gnomes, Pavel Machek, hpa, monstr,
	michal.simek, rdunlap, linux-kernel, devicetree,
	pantelis.antoniou, robh+dt, grant.likely, iws, linux-doc,
	broonie, philip, rubini, s.trumtrar, jason, kyle.teske, nico,
	balbi, m.chehab, davidb, rob, davem, cesarb, sameo, akpm,
	linus.walleij, pawel.moll, mark.rutland, ijc+devicetree, galak,
	devel, delicious.quinoa, dinguyen, yvanderv

On Thu, Jan 15, 2015 at 10:34:39AM -0600, atull wrote:

> This is great!  The way I had it working was using Pantelis' devicetree
> configfs interface.

I figured you were very close to this already in your overlay work..
 
> The DT fragment described the FPGA logic and included a filename
> for firmware to load. In another branch of this thread, I see discussion
> starting on what the overlay should look like and whether it could somehow
> contain the DT itself.

It is a novel idea, my concern would be that embedding the FPGA in the
DT makes it permanent unswappable kernel memory.

Not having the kernel hold the FPGA is best for many uses.

Having the kernel hold the FPGA as a swppable file handle/mmap of some
sort is next best (obviously the fs must be operating during resume)

Unswappable kernel memory is the worst choice

> Long ago this driver started out with a /dev interface.  It didn't have
> an ioctl yet at that point, but programming the fpga was by opening
> the devnode and writing to it.  Greg KH preferred sysfs or configfs
> over adding another ioctl:

I think to justify the ioctls you need a reason to have the context
of a FD. sysfs is stateless, so if my process dies the kernel doesn't
know.

But now that we are talking about adding locking and ownership
concepts a FD is the natural anchor for that in user space.

Ie, if I open the dev node, program a FPGA and then crash the kernel
doesn't attach drivers, and immediately de-programs the
chip. Userspace has to make it all the way through to the DT bind
before the FPGA lifetime would exceed the FD.

> https://lkml.org/lkml/2013/10/8/677

I think Greg's reply makes sense in the context of the question being
asked. Thinking of the FPGA as lockable ref counted kind of resource
changes the question somewhat.

Identifying the ioctls needed would probably clarify things. My
rough start would be 

- Get status: programed, not programmed, error?
  Bitfile type? (eg Xilinx has nearly every permutation of bit/byte
  ordering)
- Start Program with with some kind of context (ie this a new bit
  file, partial reconfiguration basis X, partial reconfiguration
  overlay on X)
- for (;;) write() to do programming
- Get Error to return detailed failure information (CRC error,
  auth error, etc)
- Hand over to a DT overlay (how does this work?) Lock transfers
  from FD to kernel

-  .. something something VFIO .. ?

Where start program is refused if the FPGA is already locked, and
  locks it 
Where start program -> close() returns the FPGA back to reset and
  unlocks
Where start program -> hand over -> close() keeps the FPGA loaded with
 kernel drivers attached and fpga locked (remove the overlay to
 de-program and unlock)

Not sure exactly how to tie together DT overlays with the FPGA state,
but that seems the natural combination..

Not sure about partial reconfiguration - clearly the kernel needs to
know and check that the bitfiles are of the correct family, I wonder
if the approach should be to program a basis on the FPGA which then
creates a new FPGA device in the system that can accept the partial
reconfiguration - this way the locking makes sense, the basis is
locked by the kernel and devices and the overlay remains
lockable/swappable/whatever by a dedicated /dev/ node ??

Just thinking aloud, I've never had a use case for partial
reconfiguration.

Jason

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-15 18:47                       ` Jason Gunthorpe
@ 2015-01-15 20:45                         ` One Thousand Gnomes
  2015-01-15 20:54                           ` Pantelis Antoniou
  2015-01-15 21:42                           ` Jason Gunthorpe
  0 siblings, 2 replies; 64+ messages in thread
From: One Thousand Gnomes @ 2015-01-15 20:45 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: atull, gregkh, Pavel Machek, hpa, monstr, michal.simek, rdunlap,
	linux-kernel, devicetree, pantelis.antoniou, robh+dt,
	grant.likely, iws, linux-doc, broonie, philip, rubini,
	s.trumtrar, jason, kyle.teske, nico, balbi, m.chehab, davidb,
	rob, davem, cesarb, sameo, akpm, linus.walleij, pawel.moll,
	mark.rutland, ijc+devicetree, galak, devel, delicious.quinoa,
	dinguyen, yvanderv

On Thu, 15 Jan 2015 11:47:26 -0700
Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:
> It is a novel idea, my concern would be that embedding the FPGA in the
> DT makes it permanent unswappable kernel memory.
> Not having the kernel hold the FPGA is best for many uses.

If you have a filesysytem before the FPGA is set up then it belongs in
the file system. As you presumably loaded the kernel from somewhere there
ought to be a file system (even an initrd).

> Having the kernel hold the FPGA as a swppable file handle/mmap of some
> sort is next best (obviously the fs must be operating during resume)

For a lot of hardware that gets somewhat hairy because you don't know
what the dependancies between devices are on the resume but yes.

> Unswappable kernel memory is the worst choice

There is another case here - which is where the firmware is somewhere
else physically. For example in PCI ROM space, or flash, but designed to
be loaded by the OS.


> I think to justify the ioctls you need a reason to have the context
> of a FD. sysfs is stateless, so if my process dies the kernel doesn't
> know.

That isn't to say the stateless information doesn't belong in sysfs. Eg
you don't neccessarily want to open the device node and go through ioctls
just for bits of information that are interesting to reporting tools.

(Imagine an 'lsfpga' tool for the kind of things you'd leave in sysfs)

> Identifying the ioctls needed would probably clarify things. My
> rough start would be 
> 
> - Get status: programed, not programmed, error?
>   Bitfile type? (eg Xilinx has nearly every permutation of bit/byte
>   ordering)

That's probably some kind of "what are you" ioctl that returns the
vendor/type information.

> - Hand over to a DT overlay (how does this work?) Lock transfers
>   from FD to kernel

That bit isn't stateful so I would actually have expected something in
the kernel ABI along the lines of 

           request_fpga(blah)

which does

             if in use by user
                    EBUSY
             lock it (all user opens will fail)

and

            release_fpga(blah)

and a sysfs node indicating its busy (and perhaps also what for if the
request_fpga passes a textual name)

> Not sure about partial reconfiguration - clearly the kernel needs to
> know and check that the bitfiles are of the correct family, I wonder
> if the approach should be to program a basis on the FPGA which then
> creates a new FPGA device in the system that can accept the partial
> reconfiguration - this way the locking makes sense, the basis is
> locked by the kernel and devices and the overlay remains
> lockable/swappable/whatever by a dedicated /dev/ node ??

I think so.

If you closed the device you have no idea what happened between the close
and a re-open, therefore you have no idea what the FPGA state is.
 
> Just thinking aloud, I've never had a use case for partial
> reconfiguration.

The API handles it but whether it needs to be there from day one I don't
know.

Alan


^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-15 20:45                         ` One Thousand Gnomes
@ 2015-01-15 20:54                           ` Pantelis Antoniou
  2015-01-21 16:01                             ` One Thousand Gnomes
  2015-01-15 21:42                           ` Jason Gunthorpe
  1 sibling, 1 reply; 64+ messages in thread
From: Pantelis Antoniou @ 2015-01-15 20:54 UTC (permalink / raw)
  To: One Thousand Gnomes
  Cc: Jason Gunthorpe, atull, Greg Kroah-Hartman, Pavel Machek, hpa,
	Michal Simek, Michal Simek, rdunlap, Linux Kernel Mailing List,
	devicetree, robh+dt, Grant Likely, iws, linux-doc, Mark Brown,
	philip, rubini, Steffen Trumtrar, jason, kyle.teske, nico,
	Felipe Balbi, m.chehab, davidb, Rob Landley, davem, cesarb,
	sameo, akpm, Linus Walleij, pawel.moll, mark.rutland,
	ijc+devicetree, galak, devel, Alan Tull, dinguyen, yvanderv

Hi Alan,

> On Jan 15, 2015, at 22:45 , One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> wrote:
> 
> On Thu, 15 Jan 2015 11:47:26 -0700
> Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:
>> It is a novel idea, my concern would be that embedding the FPGA in the
>> DT makes it permanent unswappable kernel memory.
>> Not having the kernel hold the FPGA is best for many uses.
> 
> If you have a filesysytem before the FPGA is set up then it belongs in
> the file system. As you presumably loaded the kernel from somewhere there
> ought to be a file system (even an initrd).
> 

Request firmware does not imply keeping it around. You can always re-request
when reloading (although there’s a nasty big of caching that needs to be
resolved with the firmware loader).

>> Having the kernel hold the FPGA as a swppable file handle/mmap of some
>> sort is next best (obviously the fs must be operating during resume)
> 
> For a lot of hardware that gets somewhat hairy because you don't know
> what the dependancies between devices are on the resume but yes.
> 
>> Unswappable kernel memory is the worst choice
> 
> There is another case here - which is where the firmware is somewhere
> else physically. For example in PCI ROM space, or flash, but designed to
> be loaded by the OS.
> 
> 

One of the ideas rolling about is to put the device tree overlay blob in
an EEPROM and then load it from there (not from the filesystem).

>> I think to justify the ioctls you need a reason to have the context
>> of a FD. sysfs is stateless, so if my process dies the kernel doesn't
>> know.
> 
> That isn't to say the stateless information doesn't belong in sysfs. Eg
> you don't neccessarily want to open the device node and go through ioctls
> just for bits of information that are interesting to reporting tools.
> 
> (Imagine an 'lsfpga' tool for the kind of things you'd leave in sysfs)
> 
>> Identifying the ioctls needed would probably clarify things. My
>> rough start would be 
>> 
>> - Get status: programed, not programmed, error?
>>  Bitfile type? (eg Xilinx has nearly every permutation of bit/byte
>>  ordering)
> 
> That's probably some kind of "what are you" ioctl that returns the
> vendor/type information.
> 

Can we please not use ioctls if possible. Configfs seems to work just fine
for configuration and for any other higher speed API we should use read/write/mmap.

Ioctls are a pain for scripting and interpreted languages usually.

>> - Hand over to a DT overlay (how does this work?) Lock transfers
>>  from FD to kernel
> 
> That bit isn't stateful so I would actually have expected something in
> the kernel ABI along the lines of 
> 
>           request_fpga(blah)
> 
> which does
> 
>             if in use by user
>                    EBUSY
>             lock it (all user opens will fail)
> 
> and
> 
>            release_fpga(blah)
> 
> and a sysfs node indicating its busy (and perhaps also what for if the
> request_fpga passes a textual name)
> 
>> Not sure about partial reconfiguration - clearly the kernel needs to
>> know and check that the bitfiles are of the correct family, I wonder
>> if the approach should be to program a basis on the FPGA which then
>> creates a new FPGA device in the system that can accept the partial
>> reconfiguration - this way the locking makes sense, the basis is
>> locked by the kernel and devices and the overlay remains
>> lockable/swappable/whatever by a dedicated /dev/ node ??
> 
> I think so.
> 
> If you closed the device you have no idea what happened between the close
> and a re-open, therefore you have no idea what the FPGA state is.
> 
>> Just thinking aloud, I've never had a use case for partial
>> reconfiguration.
> 
> The API handles it but whether it needs to be there from day one I don't
> know.
> 

Making the API handle partial reconfiguration from day one might be pushing tricky.
I don’t remember any case where I came across a need for it.

We could do with a virtual FPGA topology; have a root FPGA that’s keeping the
common unchanged bits and another that’s keeping the bits that do change.

Dunno how nice it would be in practice though.

> Alan
> 

Regards

— Pantelis


^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-15 20:45                         ` One Thousand Gnomes
  2015-01-15 20:54                           ` Pantelis Antoniou
@ 2015-01-15 21:42                           ` Jason Gunthorpe
  1 sibling, 0 replies; 64+ messages in thread
From: Jason Gunthorpe @ 2015-01-15 21:42 UTC (permalink / raw)
  To: One Thousand Gnomes
  Cc: atull, gregkh, Pavel Machek, hpa, monstr, michal.simek, rdunlap,
	linux-kernel, devicetree, pantelis.antoniou, robh+dt,
	grant.likely, iws, linux-doc, broonie, philip, rubini,
	s.trumtrar, jason, kyle.teske, nico, balbi, m.chehab, davidb,
	rob, davem, cesarb, sameo, akpm, linus.walleij, pawel.moll,
	mark.rutland, ijc+devicetree, galak, devel, delicious.quinoa,
	dinguyen, yvanderv

On Thu, Jan 15, 2015 at 08:45:02PM +0000, One Thousand Gnomes wrote:

> > - Hand over to a DT overlay (how does this work?) Lock transfers
> >   from FD to kernel
> 
> That bit isn't stateful so I would actually have expected something in
> the kernel ABI along the lines of 
> 
>            request_fpga(blah)
> 
> which does
> 
>              if in use by user
>                     EBUSY
>              lock it (all user opens will fail)
> 
> and
> 
>             release_fpga(blah)

I am imagining two ways to start a kernel FPGA, the in-kernel method
triggered by a DT node:

  fpga = request_and_lock_fpga(of_get_fpga_controller(dev->of_node))
  fw = request_firmat(of_get_fpga_firmware(dev->of_node));
  fpga_program_fw(fpga, fw);

  for_each_child_of_node(dev->of_node, child)
    .. of_platform_bus_probe(child ... ) ..

  .. somehow fpga and its lock transfer to dev->of_node ..

The problem with this is it assumes the FPGA is ready to go
immediately after fpga_program_fw. There are a few platforms that can
manage this, but many others require at least some kind of startup
sequence - eg wait for clocking PLLs to lock, do low level setup, etc.

For very common cases (like Zynq can have a pretty common setup scheme
for the PL side) the DT binding can guide the kernel to run the right
code, and the code can live in the kernel because it is simple and
broadly useful.

For wild cases I'd like to just punt the setup code to user space. It
is safer and simpler to run that complexity in a user process than in
the kernel.

Maybe there is a better way to handle this, but I have been under the
impression that these days it is frowned on for the kernel to wait for
userspace?

So my idea is to use the user space method to also load a 'kernel'
fpga, the process follows the kernel flow:

   fd = open("/dev/fpga0"); // request_and_lock_fpga

   ioctl(fd,START_PROGRAMMING); // fpga_program_fw
   write(fd,fw,fw_size);
   
   // Arbitary complexity here
   userspace_setup_fpga();

   icotl(fd,BIND_DT_OVERLAY, .. ?? ..) // for_each_child_of_node

This is what the state is about, if the setup fails I expect the
kernel to go and unlock and deprogram the FPGA if fd is closed. Only a
success on BIND_DT_OVERLAY will make the FPGA permanent beyond closing
fd.

Pantelis: I think this explains why a fd and ioctls. configfs, sysfs,
etc lack the FD context to do the cleanup. 

Essentially, I think a running FPGA should always be attached to some
context that is the user - either the user is in-kernel (a DT of_node)
or the user is in userspace (the fd).

The invarient is - if there is no user then the kernel automatically
makes the FPGA deprogrammed.

Having a device that is potentially attached to the CPU bus running
'rouge' is dangerous. We can't realistically deprogram it safely if we
don't know what it is doing. Eg deprogramming in the middle of a DMA
operation between the CPU and FPGA will often crash the entire system.

The only way to provide reasonable safe guards is to have a clear
chain of ownership. When the kernel takes ownership of the FPGA it
also takes ownership of any cleanup required prior to deprogram.

Jason

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-13 16:21                 ` One Thousand Gnomes
@ 2015-01-15 21:52                   ` Pavel Machek
  0 siblings, 0 replies; 64+ messages in thread
From: Pavel Machek @ 2015-01-15 21:52 UTC (permalink / raw)
  To: One Thousand Gnomes
  Cc: Jason Gunthorpe, atull, Pantelis Antoniou, Greg Kroah-Hartman,
	hpa, Michal Simek, Michal Simek, rdunlap,
	Linux Kernel Mailing List, devicetree, robh+dt, Grant Likely,
	iws, linux-doc, Mark Brown, philip, rubini, Steffen Trumtrar,
	jason, kyle.teske, nico, Felipe Balbi, m.chehab, davidb,
	Rob Landley, davem, cesarb, sameo, akpm, Linus Walleij,
	pawel.moll, mark.rutland, ijc+devicetree, galak, devel,
	Alan Tull, dinguyen, yvanderv

On Tue 2015-01-13 16:21:33, One Thousand Gnomes wrote:
> On Mon, 12 Jan 2015 11:06:08 -0700
> Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:
> 
> > On Sun, Jan 11, 2015 at 10:29:00AM -0600, atull wrote:
> > > the FPGA image.  If someone wants there to be only one FPGA image on
> > > the FGPA forever, they will probably not be using this framework; their
> > > FPGA will probably be loaded before Linux boots up.
> > 
> > Nonsense, loading the FPGA through Linux is much better
> 
> You need both. There are platforms where the FPGA must be loaded to even
> boot the OS (for example Linux running an FPGA soft 486 can't run until
> the FPGA has the CPU loaded into it.

Well, but then the thing that programs FPGA is not Linux, so this is
little off-topic here. [But yes, there are systems where you want to
program FPGA from bootloader, because it is easier that way, or maybe
because it provides your video card.]

									Pavel

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

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-15 16:34                     ` atull
  2015-01-15 18:47                       ` Jason Gunthorpe
@ 2015-01-17 21:11                       ` Pavel Machek
  1 sibling, 0 replies; 64+ messages in thread
From: Pavel Machek @ 2015-01-17 21:11 UTC (permalink / raw)
  To: atull
  Cc: Jason Gunthorpe, gregkh, One Thousand Gnomes, hpa, monstr,
	michal.simek, rdunlap, linux-kernel, devicetree,
	pantelis.antoniou, robh+dt, grant.likely, iws, linux-doc,
	broonie, philip, rubini, s.trumtrar, jason, kyle.teske, nico,
	balbi, m.chehab, davidb, rob, davem, cesarb, sameo, akpm,
	linus.walleij, pawel.moll, mark.rutland, ijc+devicetree, galak,
	devel, delicious.quinoa, dinguyen, yvanderv

Hi!

> I'm looking into moving the sysfs interface to debugfs.  Doesn't look too
> hard and you and Alan are making lots of sense about this.

Debugfs: please don't. That's meant for debugging, not for production
use.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-15 20:54                           ` Pantelis Antoniou
@ 2015-01-21 16:01                             ` One Thousand Gnomes
  2015-01-21 16:33                               ` Pantelis Antoniou
  0 siblings, 1 reply; 64+ messages in thread
From: One Thousand Gnomes @ 2015-01-21 16:01 UTC (permalink / raw)
  To: Pantelis Antoniou
  Cc: Jason Gunthorpe, atull, Greg Kroah-Hartman, Pavel Machek, hpa,
	Michal Simek, Michal Simek, rdunlap, Linux Kernel Mailing List,
	devicetree, robh+dt, Grant Likely, iws, linux-doc, Mark Brown,
	philip, rubini, Steffen Trumtrar, jason, kyle.teske, nico,
	Felipe Balbi, m.chehab, davidb, Rob Landley, davem, cesarb,
	sameo, akpm, Linus Walleij, pawel.moll, mark.rutland,
	ijc+devicetree, galak, devel, Alan Tull, dinguyen, yvanderv

On Thu, 15 Jan 2015 22:54:46 +0200
Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:

> Hi Alan,
> 
> > On Jan 15, 2015, at 22:45 , One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> wrote:
> > 
> > On Thu, 15 Jan 2015 11:47:26 -0700
> > Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:
> >> It is a novel idea, my concern would be that embedding the FPGA in the
> >> DT makes it permanent unswappable kernel memory.
> >> Not having the kernel hold the FPGA is best for many uses.
> > 
> > If you have a filesysytem before the FPGA is set up then it belongs in
> > the file system. As you presumably loaded the kernel from somewhere there
> > ought to be a file system (even an initrd).
> > 
> 
> Request firmware does not imply keeping it around. You can always re-request
> when reloading (although there’s a nasty big of caching that needs to be
> resolved with the firmware loader).

Which comes down to the same thing. Unless you can prove that there is a
path to recover the firmware file that does not have any dependancies
upon the firmware executing (and those can be subtle and horrid at times)
you need to keep it around for suspend/resume at least and potentially
any unexpected error/reset.

> One of the ideas rolling about is to put the device tree overlay blob in
> an EEPROM and then load it from there (not from the filesystem).

That's a fine example of one you can probably always get to and avoid
caching. However if its in eeprom you don't need request_firmware anyway !

> Can we please not use ioctls if possible. Configfs seems to work just fine
> for configuration and for any other higher speed API we should use read/write/mmap.

You don't have the needed state in configfs as far as I can see.

> Ioctls are a pain for scripting and interpreted languages usually.

You can do ioctls in perl just fine if you are mad (and if you are
using perl you are ;-) ) while python has a complete explicit fcntl.ioctl
model.

> Making the API handle partial reconfiguration from day one might be pushing tricky.
> I don’t remember any case where I came across a need for it.

Agreed - I don't see the point in adding it until someone needs it and can
describe what is needed accurately.

Alan

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-21 16:01                             ` One Thousand Gnomes
@ 2015-01-21 16:33                               ` Pantelis Antoniou
  2015-01-21 20:27                                 ` Jason Gunthorpe
  0 siblings, 1 reply; 64+ messages in thread
From: Pantelis Antoniou @ 2015-01-21 16:33 UTC (permalink / raw)
  To: One Thousand Gnomes
  Cc: Jason Gunthorpe, atull, Greg Kroah-Hartman, Pavel Machek, hpa,
	Michal Simek, Michal Simek, Randy Dunlap,
	Linux Kernel Mailing List, devicetree, robh+dt, Grant Likely,
	iws, linux-doc, Mark Brown, philip, rubini, Steffen Trumtrar,
	jason, kyle.teske, nico, Felipe Balbi, m.chehab, davidb,
	Rob Landley, davem, cesarb, sameo, Andrew Morton, Linus Walleij,
	pawel.moll, mark.rutland, ijc+devicetree, galak, devel,
	Alan Tull, dinguyen, yvanderv

Hi Alan,

> On Jan 21, 2015, at 18:01 , One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> wrote:
> 
> On Thu, 15 Jan 2015 22:54:46 +0200
> Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:
> 
>> Hi Alan,
>> 
>>> On Jan 15, 2015, at 22:45 , One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> wrote:
>>> 
>>> On Thu, 15 Jan 2015 11:47:26 -0700
>>> Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:
>>>> It is a novel idea, my concern would be that embedding the FPGA in the
>>>> DT makes it permanent unswappable kernel memory.
>>>> Not having the kernel hold the FPGA is best for many uses.
>>> 
>>> If you have a filesysytem before the FPGA is set up then it belongs in
>>> the file system. As you presumably loaded the kernel from somewhere there
>>> ought to be a file system (even an initrd).
>>> 
>> 
>> Request firmware does not imply keeping it around. You can always re-request
>> when reloading (although there’s a nasty big of caching that needs to be
>> resolved with the firmware loader).
> 
> Which comes down to the same thing. Unless you can prove that there is a
> path to recover the firmware file that does not have any dependancies
> upon the firmware executing (and those can be subtle and horrid at times)
> you need to keep it around for suspend/resume at least and potentially
> any unexpected error/reset.
> 

In that case the only safe place to put it is in the kernel image itself, which
is something the firmware loader already supports.

>> One of the ideas rolling about is to put the device tree overlay blob in
>> an EEPROM and then load it from there (not from the filesystem).
> 
> That's a fine example of one you can probably always get to and avoid
> caching. However if its in eeprom you don't need request_firmware anyway !
> 

Sure, I never said that request_firmware is the only way to get hold of a blob.
It just happens to be the most convenient one for the kernel (when the blob
resides somewhere on a filesystem).

>> Can we please not use ioctls if possible. Configfs seems to work just fine
>> for configuration and for any other higher speed API we should use read/write/mmap.
> 
> You don't have the needed state in configfs as far as I can see.
> 

Sure, but there’s no reason for it not to be there.

>> Ioctls are a pain for scripting and interpreted languages usually.
> 
> You can do ioctls in perl just fine if you are mad (and if you are
> using perl you are ;-) ) while python has a complete explicit fcntl.ioctl
> model.
> 

Sure, it can be made to work, but it is a pain.

>> Making the API handle partial reconfiguration from day one might be pushing tricky.
>> I don’t remember any case where I came across a need for it.
> 
> Agreed - I don't see the point in adding it until someone needs it and can
> describe what is needed accurately.
> 

/me nods.

> Alan

Regards

— Pantelis


^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-21 16:33                               ` Pantelis Antoniou
@ 2015-01-21 20:27                                 ` Jason Gunthorpe
  2015-01-21 20:32                                   ` Pantelis Antoniou
  2015-02-15 22:40                                   ` Pavel Machek
  0 siblings, 2 replies; 64+ messages in thread
From: Jason Gunthorpe @ 2015-01-21 20:27 UTC (permalink / raw)
  To: Pantelis Antoniou
  Cc: One Thousand Gnomes, atull, Greg Kroah-Hartman, Pavel Machek,
	hpa, Michal Simek, Michal Simek, Randy Dunlap,
	Linux Kernel Mailing List, devicetree, robh+dt, Grant Likely,
	iws, linux-doc, Mark Brown, philip, rubini, Steffen Trumtrar,
	jason, kyle.teske, nico, Felipe Balbi, m.chehab, davidb,
	Rob Landley, davem, cesarb, sameo, Andrew Morton, Linus Walleij,
	pawel.moll, mark.rutland, ijc+devicetree, galak, devel,
	Alan Tull, dinguyen, yvanderv

On Wed, Jan 21, 2015 at 06:33:12PM +0200, Pantelis Antoniou wrote:
> Hi Alan,
> 
> > On Jan 21, 2015, at 18:01 , One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> wrote:
> > 
> > On Thu, 15 Jan 2015 22:54:46 +0200
> > Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:
> > 
> >> Hi Alan,
> >> 
> >>> On Jan 15, 2015, at 22:45 , One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> wrote:
> >>> 
> >>> On Thu, 15 Jan 2015 11:47:26 -0700
> >>> Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:
> >>>> It is a novel idea, my concern would be that embedding the FPGA in the
> >>>> DT makes it permanent unswappable kernel memory.
> >>>> Not having the kernel hold the FPGA is best for many uses.
> >>> 
> >>> If you have a filesysytem before the FPGA is set up then it belongs in
> >>> the file system. As you presumably loaded the kernel from somewhere there
> >>> ought to be a file system (even an initrd).
> >>> 
> >> 
> >> Request firmware does not imply keeping it around. You can always re-request
> >> when reloading (although there’s a nasty big of caching that needs to be
> >> resolved with the firmware loader).
> > 
> > Which comes down to the same thing. Unless you can prove that there is a
> > path to recover the firmware file that does not have any dependancies
> > upon the firmware executing (and those can be subtle and horrid at times)
> > you need to keep it around for suspend/resume at least and potentially
> > any unexpected error/reset.
> > 
> 
> In that case the only safe place to put it is in the kernel image itself, which
> is something the firmware loader already supports.

My point is that the current firmware layer is overly cautious and
FPGAs are very big. My current project on small Xilinx device has a
10MB programming file. The biggest Xilinx device today has a max
bitfile size around 122MB.

So keeping that much memory pinned in the kernel when I can prove it
is uncessary for my system (either because there is no suspend/resume
possibility, or because I know the CPU can always access the
filesytem) is very undesirable.

Other systems might have to take the ram hit.

Since it seems like the kernel has no way to know, we probably have
have a way to tell it not to cache the bitfile.

Jason

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-21 20:27                                 ` Jason Gunthorpe
@ 2015-01-21 20:32                                   ` Pantelis Antoniou
  2015-02-15 22:40                                   ` Pavel Machek
  1 sibling, 0 replies; 64+ messages in thread
From: Pantelis Antoniou @ 2015-01-21 20:32 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: One Thousand Gnomes, atull, Greg Kroah-Hartman, Pavel Machek,
	hpa, Michal Simek, Michal Simek, Randy Dunlap,
	Linux Kernel Mailing List, devicetree, robh+dt, Grant Likely,
	iws, linux-doc, Mark Brown, philip, rubini, Steffen Trumtrar,
	jason, kyle.teske, nico, Felipe Balbi, m.chehab, davidb,
	Rob Landley, davem, cesarb, sameo, Andrew Morton, Linus Walleij,
	pawel.moll, mark.rutland, ijc+devicetree, galak, devel,
	Alan Tull, dinguyen, yvanderv

Hi Jason,

> On Jan 21, 2015, at 22:27 , Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:
> 
> On Wed, Jan 21, 2015 at 06:33:12PM +0200, Pantelis Antoniou wrote:
>> Hi Alan,
>> 
>>> On Jan 21, 2015, at 18:01 , One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> wrote:
>>> 
>>> On Thu, 15 Jan 2015 22:54:46 +0200
>>> Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:
>>> 
>>>> Hi Alan,
>>>> 
>>>>> On Jan 15, 2015, at 22:45 , One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> wrote:
>>>>> 
>>>>> On Thu, 15 Jan 2015 11:47:26 -0700
>>>>> Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:
>>>>>> It is a novel idea, my concern would be that embedding the FPGA in the
>>>>>> DT makes it permanent unswappable kernel memory.
>>>>>> Not having the kernel hold the FPGA is best for many uses.
>>>>> 
>>>>> If you have a filesysytem before the FPGA is set up then it belongs in
>>>>> the file system. As you presumably loaded the kernel from somewhere there
>>>>> ought to be a file system (even an initrd).
>>>>> 
>>>> 
>>>> Request firmware does not imply keeping it around. You can always re-request
>>>> when reloading (although there’s a nasty big of caching that needs to be
>>>> resolved with the firmware loader).
>>> 
>>> Which comes down to the same thing. Unless you can prove that there is a
>>> path to recover the firmware file that does not have any dependancies
>>> upon the firmware executing (and those can be subtle and horrid at times)
>>> you need to keep it around for suspend/resume at least and potentially
>>> any unexpected error/reset.
>>> 
>> 
>> In that case the only safe place to put it is in the kernel image itself, which
>> is something the firmware loader already supports.
> 
> My point is that the current firmware layer is overly cautious and
> FPGAs are very big. My current project on small Xilinx device has a
> 10MB programming file. The biggest Xilinx device today has a max
> bitfile size around 122MB.
> 
> So keeping that much memory pinned in the kernel when I can prove it
> is uncessary for my system (either because there is no suspend/resume
> possibility, or because I know the CPU can always access the
> filesytem) is very undesirable.
> 
> Other systems might have to take the ram hit.
> 
> Since it seems like the kernel has no way to know, we probably have
> have a way to tell it not to cache the bitfile.
> 

The firmware loader was not originally meant to handle these cases, but
I’m sure it’s not an insurmountable obstacle.

> Jason

Regards

— Pantelis


^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-01-21 20:27                                 ` Jason Gunthorpe
  2015-01-21 20:32                                   ` Pantelis Antoniou
@ 2015-02-15 22:40                                   ` Pavel Machek
  2015-02-17 17:07                                     ` Rob Landley
  2015-02-17 18:12                                     ` Jason Gunthorpe
  1 sibling, 2 replies; 64+ messages in thread
From: Pavel Machek @ 2015-02-15 22:40 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Pantelis Antoniou, One Thousand Gnomes, atull,
	Greg Kroah-Hartman, hpa, Michal Simek, Michal Simek,
	Randy Dunlap, Linux Kernel Mailing List, devicetree, robh+dt,
	Grant Likely, iws, linux-doc, Mark Brown, philip, rubini,
	Steffen Trumtrar, jason, kyle.teske, nico, Felipe Balbi,
	m.chehab, davidb, Rob Landley, davem, cesarb, sameo,
	Andrew Morton, Linus Walleij, pawel.moll, mark.rutland,
	ijc+devicetree, galak, devel, Alan Tull, dinguyen, yvanderv

On Wed 2015-01-21 13:27:00, Jason Gunthorpe wrote:
> On Wed, Jan 21, 2015 at 06:33:12PM +0200, Pantelis Antoniou wrote:
> > Hi Alan,
> > 
> > > On Jan 21, 2015, at 18:01 , One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> wrote:
> > > 
> > > On Thu, 15 Jan 2015 22:54:46 +0200
> > > Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:
> > > 
> > >> Hi Alan,
> > >> 
> > >>> On Jan 15, 2015, at 22:45 , One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk> wrote:
> > >>> 
> > >>> On Thu, 15 Jan 2015 11:47:26 -0700
> > >>> Jason Gunthorpe <jgunthorpe@obsidianresearch.com> wrote:
> > >>>> It is a novel idea, my concern would be that embedding the FPGA in the
> > >>>> DT makes it permanent unswappable kernel memory.
> > >>>> Not having the kernel hold the FPGA is best for many uses.
> > >>> 
> > >>> If you have a filesysytem before the FPGA is set up then it belongs in
> > >>> the file system. As you presumably loaded the kernel from somewhere there
> > >>> ought to be a file system (even an initrd).
> > >>> 
> > >> 
> > >> Request firmware does not imply keeping it around. You can always re-request
> > >> when reloading (although there’s a nasty big of caching that needs to be
> > >> resolved with the firmware loader).
> > > 
> > > Which comes down to the same thing. Unless you can prove that there is a
> > > path to recover the firmware file that does not have any dependancies
> > > upon the firmware executing (and those can be subtle and horrid at times)
> > > you need to keep it around for suspend/resume at least and potentially
> > > any unexpected error/reset.
> > > 
> > 
> > In that case the only safe place to put it is in the kernel image itself, which
> > is something the firmware loader already supports.
> 
> My point is that the current firmware layer is overly cautious and
> FPGAs are very big. My current project on small Xilinx device has a
> 10MB programming file. The biggest Xilinx device today has a max
> bitfile size around 122MB.
> 
> So keeping that much memory pinned in the kernel when I can prove it
> is uncessary for my system (either because there is no suspend/resume
> possibility, or because I know the CPU can always access the
> filesytem) is very undesirable.

Well, your current device aalso has 1GB RAM, no?

> Other systems might have to take the ram hit.

I'd say the general case is "store bitstream in RAM" we can add
optimalizations later.
										Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-02-15 22:40                                   ` Pavel Machek
@ 2015-02-17 17:07                                     ` Rob Landley
  2015-02-17 19:17                                       ` Pavel Machek
  2015-02-17 18:12                                     ` Jason Gunthorpe
  1 sibling, 1 reply; 64+ messages in thread
From: Rob Landley @ 2015-02-17 17:07 UTC (permalink / raw)
  To: Pavel Machek, Jason Gunthorpe
  Cc: Pantelis Antoniou, One Thousand Gnomes, atull,
	Greg Kroah-Hartman, hpa, Michal Simek, Michal Simek,
	Randy Dunlap, Linux Kernel Mailing List, devicetree, robh+dt,
	Grant Likely, iws, linux-doc, Mark Brown, philip, rubini,
	Steffen Trumtrar, jason, kyle.teske, nico, Felipe Balbi,
	m.chehab, davidb, davem, cesarb, sameo, Andrew Morton,
	Linus Walleij, pawel.moll, mark.rutland, ijc+devicetree, galak,
	devel, Alan Tull, dinguyen, yvanderv



On 02/15/2015 04:40 PM, Pavel Machek wrote:
> On Wed 2015-01-21 13:27:00, Jason Gunthorpe wrote:
>> On Wed, Jan 21, 2015 at 06:33:12PM +0200, Pantelis Antoniou wrote:
>> My point is that the current firmware layer is overly cautious and
>> FPGAs are very big. My current project on small Xilinx device has a
>> 10MB programming file. The biggest Xilinx device today has a max
>> bitfile size around 122MB.
>>
>> So keeping that much memory pinned in the kernel when I can prove it
>> is uncessary for my system (either because there is no suspend/resume
>> possibility, or because I know the CPU can always access the
>> filesytem) is very undesirable.
> 
> Well, your current device aalso has 1GB RAM, no?

Unnecessarily pinning 10% of your ram is a good solution?

> I'd say the general case is "store bitstream in RAM" we can add
> optimalizations later.

We _have_ a firmware loading layer that doesn't store other kinds of
firmware in pinned kernel memory. I guess that was retroactively a bad
idea. As was the "freeing kernel memory" part at the end of boot.

There honestly are small embedded devices. Still. When you have your
DRAM on the SOC instead of as an external chip, a gigabyte is not in the
cards.

If you're saying "Linux should not care about this niche, let's open
ourselves to a new disruptive technology attack with that as its initial
protected base, because the average kernel developer age is about 44 now
and we're not really recruiting anybody younger than that, so we'll all
be dead before we have to care about being replaced"...

Rob

(At $DAYJOB we're building a chip that runs off of induction current to
retrofit monitoring sensors onto things. It currently has 64 megs of ram
but they're trying to trim that _down_ to free up chip space.)

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-02-15 22:40                                   ` Pavel Machek
  2015-02-17 17:07                                     ` Rob Landley
@ 2015-02-17 18:12                                     ` Jason Gunthorpe
  1 sibling, 0 replies; 64+ messages in thread
From: Jason Gunthorpe @ 2015-02-17 18:12 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Pantelis Antoniou, One Thousand Gnomes, atull,
	Greg Kroah-Hartman, hpa, Michal Simek, Michal Simek,
	Randy Dunlap, Linux Kernel Mailing List, devicetree, robh+dt,
	Grant Likely, iws, linux-doc, Mark Brown, philip, rubini,
	Steffen Trumtrar, jason, kyle.teske, nico, Felipe Balbi,
	m.chehab, davidb, Rob Landley, davem, cesarb, sameo,
	Andrew Morton, Linus Walleij, pawel.moll, mark.rutland,
	ijc+devicetree, galak, devel, Alan Tull, dinguyen, yvanderv

On Sun, Feb 15, 2015 at 11:40:06PM +0100, Pavel Machek wrote:

> > So keeping that much memory pinned in the kernel when I can prove it
> > is uncessary for my system (either because there is no suspend/resume
> > possibility, or because I know the CPU can always access the
> > filesytem) is very undesirable.
> 
> Well, your current device aalso has 1GB RAM, no?

No, certainly not. 256MB - the board only has space for two x16 DDR3
chips, and at design time 1GBit was about the biggest that could be
reasonably obtained.

It is also using a Zynq chip for management and the next firmware spin
is likely to throw away 50% of that space to enable Xilinx's narrow ECC
scheme on the ram.

The Linux environment requires only about 40M of ram for runtime, as
it was designed for systems with only 64M of ram, so even the 128M is
overkill.

Jason

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-02-17 17:07                                     ` Rob Landley
@ 2015-02-17 19:17                                       ` Pavel Machek
  2015-02-19 12:46                                         ` Michal Simek
  0 siblings, 1 reply; 64+ messages in thread
From: Pavel Machek @ 2015-02-17 19:17 UTC (permalink / raw)
  To: Rob Landley
  Cc: Jason Gunthorpe, Pantelis Antoniou, One Thousand Gnomes, atull,
	Greg Kroah-Hartman, hpa, Michal Simek, Michal Simek,
	Randy Dunlap, Linux Kernel Mailing List, devicetree, robh+dt,
	Grant Likely, iws, linux-doc, Mark Brown, philip, rubini,
	Steffen Trumtrar, jason, kyle.teske, nico, Felipe Balbi,
	m.chehab, davidb, davem, cesarb, sameo, Andrew Morton,
	Linus Walleij, pawel.moll, mark.rutland, ijc+devicetree, galak,
	devel, Alan Tull, dinguyen, yvanderv

On Tue 2015-02-17 11:07:53, Rob Landley wrote:
> 
> 
> On 02/15/2015 04:40 PM, Pavel Machek wrote:
> > On Wed 2015-01-21 13:27:00, Jason Gunthorpe wrote:
> >> On Wed, Jan 21, 2015 at 06:33:12PM +0200, Pantelis Antoniou wrote:
> >> My point is that the current firmware layer is overly cautious and
> >> FPGAs are very big. My current project on small Xilinx device has a
> >> 10MB programming file. The biggest Xilinx device today has a max
> >> bitfile size around 122MB.
> >>
> >> So keeping that much memory pinned in the kernel when I can prove it
> >> is uncessary for my system (either because there is no suspend/resume
> >> possibility, or because I know the CPU can always access the
> >> filesytem) is very undesirable.
> > 
> > Well, your current device aalso has 1GB RAM, no?
> 
> Unnecessarily pinning 10% of your ram is a good solution?

Never said that. But I'd rather have _some_ API proposed, then try to
design in everthing including kitchen sink and do nothing.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-02-17 19:17                                       ` Pavel Machek
@ 2015-02-19 12:46                                         ` Michal Simek
  2015-02-21  6:31                                           ` atull
  0 siblings, 1 reply; 64+ messages in thread
From: Michal Simek @ 2015-02-19 12:46 UTC (permalink / raw)
  To: Pavel Machek, Rob Landley
  Cc: Jason Gunthorpe, Pantelis Antoniou, One Thousand Gnomes, atull,
	Greg Kroah-Hartman, hpa, Michal Simek, Michal Simek,
	Randy Dunlap, Linux Kernel Mailing List, devicetree, robh+dt,
	Grant Likely, iws, linux-doc, Mark Brown, philip, rubini,
	Steffen Trumtrar, jason, kyle.teske, nico, Felipe Balbi,
	m.chehab, davidb, davem, cesarb, sameo, Andrew Morton,
	Linus Walleij, pawel.moll, mark.rutland, ijc+devicetree, galak,
	devel, Alan Tull, dinguyen, yvanderv

On 02/17/2015 08:17 PM, Pavel Machek wrote:
> On Tue 2015-02-17 11:07:53, Rob Landley wrote:
>>
>>
>> On 02/15/2015 04:40 PM, Pavel Machek wrote:
>>> On Wed 2015-01-21 13:27:00, Jason Gunthorpe wrote:
>>>> On Wed, Jan 21, 2015 at 06:33:12PM +0200, Pantelis Antoniou wrote:
>>>> My point is that the current firmware layer is overly cautious and
>>>> FPGAs are very big. My current project on small Xilinx device has a
>>>> 10MB programming file. The biggest Xilinx device today has a max
>>>> bitfile size around 122MB.
>>>>
>>>> So keeping that much memory pinned in the kernel when I can prove it
>>>> is uncessary for my system (either because there is no suspend/resume
>>>> possibility, or because I know the CPU can always access the
>>>> filesytem) is very undesirable.
>>>
>>> Well, your current device aalso has 1GB RAM, no?
>>
>> Unnecessarily pinning 10% of your ram is a good solution?
> 
> Never said that. But I'd rather have _some_ API proposed, then try to
> design in everthing including kitchen sink and do nothing.

+1 on this.

Thanks,
Michal


^ permalink raw reply	[flat|nested] 64+ messages in thread

* Re: [PATCH v8 2/4] fpga manager: add sysfs interface document
  2015-02-19 12:46                                         ` Michal Simek
@ 2015-02-21  6:31                                           ` atull
  0 siblings, 0 replies; 64+ messages in thread
From: atull @ 2015-02-21  6:31 UTC (permalink / raw)
  To: Jason Gunthorpe, Ming Lei, Michal Simek
  Cc: Pavel Machek, Rob Landley, Pantelis Antoniou,
	One Thousand Gnomes, Greg Kroah-Hartman, hpa, Michal Simek,
	Randy Dunlap, Linux Kernel Mailing List, devicetree, robh+dt,
	Grant Likely, iws, linux-doc, Mark Brown, philip, rubini,
	Steffen Trumtrar, jason, kyle.teske, nico, Felipe Balbi,
	m.chehab, davidb, davem, cesarb, sameo, Andrew Morton,
	Linus Walleij, pawel.moll, mark.rutland, ijc+devicetree, galak,
	devel, Alan Tull, dinguyen, yvanderv

On Thu, 19 Feb 2015, Michal Simek wrote:

> On 02/17/2015 08:17 PM, Pavel Machek wrote:
> > On Tue 2015-02-17 11:07:53, Rob Landley wrote:
> >>
> >>
> >> On 02/15/2015 04:40 PM, Pavel Machek wrote:
> >>> On Wed 2015-01-21 13:27:00, Jason Gunthorpe wrote:
> >>>> On Wed, Jan 21, 2015 at 06:33:12PM +0200, Pantelis Antoniou wrote:
> >>>> My point is that the current firmware layer is overly cautious and
> >>>> FPGAs are very big. My current project on small Xilinx device has a
> >>>> 10MB programming file. The biggest Xilinx device today has a max
> >>>> bitfile size around 122MB.
> >>>>
> >>>> So keeping that much memory pinned in the kernel when I can prove it
> >>>> is uncessary for my system (either because there is no suspend/resume
> >>>> possibility, or because I know the CPU can always access the
> >>>> filesytem) is very undesirable.
> >>>
> >>> Well, your current device aalso has 1GB RAM, no?
> >>
> >> Unnecessarily pinning 10% of your ram is a good solution?
> > 
> > Never said that. But I'd rather have _some_ API proposed, then try to
> > design in everthing including kitchen sink and do nothing.
> 

I propose an extension to the firmware class:

int request_firmware_streamed(const struct firmware **firmware_p,
                              const char *name,
                              struct device *device,
                              int (*consumer)(char *buf,
                                              size_t size,
                                              void *priv))

This is a new function that streams the firmware file in 4k chunks to
a callback function.  So firmware is not limited to the allocation
size of vmalloc.

This new function would have the same parameters as request_firmware except
adding a pointer to a consumer function.  In the case of fpga's, the
consumer function is writing the 4k chunks to the fpga.

The new function will:
 * open the file
 * read 4k
 * hand that buffer to the consumer
 * sleep until the consumer returns
 * give up if consumer has a nonzero exit, continue reading otherwise

Admittedly this is really different from the current firmeware class's
structures.

The real problem this is trying to solve is that bitstreams can be huge,
especially as fpga's can be daisychained.  And the CPU doing the loading
could be embedded (limited resources).  IFAIK fpga's don't need to have
the whole image in a ram buffer.

This gives us the convenience of request_firmware() and gives the kernel an
extension of the firmware class that others might useful instead of solving
this problem that we keep hovering around for fpga's only.

Alan Tull


^ permalink raw reply	[flat|nested] 64+ messages in thread

end of thread, other threads:[~2015-02-21  7:05 UTC | newest]

Thread overview: 64+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-06 20:13 [PATCH v8 0/4] FPGA Manager Framework atull
2015-01-06 20:13 ` [PATCH v8 1/4] doc: add bindings document for altera fpga manager atull
2015-01-06 22:05   ` Rob Herring
2015-01-06 22:34     ` atull
2015-01-09 15:50       ` Rob Herring
2015-01-09 18:58         ` atull
2015-01-06 20:13 ` [PATCH v8 2/4] fpga manager: add sysfs interface document atull
2015-01-07  8:48   ` Pavel Machek
2015-01-09 19:14     ` atull
2015-01-09 20:56       ` Pavel Machek
2015-01-10  8:10         ` Pantelis Antoniou
2015-01-10 15:11           ` Pavel Machek
2015-01-11 16:29             ` atull
2015-01-12  8:45               ` Pavel Machek
2015-01-12 13:48                 ` Michal Simek
2015-01-13  7:28                   ` Pavel Machek
2015-01-13  7:40                     ` Pantelis Antoniou
2015-01-13  7:56                       ` Pavel Machek
2015-01-13 17:27                         ` atull
2015-01-12 16:05               ` Rob Herring
2015-01-12 16:26                 ` Mark Brown
2015-01-12 18:06               ` Jason Gunthorpe
2015-01-13 16:21                 ` One Thousand Gnomes
2015-01-15 21:52                   ` Pavel Machek
2015-01-12 21:01         ` One Thousand Gnomes
2015-01-12 21:43           ` Jason Gunthorpe
2015-01-13 16:28             ` One Thousand Gnomes
2015-01-13 17:26               ` Pantelis Antoniou
2015-01-13 19:44                 ` atull
2015-01-14 15:58                 ` One Thousand Gnomes
2015-01-13 20:00               ` Jason Gunthorpe
2015-01-13 21:37                 ` atull
2015-01-13 22:24                   ` Jason Gunthorpe
2015-01-14 16:06                     ` One Thousand Gnomes
2015-01-14 18:12                       ` Jason Gunthorpe
2015-01-14 19:01                         ` Pantelis Antoniou
2015-01-15 11:36                         ` One Thousand Gnomes
2015-01-15 11:44                           ` Mark Brown
2015-01-15 16:34                     ` atull
2015-01-15 18:47                       ` Jason Gunthorpe
2015-01-15 20:45                         ` One Thousand Gnomes
2015-01-15 20:54                           ` Pantelis Antoniou
2015-01-21 16:01                             ` One Thousand Gnomes
2015-01-21 16:33                               ` Pantelis Antoniou
2015-01-21 20:27                                 ` Jason Gunthorpe
2015-01-21 20:32                                   ` Pantelis Antoniou
2015-02-15 22:40                                   ` Pavel Machek
2015-02-17 17:07                                     ` Rob Landley
2015-02-17 19:17                                       ` Pavel Machek
2015-02-19 12:46                                         ` Michal Simek
2015-02-21  6:31                                           ` atull
2015-02-17 18:12                                     ` Jason Gunthorpe
2015-01-15 21:42                           ` Jason Gunthorpe
2015-01-17 21:11                       ` Pavel Machek
2015-01-06 20:13 ` [PATCH v8 3/4] staging: fpga manager: framework core atull
2015-01-06 20:13 ` [PATCH v8 4/4] staging: fpga manager: add driver for socfpga fpga manager atull
2015-01-10 18:11 ` [PATCH v8 0/4] FPGA Manager Framework Konrad Zapalowicz
2015-01-11 16:08   ` atull
2015-01-11 16:24     ` Konrad Zapalowicz
2015-01-11 19:52       ` Pavel Machek
2015-01-11 20:58         ` Konrad Zapalowicz
2015-01-11 21:31           ` Pavel Machek
2015-01-12 13:50             ` Michal Simek
2015-01-12 14:06         ` Dan Carpenter

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).