From: Alan Tull <[email protected]>
Improvements in this v3 and v4:
I've moved the driver to drivers/staging.
I'm including the altera low level driver, defconfig changes, and
DTS changes - everything to make it work on socfpga platform.
Fixed things in the framework that were left over from my original
version (that Michal started with) that was a character driver.
In particular, make the device be present in the fpga_manager struct,
not as a pointer, so container_of will work.
Since not enough of the Device Tree Overlays code is in the main
kernel yet, I'm leaving out the (small) changes to make this accept
notifications when a device tree overlay is accepted. That can
wait a little until enough of is in that it can build and work.
That is a little disappointing to leave it out for now because
it was very cool and, I think, more Linux-like than this is by
itself.
Alan Tull (6):
doc: add bindings document for altera fpga manager
arm: dts: socfpga: add altera fpga manager
ARM: socfpga: defconfig: enable fpga manager
fpga manager: add sysfs interface document
staging: fpga manager: framework core
staging: fpga manager: add driver for altera socfpga manager
Documentation/ABI/testing/sysfs-class-fpga-manager | 38 +
.../devicetree/bindings/fpga/altera-fpga-mgr.txt | 17 +
arch/arm/boot/dts/socfpga.dtsi | 10 +
arch/arm/configs/socfpga_defconfig | 4 +
drivers/staging/Kconfig | 2 +
drivers/staging/Makefile | 1 +
drivers/staging/fpga/Kconfig | 27 +
drivers/staging/fpga/Makefile | 11 +
drivers/staging/fpga/altera.c | 789 ++++++++++++++++++++
drivers/staging/fpga/fpga-mgr.c | 485 ++++++++++++
include/linux/fpga-mgr.h | 104 +++
11 files changed, 1488 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-class-fpga-manager
create mode 100644 Documentation/devicetree/bindings/fpga/altera-fpga-mgr.txt
create mode 100644 drivers/staging/fpga/Kconfig
create mode 100644 drivers/staging/fpga/Makefile
create mode 100644 drivers/staging/fpga/altera.c
create mode 100644 drivers/staging/fpga/fpga-mgr.c
create mode 100644 include/linux/fpga-mgr.h
--
1.7.9.5
From: Alan Tull <[email protected]>
New bindings document for Altera fpga manager.
Signed-off-by: Alan Tull <[email protected]>
---
.../devicetree/bindings/fpga/altera-fpga-mgr.txt | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
create mode 100644 Documentation/devicetree/bindings/fpga/altera-fpga-mgr.txt
diff --git a/Documentation/devicetree/bindings/fpga/altera-fpga-mgr.txt b/Documentation/devicetree/bindings/fpga/altera-fpga-mgr.txt
new file mode 100644
index 0000000..373af7b
--- /dev/null
+++ b/Documentation/devicetree/bindings/fpga/altera-fpga-mgr.txt
@@ -0,0 +1,17 @@
+Altera FPGA Manager
+
+Required properties:
+- compatible : should contain "altr,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,fpga-mgr";
+ reg = <0xFF706000 0x1000
+ 0xFFB90000 0x1000>;
+ interrupts = <0 175 4>;
+ };
--
1.7.9.5
From: Alan Tull <[email protected]>
Add Altera FGPA manager to device tree.
Signed-off-by: Alan Tull <[email protected]>
---
arch/arm/boot/dts/socfpga.dtsi | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index 4472fd9..bab98b6 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -751,5 +751,15 @@
compatible = "altr,sys-mgr", "syscon";
reg = <0xffd08000 0x4000>;
};
+
+ hps_0_fpgamgr: fpgamgr@0xff706000 {
+ compatible = "altr,fpga-mgr", "simple-bus";
+ reg = <0xFF706000 0x1000
+ 0xFFB90000 0x1000>;
+ interrupts = <0 175 4>;
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+ };
};
};
--
1.7.9.5
From: Alan Tull <[email protected]>
Add documentation for new fpga manager sysfs interface.
Signed-off-by: Alan Tull <[email protected]>
---
Documentation/ABI/testing/sysfs-class-fpga-manager | 38 ++++++++++++++++++++
1 file changed, 38 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-class-fpga-manager
diff --git a/Documentation/ABI/testing/sysfs-class-fpga-manager b/Documentation/ABI/testing/sysfs-class-fpga-manager
new file mode 100644
index 0000000..eb600f2
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-fpga-manager
@@ -0,0 +1,38 @@
+What: /sys/class/fpga_manager/<fpga>/name
+Date: October 2014
+KernelVersion: 3.18
+Contact: Alan Tull <[email protected]>
+Description: Name of low level driver.
+
+What: /sys/class/fpga_manager/<fpga>/firmware
+Date: October 2014
+KernelVersion: 3.18
+Contact: Alan Tull <[email protected]>
+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 <[email protected]>
+Description: Write 1 to reset the FPGA
+
+What: /sys/class/fpga_manager/<fpga>/state
+Date: October 2014
+KernelVersion: 3.18
+Contact: Alan Tull <[email protected]>
+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
From: Alan Tull <[email protected]>
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 <[email protected]>
---
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
---
drivers/staging/Kconfig | 2 +
drivers/staging/Makefile | 1 +
drivers/staging/fpga/Kconfig | 21 ++
drivers/staging/fpga/Makefile | 10 +
drivers/staging/fpga/fpga-mgr.c | 485 +++++++++++++++++++++++++++++++++++++++
include/linux/fpga-mgr.h | 104 +++++++++
6 files changed, 623 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-mgr.h
diff --git a/drivers/staging/Kconfig b/drivers/staging/Kconfig
index 4690ae9..4338a4c 100644
--- a/drivers/staging/Kconfig
+++ b/drivers/staging/Kconfig
@@ -108,4 +108,6 @@ source "drivers/staging/skein/Kconfig"
source "drivers/staging/unisys/Kconfig"
+source "drivers/staging/fpga/Kconfig"
+
endif # STAGING
diff --git a/drivers/staging/Makefile b/drivers/staging/Makefile
index c780a0e..43654a2 100644
--- a/drivers/staging/Makefile
+++ b/drivers/staging/Makefile
@@ -46,3 +46,4 @@ obj-$(CONFIG_MTD_SPINAND_MT29F) += mt29f_spinand/
obj-$(CONFIG_GS_FPGABOOT) += gs_fpgaboot/
obj-$(CONFIG_CRYPTO_SKEIN) += skein/
obj-$(CONFIG_UNISYSSPAR) += unisys/
+obj-$(CONFIG_FPGA) += fpga/
diff --git a/drivers/staging/fpga/Kconfig b/drivers/staging/fpga/Kconfig
new file mode 100644
index 0000000..e775b17
--- /dev/null
+++ b/drivers/staging/fpga/Kconfig
@@ -0,0 +1,21 @@
+#
+# 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.
+
+config FPGA_MGR_SYSFS
+ bool "FPGA Manager SysFS Interface"
+ depends on FPGA
+ depends on SYSFS
+ help
+ FPGA Manager SysFS interface.
+
+endmenu
diff --git a/drivers/staging/fpga/Makefile b/drivers/staging/fpga/Makefile
new file mode 100644
index 0000000..c8a676f
--- /dev/null
+++ b/drivers/staging/fpga/Makefile
@@ -0,0 +1,10 @@
+#
+# Makefile for the fpga framework and fpga manager drivers.
+#
+
+fpga-mgr-core-y += fpga-mgr.o
+
+# Core FPGA Manager Framework
+obj-$(CONFIG_FPGA) += fpga-mgr-core.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..d08cb82
--- /dev/null
+++ b/drivers/staging/fpga/fpga-mgr.c
@@ -0,0 +1,485 @@
+/*
+ * 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-mgr.h>
+#include <linux/fs.h>
+#include <linux/idr.h>
+#include <linux/interrupt.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/pm.h>
+#include <linux/slab.h>
+#include <linux/vmalloc.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);
+
+/*
+ * Get FPGA state from low level driver
+ * return value is same enum as fpga_mgr_framework_state
+ *
+ * This will be used to initialize framework state
+ */
+int 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);
+}
+
+/*
+ * Unlocked version
+ */
+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);
+
+ return ret;
+}
+
+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);
+
+/*
+ * Unlocked
+ */
+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;
+}
+
+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;
+}
+
+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;
+}
+
+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);
+}
+
+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);
+
+/*
+ * 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.
+ */
+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;
+
+ strcpy(mgr->image_name, image_name);
+
+fw_wr_exit:
+ mutex_unlock(&mgr->lock);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(fpga_mgr_firmware_write);
+
+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);
+
+#if IS_ENABLED(CONFIG_FPGA_MGR_SYSFS)
+const char *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 fpga_mgr_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 fpga_mgr_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 fpga_mgr_firmware_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct fpga_manager *mgr = to_fpga_manager(dev);
+
+ return sprintf(buf, "%s\n", mgr->image_name);
+}
+
+static ssize_t fpga_mgr_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 fpga_mgr_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(name, S_IRUGO, fpga_mgr_name_show, NULL);
+static DEVICE_ATTR(state, S_IRUGO, fpga_mgr_state_show, NULL);
+static DEVICE_ATTR(firmware, S_IRUGO | S_IWUSR,
+ fpga_mgr_firmware_show, fpga_mgr_firmware_store);
+static DEVICE_ATTR(reset, S_IWUSR, NULL, fpga_mgr_reset_store);
+
+static struct attribute *fpga_mgr_attrs[] = {
+ &dev_attr_name.attr,
+ &dev_attr_state.attr,
+ &dev_attr_firmware.attr,
+ &dev_attr_reset.attr,
+ NULL,
+};
+
+static const struct attribute_group fpga_mgr_group = {
+ .attrs = fpga_mgr_attrs,
+};
+
+const struct attribute_group *fpga_mgr_groups[] = {
+ &fpga_mgr_group,
+ NULL,
+};
+#else
+#define fpga_mgr_groups NULL
+#endif /* CONFIG_FPGA_MGR_SYSFS */
+
+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;
+}
+
+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", dev_name(dev));
+ kfree(mgr);
+}
+
+int fpga_mgr_register(struct device *dev, const char *name,
+ struct fpga_manager_ops *mops,
+ void *priv)
+{
+ struct fpga_manager *mgr;
+ int id, ret;
+
+ BUG_ON(!mops || !name || !strlen(name));
+
+ id = ida_simple_get(&fpga_mgr_ida, 0, 0, GFP_KERNEL);
+ if (id < 0)
+ return id;
+
+ mgr = kzalloc(sizeof(*mgr), GFP_KERNEL);
+ if (!mgr)
+ return -ENOMEM;
+
+ 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, "fpga manager [%s] registered as id %d\n",
+ dev_name(&mgr->dev), id);
+
+ return 0;
+
+error_device:
+ ida_simple_remove(&fpga_mgr_ida, id);
+ kfree(mgr);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(fpga_mgr_register);
+
+void fpga_mgr_remove(struct device *dev)
+{
+ struct fpga_manager *mgr = to_fpga_manager(dev);
+ int id;
+
+ if (mgr)
+ id = mgr->dev.id;
+
+ device_unregister(dev);
+
+ if (mgr) {
+ if (mgr->mops->fpga_remove)
+ mgr->mops->fpga_remove(mgr);
+
+ mutex_lock(&fpga_mgr_mutex);
+ list_del(&mgr->list);
+ mutex_unlock(&fpga_mgr_mutex);
+ kfree(mgr);
+ ida_simple_remove(&fpga_mgr_ida, id);
+ }
+}
+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);
+
+ 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 <[email protected]>");
+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-mgr.h b/include/linux/fpga-mgr.h
new file mode 100644
index 0000000..7ac762c
--- /dev/null
+++ b/include/linux/fpga-mgr.h
@@ -0,0 +1,104 @@
+/*
+ * 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;
+
+/*
+ * fpga_manager_ops are the low level functions implemented by a specific
+ * fpga manager driver. Leaving any of these out that aren't needed is fine
+ * as they are all tested for NULL before being called.
+ */
+struct fpga_manager_ops {
+ /* Returns an enum value of the FPGA's state */
+ int (*state)(struct fpga_manager *mgr);
+
+ /* Put FPGA into reset state */
+ int (*reset)(struct fpga_manager *mgr);
+
+ /* Prepare the FPGA to receive confuration data */
+ int (*write_init)(struct fpga_manager *mgr);
+
+ /* Write count bytes of configuration data to the FPGA */
+ int (*write)(struct fpga_manager *mgr, const char *buf, size_t count);
+
+ /* Return FPGA to default state after writing is done */
+ int (*write_complete)(struct fpga_manager *mgr);
+
+ /* Optional: Set FPGA into a specific state during driver remove */
+ void (*fpga_remove)(struct fpga_manager *mgr);
+
+ int (*suspend)(struct fpga_manager *mgr);
+
+ int (*resume)(struct fpga_manager *mgr);
+};
+
+/* Valid states may vary by manufacturer; superset is: */
+enum fpga_mgr_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 held in reset state */
+
+ /* write sequence */
+ 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, /* FPGA ready to receive image data */
+ FPGA_MGR_STATE_WRITE_ERR, /* Error while programming 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 */
+};
+
+struct fpga_manager {
+ const char *name;
+ struct device dev;
+ struct list_head list;
+ struct mutex lock;
+ enum fpga_mgr_states state;
+ char image_name[NAME_MAX];
+
+ struct fpga_manager_ops *mops;
+ void *priv;
+};
+
+#define to_fpga_manager(d) container_of(d, struct fpga_manager, dev)
+
+#if IS_ENABLED(CONFIG_FPGA)
+
+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,
+ struct fpga_manager_ops *mops, void *priv);
+void fpga_mgr_remove(struct device *dev);
+
+#endif /* CONFIG_FPGA */
+#endif /*_LINUX_FPGA_MGR_H */
--
1.7.9.5
From: Alan Tull <[email protected]>
Add driver to fpga manager framework to allow configuration
of FPGA in Altera SoC FPGA parts.
Signed-off-by: Alan Tull <[email protected]>
---
v2: fpga_manager struct now contains struct device
fpga_manager_register parameters now take device
v3: move to drivers/staging
---
drivers/staging/fpga/Kconfig | 6 +
drivers/staging/fpga/Makefile | 1 +
drivers/staging/fpga/altera.c | 789 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 796 insertions(+)
create mode 100644 drivers/staging/fpga/altera.c
diff --git a/drivers/staging/fpga/Kconfig b/drivers/staging/fpga/Kconfig
index e775b17..2944e3c 100644
--- a/drivers/staging/fpga/Kconfig
+++ b/drivers/staging/fpga/Kconfig
@@ -18,4 +18,10 @@ config FPGA_MGR_SYSFS
help
FPGA Manager SysFS interface.
+config FPGA_MGR_ALTERA
+ tristate "Altera"
+ depends on FPGA
+ help
+ FPGA manager driver support for configuring Altera FPGAs.
+
endmenu
diff --git a/drivers/staging/fpga/Makefile b/drivers/staging/fpga/Makefile
index c8a676f..6e0d2bf 100644
--- a/drivers/staging/fpga/Makefile
+++ b/drivers/staging/fpga/Makefile
@@ -8,3 +8,4 @@ fpga-mgr-core-y += fpga-mgr.o
obj-$(CONFIG_FPGA) += fpga-mgr-core.o
# FPGA Manager Drivers
+obj-$(CONFIG_FPGA_MGR_ALTERA) += altera.o
diff --git a/drivers/staging/fpga/altera.c b/drivers/staging/fpga/altera.c
new file mode 100644
index 0000000..14a16b4
--- /dev/null
+++ b/drivers/staging/fpga/altera.c
@@ -0,0 +1,789 @@
+/*
+ * FPGA Manager Driver for Altera FPGAs
+ *
+ * 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/cdev.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/fs.h>
+#include <linux/fpga-mgr.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/mm.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/pm.h>
+#include <linux/slab.h>
+#include <linux/string.h>
+#include <linux/types.h>
+#include <linux/regulator/consumer.h>
+
+/* Controls whether to use the Configuration with DCLK steps */
+#ifndef _ALTTERA_FPGAMGR_USE_DCLK
+#define _ALTTERA_FPGAMGR_USE_DCLK 0
+#endif
+
+/* Register offsets */
+#define ALTERA_FPGAMGR_STAT_OFST 0x0
+#define ALTERA_FPGAMGR_CTL_OFST 0x4
+#define ALTERA_FPGAMGR_DCLKCNT_OFST 0x8
+#define ALTERA_FPGAMGR_DCLKSTAT_OFST 0xc
+#define ALTERA_FPGAMGR_GPIO_INTEN_OFST 0x830
+#define ALTERA_FPGAMGR_GPIO_INTMSK_OFST 0x834
+#define ALTERA_FPGAMGR_GPIO_INTTYPE_LEVEL_OFST 0x838
+#define ALTERA_FPGAMGR_GPIO_INT_POL_OFST 0x83c
+#define ALTERA_FPGAMGR_GPIO_INTSTAT_OFST 0x840
+#define ALTERA_FPGAMGR_GPIO_RAW_INTSTAT_OFST 0x844
+#define ALTERA_FPGAMGR_GPIO_PORTA_EOI_OFST 0x84c
+#define ALTERA_FPGAMGR_GPIO_EXT_PORTA_OFST 0x850
+
+/* Register bit defines */
+/* ALTERA_FPGAMGR_STAT register mode field values */
+#define ALTERA_FPGAMGR_STAT_POWER_UP 0x0 /*ramping*/
+#define ALTERA_FPGAMGR_STAT_RESET 0x1
+#define ALTERA_FPGAMGR_STAT_CFG 0x2
+#define ALTERA_FPGAMGR_STAT_INIT 0x3
+#define ALTERA_FPGAMGR_STAT_USER_MODE 0x4
+#define ALTERA_FPGAMGR_STAT_UNKNOWN 0x5
+#define ALTERA_FPGAMGR_STAT_STATE_MASK 0x7
+/* This is a flag value that doesn't really happen in this register field */
+#define ALTERA_FPGAMGR_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 ALTERA_FPGAMGR_STAT_MSEL_MASK 0x000000f8
+#define ALTERA_FPGAMGR_STAT_MSEL_SHIFT 3
+
+/* ALTERA_FPGAMGR_CTL register */
+#define ALTERA_FPGAMGR_CTL_EN 0x00000001
+#define ALTERA_FPGAMGR_CTL_NCE 0x00000002
+#define ALTERA_FPGAMGR_CTL_NCFGPULL 0x00000004
+
+#define CDRATIO_X1 0x00000000
+#define CDRATIO_X2 0x00000040
+#define CDRATIO_X4 0x00000080
+#define CDRATIO_X8 0x000000c0
+#define ALTERA_FPGAMGR_CTL_CDRATIO_MASK 0x000000c0
+
+#define ALTERA_FPGAMGR_CTL_AXICFGEN 0x00000100
+
+#define CFGWDTH_16 0x00000000
+#define CFGWDTH_32 0x00000200
+#define ALTERA_FPGAMGR_CTL_CFGWDTH_MASK 0x00000200
+
+/* ALTERA_FPGAMGR_DCLKSTAT register */
+#define ALTERA_FPGAMGR_DCLKSTAT_DCNTDONE_E_DONE 0x1
+
+/* ALTERA_FPGAMGR_GPIO_* registers share the same bit positions */
+#define ALTERA_FPGAMGR_MON_NSTATUS 0x0001
+#define ALTERA_FPGAMGR_MON_CONF_DONE 0x0002
+#define ALTERA_FPGAMGR_MON_INIT_DONE 0x0004
+#define ALTERA_FPGAMGR_MON_CRC_ERROR 0x0008
+#define ALTERA_FPGAMGR_MON_CVP_CONF_DONE 0x0010
+#define ALTERA_FPGAMGR_MON_PR_READY 0x0020
+#define ALTERA_FPGAMGR_MON_PR_ERROR 0x0040
+#define ALTERA_FPGAMGR_MON_PR_DONE 0x0080
+#define ALTERA_FPGAMGR_MON_NCONFIG_PIN 0x0100
+#define ALTERA_FPGAMGR_MON_NSTATUS_PIN 0x0200
+#define ALTERA_FPGAMGR_MON_CONF_DONE_PIN 0x0400
+#define ALTERA_FPGAMGR_MON_FPGA_POWER_ON 0x0800
+#define ALTERA_FPGAMGR_MON_STATUS_MASK 0x0fff
+
+#define ALTERA_FPGAMGR_NUM_SUPPLIES 3
+#define ALTERA_RESUME_TIMEOUT 3
+
+#if IS_ENABLED(CONFIG_REGULATOR)
+/* In power-up order. Reverse for power-down. */
+static const char *supply_names[ALTERA_FPGAMGR_NUM_SUPPLIES] = {
+ "FPGA-1.5V",
+ "FPGA-1.1V",
+ "FPGA-2.5V",
+};
+#endif
+
+struct altera_fpga_priv {
+ void __iomem *fpga_base_addr;
+ void __iomem *fpga_data_addr;
+ struct completion status_complete;
+ int irq;
+ struct regulator *fpga_supplies[ALTERA_FPGAMGR_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 ALTERA_FPGAMGR_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 altera_fpga_reg_readl(struct altera_fpga_priv *priv, u32 reg_offset)
+{
+ return readl(priv->fpga_base_addr + reg_offset);
+}
+
+static void altera_fpga_reg_writel(struct altera_fpga_priv *priv,
+ u32 reg_offset, u32 value)
+{
+ writel(value, priv->fpga_base_addr + reg_offset);
+}
+
+static u32 altera_fpga_reg_raw_readl(struct altera_fpga_priv *priv,
+ u32 reg_offset)
+{
+ return __raw_readl(priv->fpga_base_addr + reg_offset);
+}
+
+static void altera_fpga_reg_raw_writel(struct altera_fpga_priv *priv,
+ u32 reg_offset, u32 value)
+{
+ __raw_writel(value, priv->fpga_base_addr + reg_offset);
+}
+
+static void altera_fpga_data_writel(struct altera_fpga_priv *priv, u32 value)
+{
+ writel(value, priv->fpga_data_addr);
+}
+
+static inline void altera_fpga_reg_set_bitsl(struct altera_fpga_priv *priv,
+ u32 offset, u32 bits)
+{
+ u32 val;
+
+ val = altera_fpga_reg_readl(priv, offset);
+ val |= bits;
+ altera_fpga_reg_writel(priv, offset, val);
+}
+
+static inline void altera_fpga_reg_clr_bitsl(struct altera_fpga_priv *priv,
+ u32 offset, u32 bits)
+{
+ u32 val;
+
+ val = altera_fpga_reg_readl(priv, offset);
+ val &= ~bits;
+ altera_fpga_reg_writel(priv, offset, val);
+}
+
+static int altera_fpga_mon_status_get(struct altera_fpga_priv *priv)
+{
+ return altera_fpga_reg_readl(priv,
+ ALTERA_FPGAMGR_GPIO_EXT_PORTA_OFST) &
+ ALTERA_FPGAMGR_MON_STATUS_MASK;
+}
+
+static int altera_fpga_state_get(struct altera_fpga_priv *priv)
+{
+ int status = altera_fpga_mon_status_get(priv);
+
+ if ((status & ALTERA_FPGAMGR_MON_FPGA_POWER_ON) == 0)
+ return ALTERA_FPGAMGR_STAT_POWER_OFF;
+
+ return altera_fpga_reg_readl(priv, ALTERA_FPGAMGR_STAT_OFST) &
+ ALTERA_FPGAMGR_STAT_STATE_MASK;
+}
+
+static void altera_fpga_clear_done_status(struct altera_fpga_priv *priv)
+{
+ altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_DCLKSTAT_OFST,
+ ALTERA_FPGAMGR_DCLKSTAT_DCNTDONE_E_DONE);
+}
+
+/*
+ * Set the DCLKCNT, wait for DCLKSTAT to report the count completed, and clear
+ * the complete status.
+ */
+static int altera_fpga_dclk_set_and_wait_clear(struct altera_fpga_priv *priv,
+ u32 count)
+{
+ int timeout = 2;
+ u32 done;
+
+ /* Clear any existing DONE status. */
+ if (altera_fpga_reg_readl(priv, ALTERA_FPGAMGR_DCLKSTAT_OFST))
+ altera_fpga_clear_done_status(priv);
+
+ /* Issue the DCLK count. */
+ altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_DCLKCNT_OFST, count);
+
+ /* Poll DCLKSTAT to see if it completed in the timeout period. */
+ do {
+ done = altera_fpga_reg_readl(priv,
+ ALTERA_FPGAMGR_DCLKSTAT_OFST);
+ if (done == ALTERA_FPGAMGR_DCLKSTAT_DCNTDONE_E_DONE) {
+ altera_fpga_clear_done_status(priv);
+ return 0;
+ }
+ if (count <= 4)
+ udelay(1);
+ else
+ msleep(20);
+ } while (timeout--);
+
+ return -ETIMEDOUT;
+}
+
+static int altera_fpga_wait_for_state(struct altera_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 ((altera_fpga_state_get(priv) & state) != 0)
+ return 0;
+ msleep(20);
+ } while (timeout--);
+
+ return -ETIMEDOUT;
+}
+
+static void altera_fpga_enable_irqs(struct altera_fpga_priv *priv, u32 irqs)
+{
+ /* set irqs to level sensitive */
+ altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_GPIO_INTTYPE_LEVEL_OFST, 0);
+
+ /* set interrupt polarity */
+ altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_GPIO_INT_POL_OFST, irqs);
+
+ /* clear irqs */
+ altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_GPIO_PORTA_EOI_OFST, irqs);
+
+ /* unmask interrupts */
+ altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_GPIO_INTMSK_OFST, 0);
+
+ /* enable interrupts */
+ altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_GPIO_INTEN_OFST, irqs);
+}
+
+static void altera_fpga_disable_irqs(struct altera_fpga_priv *priv)
+{
+ altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_GPIO_INTEN_OFST, 0);
+}
+
+static irqreturn_t altera_fpga_isr(int irq, void *dev_id)
+{
+ struct altera_fpga_priv *priv = dev_id;
+ u32 irqs, st;
+ bool conf_done, nstatus;
+
+ /* clear irqs */
+ irqs = altera_fpga_reg_raw_readl(priv,
+ ALTERA_FPGAMGR_GPIO_INTSTAT_OFST);
+
+ altera_fpga_reg_raw_writel(priv, ALTERA_FPGAMGR_GPIO_PORTA_EOI_OFST,
+ irqs);
+
+ st = altera_fpga_reg_raw_readl(priv,
+ ALTERA_FPGAMGR_GPIO_EXT_PORTA_OFST);
+ conf_done = (st & ALTERA_FPGAMGR_MON_CONF_DONE) != 0;
+ nstatus = (st & ALTERA_FPGAMGR_MON_NSTATUS) != 0;
+
+ /* success */
+ if (conf_done && nstatus) {
+ /* disable irqs */
+ altera_fpga_reg_raw_writel(priv,
+ ALTERA_FPGAMGR_GPIO_INTEN_OFST, 0);
+ complete(&priv->status_complete);
+ }
+
+ return IRQ_HANDLED;
+}
+
+static int altera_fpga_wait_for_config_done(struct altera_fpga_priv *priv)
+{
+ int timeout, ret = 0;
+
+ altera_fpga_disable_irqs(priv);
+ init_completion(&priv->status_complete);
+ altera_fpga_enable_irqs(priv, ALTERA_FPGAMGR_MON_CONF_DONE);
+
+ timeout = wait_for_completion_interruptible_timeout(
+ &priv->status_complete,
+ msecs_to_jiffies(10));
+ if (timeout == 0)
+ ret = -ETIMEDOUT;
+
+ altera_fpga_disable_irqs(priv);
+ return ret;
+}
+
+static int altera_fpga_cfg_mode_get(struct altera_fpga_priv *priv)
+{
+ u32 msel;
+
+ msel = altera_fpga_reg_readl(priv, ALTERA_FPGAMGR_STAT_OFST);
+ msel &= ALTERA_FPGAMGR_STAT_MSEL_MASK;
+ msel >>= ALTERA_FPGAMGR_STAT_MSEL_SHIFT;
+
+ /* Check that this MSEL setting is supported */
+ if ((msel >= sizeof(cfgmgr_modes)/sizeof(struct cfgmgr_mode)) ||
+ !cfgmgr_modes[msel].valid)
+ return -EINVAL;
+
+ return msel;
+}
+
+static int altera_fpga_cfg_mode_set(struct altera_fpga_priv *priv)
+{
+ u32 ctrl_reg, mode;
+
+ /* get value from MSEL pins */
+ mode = altera_fpga_cfg_mode_get(priv);
+ if (mode < 0)
+ return mode;
+
+ /* Adjust CTRL for the CDRATIO */
+ ctrl_reg = altera_fpga_reg_readl(priv, ALTERA_FPGAMGR_CTL_OFST);
+ ctrl_reg &= ~ALTERA_FPGAMGR_CTL_CDRATIO_MASK;
+ ctrl_reg &= ~ALTERA_FPGAMGR_CTL_CFGWDTH_MASK;
+ ctrl_reg |= cfgmgr_modes[mode].ctrl;
+
+ /* Set NCE to 0. */
+ ctrl_reg &= ~ALTERA_FPGAMGR_CTL_NCE;
+ altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_CTL_OFST, ctrl_reg);
+
+ return 0;
+}
+
+static int altera_fpga_reset(struct altera_fpga_priv *priv)
+{
+ u32 ctrl_reg, status;
+ int ret;
+
+ /*
+ * Step 1:
+ * - Set CTRL.CFGWDTH, CTRL.CDRATIO to match cfg mode
+ * - Set CTRL.NCE to 0
+ */
+ ret = altera_fpga_cfg_mode_set(priv);
+ if (ret)
+ return ret;
+
+ /* Step 2: Set CTRL.EN to 1 */
+ altera_fpga_reg_set_bitsl(priv, ALTERA_FPGAMGR_CTL_OFST,
+ ALTERA_FPGAMGR_CTL_EN);
+
+ /* Step 3: Set CTRL.NCONFIGPULL to 1 to put FPGA in reset */
+ ctrl_reg = altera_fpga_reg_readl(priv, ALTERA_FPGAMGR_CTL_OFST);
+ ctrl_reg |= ALTERA_FPGAMGR_CTL_NCFGPULL;
+ altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_CTL_OFST, ctrl_reg);
+
+ /* Step 4: Wait for STATUS.MODE to report FPGA is in reset phase */
+ status = altera_fpga_wait_for_state(priv, ALTERA_FPGAMGR_STAT_RESET);
+
+ /* Step 5: Set CONTROL.NCONFIGPULL to 0 to release FPGA from reset */
+ ctrl_reg &= ~ALTERA_FPGAMGR_CTL_NCFGPULL;
+ altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_CTL_OFST, ctrl_reg);
+
+ /* Timeout waiting for reset */
+ if (status)
+ return -ETIMEDOUT;
+
+ return 0;
+}
+
+/*
+ * Prepare the FPGA to receive the configuration data.
+ */
+static int altera_fpga_ops_configure_init(struct fpga_manager *mgr)
+{
+ struct altera_fpga_priv *priv = mgr->priv;
+ int ret;
+
+ /* Steps 1 - 5: Reset the FPGA */
+ ret = altera_fpga_reset(priv);
+ if (ret)
+ return ret;
+
+ /* Step 6: Wait for FPGA to enter configuration phase */
+ if (altera_fpga_wait_for_state(priv, ALTERA_FPGAMGR_STAT_CFG))
+ return -ETIMEDOUT;
+
+ /* Step 7: Clear nSTATUS interrupt */
+ altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_GPIO_PORTA_EOI_OFST,
+ ALTERA_FPGAMGR_MON_NSTATUS);
+
+ /* Step 8: Set CTRL.AXICFGEN to 1 to enable transfer of config data */
+ altera_fpga_reg_set_bitsl(priv, ALTERA_FPGAMGR_CTL_OFST,
+ ALTERA_FPGAMGR_CTL_AXICFGEN);
+
+ return 0;
+}
+
+/*
+ * Step 9: write data to the FPGA data register
+ */
+static int altera_fpga_ops_configure_write(struct fpga_manager *mgr,
+ const char *buf, size_t count)
+{
+ struct altera_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)) {
+ altera_fpga_data_writel(priv, buffer_32[i++]);
+ count -= sizeof(u32);
+ }
+
+ /* Write out remaining non 32-bit chunks. */
+ switch (count) {
+ case 3:
+ altera_fpga_data_writel(priv, buffer_32[i++] & 0x00ffffff);
+ break;
+ case 2:
+ altera_fpga_data_writel(priv, buffer_32[i++] & 0x0000ffff);
+ break;
+ case 1:
+ altera_fpga_data_writel(priv, buffer_32[i++] & 0x000000ff);
+ break;
+ default:
+ /* This will never happen. */
+ break;
+ }
+
+ return 0;
+}
+
+static int altera_fpga_ops_configure_complete(struct fpga_manager *mgr)
+{
+ struct altera_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 = altera_fpga_wait_for_config_done(priv);
+ if (status)
+ return status;
+
+ /* Step 11: Clear CTRL.AXICFGEN to disable transfer of config data */
+ altera_fpga_reg_clr_bitsl(priv, ALTERA_FPGAMGR_CTL_OFST,
+ ALTERA_FPGAMGR_CTL_AXICFGEN);
+
+ /*
+ * Step 12:
+ * - Write 4 to DCLKCNT
+ * - Wait for STATUS.DCNTDONE = 1
+ * - Clear W1C bit in STATUS.DCNTDONE
+ */
+ if (altera_fpga_dclk_set_and_wait_clear(priv, 4))
+ return -ETIMEDOUT;
+
+#if _ALTTERA_FPGAMGR_USE_DCLK
+ /* Step 13: Wait for STATUS.MODE to report INIT or USER MODE */
+ if (altera_fpga_wait_for_state(priv, ALTERA_FPGAMGR_STAT_INIT |
+ ALTERA_FPGAMGR_STAT_USER_MODE))
+ return -ETIMEDOUT;
+
+ /*
+ * Extra steps for Configuration with DCLK for Initialization Phase
+ * Step 14 (using 4.2.1.2 steps), 15 (using 4.2.1.2 steps)
+ * - Write 0x5000 to DCLKCNT == the number of clocks needed to exit
+ * the Initialization Phase.
+ * - Poll until STATUS.DCNTDONE = 1, write 1 to clear
+ */
+ if (altera_fpga_dclk_set_and_wait_clear(priv, 0x5000))
+ return -ETIMEDOUT;
+#endif
+
+ /* Step 13: Wait for STATUS.MODE to report USER MODE */
+ if (altera_fpga_wait_for_state(priv, ALTERA_FPGAMGR_STAT_USER_MODE))
+ return -ETIMEDOUT;
+
+ /* Step 14: Set CTRL.EN to 0 */
+ altera_fpga_reg_clr_bitsl(priv, ALTERA_FPGAMGR_CTL_OFST,
+ ALTERA_FPGAMGR_CTL_EN);
+
+ return 0;
+}
+
+static int altera_fpga_ops_reset(struct fpga_manager *mgr)
+{
+ return altera_fpga_reset(mgr->priv);
+}
+
+/* Translate state register values to FPGA framework state */
+static const int altera_state_to_framework_state[] = {
+ [ALTERA_FPGAMGR_STAT_POWER_OFF] = FPGA_MGR_STATE_POWER_OFF,
+ [ALTERA_FPGAMGR_STAT_RESET] = FPGA_MGR_STATE_RESET,
+ [ALTERA_FPGAMGR_STAT_CFG] = FPGA_MGR_STATE_WRITE_INIT,
+ [ALTERA_FPGAMGR_STAT_INIT] = FPGA_MGR_STATE_WRITE_INIT,
+ [ALTERA_FPGAMGR_STAT_USER_MODE] = FPGA_MGR_STATE_OPERATING,
+ [ALTERA_FPGAMGR_STAT_UNKNOWN] = FPGA_MGR_STATE_UNKNOWN,
+};
+
+static int altera_fpga_ops_state(struct fpga_manager *mgr)
+{
+ struct altera_fpga_priv *priv = mgr->priv;
+ u32 state;
+ int ret;
+
+ state = altera_fpga_state_get(priv);
+
+ if (state < ARRAY_SIZE(altera_state_to_framework_state))
+ ret = altera_state_to_framework_state[state];
+ else
+ ret = FPGA_MGR_STATE_UNKNOWN;
+
+ return ret;
+}
+
+#if IS_ENABLED(CONFIG_REGULATOR)
+static int altera_fpga_regulators_on(struct altera_fpga_priv *priv)
+{
+ int i, ret;
+
+ for (i = 0; i < ALTERA_FPGAMGR_NUM_SUPPLIES; i++) {
+ ret = regulator_enable(priv->fpga_supplies[i]);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static void altera_fpga_regulators_power_off(struct altera_fpga_priv *priv)
+{
+ int i;
+
+ for (i = ALTERA_FPGAMGR_NUM_SUPPLIES - 1; i >= 0; i--)
+ regulator_disable(priv->fpga_supplies[i]);
+}
+
+static int altera_fpga_regulator_probe(struct platform_device *pdev,
+ struct altera_fpga_priv *priv)
+{
+ struct regulator *supply;
+ unsigned int i;
+
+ for (i = 0; i < ALTERA_FPGAMGR_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 altera_fpga_regulators_on(priv);
+}
+#else
+static int altera_fpga_regulators_on(struct altera_fpga_priv *priv)
+{
+ return 0;
+}
+
+static void altera_fpga_regulators_power_off(struct altera_fpga_priv *priv)
+{
+}
+
+static int altera_fpga_regulator_probe(struct platform_device *pdev,
+ struct altera_fpga_priv *priv)
+{
+ return 0;
+}
+#endif
+
+static int altera_fpga_suspend(struct fpga_manager *mgr)
+{
+ struct altera_fpga_priv *priv = mgr->priv;
+
+ altera_fpga_regulators_power_off(priv);
+
+ return 0;
+}
+
+static int altera_fpga_resume(struct fpga_manager *mgr)
+{
+ struct altera_fpga_priv *priv = mgr->priv;
+ u32 state;
+ unsigned int timeout;
+ int ret;
+
+ ret = altera_fpga_regulators_on(priv);
+ if (ret)
+ return ret;
+
+ for (timeout = 0; timeout < ALTERA_RESUME_TIMEOUT; timeout++) {
+ state = altera_fpga_state_get(priv);
+ if (state != ALTERA_FPGAMGR_STAT_POWER_OFF)
+ break;
+ msleep(20);
+ }
+ if (state == ALTERA_FPGAMGR_STAT_POWER_OFF)
+ return -ETIMEDOUT;
+
+ return ret;
+}
+
+struct fpga_manager_ops altera_altera_fpga_ops = {
+ .reset = altera_fpga_ops_reset,
+ .state = altera_fpga_ops_state,
+ .write_init = altera_fpga_ops_configure_init,
+ .write = altera_fpga_ops_configure_write,
+ .write_complete = altera_fpga_ops_configure_complete,
+ .suspend = altera_fpga_suspend,
+ .resume = altera_fpga_resume,
+};
+
+static int altera_fpga_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct device_node *np = pdev->dev.of_node;
+ struct altera_fpga_priv *priv;
+ int ret;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ ret = altera_fpga_regulator_probe(pdev, priv);
+ if (ret)
+ return ret;
+
+ priv->fpga_base_addr = of_iomap(np, 0);
+ if (!priv->fpga_base_addr)
+ return -EINVAL;
+
+ priv->fpga_data_addr = of_iomap(np, 1);
+ if (!priv->fpga_data_addr) {
+ ret = -EINVAL;
+ goto err_unmap_base_addr;
+ }
+
+ priv->irq = irq_of_parse_and_map(np, 0);
+ if (priv->irq == NO_IRQ) {
+ ret = -EINVAL;
+ goto err_unmap_data_addr;
+ }
+
+ ret = request_irq(priv->irq, altera_fpga_isr, 0, "altera-fpga-mgr",
+ priv);
+ if (ret < 0)
+ goto err_dispose_irq;
+
+ ret = fpga_mgr_register(dev, "Altera FPGA Manager",
+ &altera_altera_fpga_ops, priv);
+ if (ret)
+ goto err_free_irq;
+
+ return 0;
+
+err_free_irq:
+ free_irq(priv->irq, priv);
+err_dispose_irq:
+ irq_dispose_mapping(priv->irq);
+err_unmap_data_addr:
+ iounmap(priv->fpga_data_addr);
+err_unmap_base_addr:
+ iounmap(priv->fpga_base_addr);
+ return ret;
+}
+
+static int altera_fpga_remove(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct fpga_manager *mgr = to_fpga_manager(dev);
+ struct altera_fpga_priv *priv = mgr->priv;
+
+ fpga_mgr_remove(dev);
+ free_irq(priv->irq, priv);
+ irq_dispose_mapping(priv->irq);
+ iounmap(priv->fpga_data_addr);
+ iounmap(priv->fpga_base_addr);
+
+ return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id altera_fpga_of_match[] = {
+ { .compatible = "altr,fpga-mgr", },
+ {},
+};
+
+MODULE_DEVICE_TABLE(of, altera_fpga_of_match);
+#endif
+
+static struct platform_driver altera_fpga_driver = {
+ .remove = altera_fpga_remove,
+ .driver = {
+ .name = "altera_fpga_manager",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(altera_fpga_of_match),
+ },
+ .probe = altera_fpga_probe,
+};
+
+static int __init altera_fpga_init(void)
+{
+ return platform_driver_register(&altera_fpga_driver);
+}
+
+static void __exit altera_fpga_exit(void)
+{
+ platform_driver_unregister(&altera_fpga_driver);
+}
+
+module_init(altera_fpga_init);
+module_exit(altera_fpga_exit);
+
+MODULE_AUTHOR("Alan Tull <[email protected]>");
+MODULE_DESCRIPTION("Altera FPGA Manager");
+MODULE_LICENSE("GPL v2");
--
1.7.9.5
From: Alan Tull <[email protected]>
Enable FPGA manager for Altera socfpga.
Signed-off-by: Alan Tull <[email protected]>
---
arch/arm/configs/socfpga_defconfig | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/arm/configs/socfpga_defconfig b/arch/arm/configs/socfpga_defconfig
index a2956c3..2740057 100644
--- a/arch/arm/configs/socfpga_defconfig
+++ b/arch/arm/configs/socfpga_defconfig
@@ -86,6 +86,10 @@ CONFIG_USB_DWC2=y
CONFIG_USB_DWC2_HOST=y
CONFIG_MMC=y
CONFIG_MMC_DW=y
+CONFIG_STAGING=y
+CONFIG_FPGA=y
+CONFIG_FPGA_MGR_SYSFS=y
+CONFIG_FPGA_MGR_ALTERA=y
CONFIG_EXT2_FS=y
CONFIG_EXT2_FS_XATTR=y
CONFIG_EXT2_FS_POSIX_ACL=y
--
1.7.9.5
Hi Alan,
On 12/09/2014 09:14 PM, [email protected] wrote:
> From: Alan Tull <[email protected]>
>
> Enable FPGA manager for Altera socfpga.
>
> Signed-off-by: Alan Tull <[email protected]>
> ---
> arch/arm/configs/socfpga_defconfig | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/arch/arm/configs/socfpga_defconfig b/arch/arm/configs/socfpga_defconfig
> index a2956c3..2740057 100644
> --- a/arch/arm/configs/socfpga_defconfig
> +++ b/arch/arm/configs/socfpga_defconfig
> @@ -86,6 +86,10 @@ CONFIG_USB_DWC2=y
> CONFIG_USB_DWC2_HOST=y
> CONFIG_MMC=y
> CONFIG_MMC_DW=y
> +CONFIG_STAGING=y
> +CONFIG_FPGA=y
> +CONFIG_FPGA_MGR_SYSFS=y
> +CONFIG_FPGA_MGR_ALTERA=y
> CONFIG_EXT2_FS=y
> CONFIG_EXT2_FS_XATTR=y
> CONFIG_EXT2_FS_POSIX_ACL=y
>
This should the last patch in this series because you are changing Kconfig
in 5/6 and 6/6.
Thanks,
Michal
Hi Greg,
On 12/09/2014 09:14 PM, [email protected] wrote:
> From: Alan Tull <[email protected]>
>
> Add documentation for new fpga manager sysfs interface.
>
> Signed-off-by: Alan Tull <[email protected]>
> ---
> Documentation/ABI/testing/sysfs-class-fpga-manager | 38 ++++++++++++++++++++
> 1 file changed, 38 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-class-fpga-manager
What's the rule for adding sysfs doc for driver which is added to staging?
Is this location fine?
Thanks,
Michal
On 12/09/2014 09:14 PM, [email protected] wrote:
> From: Alan Tull <[email protected]>
>
> Add driver to fpga manager framework to allow configuration
> of FPGA in Altera SoC FPGA parts.
>
> Signed-off-by: Alan Tull <[email protected]>
> ---
> v2: fpga_manager struct now contains struct device
> fpga_manager_register parameters now take device
>
> v3: move to drivers/staging
here should be that v4 is completely new patch in this series
as you are describing in cover letter.
> ---
> drivers/staging/fpga/Kconfig | 6 +
> drivers/staging/fpga/Makefile | 1 +
> drivers/staging/fpga/altera.c | 789 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 796 insertions(+)
> create mode 100644 drivers/staging/fpga/altera.c
>
> diff --git a/drivers/staging/fpga/Kconfig b/drivers/staging/fpga/Kconfig
> index e775b17..2944e3c 100644
> --- a/drivers/staging/fpga/Kconfig
> +++ b/drivers/staging/fpga/Kconfig
> @@ -18,4 +18,10 @@ config FPGA_MGR_SYSFS
> help
> FPGA Manager SysFS interface.
>
> +config FPGA_MGR_ALTERA
> + tristate "Altera"
> + depends on FPGA
> + help
> + FPGA manager driver support for configuring Altera FPGAs.
> +
> endmenu
> diff --git a/drivers/staging/fpga/Makefile b/drivers/staging/fpga/Makefile
> index c8a676f..6e0d2bf 100644
> --- a/drivers/staging/fpga/Makefile
> +++ b/drivers/staging/fpga/Makefile
> @@ -8,3 +8,4 @@ fpga-mgr-core-y += fpga-mgr.o
> obj-$(CONFIG_FPGA) += fpga-mgr-core.o
>
> # FPGA Manager Drivers
> +obj-$(CONFIG_FPGA_MGR_ALTERA) += altera.o
> diff --git a/drivers/staging/fpga/altera.c b/drivers/staging/fpga/altera.c
> new file mode 100644
> index 0000000..14a16b4
> --- /dev/null
> +++ b/drivers/staging/fpga/altera.c
> @@ -0,0 +1,789 @@
> +/*
> + * FPGA Manager Driver for Altera FPGAs
> + *
> + * 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/cdev.h>
> +#include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/fs.h>
> +#include <linux/fpga-mgr.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/kernel.h>
> +#include <linux/list.h>
> +#include <linux/mm.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/pm.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +#include <linux/regulator/consumer.h>
This is pretty long list I believe you will be able to shorten it a little bit.
> +
> +/* Controls whether to use the Configuration with DCLK steps */
> +#ifndef _ALTTERA_FPGAMGR_USE_DCLK
> +#define _ALTTERA_FPGAMGR_USE_DCLK 0
> +#endif
This is more or less config option which you should reflect in binding.
> +
> +/* Register offsets */
> +#define ALTERA_FPGAMGR_STAT_OFST 0x0
> +#define ALTERA_FPGAMGR_CTL_OFST 0x4
> +#define ALTERA_FPGAMGR_DCLKCNT_OFST 0x8
> +#define ALTERA_FPGAMGR_DCLKSTAT_OFST 0xc
> +#define ALTERA_FPGAMGR_GPIO_INTEN_OFST 0x830
> +#define ALTERA_FPGAMGR_GPIO_INTMSK_OFST 0x834
> +#define ALTERA_FPGAMGR_GPIO_INTTYPE_LEVEL_OFST 0x838
> +#define ALTERA_FPGAMGR_GPIO_INT_POL_OFST 0x83c
> +#define ALTERA_FPGAMGR_GPIO_INTSTAT_OFST 0x840
> +#define ALTERA_FPGAMGR_GPIO_RAW_INTSTAT_OFST 0x844
> +#define ALTERA_FPGAMGR_GPIO_PORTA_EOI_OFST 0x84c
> +#define ALTERA_FPGAMGR_GPIO_EXT_PORTA_OFST 0x850
> +
> +/* Register bit defines */
> +/* ALTERA_FPGAMGR_STAT register mode field values */
> +#define ALTERA_FPGAMGR_STAT_POWER_UP 0x0 /*ramping*/
> +#define ALTERA_FPGAMGR_STAT_RESET 0x1
> +#define ALTERA_FPGAMGR_STAT_CFG 0x2
> +#define ALTERA_FPGAMGR_STAT_INIT 0x3
> +#define ALTERA_FPGAMGR_STAT_USER_MODE 0x4
> +#define ALTERA_FPGAMGR_STAT_UNKNOWN 0x5
> +#define ALTERA_FPGAMGR_STAT_STATE_MASK 0x7
> +/* This is a flag value that doesn't really happen in this register field */
> +#define ALTERA_FPGAMGR_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 ALTERA_FPGAMGR_STAT_MSEL_MASK 0x000000f8
> +#define ALTERA_FPGAMGR_STAT_MSEL_SHIFT 3
> +
> +/* ALTERA_FPGAMGR_CTL register */
> +#define ALTERA_FPGAMGR_CTL_EN 0x00000001
> +#define ALTERA_FPGAMGR_CTL_NCE 0x00000002
> +#define ALTERA_FPGAMGR_CTL_NCFGPULL 0x00000004
> +
> +#define CDRATIO_X1 0x00000000
> +#define CDRATIO_X2 0x00000040
> +#define CDRATIO_X4 0x00000080
> +#define CDRATIO_X8 0x000000c0
> +#define ALTERA_FPGAMGR_CTL_CDRATIO_MASK 0x000000c0
> +
> +#define ALTERA_FPGAMGR_CTL_AXICFGEN 0x00000100
> +
> +#define CFGWDTH_16 0x00000000
> +#define CFGWDTH_32 0x00000200
> +#define ALTERA_FPGAMGR_CTL_CFGWDTH_MASK 0x00000200
> +
> +/* ALTERA_FPGAMGR_DCLKSTAT register */
> +#define ALTERA_FPGAMGR_DCLKSTAT_DCNTDONE_E_DONE 0x1
> +
> +/* ALTERA_FPGAMGR_GPIO_* registers share the same bit positions */
> +#define ALTERA_FPGAMGR_MON_NSTATUS 0x0001
> +#define ALTERA_FPGAMGR_MON_CONF_DONE 0x0002
> +#define ALTERA_FPGAMGR_MON_INIT_DONE 0x0004
> +#define ALTERA_FPGAMGR_MON_CRC_ERROR 0x0008
> +#define ALTERA_FPGAMGR_MON_CVP_CONF_DONE 0x0010
> +#define ALTERA_FPGAMGR_MON_PR_READY 0x0020
> +#define ALTERA_FPGAMGR_MON_PR_ERROR 0x0040
> +#define ALTERA_FPGAMGR_MON_PR_DONE 0x0080
> +#define ALTERA_FPGAMGR_MON_NCONFIG_PIN 0x0100
> +#define ALTERA_FPGAMGR_MON_NSTATUS_PIN 0x0200
> +#define ALTERA_FPGAMGR_MON_CONF_DONE_PIN 0x0400
> +#define ALTERA_FPGAMGR_MON_FPGA_POWER_ON 0x0800
> +#define ALTERA_FPGAMGR_MON_STATUS_MASK 0x0fff
> +
> +#define ALTERA_FPGAMGR_NUM_SUPPLIES 3
> +#define ALTERA_RESUME_TIMEOUT 3
> +
> +#if IS_ENABLED(CONFIG_REGULATOR)
> +/* In power-up order. Reverse for power-down. */
> +static const char *supply_names[ALTERA_FPGAMGR_NUM_SUPPLIES] = {
> + "FPGA-1.5V",
> + "FPGA-1.1V",
> + "FPGA-2.5V",
> +};
> +#endif
__maybe_unused should work here.
> +
> +struct altera_fpga_priv {
> + void __iomem *fpga_base_addr;
> + void __iomem *fpga_data_addr;
> + struct completion status_complete;
> + int irq;
> + struct regulator *fpga_supplies[ALTERA_FPGAMGR_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 ALTERA_FPGAMGR_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 altera_fpga_reg_readl(struct altera_fpga_priv *priv, u32 reg_offset)
> +{
> + return readl(priv->fpga_base_addr + reg_offset);
> +}
> +
> +static void altera_fpga_reg_writel(struct altera_fpga_priv *priv,
> + u32 reg_offset, u32 value)
> +{
> + writel(value, priv->fpga_base_addr + reg_offset);
> +}
> +
> +static u32 altera_fpga_reg_raw_readl(struct altera_fpga_priv *priv,
> + u32 reg_offset)
> +{
> + return __raw_readl(priv->fpga_base_addr + reg_offset);
> +}
> +
> +static void altera_fpga_reg_raw_writel(struct altera_fpga_priv *priv,
> + u32 reg_offset, u32 value)
> +{
> + __raw_writel(value, priv->fpga_base_addr + reg_offset);
> +}
> +
> +static void altera_fpga_data_writel(struct altera_fpga_priv *priv, u32 value)
> +{
> + writel(value, priv->fpga_data_addr);
> +}
> +
> +static inline void altera_fpga_reg_set_bitsl(struct altera_fpga_priv *priv,
> + u32 offset, u32 bits)
> +{
> + u32 val;
> +
> + val = altera_fpga_reg_readl(priv, offset);
> + val |= bits;
> + altera_fpga_reg_writel(priv, offset, val);
> +}
> +
> +static inline void altera_fpga_reg_clr_bitsl(struct altera_fpga_priv *priv,
> + u32 offset, u32 bits)
> +{
> + u32 val;
> +
> + val = altera_fpga_reg_readl(priv, offset);
> + val &= ~bits;
> + altera_fpga_reg_writel(priv, offset, val);
> +}
> +
> +static int altera_fpga_mon_status_get(struct altera_fpga_priv *priv)
> +{
> + return altera_fpga_reg_readl(priv,
> + ALTERA_FPGAMGR_GPIO_EXT_PORTA_OFST) &
> + ALTERA_FPGAMGR_MON_STATUS_MASK;
> +}
> +
> +static int altera_fpga_state_get(struct altera_fpga_priv *priv)
> +{
> + int status = altera_fpga_mon_status_get(priv);
> +
> + if ((status & ALTERA_FPGAMGR_MON_FPGA_POWER_ON) == 0)
> + return ALTERA_FPGAMGR_STAT_POWER_OFF;
> +
> + return altera_fpga_reg_readl(priv, ALTERA_FPGAMGR_STAT_OFST) &
> + ALTERA_FPGAMGR_STAT_STATE_MASK;
> +}
> +
> +static void altera_fpga_clear_done_status(struct altera_fpga_priv *priv)
> +{
> + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_DCLKSTAT_OFST,
> + ALTERA_FPGAMGR_DCLKSTAT_DCNTDONE_E_DONE);
> +}
> +
> +/*
> + * Set the DCLKCNT, wait for DCLKSTAT to report the count completed, and clear
> + * the complete status.
> + */
kernel-doc format please.
> +static int altera_fpga_dclk_set_and_wait_clear(struct altera_fpga_priv *priv,
> + u32 count)
> +{
> + int timeout = 2;
> + u32 done;
> +
> + /* Clear any existing DONE status. */
> + if (altera_fpga_reg_readl(priv, ALTERA_FPGAMGR_DCLKSTAT_OFST))
> + altera_fpga_clear_done_status(priv);
> +
> + /* Issue the DCLK count. */
> + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_DCLKCNT_OFST, count);
> +
> + /* Poll DCLKSTAT to see if it completed in the timeout period. */
> + do {
> + done = altera_fpga_reg_readl(priv,
> + ALTERA_FPGAMGR_DCLKSTAT_OFST);
> + if (done == ALTERA_FPGAMGR_DCLKSTAT_DCNTDONE_E_DONE) {
> + altera_fpga_clear_done_status(priv);
> + return 0;
> + }
> + if (count <= 4)
> + udelay(1);
> + else
> + msleep(20);
This looks weird to me.
> + } while (timeout--);
> +
> + return -ETIMEDOUT;
> +}
> +
> +static int altera_fpga_wait_for_state(struct altera_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 ((altera_fpga_state_get(priv) & state) != 0)
> + return 0;
> + msleep(20);
> + } while (timeout--);
> +
> + return -ETIMEDOUT;
> +}
> +
> +static void altera_fpga_enable_irqs(struct altera_fpga_priv *priv, u32 irqs)
> +{
> + /* set irqs to level sensitive */
> + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_GPIO_INTTYPE_LEVEL_OFST, 0);
> +
> + /* set interrupt polarity */
> + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_GPIO_INT_POL_OFST, irqs);
> +
> + /* clear irqs */
> + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_GPIO_PORTA_EOI_OFST, irqs);
> +
> + /* unmask interrupts */
> + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_GPIO_INTMSK_OFST, 0);
> +
> + /* enable interrupts */
> + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_GPIO_INTEN_OFST, irqs);
> +}
> +
> +static void altera_fpga_disable_irqs(struct altera_fpga_priv *priv)
> +{
> + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_GPIO_INTEN_OFST, 0);
> +}
> +
> +static irqreturn_t altera_fpga_isr(int irq, void *dev_id)
> +{
> + struct altera_fpga_priv *priv = dev_id;
> + u32 irqs, st;
> + bool conf_done, nstatus;
> +
> + /* clear irqs */
> + irqs = altera_fpga_reg_raw_readl(priv,
> + ALTERA_FPGAMGR_GPIO_INTSTAT_OFST);
> +
> + altera_fpga_reg_raw_writel(priv, ALTERA_FPGAMGR_GPIO_PORTA_EOI_OFST,
> + irqs);
> +
> + st = altera_fpga_reg_raw_readl(priv,
> + ALTERA_FPGAMGR_GPIO_EXT_PORTA_OFST);
> + conf_done = (st & ALTERA_FPGAMGR_MON_CONF_DONE) != 0;
> + nstatus = (st & ALTERA_FPGAMGR_MON_NSTATUS) != 0;
> +
> + /* success */
> + if (conf_done && nstatus) {
> + /* disable irqs */
> + altera_fpga_reg_raw_writel(priv,
> + ALTERA_FPGAMGR_GPIO_INTEN_OFST, 0);
> + complete(&priv->status_complete);
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int altera_fpga_wait_for_config_done(struct altera_fpga_priv *priv)
> +{
> + int timeout, ret = 0;
> +
> + altera_fpga_disable_irqs(priv);
> + init_completion(&priv->status_complete);
> + altera_fpga_enable_irqs(priv, ALTERA_FPGAMGR_MON_CONF_DONE);
> +
> + timeout = wait_for_completion_interruptible_timeout(
> + &priv->status_complete,
> + msecs_to_jiffies(10));
> + if (timeout == 0)
> + ret = -ETIMEDOUT;
> +
> + altera_fpga_disable_irqs(priv);
> + return ret;
> +}
> +
> +static int altera_fpga_cfg_mode_get(struct altera_fpga_priv *priv)
> +{
> + u32 msel;
> +
> + msel = altera_fpga_reg_readl(priv, ALTERA_FPGAMGR_STAT_OFST);
> + msel &= ALTERA_FPGAMGR_STAT_MSEL_MASK;
> + msel >>= ALTERA_FPGAMGR_STAT_MSEL_SHIFT;
> +
> + /* Check that this MSEL setting is supported */
> + if ((msel >= sizeof(cfgmgr_modes)/sizeof(struct cfgmgr_mode)) ||
> + !cfgmgr_modes[msel].valid)
> + return -EINVAL;
> +
> + return msel;
> +}
> +
> +static int altera_fpga_cfg_mode_set(struct altera_fpga_priv *priv)
> +{
> + u32 ctrl_reg, mode;
> +
> + /* get value from MSEL pins */
> + mode = altera_fpga_cfg_mode_get(priv);
> + if (mode < 0)
> + return mode;
> +
> + /* Adjust CTRL for the CDRATIO */
> + ctrl_reg = altera_fpga_reg_readl(priv, ALTERA_FPGAMGR_CTL_OFST);
> + ctrl_reg &= ~ALTERA_FPGAMGR_CTL_CDRATIO_MASK;
> + ctrl_reg &= ~ALTERA_FPGAMGR_CTL_CFGWDTH_MASK;
> + ctrl_reg |= cfgmgr_modes[mode].ctrl;
> +
> + /* Set NCE to 0. */
> + ctrl_reg &= ~ALTERA_FPGAMGR_CTL_NCE;
> + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_CTL_OFST, ctrl_reg);
> +
> + return 0;
> +}
> +
> +static int altera_fpga_reset(struct altera_fpga_priv *priv)
> +{
> + u32 ctrl_reg, status;
> + int ret;
> +
> + /*
> + * Step 1:
> + * - Set CTRL.CFGWDTH, CTRL.CDRATIO to match cfg mode
> + * - Set CTRL.NCE to 0
> + */
> + ret = altera_fpga_cfg_mode_set(priv);
> + if (ret)
> + return ret;
> +
> + /* Step 2: Set CTRL.EN to 1 */
> + altera_fpga_reg_set_bitsl(priv, ALTERA_FPGAMGR_CTL_OFST,
> + ALTERA_FPGAMGR_CTL_EN);
> +
> + /* Step 3: Set CTRL.NCONFIGPULL to 1 to put FPGA in reset */
> + ctrl_reg = altera_fpga_reg_readl(priv, ALTERA_FPGAMGR_CTL_OFST);
> + ctrl_reg |= ALTERA_FPGAMGR_CTL_NCFGPULL;
> + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_CTL_OFST, ctrl_reg);
> +
> + /* Step 4: Wait for STATUS.MODE to report FPGA is in reset phase */
> + status = altera_fpga_wait_for_state(priv, ALTERA_FPGAMGR_STAT_RESET);
> +
> + /* Step 5: Set CONTROL.NCONFIGPULL to 0 to release FPGA from reset */
> + ctrl_reg &= ~ALTERA_FPGAMGR_CTL_NCFGPULL;
> + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_CTL_OFST, ctrl_reg);
> +
> + /* Timeout waiting for reset */
> + if (status)
> + return -ETIMEDOUT;
> +
> + return 0;
> +}
> +
> +/*
> + * Prepare the FPGA to receive the configuration data.
> + */
kernel-doc
> +static int altera_fpga_ops_configure_init(struct fpga_manager *mgr)
> +{
> + struct altera_fpga_priv *priv = mgr->priv;
> + int ret;
> +
> + /* Steps 1 - 5: Reset the FPGA */
> + ret = altera_fpga_reset(priv);
> + if (ret)
> + return ret;
> +
> + /* Step 6: Wait for FPGA to enter configuration phase */
> + if (altera_fpga_wait_for_state(priv, ALTERA_FPGAMGR_STAT_CFG))
> + return -ETIMEDOUT;
> +
> + /* Step 7: Clear nSTATUS interrupt */
> + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_GPIO_PORTA_EOI_OFST,
> + ALTERA_FPGAMGR_MON_NSTATUS);
> +
> + /* Step 8: Set CTRL.AXICFGEN to 1 to enable transfer of config data */
> + altera_fpga_reg_set_bitsl(priv, ALTERA_FPGAMGR_CTL_OFST,
> + ALTERA_FPGAMGR_CTL_AXICFGEN);
> +
> + return 0;
> +}
> +
> +/*
> + * Step 9: write data to the FPGA data register
> + */
step 9 here?
> +static int altera_fpga_ops_configure_write(struct fpga_manager *mgr,
> + const char *buf, size_t count)
> +{
> + struct altera_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)) {
> + altera_fpga_data_writel(priv, buffer_32[i++]);
> + count -= sizeof(u32);
> + }
> +
> + /* Write out remaining non 32-bit chunks. */
> + switch (count) {
> + case 3:
> + altera_fpga_data_writel(priv, buffer_32[i++] & 0x00ffffff);
> + break;
> + case 2:
> + altera_fpga_data_writel(priv, buffer_32[i++] & 0x0000ffff);
> + break;
> + case 1:
> + altera_fpga_data_writel(priv, buffer_32[i++] & 0x000000ff);
> + break;
> + default:
> + /* This will never happen. */
> + break;
> + }
> +
> + return 0;
> +}
> +
> +static int altera_fpga_ops_configure_complete(struct fpga_manager *mgr)
> +{
> + struct altera_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 = altera_fpga_wait_for_config_done(priv);
> + if (status)
> + return status;
> +
> + /* Step 11: Clear CTRL.AXICFGEN to disable transfer of config data */
> + altera_fpga_reg_clr_bitsl(priv, ALTERA_FPGAMGR_CTL_OFST,
> + ALTERA_FPGAMGR_CTL_AXICFGEN);
> +
> + /*
> + * Step 12:
> + * - Write 4 to DCLKCNT
> + * - Wait for STATUS.DCNTDONE = 1
> + * - Clear W1C bit in STATUS.DCNTDONE
> + */
> + if (altera_fpga_dclk_set_and_wait_clear(priv, 4))
> + return -ETIMEDOUT;
> +
> +#if _ALTTERA_FPGAMGR_USE_DCLK
> + /* Step 13: Wait for STATUS.MODE to report INIT or USER MODE */
> + if (altera_fpga_wait_for_state(priv, ALTERA_FPGAMGR_STAT_INIT |
> + ALTERA_FPGAMGR_STAT_USER_MODE))
> + return -ETIMEDOUT;
> +
> + /*
> + * Extra steps for Configuration with DCLK for Initialization Phase
> + * Step 14 (using 4.2.1.2 steps), 15 (using 4.2.1.2 steps)
> + * - Write 0x5000 to DCLKCNT == the number of clocks needed to exit
> + * the Initialization Phase.
> + * - Poll until STATUS.DCNTDONE = 1, write 1 to clear
> + */
> + if (altera_fpga_dclk_set_and_wait_clear(priv, 0x5000))
> + return -ETIMEDOUT;
> +#endif
> +
> + /* Step 13: Wait for STATUS.MODE to report USER MODE */
> + if (altera_fpga_wait_for_state(priv, ALTERA_FPGAMGR_STAT_USER_MODE))
> + return -ETIMEDOUT;
> +
> + /* Step 14: Set CTRL.EN to 0 */
> + altera_fpga_reg_clr_bitsl(priv, ALTERA_FPGAMGR_CTL_OFST,
> + ALTERA_FPGAMGR_CTL_EN);
> +
> + return 0;
> +}
> +
> +static int altera_fpga_ops_reset(struct fpga_manager *mgr)
> +{
> + return altera_fpga_reset(mgr->priv);
> +}
looks like not useful code - just use altera_fpga_reset instead of ops.
btw: just a generic question - isn't it better to use any better
name than altera_fpga. You can have different loader in future
and this is very generic.
> +
> +/* Translate state register values to FPGA framework state */
> +static const int altera_state_to_framework_state[] = {
> + [ALTERA_FPGAMGR_STAT_POWER_OFF] = FPGA_MGR_STATE_POWER_OFF,
> + [ALTERA_FPGAMGR_STAT_RESET] = FPGA_MGR_STATE_RESET,
> + [ALTERA_FPGAMGR_STAT_CFG] = FPGA_MGR_STATE_WRITE_INIT,
> + [ALTERA_FPGAMGR_STAT_INIT] = FPGA_MGR_STATE_WRITE_INIT,
> + [ALTERA_FPGAMGR_STAT_USER_MODE] = FPGA_MGR_STATE_OPERATING,
> + [ALTERA_FPGAMGR_STAT_UNKNOWN] = FPGA_MGR_STATE_UNKNOWN,
> +};
> +
> +static int altera_fpga_ops_state(struct fpga_manager *mgr)
here should return enum - look at 5/6 comment.
> +{
> + struct altera_fpga_priv *priv = mgr->priv;
> + u32 state;
this is also int not unsigned.
> + int ret;
> +
> + state = altera_fpga_state_get(priv);
> +
> + if (state < ARRAY_SIZE(altera_state_to_framework_state))
> + ret = altera_state_to_framework_state[state];
> + else
> + ret = FPGA_MGR_STATE_UNKNOWN;
> +
> + return ret;
> +}
> +
> +#if IS_ENABLED(CONFIG_REGULATOR)
Instead of this look below
> +static int altera_fpga_regulators_on(struct altera_fpga_priv *priv)
> +{
> + int i, ret;
> +
use this
if (!IS_ENABLED(CONFIG_REGULATOR))
return 0;
Then you can simple compile code just once.
The same change for all these functions.
> + for (i = 0; i < ALTERA_FPGAMGR_NUM_SUPPLIES; i++) {
> + ret = regulator_enable(priv->fpga_supplies[i]);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static void altera_fpga_regulators_power_off(struct altera_fpga_priv *priv)
> +{
> + int i;
> +
> + for (i = ALTERA_FPGAMGR_NUM_SUPPLIES - 1; i >= 0; i--)
> + regulator_disable(priv->fpga_supplies[i]);
> +}
> +
> +static int altera_fpga_regulator_probe(struct platform_device *pdev,
> + struct altera_fpga_priv *priv)
> +{
> + struct regulator *supply;
> + unsigned int i;
> +
> + for (i = 0; i < ALTERA_FPGAMGR_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 altera_fpga_regulators_on(priv);
> +}
> +#else
> +static int altera_fpga_regulators_on(struct altera_fpga_priv *priv)
> +{
> + return 0;
> +}
> +
> +static void altera_fpga_regulators_power_off(struct altera_fpga_priv *priv)
> +{
> +}
> +
> +static int altera_fpga_regulator_probe(struct platform_device *pdev,
> + struct altera_fpga_priv *priv)
> +{
> + return 0;
> +}
> +#endif
> +
> +static int altera_fpga_suspend(struct fpga_manager *mgr)
> +{
> + struct altera_fpga_priv *priv = mgr->priv;
> +
> + altera_fpga_regulators_power_off(priv);
> +
> + return 0;
> +}
> +
> +static int altera_fpga_resume(struct fpga_manager *mgr)
> +{
> + struct altera_fpga_priv *priv = mgr->priv;
> + u32 state;
> + unsigned int timeout;
> + int ret;
> +
> + ret = altera_fpga_regulators_on(priv);
> + if (ret)
> + return ret;
> +
> + for (timeout = 0; timeout < ALTERA_RESUME_TIMEOUT; timeout++) {
> + state = altera_fpga_state_get(priv);
> + if (state != ALTERA_FPGAMGR_STAT_POWER_OFF)
> + break;
> + msleep(20);
> + }
> + if (state == ALTERA_FPGAMGR_STAT_POWER_OFF)
> + return -ETIMEDOUT;
> +
> + return ret;
> +}
> +
> +struct fpga_manager_ops altera_altera_fpga_ops = {
static here.
> + .reset = altera_fpga_ops_reset,
> + .state = altera_fpga_ops_state,
> + .write_init = altera_fpga_ops_configure_init,
> + .write = altera_fpga_ops_configure_write,
> + .write_complete = altera_fpga_ops_configure_complete,
> + .suspend = altera_fpga_suspend,
> + .resume = altera_fpga_resume,
> +};
> +
> +static int altera_fpga_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *np = pdev->dev.of_node;
> + struct altera_fpga_priv *priv;
> + int ret;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + ret = altera_fpga_regulator_probe(pdev, priv);
> + if (ret)
> + return ret;
> +
> + priv->fpga_base_addr = of_iomap(np, 0);
> + if (!priv->fpga_base_addr)
> + return -EINVAL;
> +
> + priv->fpga_data_addr = of_iomap(np, 1);
> + if (!priv->fpga_data_addr) {
> + ret = -EINVAL;
> + goto err_unmap_base_addr;
> + }
> +
> + priv->irq = irq_of_parse_and_map(np, 0);
> + if (priv->irq == NO_IRQ) {
NO_IRQ is not defined for all archs that's why you will get compilation error.
<= 0 should be fine here.
> + ret = -EINVAL;
> + goto err_unmap_data_addr;
> + }
Anyway for all of these you should be able to use
platform_get_resource
platform_get_irq functions
then you have simplified error path here.
> +
> + ret = request_irq(priv->irq, altera_fpga_isr, 0, "altera-fpga-mgr",
> + priv);
> + if (ret < 0)
> + goto err_dispose_irq;
> +
> + ret = fpga_mgr_register(dev, "Altera FPGA Manager",
> + &altera_altera_fpga_ops, priv);
> + if (ret)
> + goto err_free_irq;
> +
> + return 0;
> +
> +err_free_irq:
> + free_irq(priv->irq, priv);
> +err_dispose_irq:
> + irq_dispose_mapping(priv->irq);
> +err_unmap_data_addr:
> + iounmap(priv->fpga_data_addr);
> +err_unmap_base_addr:
> + iounmap(priv->fpga_base_addr);
> + return ret;
> +}
> +
> +static int altera_fpga_remove(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct fpga_manager *mgr = to_fpga_manager(dev);
> + struct altera_fpga_priv *priv = mgr->priv;
> +
> + fpga_mgr_remove(dev);
> + free_irq(priv->irq, priv);
> + irq_dispose_mapping(priv->irq);
> + iounmap(priv->fpga_data_addr);
> + iounmap(priv->fpga_base_addr);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id altera_fpga_of_match[] = {
> + { .compatible = "altr,fpga-mgr", },
> + {},
> +};
> +
> +MODULE_DEVICE_TABLE(of, altera_fpga_of_match);
> +#endif
> +
> +static struct platform_driver altera_fpga_driver = {
> + .remove = altera_fpga_remove,
> + .driver = {
> + .name = "altera_fpga_manager",
> + .owner = THIS_MODULE,
This line should go away
> + .of_match_table = of_match_ptr(altera_fpga_of_match),
> + },
> + .probe = altera_fpga_probe,
I tend to have probe and remove close to each other.
> +};
> +
> +static int __init altera_fpga_init(void)
> +{
> + return platform_driver_register(&altera_fpga_driver);
> +}
> +
> +static void __exit altera_fpga_exit(void)
> +{
> + platform_driver_unregister(&altera_fpga_driver);
> +}
> +
> +module_init(altera_fpga_init);
> +module_exit(altera_fpga_exit);
module_platform_driver here
> +
> +MODULE_AUTHOR("Alan Tull <[email protected]>");
> +MODULE_DESCRIPTION("Altera FPGA Manager");
> +MODULE_LICENSE("GPL v2");
>
Thanks,
Michal
On 12/09/2014 09:14 PM, [email protected] wrote:
> From: Alan Tull <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
> ---
> drivers/staging/Kconfig | 2 +
> drivers/staging/Makefile | 1 +
> drivers/staging/fpga/Kconfig | 21 ++
> drivers/staging/fpga/Makefile | 10 +
> drivers/staging/fpga/fpga-mgr.c | 485 +++++++++++++++++++++++++++++++++++++++
> include/linux/fpga-mgr.h | 104 +++++++++
> 6 files changed, 623 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-mgr.h
>
> diff --git a/drivers/staging/Kconfig b/drivers/staging/Kconfig
> index 4690ae9..4338a4c 100644
> --- a/drivers/staging/Kconfig
> +++ b/drivers/staging/Kconfig
> @@ -108,4 +108,6 @@ source "drivers/staging/skein/Kconfig"
>
> source "drivers/staging/unisys/Kconfig"
>
> +source "drivers/staging/fpga/Kconfig"
> +
> endif # STAGING
> diff --git a/drivers/staging/Makefile b/drivers/staging/Makefile
> index c780a0e..43654a2 100644
> --- a/drivers/staging/Makefile
> +++ b/drivers/staging/Makefile
> @@ -46,3 +46,4 @@ obj-$(CONFIG_MTD_SPINAND_MT29F) += mt29f_spinand/
> obj-$(CONFIG_GS_FPGABOOT) += gs_fpgaboot/
> obj-$(CONFIG_CRYPTO_SKEIN) += skein/
> obj-$(CONFIG_UNISYSSPAR) += unisys/
> +obj-$(CONFIG_FPGA) += fpga/
> diff --git a/drivers/staging/fpga/Kconfig b/drivers/staging/fpga/Kconfig
> new file mode 100644
> index 0000000..e775b17
> --- /dev/null
> +++ b/drivers/staging/fpga/Kconfig
> @@ -0,0 +1,21 @@
> +#
> +# 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.
> +
I would add here
if FPGA
> +config FPGA_MGR_SYSFS
> + bool "FPGA Manager SysFS Interface"
> + depends on FPGA
remove this line.
> + depends on SYSFS
> + help
> + FPGA Manager SysFS interface.
> +
maybe this is not needed at, i2c has no this dependency too and compilation
looks good too.
endif here
The reason is that all "low level" drivers will depend on FPGA that's why with this
we don't need to add depends on FPGA to every entry.
> +endmenu
> diff --git a/drivers/staging/fpga/Makefile b/drivers/staging/fpga/Makefile
> new file mode 100644
> index 0000000..c8a676f
> --- /dev/null
> +++ b/drivers/staging/fpga/Makefile
> @@ -0,0 +1,10 @@
> +#
> +# Makefile for the fpga framework and fpga manager drivers.
> +#
> +
> +fpga-mgr-core-y += fpga-mgr.o
> +
> +# Core FPGA Manager Framework
> +obj-$(CONFIG_FPGA) += fpga-mgr-core.o
Just
obj-$(CONFIG_FPGA) += fpga-mgr.o
should be enough here.
> +
> +# 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..d08cb82
> --- /dev/null
> +++ b/drivers/staging/fpga/fpga-mgr.c
> @@ -0,0 +1,485 @@
> +/*
> + * 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/>.
> + */
Greg, Grant: What about to use just SPDX?
SPDX-License-Identifier: GPL-2.0
> +#include <linux/delay.h>
> +#include <linux/firmware.h>
> +#include <linux/fpga-mgr.h>
I think it will be worth to add this to linux/fpga/ folder.
> +#include <linux/fs.h>
> +#include <linux/idr.h>
> +#include <linux/interrupt.h>
Already added by fpga-mgr.h
> +#include <linux/kernel.h>
linux/delay.h is adding this header already.
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/pm.h>
> +#include <linux/slab.h>
> +#include <linux/vmalloc.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);
> +
> +/*
> + * Get FPGA state from low level driver
> + * return value is same enum as fpga_mgr_framework_state
This is enum fpga_mgr_states based on fpga-mgr.h
> + *
> + * This will be used to initialize framework state
> + */
We have an opportunity to have this in kernel-doc format.
Description is pretty similar to kernel doc anyway.
> +int fpga_mgr_low_level_state(struct fpga_manager *mgr)
static enum fpga_mgr_states here
> +{
> + if (!mgr || !mgr->mops || !mgr->mops->state)
> + return FPGA_MGR_STATE_UNKNOWN;
> +
> + return mgr->mops->state(mgr);
> +}
> +
> +/*
> + * Unlocked version
> + */
> +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);
> +
> + return ret;
> +}
> +
> +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);
> +
> +/*
> + * Unlocked
> + */
> +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;
> +}
> +
> +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;
> +}
> +
> +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;
> +}
> +
> +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);
> +}
> +
> +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);
> +
> +/*
> + * 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.
> + */
> +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;
> +
> + strcpy(mgr->image_name, image_name);
> +
> +fw_wr_exit:
> + mutex_unlock(&mgr->lock);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(fpga_mgr_firmware_write);
> +
> +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);
> +
> +#if IS_ENABLED(CONFIG_FPGA_MGR_SYSFS)
This shouldn't be needed.
> +const char *state_str[] = {
static const ...
> + [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 fpga_mgr_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 fpga_mgr_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 fpga_mgr_firmware_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct fpga_manager *mgr = to_fpga_manager(dev);
> +
> + return sprintf(buf, "%s\n", mgr->image_name);
> +}
> +
> +static ssize_t fpga_mgr_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 fpga_mgr_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(name, S_IRUGO, fpga_mgr_name_show, NULL);
DEVICE_ATTR_RO - name change.
> +static DEVICE_ATTR(state, S_IRUGO, fpga_mgr_state_show, NULL);
DEVICE_ATTR_RO
> +static DEVICE_ATTR(firmware, S_IRUGO | S_IWUSR,
> + fpga_mgr_firmware_show, fpga_mgr_firmware_store);
DEVICE_ATTR_RW
> +static DEVICE_ATTR(reset, S_IWUSR, NULL, fpga_mgr_reset_store);
DEVICE_ATTR_WO
> +
> +static struct attribute *fpga_mgr_attrs[] = {
> + &dev_attr_name.attr,
> + &dev_attr_state.attr,
> + &dev_attr_firmware.attr,
> + &dev_attr_reset.attr,
> + NULL,
> +};
> +
> +static const struct attribute_group fpga_mgr_group = {
> + .attrs = fpga_mgr_attrs,
> +};
> +
> +const struct attribute_group *fpga_mgr_groups[] = {
> + &fpga_mgr_group,
> + NULL,
> +};
> +#else
> +#define fpga_mgr_groups NULL
> +#endif /* CONFIG_FPGA_MGR_SYSFS */
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;
> +}
> +
> +const struct dev_pm_ops fpga_mgr_dev_pm_ops = {
static
> + .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", dev_name(dev));
> + kfree(mgr);
> +}
> +
> +int fpga_mgr_register(struct device *dev, const char *name,
> + struct fpga_manager_ops *mops,
> + void *priv)
> +{
> + struct fpga_manager *mgr;
> + int id, ret;
> +
> + BUG_ON(!mops || !name || !strlen(name));
Isn't this pretty strong? Any reasonable reaction will be better.
> +
> + id = ida_simple_get(&fpga_mgr_ida, 0, 0, GFP_KERNEL);
> + if (id < 0)
> + return id;
> +
> + mgr = kzalloc(sizeof(*mgr), GFP_KERNEL);
> + if (!mgr)
> + return -ENOMEM;
here is missing error path handling because you got id.
It should be moved before ida_simple_get call
> +
> + 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, "fpga manager [%s] registered as id %d\n",
> + dev_name(&mgr->dev), id);
> +
> + return 0;
> +
> +error_device:
> + ida_simple_remove(&fpga_mgr_ida, id);
> + kfree(mgr);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(fpga_mgr_register);
> +
> +void fpga_mgr_remove(struct device *dev)
> +{
> + struct fpga_manager *mgr = to_fpga_manager(dev);
> + int id;
> +
> + if (mgr)
> + id = mgr->dev.id;
Why is here the checking? What cases this can cover if mrg
is not there?
> +
> + device_unregister(dev);
> +
> + if (mgr) {
> + if (mgr->mops->fpga_remove)
> + mgr->mops->fpga_remove(mgr);
> +
> + mutex_lock(&fpga_mgr_mutex);
> + list_del(&mgr->list);
> + mutex_unlock(&fpga_mgr_mutex);
> + kfree(mgr);
> + ida_simple_remove(&fpga_mgr_ida, id);
> + }
> +}
> +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);
> +
> + 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 <[email protected]>");
> +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-mgr.h b/include/linux/fpga-mgr.h
> new file mode 100644
> index 0000000..7ac762c
> --- /dev/null
> +++ b/include/linux/fpga-mgr.h
> @@ -0,0 +1,104 @@
> +/*
> + * 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;
> +
> +/*
> + * fpga_manager_ops are the low level functions implemented by a specific
> + * fpga manager driver. Leaving any of these out that aren't needed is fine
> + * as they are all tested for NULL before being called.
> + */
> +struct fpga_manager_ops {
> + /* Returns an enum value of the FPGA's state */
> + int (*state)(struct fpga_manager *mgr);
here return should be enum not int.
> +
> + /* Put FPGA into reset state */
> + int (*reset)(struct fpga_manager *mgr);
> +
> + /* Prepare the FPGA to receive confuration data */
> + int (*write_init)(struct fpga_manager *mgr);
> +
> + /* Write count bytes of configuration data to the FPGA */
> + int (*write)(struct fpga_manager *mgr, const char *buf, size_t count);
> +
> + /* Return FPGA to default state after writing is done */
> + int (*write_complete)(struct fpga_manager *mgr);
> +
Here are missing read options which were in any previous series.
But they can be added in the following patch.
> + /* Optional: Set FPGA into a specific state during driver remove */
> + void (*fpga_remove)(struct fpga_manager *mgr);
> +
> + int (*suspend)(struct fpga_manager *mgr);
> +
> + int (*resume)(struct fpga_manager *mgr);
> +};
> +
> +/* Valid states may vary by manufacturer; superset is: */
> +enum fpga_mgr_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 held in reset state */
> +
> + /* write sequence */
> + 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, /* FPGA ready to receive image data */
> + FPGA_MGR_STATE_WRITE_ERR, /* Error while programming 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 */
> +};
> +
kernel-doc here will be good.
> +struct fpga_manager {
> + const char *name;
> + struct device dev;
> + struct list_head list;
> + struct mutex lock;
> + enum fpga_mgr_states state;
> + char image_name[NAME_MAX];
> +
> + struct fpga_manager_ops *mops;
> + void *priv;
> +};
> +
> +#define to_fpga_manager(d) container_of(d, struct fpga_manager, dev)
> +
> +#if IS_ENABLED(CONFIG_FPGA)
you can simple remove this if.
> +
> +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,
> + struct fpga_manager_ops *mops, void *priv);
> +void fpga_mgr_remove(struct device *dev);
> +
> +#endif /* CONFIG_FPGA */
> +#endif /*_LINUX_FPGA_MGR_H */
>
Thanks,
Michal
On Wed, Dec 10, 2014 at 10:42:39AM +0100, Michal Simek wrote:
> Hi Greg,
>
> On 12/09/2014 09:14 PM, [email protected] wrote:
> > From: Alan Tull <[email protected]>
> >
> > Add documentation for new fpga manager sysfs interface.
> >
> > Signed-off-by: Alan Tull <[email protected]>
> > ---
> > Documentation/ABI/testing/sysfs-class-fpga-manager | 38 ++++++++++++++++++++
> > 1 file changed, 38 insertions(+)
> > create mode 100644 Documentation/ABI/testing/sysfs-class-fpga-manager
>
> What's the rule for adding sysfs doc for driver which is added to staging?
> Is this location fine?
No, staging drivers should be "self-contained" so when^Wif we delete
them, it is easy to do so :)
Put it in the same directory as your driver.
thanks,
greg k-h
On Tue, Dec 09, 2014 at 02:14:44PM -0600, [email protected] wrote:
> From: Alan Tull <[email protected]>
>
> Improvements in this v3 and v4:
Due to the review comments here, and all of the kbuild issues found,
please just work on a v5 series that I can apply after 3.19-rc1 is out.
I'm dropping this series from my queue now because of those issues.
thanks,
greg k-h
On Tue, 9 Dec 2014 14:14:48 -0600
> +Contact: Alan Tull <[email protected]>
> +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
How does this work as a security model?
I want to assign an FPGA resource to a specific end user. How do I do
that with a pile of sysfs nodes ?
Hi!
On Tue, Dec 09, 2014 at 02:14:46PM -0600, [email protected] wrote:
> From: Alan Tull <[email protected]>
>
> Add Altera FGPA manager to device tree.
>
> Signed-off-by: Alan Tull <[email protected]>
> ---
> arch/arm/boot/dts/socfpga.dtsi | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> index 4472fd9..bab98b6 100644
> --- a/arch/arm/boot/dts/socfpga.dtsi
> +++ b/arch/arm/boot/dts/socfpga.dtsi
> @@ -751,5 +751,15 @@
> compatible = "altr,sys-mgr", "syscon";
> reg = <0xffd08000 0x4000>;
> };
> +
> + hps_0_fpgamgr: fpgamgr@0xff706000 {
> + compatible = "altr,fpga-mgr", "simple-bus";
> + reg = <0xFF706000 0x1000
> + 0xFFB90000 0x1000>;
> + interrupts = <0 175 4>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> + };
The fpgamgr is NOT a bus. You don't need ranges and/or simple-bus.
Regards,
Steffen
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Tue, Dec 09, 2014 at 02:14:50PM -0600, [email protected] wrote:
> From: Alan Tull <[email protected]>
>
> Add driver to fpga manager framework to allow configuration
> of FPGA in Altera SoC FPGA parts.
>
> Signed-off-by: Alan Tull <[email protected]>
> ---
> v2: fpga_manager struct now contains struct device
> fpga_manager_register parameters now take device
>
> v3: move to drivers/staging
> ---
> drivers/staging/fpga/Kconfig | 6 +
> drivers/staging/fpga/Makefile | 1 +
> drivers/staging/fpga/altera.c | 789 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 796 insertions(+)
> create mode 100644 drivers/staging/fpga/altera.c
>
> diff --git a/drivers/staging/fpga/Kconfig b/drivers/staging/fpga/Kconfig
> index e775b17..2944e3c 100644
> --- a/drivers/staging/fpga/Kconfig
> +++ b/drivers/staging/fpga/Kconfig
> @@ -18,4 +18,10 @@ config FPGA_MGR_SYSFS
> help
> FPGA Manager SysFS interface.
>
> +config FPGA_MGR_ALTERA
> + tristate "Altera"
> + depends on FPGA
> + help
> + FPGA manager driver support for configuring Altera FPGAs.
> +
> endmenu
> diff --git a/drivers/staging/fpga/Makefile b/drivers/staging/fpga/Makefile
> index c8a676f..6e0d2bf 100644
> --- a/drivers/staging/fpga/Makefile
> +++ b/drivers/staging/fpga/Makefile
> @@ -8,3 +8,4 @@ fpga-mgr-core-y += fpga-mgr.o
> obj-$(CONFIG_FPGA) += fpga-mgr-core.o
>
> # FPGA Manager Drivers
> +obj-$(CONFIG_FPGA_MGR_ALTERA) += altera.o
> diff --git a/drivers/staging/fpga/altera.c b/drivers/staging/fpga/altera.c
> new file mode 100644
> index 0000000..14a16b4
> --- /dev/null
> +++ b/drivers/staging/fpga/altera.c
> @@ -0,0 +1,789 @@
> +/*
> + * FPGA Manager Driver for Altera FPGAs
> + *
> + * 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/cdev.h>
> +#include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/fs.h>
> +#include <linux/fpga-mgr.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/kernel.h>
> +#include <linux/list.h>
> +#include <linux/mm.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_device.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/pm.h>
> +#include <linux/slab.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +#include <linux/regulator/consumer.h>
> +
> +/* Controls whether to use the Configuration with DCLK steps */
> +#ifndef _ALTTERA_FPGAMGR_USE_DCLK
> +#define _ALTTERA_FPGAMGR_USE_DCLK 0
> +#endif
> +
> +/* Register offsets */
> +#define ALTERA_FPGAMGR_STAT_OFST 0x0
> +#define ALTERA_FPGAMGR_CTL_OFST 0x4
> +#define ALTERA_FPGAMGR_DCLKCNT_OFST 0x8
> +#define ALTERA_FPGAMGR_DCLKSTAT_OFST 0xc
> +#define ALTERA_FPGAMGR_GPIO_INTEN_OFST 0x830
> +#define ALTERA_FPGAMGR_GPIO_INTMSK_OFST 0x834
> +#define ALTERA_FPGAMGR_GPIO_INTTYPE_LEVEL_OFST 0x838
> +#define ALTERA_FPGAMGR_GPIO_INT_POL_OFST 0x83c
> +#define ALTERA_FPGAMGR_GPIO_INTSTAT_OFST 0x840
> +#define ALTERA_FPGAMGR_GPIO_RAW_INTSTAT_OFST 0x844
> +#define ALTERA_FPGAMGR_GPIO_PORTA_EOI_OFST 0x84c
> +#define ALTERA_FPGAMGR_GPIO_EXT_PORTA_OFST 0x850
> +
> +/* Register bit defines */
> +/* ALTERA_FPGAMGR_STAT register mode field values */
> +#define ALTERA_FPGAMGR_STAT_POWER_UP 0x0 /*ramping*/
> +#define ALTERA_FPGAMGR_STAT_RESET 0x1
> +#define ALTERA_FPGAMGR_STAT_CFG 0x2
> +#define ALTERA_FPGAMGR_STAT_INIT 0x3
> +#define ALTERA_FPGAMGR_STAT_USER_MODE 0x4
> +#define ALTERA_FPGAMGR_STAT_UNKNOWN 0x5
> +#define ALTERA_FPGAMGR_STAT_STATE_MASK 0x7
> +/* This is a flag value that doesn't really happen in this register field */
> +#define ALTERA_FPGAMGR_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 ALTERA_FPGAMGR_STAT_MSEL_MASK 0x000000f8
> +#define ALTERA_FPGAMGR_STAT_MSEL_SHIFT 3
> +
> +/* ALTERA_FPGAMGR_CTL register */
> +#define ALTERA_FPGAMGR_CTL_EN 0x00000001
> +#define ALTERA_FPGAMGR_CTL_NCE 0x00000002
> +#define ALTERA_FPGAMGR_CTL_NCFGPULL 0x00000004
> +
> +#define CDRATIO_X1 0x00000000
> +#define CDRATIO_X2 0x00000040
> +#define CDRATIO_X4 0x00000080
> +#define CDRATIO_X8 0x000000c0
> +#define ALTERA_FPGAMGR_CTL_CDRATIO_MASK 0x000000c0
> +
> +#define ALTERA_FPGAMGR_CTL_AXICFGEN 0x00000100
> +
> +#define CFGWDTH_16 0x00000000
> +#define CFGWDTH_32 0x00000200
> +#define ALTERA_FPGAMGR_CTL_CFGWDTH_MASK 0x00000200
> +
> +/* ALTERA_FPGAMGR_DCLKSTAT register */
> +#define ALTERA_FPGAMGR_DCLKSTAT_DCNTDONE_E_DONE 0x1
> +
> +/* ALTERA_FPGAMGR_GPIO_* registers share the same bit positions */
> +#define ALTERA_FPGAMGR_MON_NSTATUS 0x0001
> +#define ALTERA_FPGAMGR_MON_CONF_DONE 0x0002
> +#define ALTERA_FPGAMGR_MON_INIT_DONE 0x0004
> +#define ALTERA_FPGAMGR_MON_CRC_ERROR 0x0008
> +#define ALTERA_FPGAMGR_MON_CVP_CONF_DONE 0x0010
> +#define ALTERA_FPGAMGR_MON_PR_READY 0x0020
> +#define ALTERA_FPGAMGR_MON_PR_ERROR 0x0040
> +#define ALTERA_FPGAMGR_MON_PR_DONE 0x0080
> +#define ALTERA_FPGAMGR_MON_NCONFIG_PIN 0x0100
> +#define ALTERA_FPGAMGR_MON_NSTATUS_PIN 0x0200
> +#define ALTERA_FPGAMGR_MON_CONF_DONE_PIN 0x0400
> +#define ALTERA_FPGAMGR_MON_FPGA_POWER_ON 0x0800
> +#define ALTERA_FPGAMGR_MON_STATUS_MASK 0x0fff
> +
> +#define ALTERA_FPGAMGR_NUM_SUPPLIES 3
> +#define ALTERA_RESUME_TIMEOUT 3
> +
> +#if IS_ENABLED(CONFIG_REGULATOR)
> +/* In power-up order. Reverse for power-down. */
> +static const char *supply_names[ALTERA_FPGAMGR_NUM_SUPPLIES] = {
> + "FPGA-1.5V",
> + "FPGA-1.1V",
> + "FPGA-2.5V",
> +};
> +#endif
> +
> +struct altera_fpga_priv {
> + void __iomem *fpga_base_addr;
> + void __iomem *fpga_data_addr;
> + struct completion status_complete;
> + int irq;
> + struct regulator *fpga_supplies[ALTERA_FPGAMGR_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 ALTERA_FPGAMGR_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 altera_fpga_reg_readl(struct altera_fpga_priv *priv, u32 reg_offset)
> +{
> + return readl(priv->fpga_base_addr + reg_offset);
> +}
> +
> +static void altera_fpga_reg_writel(struct altera_fpga_priv *priv,
> + u32 reg_offset, u32 value)
> +{
> + writel(value, priv->fpga_base_addr + reg_offset);
> +}
> +
> +static u32 altera_fpga_reg_raw_readl(struct altera_fpga_priv *priv,
> + u32 reg_offset)
> +{
> + return __raw_readl(priv->fpga_base_addr + reg_offset);
> +}
> +
> +static void altera_fpga_reg_raw_writel(struct altera_fpga_priv *priv,
> + u32 reg_offset, u32 value)
> +{
> + __raw_writel(value, priv->fpga_base_addr + reg_offset);
> +}
> +
> +static void altera_fpga_data_writel(struct altera_fpga_priv *priv, u32 value)
> +{
> + writel(value, priv->fpga_data_addr);
> +}
> +
> +static inline void altera_fpga_reg_set_bitsl(struct altera_fpga_priv *priv,
> + u32 offset, u32 bits)
> +{
> + u32 val;
> +
> + val = altera_fpga_reg_readl(priv, offset);
> + val |= bits;
> + altera_fpga_reg_writel(priv, offset, val);
> +}
> +
> +static inline void altera_fpga_reg_clr_bitsl(struct altera_fpga_priv *priv,
> + u32 offset, u32 bits)
> +{
> + u32 val;
> +
> + val = altera_fpga_reg_readl(priv, offset);
> + val &= ~bits;
> + altera_fpga_reg_writel(priv, offset, val);
> +}
> +
Why do you need these? Can't you just use regmap_mmio?
> +static int altera_fpga_mon_status_get(struct altera_fpga_priv *priv)
> +{
> + return altera_fpga_reg_readl(priv,
> + ALTERA_FPGAMGR_GPIO_EXT_PORTA_OFST) &
> + ALTERA_FPGAMGR_MON_STATUS_MASK;
> +}
> +
> +static int altera_fpga_state_get(struct altera_fpga_priv *priv)
> +{
> + int status = altera_fpga_mon_status_get(priv);
> +
> + if ((status & ALTERA_FPGAMGR_MON_FPGA_POWER_ON) == 0)
> + return ALTERA_FPGAMGR_STAT_POWER_OFF;
> +
> + return altera_fpga_reg_readl(priv, ALTERA_FPGAMGR_STAT_OFST) &
> + ALTERA_FPGAMGR_STAT_STATE_MASK;
> +}
> +
> +static void altera_fpga_clear_done_status(struct altera_fpga_priv *priv)
> +{
> + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_DCLKSTAT_OFST,
> + ALTERA_FPGAMGR_DCLKSTAT_DCNTDONE_E_DONE);
> +}
> +
> +/*
> + * Set the DCLKCNT, wait for DCLKSTAT to report the count completed, and clear
> + * the complete status.
> + */
> +static int altera_fpga_dclk_set_and_wait_clear(struct altera_fpga_priv *priv,
> + u32 count)
> +{
> + int timeout = 2;
> + u32 done;
> +
> + /* Clear any existing DONE status. */
> + if (altera_fpga_reg_readl(priv, ALTERA_FPGAMGR_DCLKSTAT_OFST))
> + altera_fpga_clear_done_status(priv);
> +
> + /* Issue the DCLK count. */
> + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_DCLKCNT_OFST, count);
> +
> + /* Poll DCLKSTAT to see if it completed in the timeout period. */
> + do {
> + done = altera_fpga_reg_readl(priv,
> + ALTERA_FPGAMGR_DCLKSTAT_OFST);
> + if (done == ALTERA_FPGAMGR_DCLKSTAT_DCNTDONE_E_DONE) {
> + altera_fpga_clear_done_status(priv);
> + return 0;
> + }
> + if (count <= 4)
> + udelay(1);
> + else
> + msleep(20);
> + } while (timeout--);
> +
> + return -ETIMEDOUT;
> +}
> +
> +static int altera_fpga_wait_for_state(struct altera_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 ((altera_fpga_state_get(priv) & state) != 0)
> + return 0;
> + msleep(20);
> + } while (timeout--);
> +
> + return -ETIMEDOUT;
> +}
> +
> +static void altera_fpga_enable_irqs(struct altera_fpga_priv *priv, u32 irqs)
> +{
> + /* set irqs to level sensitive */
> + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_GPIO_INTTYPE_LEVEL_OFST, 0);
> +
> + /* set interrupt polarity */
> + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_GPIO_INT_POL_OFST, irqs);
> +
> + /* clear irqs */
> + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_GPIO_PORTA_EOI_OFST, irqs);
> +
> + /* unmask interrupts */
> + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_GPIO_INTMSK_OFST, 0);
> +
> + /* enable interrupts */
> + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_GPIO_INTEN_OFST, irqs);
> +}
> +
> +static void altera_fpga_disable_irqs(struct altera_fpga_priv *priv)
> +{
> + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_GPIO_INTEN_OFST, 0);
> +}
> +
> +static irqreturn_t altera_fpga_isr(int irq, void *dev_id)
> +{
> + struct altera_fpga_priv *priv = dev_id;
> + u32 irqs, st;
> + bool conf_done, nstatus;
> +
> + /* clear irqs */
> + irqs = altera_fpga_reg_raw_readl(priv,
> + ALTERA_FPGAMGR_GPIO_INTSTAT_OFST);
> +
> + altera_fpga_reg_raw_writel(priv, ALTERA_FPGAMGR_GPIO_PORTA_EOI_OFST,
> + irqs);
> +
> + st = altera_fpga_reg_raw_readl(priv,
> + ALTERA_FPGAMGR_GPIO_EXT_PORTA_OFST);
> + conf_done = (st & ALTERA_FPGAMGR_MON_CONF_DONE) != 0;
> + nstatus = (st & ALTERA_FPGAMGR_MON_NSTATUS) != 0;
> +
> + /* success */
> + if (conf_done && nstatus) {
> + /* disable irqs */
> + altera_fpga_reg_raw_writel(priv,
> + ALTERA_FPGAMGR_GPIO_INTEN_OFST, 0);
> + complete(&priv->status_complete);
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int altera_fpga_wait_for_config_done(struct altera_fpga_priv *priv)
> +{
> + int timeout, ret = 0;
> +
> + altera_fpga_disable_irqs(priv);
> + init_completion(&priv->status_complete);
> + altera_fpga_enable_irqs(priv, ALTERA_FPGAMGR_MON_CONF_DONE);
> +
> + timeout = wait_for_completion_interruptible_timeout(
> + &priv->status_complete,
> + msecs_to_jiffies(10));
> + if (timeout == 0)
> + ret = -ETIMEDOUT;
> +
> + altera_fpga_disable_irqs(priv);
> + return ret;
> +}
> +
> +static int altera_fpga_cfg_mode_get(struct altera_fpga_priv *priv)
> +{
> + u32 msel;
> +
> + msel = altera_fpga_reg_readl(priv, ALTERA_FPGAMGR_STAT_OFST);
> + msel &= ALTERA_FPGAMGR_STAT_MSEL_MASK;
> + msel >>= ALTERA_FPGAMGR_STAT_MSEL_SHIFT;
> +
> + /* Check that this MSEL setting is supported */
> + if ((msel >= sizeof(cfgmgr_modes)/sizeof(struct cfgmgr_mode)) ||
> + !cfgmgr_modes[msel].valid)
> + return -EINVAL;
> +
> + return msel;
> +}
> +
> +static int altera_fpga_cfg_mode_set(struct altera_fpga_priv *priv)
> +{
> + u32 ctrl_reg, mode;
> +
> + /* get value from MSEL pins */
> + mode = altera_fpga_cfg_mode_get(priv);
> + if (mode < 0)
> + return mode;
> +
> + /* Adjust CTRL for the CDRATIO */
> + ctrl_reg = altera_fpga_reg_readl(priv, ALTERA_FPGAMGR_CTL_OFST);
> + ctrl_reg &= ~ALTERA_FPGAMGR_CTL_CDRATIO_MASK;
> + ctrl_reg &= ~ALTERA_FPGAMGR_CTL_CFGWDTH_MASK;
> + ctrl_reg |= cfgmgr_modes[mode].ctrl;
> +
> + /* Set NCE to 0. */
> + ctrl_reg &= ~ALTERA_FPGAMGR_CTL_NCE;
> + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_CTL_OFST, ctrl_reg);
> +
> + return 0;
> +}
> +
> +static int altera_fpga_reset(struct altera_fpga_priv *priv)
> +{
> + u32 ctrl_reg, status;
> + int ret;
> +
> + /*
> + * Step 1:
> + * - Set CTRL.CFGWDTH, CTRL.CDRATIO to match cfg mode
> + * - Set CTRL.NCE to 0
> + */
> + ret = altera_fpga_cfg_mode_set(priv);
> + if (ret)
> + return ret;
> +
> + /* Step 2: Set CTRL.EN to 1 */
> + altera_fpga_reg_set_bitsl(priv, ALTERA_FPGAMGR_CTL_OFST,
> + ALTERA_FPGAMGR_CTL_EN);
> +
> + /* Step 3: Set CTRL.NCONFIGPULL to 1 to put FPGA in reset */
> + ctrl_reg = altera_fpga_reg_readl(priv, ALTERA_FPGAMGR_CTL_OFST);
> + ctrl_reg |= ALTERA_FPGAMGR_CTL_NCFGPULL;
> + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_CTL_OFST, ctrl_reg);
> +
> + /* Step 4: Wait for STATUS.MODE to report FPGA is in reset phase */
> + status = altera_fpga_wait_for_state(priv, ALTERA_FPGAMGR_STAT_RESET);
> +
> + /* Step 5: Set CONTROL.NCONFIGPULL to 0 to release FPGA from reset */
> + ctrl_reg &= ~ALTERA_FPGAMGR_CTL_NCFGPULL;
> + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_CTL_OFST, ctrl_reg);
> +
> + /* Timeout waiting for reset */
> + if (status)
> + return -ETIMEDOUT;
> +
> + return 0;
> +}
> +
> +/*
> + * Prepare the FPGA to receive the configuration data.
> + */
> +static int altera_fpga_ops_configure_init(struct fpga_manager *mgr)
> +{
> + struct altera_fpga_priv *priv = mgr->priv;
> + int ret;
> +
> + /* Steps 1 - 5: Reset the FPGA */
> + ret = altera_fpga_reset(priv);
> + if (ret)
> + return ret;
> +
> + /* Step 6: Wait for FPGA to enter configuration phase */
> + if (altera_fpga_wait_for_state(priv, ALTERA_FPGAMGR_STAT_CFG))
> + return -ETIMEDOUT;
> +
> + /* Step 7: Clear nSTATUS interrupt */
> + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_GPIO_PORTA_EOI_OFST,
> + ALTERA_FPGAMGR_MON_NSTATUS);
> +
> + /* Step 8: Set CTRL.AXICFGEN to 1 to enable transfer of config data */
> + altera_fpga_reg_set_bitsl(priv, ALTERA_FPGAMGR_CTL_OFST,
> + ALTERA_FPGAMGR_CTL_AXICFGEN);
> +
> + return 0;
> +}
> +
> +/*
> + * Step 9: write data to the FPGA data register
> + */
> +static int altera_fpga_ops_configure_write(struct fpga_manager *mgr,
> + const char *buf, size_t count)
> +{
> + struct altera_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)) {
> + altera_fpga_data_writel(priv, buffer_32[i++]);
> + count -= sizeof(u32);
> + }
> +
> + /* Write out remaining non 32-bit chunks. */
> + switch (count) {
> + case 3:
> + altera_fpga_data_writel(priv, buffer_32[i++] & 0x00ffffff);
> + break;
> + case 2:
> + altera_fpga_data_writel(priv, buffer_32[i++] & 0x0000ffff);
> + break;
> + case 1:
> + altera_fpga_data_writel(priv, buffer_32[i++] & 0x000000ff);
> + break;
> + default:
> + /* This will never happen. */
> + break;
> + }
> +
> + return 0;
> +}
> +
> +static int altera_fpga_ops_configure_complete(struct fpga_manager *mgr)
> +{
> + struct altera_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 = altera_fpga_wait_for_config_done(priv);
> + if (status)
> + return status;
> +
> + /* Step 11: Clear CTRL.AXICFGEN to disable transfer of config data */
> + altera_fpga_reg_clr_bitsl(priv, ALTERA_FPGAMGR_CTL_OFST,
> + ALTERA_FPGAMGR_CTL_AXICFGEN);
> +
> + /*
> + * Step 12:
> + * - Write 4 to DCLKCNT
> + * - Wait for STATUS.DCNTDONE = 1
> + * - Clear W1C bit in STATUS.DCNTDONE
> + */
> + if (altera_fpga_dclk_set_and_wait_clear(priv, 4))
> + return -ETIMEDOUT;
> +
> +#if _ALTTERA_FPGAMGR_USE_DCLK
> + /* Step 13: Wait for STATUS.MODE to report INIT or USER MODE */
> + if (altera_fpga_wait_for_state(priv, ALTERA_FPGAMGR_STAT_INIT |
> + ALTERA_FPGAMGR_STAT_USER_MODE))
> + return -ETIMEDOUT;
> +
> + /*
> + * Extra steps for Configuration with DCLK for Initialization Phase
> + * Step 14 (using 4.2.1.2 steps), 15 (using 4.2.1.2 steps)
> + * - Write 0x5000 to DCLKCNT == the number of clocks needed to exit
> + * the Initialization Phase.
> + * - Poll until STATUS.DCNTDONE = 1, write 1 to clear
> + */
> + if (altera_fpga_dclk_set_and_wait_clear(priv, 0x5000))
> + return -ETIMEDOUT;
> +#endif
> +
> + /* Step 13: Wait for STATUS.MODE to report USER MODE */
> + if (altera_fpga_wait_for_state(priv, ALTERA_FPGAMGR_STAT_USER_MODE))
> + return -ETIMEDOUT;
> +
> + /* Step 14: Set CTRL.EN to 0 */
> + altera_fpga_reg_clr_bitsl(priv, ALTERA_FPGAMGR_CTL_OFST,
> + ALTERA_FPGAMGR_CTL_EN);
> +
> + return 0;
> +}
> +
> +static int altera_fpga_ops_reset(struct fpga_manager *mgr)
> +{
> + return altera_fpga_reset(mgr->priv);
> +}
> +
> +/* Translate state register values to FPGA framework state */
> +static const int altera_state_to_framework_state[] = {
> + [ALTERA_FPGAMGR_STAT_POWER_OFF] = FPGA_MGR_STATE_POWER_OFF,
> + [ALTERA_FPGAMGR_STAT_RESET] = FPGA_MGR_STATE_RESET,
> + [ALTERA_FPGAMGR_STAT_CFG] = FPGA_MGR_STATE_WRITE_INIT,
> + [ALTERA_FPGAMGR_STAT_INIT] = FPGA_MGR_STATE_WRITE_INIT,
> + [ALTERA_FPGAMGR_STAT_USER_MODE] = FPGA_MGR_STATE_OPERATING,
> + [ALTERA_FPGAMGR_STAT_UNKNOWN] = FPGA_MGR_STATE_UNKNOWN,
> +};
> +
> +static int altera_fpga_ops_state(struct fpga_manager *mgr)
> +{
> + struct altera_fpga_priv *priv = mgr->priv;
> + u32 state;
> + int ret;
> +
> + state = altera_fpga_state_get(priv);
> +
> + if (state < ARRAY_SIZE(altera_state_to_framework_state))
> + ret = altera_state_to_framework_state[state];
> + else
> + ret = FPGA_MGR_STATE_UNKNOWN;
> +
> + return ret;
> +}
> +
> +#if IS_ENABLED(CONFIG_REGULATOR)
> +static int altera_fpga_regulators_on(struct altera_fpga_priv *priv)
> +{
> + int i, ret;
> +
> + for (i = 0; i < ALTERA_FPGAMGR_NUM_SUPPLIES; i++) {
> + ret = regulator_enable(priv->fpga_supplies[i]);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static void altera_fpga_regulators_power_off(struct altera_fpga_priv *priv)
> +{
> + int i;
> +
> + for (i = ALTERA_FPGAMGR_NUM_SUPPLIES - 1; i >= 0; i--)
> + regulator_disable(priv->fpga_supplies[i]);
> +}
> +
> +static int altera_fpga_regulator_probe(struct platform_device *pdev,
> + struct altera_fpga_priv *priv)
> +{
> + struct regulator *supply;
> + unsigned int i;
> +
> + for (i = 0; i < ALTERA_FPGAMGR_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 altera_fpga_regulators_on(priv);
> +}
> +#else
> +static int altera_fpga_regulators_on(struct altera_fpga_priv *priv)
> +{
> + return 0;
> +}
> +
> +static void altera_fpga_regulators_power_off(struct altera_fpga_priv *priv)
> +{
> +}
> +
> +static int altera_fpga_regulator_probe(struct platform_device *pdev,
> + struct altera_fpga_priv *priv)
> +{
> + return 0;
> +}
> +#endif
> +
> +static int altera_fpga_suspend(struct fpga_manager *mgr)
> +{
> + struct altera_fpga_priv *priv = mgr->priv;
> +
> + altera_fpga_regulators_power_off(priv);
> +
> + return 0;
> +}
> +
> +static int altera_fpga_resume(struct fpga_manager *mgr)
> +{
> + struct altera_fpga_priv *priv = mgr->priv;
> + u32 state;
> + unsigned int timeout;
> + int ret;
> +
> + ret = altera_fpga_regulators_on(priv);
> + if (ret)
> + return ret;
> +
> + for (timeout = 0; timeout < ALTERA_RESUME_TIMEOUT; timeout++) {
> + state = altera_fpga_state_get(priv);
> + if (state != ALTERA_FPGAMGR_STAT_POWER_OFF)
> + break;
> + msleep(20);
> + }
> + if (state == ALTERA_FPGAMGR_STAT_POWER_OFF)
> + return -ETIMEDOUT;
> +
> + return ret;
> +}
> +
> +struct fpga_manager_ops altera_altera_fpga_ops = {
> + .reset = altera_fpga_ops_reset,
> + .state = altera_fpga_ops_state,
> + .write_init = altera_fpga_ops_configure_init,
> + .write = altera_fpga_ops_configure_write,
> + .write_complete = altera_fpga_ops_configure_complete,
> + .suspend = altera_fpga_suspend,
> + .resume = altera_fpga_resume,
> +};
> +
> +static int altera_fpga_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *np = pdev->dev.of_node;
> + struct altera_fpga_priv *priv;
> + int ret;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + ret = altera_fpga_regulator_probe(pdev, priv);
> + if (ret)
> + return ret;
> +
> + priv->fpga_base_addr = of_iomap(np, 0);
> + if (!priv->fpga_base_addr)
> + return -EINVAL;
> +
> + priv->fpga_data_addr = of_iomap(np, 1);
> + if (!priv->fpga_data_addr) {
> + ret = -EINVAL;
> + goto err_unmap_base_addr;
> + }
> +
> + priv->irq = irq_of_parse_and_map(np, 0);
> + if (priv->irq == NO_IRQ) {
> + ret = -EINVAL;
> + goto err_unmap_data_addr;
> + }
> +
> + ret = request_irq(priv->irq, altera_fpga_isr, 0, "altera-fpga-mgr",
> + priv);
> + if (ret < 0)
> + goto err_dispose_irq;
devm_request_irq
> +
> + ret = fpga_mgr_register(dev, "Altera FPGA Manager",
> + &altera_altera_fpga_ops, priv);
> + if (ret)
> + goto err_free_irq;
> +
> + return 0;
> +
> +err_free_irq:
> + free_irq(priv->irq, priv);
> +err_dispose_irq:
> + irq_dispose_mapping(priv->irq);
> +err_unmap_data_addr:
> + iounmap(priv->fpga_data_addr);
> +err_unmap_base_addr:
> + iounmap(priv->fpga_base_addr);
> + return ret;
> +}
> +
> +static int altera_fpga_remove(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct fpga_manager *mgr = to_fpga_manager(dev);
> + struct altera_fpga_priv *priv = mgr->priv;
> +
> + fpga_mgr_remove(dev);
> + free_irq(priv->irq, priv);
> + irq_dispose_mapping(priv->irq);
> + iounmap(priv->fpga_data_addr);
> + iounmap(priv->fpga_base_addr);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id altera_fpga_of_match[] = {
> + { .compatible = "altr,fpga-mgr", },
> + {},
> +};
> +
> +MODULE_DEVICE_TABLE(of, altera_fpga_of_match);
> +#endif
> +
> +static struct platform_driver altera_fpga_driver = {
> + .remove = altera_fpga_remove,
> + .driver = {
> + .name = "altera_fpga_manager",
> + .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(altera_fpga_of_match),
> + },
> + .probe = altera_fpga_probe,
> +};
> +
> +static int __init altera_fpga_init(void)
> +{
> + return platform_driver_register(&altera_fpga_driver);
> +}
> +
> +static void __exit altera_fpga_exit(void)
> +{
> + platform_driver_unregister(&altera_fpga_driver);
> +}
> +
> +module_init(altera_fpga_init);
> +module_exit(altera_fpga_exit);
> +
> +MODULE_AUTHOR("Alan Tull <[email protected]>");
> +MODULE_DESCRIPTION("Altera FPGA Manager");
> +MODULE_LICENSE("GPL v2");
> --
> 1.7.9.5
>
>
Regards,
Steffen
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On 12/10/2014 03:47 PM, Greg KH wrote:
> On Wed, Dec 10, 2014 at 10:42:39AM +0100, Michal Simek wrote:
>> Hi Greg,
>>
>> On 12/09/2014 09:14 PM, [email protected] wrote:
>>> From: Alan Tull <[email protected]>
>>>
>>> Add documentation for new fpga manager sysfs interface.
>>>
>>> Signed-off-by: Alan Tull <[email protected]>
>>> ---
>>> Documentation/ABI/testing/sysfs-class-fpga-manager | 38 ++++++++++++++++++++
>>> 1 file changed, 38 insertions(+)
>>> create mode 100644 Documentation/ABI/testing/sysfs-class-fpga-manager
>>
>> What's the rule for adding sysfs doc for driver which is added to staging?
>> Is this location fine?
>
> No, staging drivers should be "self-contained" so when^Wif we delete
> them, it is easy to do so :)
>
> Put it in the same directory as your driver.
That's what I thought.
Thanks for confirmation.
Thanks,
Michal
On Wed, 10 Dec 2014, Michal Simek wrote:
> Hi Alan,
>
> On 12/09/2014 09:14 PM, [email protected] wrote:
> > From: Alan Tull <[email protected]>
> >
> > Enable FPGA manager for Altera socfpga.
> >
> > Signed-off-by: Alan Tull <[email protected]>
> > ---
> > arch/arm/configs/socfpga_defconfig | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/arch/arm/configs/socfpga_defconfig b/arch/arm/configs/socfpga_defconfig
> > index a2956c3..2740057 100644
> > --- a/arch/arm/configs/socfpga_defconfig
> > +++ b/arch/arm/configs/socfpga_defconfig
> > @@ -86,6 +86,10 @@ CONFIG_USB_DWC2=y
> > CONFIG_USB_DWC2_HOST=y
> > CONFIG_MMC=y
> > CONFIG_MMC_DW=y
> > +CONFIG_STAGING=y
> > +CONFIG_FPGA=y
> > +CONFIG_FPGA_MGR_SYSFS=y
> > +CONFIG_FPGA_MGR_ALTERA=y
> > CONFIG_EXT2_FS=y
> > CONFIG_EXT2_FS_XATTR=y
> > CONFIG_EXT2_FS_POSIX_ACL=y
> >
>
> This should the last patch in this series because you are changing Kconfig
> in 5/6 and 6/6.
>
> Thanks,
> Michal
>
OK, I'm fixing this in v5, also taking your other comments. Thanks for
the review.
Alan
On Wed, 10 Dec 2014, Michal Simek wrote:
> On 12/10/2014 03:47 PM, Greg KH wrote:
> > On Wed, Dec 10, 2014 at 10:42:39AM +0100, Michal Simek wrote:
> >> Hi Greg,
> >>
> >> On 12/09/2014 09:14 PM, [email protected] wrote:
> >>> From: Alan Tull <[email protected]>
> >>>
> >>> Add documentation for new fpga manager sysfs interface.
> >>>
> >>> Signed-off-by: Alan Tull <[email protected]>
> >>> ---
> >>> Documentation/ABI/testing/sysfs-class-fpga-manager | 38 ++++++++++++++++++++
> >>> 1 file changed, 38 insertions(+)
> >>> create mode 100644 Documentation/ABI/testing/sysfs-class-fpga-manager
> >>
> >> What's the rule for adding sysfs doc for driver which is added to staging?
> >> Is this location fine?
> >
> > No, staging drivers should be "self-contained" so when^Wif we delete
> > them, it is easy to do so :)
> >
> > Put it in the same directory as your driver.
>
> That's what I thought.
> Thanks for confirmation.
>
> Thanks,
> Michal
>
>
OK I've got that in v5 also.
Alan
On Wed, 10 Dec 2014, Steffen Trumtrar wrote:
> Hi!
>
> On Tue, Dec 09, 2014 at 02:14:46PM -0600, [email protected] wrote:
> > From: Alan Tull <[email protected]>
> >
> > Add Altera FGPA manager to device tree.
> >
> > Signed-off-by: Alan Tull <[email protected]>
> > ---
> > arch/arm/boot/dts/socfpga.dtsi | 10 ++++++++++
> > 1 file changed, 10 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> > index 4472fd9..bab98b6 100644
> > --- a/arch/arm/boot/dts/socfpga.dtsi
> > +++ b/arch/arm/boot/dts/socfpga.dtsi
> > @@ -751,5 +751,15 @@
> > compatible = "altr,sys-mgr", "syscon";
> > reg = <0xffd08000 0x4000>;
> > };
> > +
> > + hps_0_fpgamgr: fpgamgr@0xff706000 {
> > + compatible = "altr,fpga-mgr", "simple-bus";
> > + reg = <0xFF706000 0x1000
> > + 0xFFB90000 0x1000>;
> > + interrupts = <0 175 4>;
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > + ranges;
> > + };
>
> The fpgamgr is NOT a bus. You don't need ranges and/or simple-bus.
>
> Regards,
> Steffen
Actually, I am going to be using this when I add the DT notification
stuff. That stuff is waiting for DT overlays to show up in the main
tree.
So the choice it to either change this file now, once, or do the minimal
changes and have to change it again later.
Alan
>
> --
> Pengutronix e.K. | |
> Industrial Linux Solutions | http://www.pengutronix.de/ |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
On Wed, Dec 10, 2014 at 11:25:59AM -0600, atull wrote:
> On Wed, 10 Dec 2014, Steffen Trumtrar wrote:
>
> > Hi!
> >
> > On Tue, Dec 09, 2014 at 02:14:46PM -0600, [email protected] wrote:
> > > From: Alan Tull <[email protected]>
> > >
> > > Add Altera FGPA manager to device tree.
> > >
> > > Signed-off-by: Alan Tull <[email protected]>
> > > ---
> > > arch/arm/boot/dts/socfpga.dtsi | 10 ++++++++++
> > > 1 file changed, 10 insertions(+)
> > >
> > > diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> > > index 4472fd9..bab98b6 100644
> > > --- a/arch/arm/boot/dts/socfpga.dtsi
> > > +++ b/arch/arm/boot/dts/socfpga.dtsi
> > > @@ -751,5 +751,15 @@
> > > compatible = "altr,sys-mgr", "syscon";
> > > reg = <0xffd08000 0x4000>;
> > > };
> > > +
> > > + hps_0_fpgamgr: fpgamgr@0xff706000 {
> > > + compatible = "altr,fpga-mgr", "simple-bus";
> > > + reg = <0xFF706000 0x1000
> > > + 0xFFB90000 0x1000>;
> > > + interrupts = <0 175 4>;
> > > + #address-cells = <1>;
> > > + #size-cells = <1>;
> > > + ranges;
> > > + };
> >
> > The fpgamgr is NOT a bus. You don't need ranges and/or simple-bus.
> >
> > Regards,
> > Steffen
>
> Actually, I am going to be using this when I add the DT notification
> stuff. That stuff is waiting for DT overlays to show up in the main
> tree.
>
> So the choice it to either change this file now, once, or do the minimal
> changes and have to change it again later.
>
Yes, that is what I thought. And still, devices can NOT show up
at the fpgamgr. They show up on the various bridges.
And as far as I understand the DT overlay stuff, you specify in
the overlay what the parent node will be.
For i2c this is one of the i2c-hosts, for spi one of the spi-hosts
and for IP cores in a bitstream one of the bridges.
The fpgamgr is, from my understanding, only responsible for writing
the bitstream to the FPGA; nothing more.
Regards,
Steffen
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Wed, 10 Dec 2014, Greg KH wrote:
> On Tue, Dec 09, 2014 at 02:14:44PM -0600, [email protected] wrote:
> > From: Alan Tull <[email protected]>
> >
> > Improvements in this v3 and v4:
>
> Due to the review comments here, and all of the kbuild issues found,
> please just work on a v5 series that I can apply after 3.19-rc1 is out.
> I'm dropping this series from my queue now because of those issues.
That sounds great.
Thank you,
Alan
>
> thanks,
>
> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
On 12/10/2014 06:49 PM, Steffen Trumtrar wrote:
> On Wed, Dec 10, 2014 at 11:25:59AM -0600, atull wrote:
>> On Wed, 10 Dec 2014, Steffen Trumtrar wrote:
>>
>>> Hi!
>>>
>>> On Tue, Dec 09, 2014 at 02:14:46PM -0600, [email protected] wrote:
>>>> From: Alan Tull <[email protected]>
>>>>
>>>> Add Altera FGPA manager to device tree.
>>>>
>>>> Signed-off-by: Alan Tull <[email protected]>
>>>> ---
>>>> arch/arm/boot/dts/socfpga.dtsi | 10 ++++++++++
>>>> 1 file changed, 10 insertions(+)
>>>>
>>>> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
>>>> index 4472fd9..bab98b6 100644
>>>> --- a/arch/arm/boot/dts/socfpga.dtsi
>>>> +++ b/arch/arm/boot/dts/socfpga.dtsi
>>>> @@ -751,5 +751,15 @@
>>>> compatible = "altr,sys-mgr", "syscon";
>>>> reg = <0xffd08000 0x4000>;
>>>> };
>>>> +
>>>> + hps_0_fpgamgr: fpgamgr@0xff706000 {
>>>> + compatible = "altr,fpga-mgr", "simple-bus";
>>>> + reg = <0xFF706000 0x1000
>>>> + 0xFFB90000 0x1000>;
>>>> + interrupts = <0 175 4>;
>>>> + #address-cells = <1>;
>>>> + #size-cells = <1>;
>>>> + ranges;
>>>> + };
>>>
>>> The fpgamgr is NOT a bus. You don't need ranges and/or simple-bus.
>>>
>>> Regards,
>>> Steffen
>>
>> Actually, I am going to be using this when I add the DT notification
>> stuff. That stuff is waiting for DT overlays to show up in the main
>> tree.
>>
>> So the choice it to either change this file now, once, or do the minimal
>> changes and have to change it again later.
>>
>
> Yes, that is what I thought. And still, devices can NOT show up
> at the fpgamgr. They show up on the various bridges.
>
> And as far as I understand the DT overlay stuff, you specify in
> the overlay what the parent node will be.
> For i2c this is one of the i2c-hosts, for spi one of the spi-hosts
> and for IP cores in a bitstream one of the bridges.
>
> The fpgamgr is, from my understanding, only responsible for writing
> the bitstream to the FPGA; nothing more.
+1 on this.
I think you can easily decide later where DT overlays will be added.
Thanks,
Michal
On Thu, 11 Dec 2014, Michal Simek wrote:
> On 12/10/2014 06:49 PM, Steffen Trumtrar wrote:
> > On Wed, Dec 10, 2014 at 11:25:59AM -0600, atull wrote:
> >> On Wed, 10 Dec 2014, Steffen Trumtrar wrote:
> >>
> >>> Hi!
> >>>
> >>> On Tue, Dec 09, 2014 at 02:14:46PM -0600, [email protected] wrote:
> >>>> From: Alan Tull <[email protected]>
> >>>>
> >>>> Add Altera FGPA manager to device tree.
> >>>>
> >>>> Signed-off-by: Alan Tull <[email protected]>
> >>>> ---
> >>>> arch/arm/boot/dts/socfpga.dtsi | 10 ++++++++++
> >>>> 1 file changed, 10 insertions(+)
> >>>>
> >>>> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> >>>> index 4472fd9..bab98b6 100644
> >>>> --- a/arch/arm/boot/dts/socfpga.dtsi
> >>>> +++ b/arch/arm/boot/dts/socfpga.dtsi
> >>>> @@ -751,5 +751,15 @@
> >>>> compatible = "altr,sys-mgr", "syscon";
> >>>> reg = <0xffd08000 0x4000>;
> >>>> };
> >>>> +
> >>>> + hps_0_fpgamgr: fpgamgr@0xff706000 {
> >>>> + compatible = "altr,fpga-mgr", "simple-bus";
> >>>> + reg = <0xFF706000 0x1000
> >>>> + 0xFFB90000 0x1000>;
> >>>> + interrupts = <0 175 4>;
> >>>> + #address-cells = <1>;
> >>>> + #size-cells = <1>;
> >>>> + ranges;
> >>>> + };
> >>>
> >>> The fpgamgr is NOT a bus. You don't need ranges and/or simple-bus.
> >>>
> >>> Regards,
> >>> Steffen
> >>
> >> Actually, I am going to be using this when I add the DT notification
> >> stuff. That stuff is waiting for DT overlays to show up in the main
> >> tree.
> >>
> >> So the choice it to either change this file now, once, or do the minimal
> >> changes and have to change it again later.
> >>
> >
> > Yes, that is what I thought. And still, devices can NOT show up
> > at the fpgamgr. They show up on the various bridges.
> >
> > And as far as I understand the DT overlay stuff, you specify in
> > the overlay what the parent node will be.
> > For i2c this is one of the i2c-hosts, for spi one of the spi-hosts
> > and for IP cores in a bitstream one of the bridges.
> >
> > The fpgamgr is, from my understanding, only responsible for writing
> > the bitstream to the FPGA; nothing more.
>
> +1 on this.
> I think you can easily decide later where DT overlays will be added.
>
> Thanks,
> Michal
>
>
OK guys, I'll simplify it down to the following in v5:
+ hps_0_fpgamgr: fpgamgr@0xff706000 {
+ compatible = "altr,fpga-mgr";
+ reg = <0xFF706000 0x1000
+ 0xFFB90000 0x1000>;
+ interrupts = <0 175 4>;
+ };
Alan
On Wed, 10 Dec 2014, Steffen Trumtrar wrote:
> > +static inline void altera_fpga_reg_set_bitsl(struct altera_fpga_priv *priv,
> > + u32 offset, u32 bits)
> > +{
> > + u32 val;
> > +
> > + val = altera_fpga_reg_readl(priv, offset);
> > + val |= bits;
> > + altera_fpga_reg_writel(priv, offset, val);
> > +}
> > +
> > +static inline void altera_fpga_reg_clr_bitsl(struct altera_fpga_priv *priv,
> > + u32 offset, u32 bits)
> > +{
> > + u32 val;
> > +
> > + val = altera_fpga_reg_readl(priv, offset);
> > + val &= ~bits;
> > + altera_fpga_reg_writel(priv, offset, val);
> > +}
> > +
>
> Why do you need these? Can't you just use regmap_mmio?
I would like to do that as an improvement as time allows, not now.
> > + ret = request_irq(priv->irq, altera_fpga_isr, 0, "altera-fpga-mgr",
> > + priv);
> > + if (ret < 0)
> > + goto err_dispose_irq;
>
> devm_request_irq
Will do, thanks.
Alan
On Wed, 10 Dec 2014, Michal Simek wrote:
Hi Michal,
Thanks for the review comments. I have incorporated most of them.
A few questions below.
> > --- /dev/null
> > +++ b/drivers/staging/fpga/Kconfig
> > @@ -0,0 +1,21 @@
> > +#
> > +# 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.
> > +
>
> I would add here
> if FPGA
>
OK
>
> > +config FPGA_MGR_SYSFS
> > + bool "FPGA Manager SysFS Interface"
> > + depends on FPGA
>
> remove this line.
Good
>
> > + depends on SYSFS
> > + help
> > + FPGA Manager SysFS interface.
> > +
>
> maybe this is not needed at, i2c has no this dependency too and compilation
> looks good too.
>
Seems like we are dependent on sysfs, so I need 'depends on SYSFS', right?
Also, I want to be able to disable the sysfs interface (more below on that).
>
> endif here
OK
>
> The reason is that all "low level" drivers will depend on FPGA that's why with this
> we don't need to add depends on FPGA to every entry.
>
> > +endmenu
> > diff --git a/drivers/staging/fpga/Makefile b/drivers/staging/fpga/Makefile
> > new file mode 100644
> > index 0000000..c8a676f
> > --- /dev/null
> > +++ b/drivers/staging/fpga/Makefile
> > @@ -0,0 +1,10 @@
> > +#
> > +# Makefile for the fpga framework and fpga manager drivers.
> > +#
> > +
> > +fpga-mgr-core-y += fpga-mgr.o
> > +
> > +# Core FPGA Manager Framework
> > +obj-$(CONFIG_FPGA) += fpga-mgr-core.o
>
> Just
> obj-$(CONFIG_FPGA) += fpga-mgr.o
> should be enough here.
Yes
>
> > +
> > +# 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..d08cb82
> > --- /dev/null
> > +++ b/drivers/staging/fpga/fpga-mgr.c
> > @@ -0,0 +1,485 @@
> > +/*
> > + * 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/>.
> > + */
>
> Greg, Grant: What about to use just SPDX?
>
> SPDX-License-Identifier: GPL-2.0
>
This is a standared GPLv2 header, we're keeping it. Our legal dept wants it.
> > +#include <linux/delay.h>
> > +#include <linux/firmware.h>
> > +#include <linux/fpga-mgr.h>
>
> I think it will be worth to add this to linux/fpga/ folder.
>
OK
> > +#include <linux/fs.h>
> > +#include <linux/idr.h>
> > +#include <linux/interrupt.h>
>
> Already added by fpga-mgr.h
>
> > +#include <linux/kernel.h>
>
> linux/delay.h is adding this header already.
I'm cleaning up the includes in v5.
> > +/*
> > + * Get FPGA state from low level driver
> > + * return value is same enum as fpga_mgr_framework_state
>
> This is enum fpga_mgr_states based on fpga-mgr.h
>
> > + *
> > + * This will be used to initialize framework state
> > + */
>
> We have an opportunity to have this in kernel-doc format.
> Description is pretty similar to kernel doc anyway.
I'll change the core (fpga-mgr.c and fpga-mgr.h) to use kernel-doc.
I'll leave it out of the altera.c part though.
>
> > +int fpga_mgr_low_level_state(struct fpga_manager *mgr)
>
> static enum fpga_mgr_states here
OK, there are several places where I've changed to use the enum.
> > +#if IS_ENABLED(CONFIG_FPGA_MGR_SYSFS)
>
> This shouldn't be needed.
>From the way the discussion has gone about this, there is a desire for
different interfaces for this. I want it to be possible to disable this sysfs
interface if I'm just using the exported functions or some other future
interface. Part of my main point here is to export enough functionality in
the core functions to make it possible to build other interfaces that people
want to support different use cases or use models. Since we are going to be
in staging, that is a good place to be doing this.
>
> > +const char *state_str[] = {
>
> static const ...
Yes, well actually 'static const *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 fpga_mgr_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 DEVICE_ATTR(name, S_IRUGO, fpga_mgr_name_show, NULL);
>
> DEVICE_ATTR_RO - name change.
>
> > +static DEVICE_ATTR(state, S_IRUGO, fpga_mgr_state_show, NULL);
>
> DEVICE_ATTR_RO
>
> > +static DEVICE_ATTR(firmware, S_IRUGO | S_IWUSR,
> > + fpga_mgr_firmware_show, fpga_mgr_firmware_store);
>
> DEVICE_ATTR_RW
>
> > +static DEVICE_ATTR(reset, S_IWUSR, NULL, fpga_mgr_reset_store);
>
> DEVICE_ATTR_WO
Good, thanks.
>
> > +
> > +static struct attribute *fpga_mgr_attrs[] = {
> > + &dev_attr_name.attr,
> > + &dev_attr_state.attr,
> > + &dev_attr_firmware.attr,
> > + &dev_attr_reset.attr,
> > + NULL,
> > +};
> > +
> > +static const struct attribute_group fpga_mgr_group = {
> > + .attrs = fpga_mgr_attrs,
> > +};
> > +
> > +const struct attribute_group *fpga_mgr_groups[] = {
> > + &fpga_mgr_group,
> > + NULL,
> > +};
> > +#else
> > +#define fpga_mgr_groups NULL
> > +#endif /* CONFIG_FPGA_MGR_SYSFS */
>
> ATTRIBUTE_GROUPS(fpga_mgr)
Yes
>
> > +
> > +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;
> > +}
> > +
> > +const struct dev_pm_ops fpga_mgr_dev_pm_ops = {
>
> static
Yes
>
> > + .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", dev_name(dev));
> > + kfree(mgr);
> > +}
> > +
> > +int fpga_mgr_register(struct device *dev, const char *name,
> > + struct fpga_manager_ops *mops,
> > + void *priv)
> > +{
> > + struct fpga_manager *mgr;
> > + int id, ret;
> > +
> > + BUG_ON(!mops || !name || !strlen(name));
>
> Isn't this pretty strong? Any reasonable reaction will be better.
You are right. Changing it to return an error.
>
> > +
> > + id = ida_simple_get(&fpga_mgr_ida, 0, 0, GFP_KERNEL);
> > + if (id < 0)
> > + return id;
> > +
> > + mgr = kzalloc(sizeof(*mgr), GFP_KERNEL);
> > + if (!mgr)
> > + return -ENOMEM;
>
> here is missing error path handling because you got id.
> It should be moved before ida_simple_get call
>
OK
> > +
> > +void fpga_mgr_remove(struct device *dev)
> > +{
> > + struct fpga_manager *mgr = to_fpga_manager(dev);
> > + int id;
> > +
> > + if (mgr)
> > + id = mgr->dev.id;
>
> Why is here the checking? What cases this can cover if mrg
> is not there?
>
Not needed. Removing it.
> > +
> > + device_unregister(dev);
> > +
> > + if (mgr) {
> > + if (mgr->mops->fpga_remove)
> > + mgr->mops->fpga_remove(mgr);
> > +
> > + mutex_lock(&fpga_mgr_mutex);
> > + list_del(&mgr->list);
> > + mutex_unlock(&fpga_mgr_mutex);
> > + kfree(mgr);
> > + ida_simple_remove(&fpga_mgr_ida, id);
> > + }
> > +}
> > +EXPORT_SYMBOL_GPL(fpga_mgr_remove);
> > +
> > +++ b/include/linux/fpga-mgr.h
> > @@ -0,0 +1,104 @@
> > +/*
> > + * 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;
> > +
> > +/*
> > + * fpga_manager_ops are the low level functions implemented by a specific
> > + * fpga manager driver. Leaving any of these out that aren't needed is fine
> > + * as they are all tested for NULL before being called.
> > + */
> > +struct fpga_manager_ops {
> > + /* Returns an enum value of the FPGA's state */
> > + int (*state)(struct fpga_manager *mgr);
>
> here return should be enum not int.
>
Yes
> > +
> > + /* Put FPGA into reset state */
> > + int (*reset)(struct fpga_manager *mgr);
> > +
> > + /* Prepare the FPGA to receive confuration data */
> > + int (*write_init)(struct fpga_manager *mgr);
> > +
> > + /* Write count bytes of configuration data to the FPGA */
> > + int (*write)(struct fpga_manager *mgr, const char *buf, size_t count);
> > +
> > + /* Return FPGA to default state after writing is done */
> > + int (*write_complete)(struct fpga_manager *mgr);
> > +
>
> Here are missing read options which were in any previous series.
> But they can be added in the following patch.
Yes, they can be added later.
>
> > + /* Optional: Set FPGA into a specific state during driver remove */
> > + void (*fpga_remove)(struct fpga_manager *mgr);
> > +
> > + int (*suspend)(struct fpga_manager *mgr);
> > +
> > + int (*resume)(struct fpga_manager *mgr);
> > +};
> > +
> > +/* Valid states may vary by manufacturer; superset is: */
> > +enum fpga_mgr_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 held in reset state */
> > +
> > + /* write sequence */
> > + 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, /* FPGA ready to receive image data */
> > + FPGA_MGR_STATE_WRITE_ERR, /* Error while programming 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 */
> > +};
> > +
>
> kernel-doc here will be good.
OK
>
> > +struct fpga_manager {
> > + const char *name;
> > + struct device dev;
> > + struct list_head list;
> > + struct mutex lock;
> > + enum fpga_mgr_states state;
> > + char image_name[NAME_MAX];
> > +
> > + struct fpga_manager_ops *mops;
> > + void *priv;
> > +};
> > +
> > +#define to_fpga_manager(d) container_of(d, struct fpga_manager, dev)
> > +
> > +#if IS_ENABLED(CONFIG_FPGA)
>
> you can simple remove this if.
Cool.
Alan
Hi!
> > > @@ -0,0 +1,485 @@
> > > +/*
> > > + * 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/>.
> > > + */
> >
> > Greg, Grant: What about to use just SPDX?
> >
> > SPDX-License-Identifier: GPL-2.0
>
> This is a standared GPLv2 header, we're keeping it. Our legal dept
> wants it.
This has nothing to do with Altera here, but... SPDX works great for
u-boot and yes, I believe it would be good idea to kernel as well.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
On Wed, 10 Dec 2014, Michal Simek wrote:
> On 12/09/2014 09:14 PM, [email protected] wrote:
> > From: Alan Tull <[email protected]>
> >
> > Add driver to fpga manager framework to allow configuration
> > of FPGA in Altera SoC FPGA parts.
> >
> > Signed-off-by: Alan Tull <[email protected]>
> > ---
> > v2: fpga_manager struct now contains struct device
> > fpga_manager_register parameters now take device
> >
> > v3: move to drivers/staging
>
> here should be that v4 is completely new patch in this series
> as you are describing in cover letter.
>
> > ---
> > drivers/staging/fpga/Kconfig | 6 +
> > drivers/staging/fpga/Makefile | 1 +
> > drivers/staging/fpga/altera.c | 789 +++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 796 insertions(+)
> > create mode 100644 drivers/staging/fpga/altera.c
> >
> > diff --git a/drivers/staging/fpga/Kconfig b/drivers/staging/fpga/Kconfig
> > index e775b17..2944e3c 100644
> > --- a/drivers/staging/fpga/Kconfig
> > +++ b/drivers/staging/fpga/Kconfig
> > @@ -18,4 +18,10 @@ config FPGA_MGR_SYSFS
> > help
> > FPGA Manager SysFS interface.
> >
> > +config FPGA_MGR_ALTERA
> > + tristate "Altera"
> > + depends on FPGA
> > + help
> > + FPGA manager driver support for configuring Altera FPGAs.
> > +
> > endmenu
> > diff --git a/drivers/staging/fpga/Makefile b/drivers/staging/fpga/Makefile
> > index c8a676f..6e0d2bf 100644
> > --- a/drivers/staging/fpga/Makefile
> > +++ b/drivers/staging/fpga/Makefile
> > @@ -8,3 +8,4 @@ fpga-mgr-core-y += fpga-mgr.o
> > obj-$(CONFIG_FPGA) += fpga-mgr-core.o
> >
> > # FPGA Manager Drivers
> > +obj-$(CONFIG_FPGA_MGR_ALTERA) += altera.o
> > diff --git a/drivers/staging/fpga/altera.c b/drivers/staging/fpga/altera.c
> > new file mode 100644
> > index 0000000..14a16b4
> > --- /dev/null
> > +++ b/drivers/staging/fpga/altera.c
> > @@ -0,0 +1,789 @@
> > +/*
> > + * FPGA Manager Driver for Altera FPGAs
> > + *
> > + * 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/cdev.h>
> > +#include <linux/completion.h>
> > +#include <linux/delay.h>
> > +#include <linux/fs.h>
> > +#include <linux/fpga-mgr.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/io.h>
> > +#include <linux/irq.h>
> > +#include <linux/kernel.h>
> > +#include <linux/list.h>
> > +#include <linux/mm.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_device.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/pm.h>
> > +#include <linux/slab.h>
> > +#include <linux/string.h>
> > +#include <linux/types.h>
> > +#include <linux/regulator/consumer.h>
>
> This is pretty long list I believe you will be able to shorten it a little bit.
>
I've done this now in v5.
> > +
> > +/* Controls whether to use the Configuration with DCLK steps */
> > +#ifndef _ALTTERA_FPGAMGR_USE_DCLK
> > +#define _ALTTERA_FPGAMGR_USE_DCLK 0
> > +#endif
>
> This is more or less config option which you should reflect in binding.
>
I just removed the unused code.
> > +
> > +/* Register offsets */
> > +#define ALTERA_FPGAMGR_STAT_OFST 0x0
> > +#define ALTERA_FPGAMGR_CTL_OFST 0x4
> > +#define ALTERA_FPGAMGR_DCLKCNT_OFST 0x8
> > +#define ALTERA_FPGAMGR_DCLKSTAT_OFST 0xc
> > +#define ALTERA_FPGAMGR_GPIO_INTEN_OFST 0x830
> > +#define ALTERA_FPGAMGR_GPIO_INTMSK_OFST 0x834
> > +#define ALTERA_FPGAMGR_GPIO_INTTYPE_LEVEL_OFST 0x838
> > +#define ALTERA_FPGAMGR_GPIO_INT_POL_OFST 0x83c
> > +#define ALTERA_FPGAMGR_GPIO_INTSTAT_OFST 0x840
> > +#define ALTERA_FPGAMGR_GPIO_RAW_INTSTAT_OFST 0x844
> > +#define ALTERA_FPGAMGR_GPIO_PORTA_EOI_OFST 0x84c
> > +#define ALTERA_FPGAMGR_GPIO_EXT_PORTA_OFST 0x850
> > +
> > +/* Register bit defines */
> > +/* ALTERA_FPGAMGR_STAT register mode field values */
> > +#define ALTERA_FPGAMGR_STAT_POWER_UP 0x0 /*ramping*/
> > +#define ALTERA_FPGAMGR_STAT_RESET 0x1
> > +#define ALTERA_FPGAMGR_STAT_CFG 0x2
> > +#define ALTERA_FPGAMGR_STAT_INIT 0x3
> > +#define ALTERA_FPGAMGR_STAT_USER_MODE 0x4
> > +#define ALTERA_FPGAMGR_STAT_UNKNOWN 0x5
> > +#define ALTERA_FPGAMGR_STAT_STATE_MASK 0x7
> > +/* This is a flag value that doesn't really happen in this register field */
> > +#define ALTERA_FPGAMGR_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 ALTERA_FPGAMGR_STAT_MSEL_MASK 0x000000f8
> > +#define ALTERA_FPGAMGR_STAT_MSEL_SHIFT 3
> > +
> > +/* ALTERA_FPGAMGR_CTL register */
> > +#define ALTERA_FPGAMGR_CTL_EN 0x00000001
> > +#define ALTERA_FPGAMGR_CTL_NCE 0x00000002
> > +#define ALTERA_FPGAMGR_CTL_NCFGPULL 0x00000004
> > +
> > +#define CDRATIO_X1 0x00000000
> > +#define CDRATIO_X2 0x00000040
> > +#define CDRATIO_X4 0x00000080
> > +#define CDRATIO_X8 0x000000c0
> > +#define ALTERA_FPGAMGR_CTL_CDRATIO_MASK 0x000000c0
> > +
> > +#define ALTERA_FPGAMGR_CTL_AXICFGEN 0x00000100
> > +
> > +#define CFGWDTH_16 0x00000000
> > +#define CFGWDTH_32 0x00000200
> > +#define ALTERA_FPGAMGR_CTL_CFGWDTH_MASK 0x00000200
> > +
> > +/* ALTERA_FPGAMGR_DCLKSTAT register */
> > +#define ALTERA_FPGAMGR_DCLKSTAT_DCNTDONE_E_DONE 0x1
> > +
> > +/* ALTERA_FPGAMGR_GPIO_* registers share the same bit positions */
> > +#define ALTERA_FPGAMGR_MON_NSTATUS 0x0001
> > +#define ALTERA_FPGAMGR_MON_CONF_DONE 0x0002
> > +#define ALTERA_FPGAMGR_MON_INIT_DONE 0x0004
> > +#define ALTERA_FPGAMGR_MON_CRC_ERROR 0x0008
> > +#define ALTERA_FPGAMGR_MON_CVP_CONF_DONE 0x0010
> > +#define ALTERA_FPGAMGR_MON_PR_READY 0x0020
> > +#define ALTERA_FPGAMGR_MON_PR_ERROR 0x0040
> > +#define ALTERA_FPGAMGR_MON_PR_DONE 0x0080
> > +#define ALTERA_FPGAMGR_MON_NCONFIG_PIN 0x0100
> > +#define ALTERA_FPGAMGR_MON_NSTATUS_PIN 0x0200
> > +#define ALTERA_FPGAMGR_MON_CONF_DONE_PIN 0x0400
> > +#define ALTERA_FPGAMGR_MON_FPGA_POWER_ON 0x0800
> > +#define ALTERA_FPGAMGR_MON_STATUS_MASK 0x0fff
> > +
> > +#define ALTERA_FPGAMGR_NUM_SUPPLIES 3
> > +#define ALTERA_RESUME_TIMEOUT 3
> > +
> > +#if IS_ENABLED(CONFIG_REGULATOR)
> > +/* In power-up order. Reverse for power-down. */
> > +static const char *supply_names[ALTERA_FPGAMGR_NUM_SUPPLIES] = {
> > + "FPGA-1.5V",
> > + "FPGA-1.1V",
> > + "FPGA-2.5V",
> > +};
> > +#endif
>
> __maybe_unused should work here.
>
OK
> > +
> > +struct altera_fpga_priv {
> > + void __iomem *fpga_base_addr;
> > + void __iomem *fpga_data_addr;
> > + struct completion status_complete;
> > + int irq;
> > + struct regulator *fpga_supplies[ALTERA_FPGAMGR_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 ALTERA_FPGAMGR_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 altera_fpga_reg_readl(struct altera_fpga_priv *priv, u32 reg_offset)
> > +{
> > + return readl(priv->fpga_base_addr + reg_offset);
> > +}
> > +
> > +static void altera_fpga_reg_writel(struct altera_fpga_priv *priv,
> > + u32 reg_offset, u32 value)
> > +{
> > + writel(value, priv->fpga_base_addr + reg_offset);
> > +}
> > +
> > +static u32 altera_fpga_reg_raw_readl(struct altera_fpga_priv *priv,
> > + u32 reg_offset)
> > +{
> > + return __raw_readl(priv->fpga_base_addr + reg_offset);
> > +}
> > +
> > +static void altera_fpga_reg_raw_writel(struct altera_fpga_priv *priv,
> > + u32 reg_offset, u32 value)
> > +{
> > + __raw_writel(value, priv->fpga_base_addr + reg_offset);
> > +}
> > +
> > +static void altera_fpga_data_writel(struct altera_fpga_priv *priv, u32 value)
> > +{
> > + writel(value, priv->fpga_data_addr);
> > +}
> > +
> > +static inline void altera_fpga_reg_set_bitsl(struct altera_fpga_priv *priv,
> > + u32 offset, u32 bits)
> > +{
> > + u32 val;
> > +
> > + val = altera_fpga_reg_readl(priv, offset);
> > + val |= bits;
> > + altera_fpga_reg_writel(priv, offset, val);
> > +}
> > +
> > +static inline void altera_fpga_reg_clr_bitsl(struct altera_fpga_priv *priv,
> > + u32 offset, u32 bits)
> > +{
> > + u32 val;
> > +
> > + val = altera_fpga_reg_readl(priv, offset);
> > + val &= ~bits;
> > + altera_fpga_reg_writel(priv, offset, val);
> > +}
> > +
> > +static int altera_fpga_mon_status_get(struct altera_fpga_priv *priv)
> > +{
> > + return altera_fpga_reg_readl(priv,
> > + ALTERA_FPGAMGR_GPIO_EXT_PORTA_OFST) &
> > + ALTERA_FPGAMGR_MON_STATUS_MASK;
> > +}
> > +
> > +static int altera_fpga_state_get(struct altera_fpga_priv *priv)
> > +{
> > + int status = altera_fpga_mon_status_get(priv);
> > +
> > + if ((status & ALTERA_FPGAMGR_MON_FPGA_POWER_ON) == 0)
> > + return ALTERA_FPGAMGR_STAT_POWER_OFF;
> > +
> > + return altera_fpga_reg_readl(priv, ALTERA_FPGAMGR_STAT_OFST) &
> > + ALTERA_FPGAMGR_STAT_STATE_MASK;
> > +}
> > +
> > +static void altera_fpga_clear_done_status(struct altera_fpga_priv *priv)
> > +{
> > + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_DCLKSTAT_OFST,
> > + ALTERA_FPGAMGR_DCLKSTAT_DCNTDONE_E_DONE);
> > +}
> > +
> > +/*
> > + * Set the DCLKCNT, wait for DCLKSTAT to report the count completed, and clear
> > + * the complete status.
> > + */
>
> kernel-doc format please.
>
Not really necessary. But I have done it for the core fpga-mgr.c.
> > +static int altera_fpga_dclk_set_and_wait_clear(struct altera_fpga_priv *priv,
> > + u32 count)
> > +{
> > + int timeout = 2;
> > + u32 done;
> > +
> > + /* Clear any existing DONE status. */
> > + if (altera_fpga_reg_readl(priv, ALTERA_FPGAMGR_DCLKSTAT_OFST))
> > + altera_fpga_clear_done_status(priv);
> > +
> > + /* Issue the DCLK count. */
> > + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_DCLKCNT_OFST, count);
> > +
> > + /* Poll DCLKSTAT to see if it completed in the timeout period. */
> > + do {
> > + done = altera_fpga_reg_readl(priv,
> > + ALTERA_FPGAMGR_DCLKSTAT_OFST);
> > + if (done == ALTERA_FPGAMGR_DCLKSTAT_DCNTDONE_E_DONE) {
> > + altera_fpga_clear_done_status(priv);
> > + return 0;
> > + }
> > + if (count <= 4)
> > + udelay(1);
> > + else
> > + msleep(20);
>
> This looks weird to me.
I simplified it, due to other code going away.
>
> > + } while (timeout--);
> > +
> > + return -ETIMEDOUT;
> > +}
> > +
> > +static int altera_fpga_wait_for_state(struct altera_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 ((altera_fpga_state_get(priv) & state) != 0)
> > + return 0;
> > + msleep(20);
> > + } while (timeout--);
> > +
> > + return -ETIMEDOUT;
> > +}
> > +
> > +static void altera_fpga_enable_irqs(struct altera_fpga_priv *priv, u32 irqs)
> > +{
> > + /* set irqs to level sensitive */
> > + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_GPIO_INTTYPE_LEVEL_OFST, 0);
> > +
> > + /* set interrupt polarity */
> > + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_GPIO_INT_POL_OFST, irqs);
> > +
> > + /* clear irqs */
> > + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_GPIO_PORTA_EOI_OFST, irqs);
> > +
> > + /* unmask interrupts */
> > + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_GPIO_INTMSK_OFST, 0);
> > +
> > + /* enable interrupts */
> > + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_GPIO_INTEN_OFST, irqs);
> > +}
> > +
> > +static void altera_fpga_disable_irqs(struct altera_fpga_priv *priv)
> > +{
> > + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_GPIO_INTEN_OFST, 0);
> > +}
> > +
> > +static irqreturn_t altera_fpga_isr(int irq, void *dev_id)
> > +{
> > + struct altera_fpga_priv *priv = dev_id;
> > + u32 irqs, st;
> > + bool conf_done, nstatus;
> > +
> > + /* clear irqs */
> > + irqs = altera_fpga_reg_raw_readl(priv,
> > + ALTERA_FPGAMGR_GPIO_INTSTAT_OFST);
> > +
> > + altera_fpga_reg_raw_writel(priv, ALTERA_FPGAMGR_GPIO_PORTA_EOI_OFST,
> > + irqs);
> > +
> > + st = altera_fpga_reg_raw_readl(priv,
> > + ALTERA_FPGAMGR_GPIO_EXT_PORTA_OFST);
> > + conf_done = (st & ALTERA_FPGAMGR_MON_CONF_DONE) != 0;
> > + nstatus = (st & ALTERA_FPGAMGR_MON_NSTATUS) != 0;
> > +
> > + /* success */
> > + if (conf_done && nstatus) {
> > + /* disable irqs */
> > + altera_fpga_reg_raw_writel(priv,
> > + ALTERA_FPGAMGR_GPIO_INTEN_OFST, 0);
> > + complete(&priv->status_complete);
> > + }
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int altera_fpga_wait_for_config_done(struct altera_fpga_priv *priv)
> > +{
> > + int timeout, ret = 0;
> > +
> > + altera_fpga_disable_irqs(priv);
> > + init_completion(&priv->status_complete);
> > + altera_fpga_enable_irqs(priv, ALTERA_FPGAMGR_MON_CONF_DONE);
> > +
> > + timeout = wait_for_completion_interruptible_timeout(
> > + &priv->status_complete,
> > + msecs_to_jiffies(10));
> > + if (timeout == 0)
> > + ret = -ETIMEDOUT;
> > +
> > + altera_fpga_disable_irqs(priv);
> > + return ret;
> > +}
> > +
> > +static int altera_fpga_cfg_mode_get(struct altera_fpga_priv *priv)
> > +{
> > + u32 msel;
> > +
> > + msel = altera_fpga_reg_readl(priv, ALTERA_FPGAMGR_STAT_OFST);
> > + msel &= ALTERA_FPGAMGR_STAT_MSEL_MASK;
> > + msel >>= ALTERA_FPGAMGR_STAT_MSEL_SHIFT;
> > +
> > + /* Check that this MSEL setting is supported */
> > + if ((msel >= sizeof(cfgmgr_modes)/sizeof(struct cfgmgr_mode)) ||
> > + !cfgmgr_modes[msel].valid)
> > + return -EINVAL;
> > +
> > + return msel;
> > +}
> > +
> > +static int altera_fpga_cfg_mode_set(struct altera_fpga_priv *priv)
> > +{
> > + u32 ctrl_reg, mode;
> > +
> > + /* get value from MSEL pins */
> > + mode = altera_fpga_cfg_mode_get(priv);
> > + if (mode < 0)
> > + return mode;
> > +
> > + /* Adjust CTRL for the CDRATIO */
> > + ctrl_reg = altera_fpga_reg_readl(priv, ALTERA_FPGAMGR_CTL_OFST);
> > + ctrl_reg &= ~ALTERA_FPGAMGR_CTL_CDRATIO_MASK;
> > + ctrl_reg &= ~ALTERA_FPGAMGR_CTL_CFGWDTH_MASK;
> > + ctrl_reg |= cfgmgr_modes[mode].ctrl;
> > +
> > + /* Set NCE to 0. */
> > + ctrl_reg &= ~ALTERA_FPGAMGR_CTL_NCE;
> > + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_CTL_OFST, ctrl_reg);
> > +
> > + return 0;
> > +}
> > +
> > +static int altera_fpga_reset(struct altera_fpga_priv *priv)
> > +{
> > + u32 ctrl_reg, status;
> > + int ret;
> > +
> > + /*
> > + * Step 1:
> > + * - Set CTRL.CFGWDTH, CTRL.CDRATIO to match cfg mode
> > + * - Set CTRL.NCE to 0
> > + */
> > + ret = altera_fpga_cfg_mode_set(priv);
> > + if (ret)
> > + return ret;
> > +
> > + /* Step 2: Set CTRL.EN to 1 */
> > + altera_fpga_reg_set_bitsl(priv, ALTERA_FPGAMGR_CTL_OFST,
> > + ALTERA_FPGAMGR_CTL_EN);
> > +
> > + /* Step 3: Set CTRL.NCONFIGPULL to 1 to put FPGA in reset */
> > + ctrl_reg = altera_fpga_reg_readl(priv, ALTERA_FPGAMGR_CTL_OFST);
> > + ctrl_reg |= ALTERA_FPGAMGR_CTL_NCFGPULL;
> > + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_CTL_OFST, ctrl_reg);
> > +
> > + /* Step 4: Wait for STATUS.MODE to report FPGA is in reset phase */
> > + status = altera_fpga_wait_for_state(priv, ALTERA_FPGAMGR_STAT_RESET);
> > +
> > + /* Step 5: Set CONTROL.NCONFIGPULL to 0 to release FPGA from reset */
> > + ctrl_reg &= ~ALTERA_FPGAMGR_CTL_NCFGPULL;
> > + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_CTL_OFST, ctrl_reg);
> > +
> > + /* Timeout waiting for reset */
> > + if (status)
> > + return -ETIMEDOUT;
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * Prepare the FPGA to receive the configuration data.
> > + */
>
> kernel-doc
>
> > +static int altera_fpga_ops_configure_init(struct fpga_manager *mgr)
> > +{
> > + struct altera_fpga_priv *priv = mgr->priv;
> > + int ret;
> > +
> > + /* Steps 1 - 5: Reset the FPGA */
> > + ret = altera_fpga_reset(priv);
> > + if (ret)
> > + return ret;
> > +
> > + /* Step 6: Wait for FPGA to enter configuration phase */
> > + if (altera_fpga_wait_for_state(priv, ALTERA_FPGAMGR_STAT_CFG))
> > + return -ETIMEDOUT;
> > +
> > + /* Step 7: Clear nSTATUS interrupt */
> > + altera_fpga_reg_writel(priv, ALTERA_FPGAMGR_GPIO_PORTA_EOI_OFST,
> > + ALTERA_FPGAMGR_MON_NSTATUS);
> > +
> > + /* Step 8: Set CTRL.AXICFGEN to 1 to enable transfer of config data */
> > + altera_fpga_reg_set_bitsl(priv, ALTERA_FPGAMGR_CTL_OFST,
> > + ALTERA_FPGAMGR_CTL_AXICFGEN);
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * Step 9: write data to the FPGA data register
> > + */
>
> step 9 here?
>
> > +static int altera_fpga_ops_configure_write(struct fpga_manager *mgr,
> > + const char *buf, size_t count)
> > +{
> > + struct altera_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)) {
> > + altera_fpga_data_writel(priv, buffer_32[i++]);
> > + count -= sizeof(u32);
> > + }
> > +
> > + /* Write out remaining non 32-bit chunks. */
> > + switch (count) {
> > + case 3:
> > + altera_fpga_data_writel(priv, buffer_32[i++] & 0x00ffffff);
> > + break;
> > + case 2:
> > + altera_fpga_data_writel(priv, buffer_32[i++] & 0x0000ffff);
> > + break;
> > + case 1:
> > + altera_fpga_data_writel(priv, buffer_32[i++] & 0x000000ff);
> > + break;
> > + default:
> > + /* This will never happen. */
> > + break;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int altera_fpga_ops_configure_complete(struct fpga_manager *mgr)
> > +{
> > + struct altera_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 = altera_fpga_wait_for_config_done(priv);
> > + if (status)
> > + return status;
> > +
> > + /* Step 11: Clear CTRL.AXICFGEN to disable transfer of config data */
> > + altera_fpga_reg_clr_bitsl(priv, ALTERA_FPGAMGR_CTL_OFST,
> > + ALTERA_FPGAMGR_CTL_AXICFGEN);
> > +
> > + /*
> > + * Step 12:
> > + * - Write 4 to DCLKCNT
> > + * - Wait for STATUS.DCNTDONE = 1
> > + * - Clear W1C bit in STATUS.DCNTDONE
> > + */
> > + if (altera_fpga_dclk_set_and_wait_clear(priv, 4))
> > + return -ETIMEDOUT;
> > +
> > +#if _ALTTERA_FPGAMGR_USE_DCLK
> > + /* Step 13: Wait for STATUS.MODE to report INIT or USER MODE */
> > + if (altera_fpga_wait_for_state(priv, ALTERA_FPGAMGR_STAT_INIT |
> > + ALTERA_FPGAMGR_STAT_USER_MODE))
> > + return -ETIMEDOUT;
> > +
> > + /*
> > + * Extra steps for Configuration with DCLK for Initialization Phase
> > + * Step 14 (using 4.2.1.2 steps), 15 (using 4.2.1.2 steps)
> > + * - Write 0x5000 to DCLKCNT == the number of clocks needed to exit
> > + * the Initialization Phase.
> > + * - Poll until STATUS.DCNTDONE = 1, write 1 to clear
> > + */
> > + if (altera_fpga_dclk_set_and_wait_clear(priv, 0x5000))
> > + return -ETIMEDOUT;
> > +#endif
> > +
> > + /* Step 13: Wait for STATUS.MODE to report USER MODE */
> > + if (altera_fpga_wait_for_state(priv, ALTERA_FPGAMGR_STAT_USER_MODE))
> > + return -ETIMEDOUT;
> > +
> > + /* Step 14: Set CTRL.EN to 0 */
> > + altera_fpga_reg_clr_bitsl(priv, ALTERA_FPGAMGR_CTL_OFST,
> > + ALTERA_FPGAMGR_CTL_EN);
> > +
> > + return 0;
> > +}
> > +
> > +static int altera_fpga_ops_reset(struct fpga_manager *mgr)
> > +{
> > + return altera_fpga_reset(mgr->priv);
> > +}
>
> looks like not useful code - just use altera_fpga_reset instead of ops.
>
Done.
> btw: just a generic question - isn't it better to use any better
> name than altera_fpga. You can have different loader in future
> and this is very generic.
>
Good feedback. Changed from altera.c to socfpga.c
> > +
> > +/* Translate state register values to FPGA framework state */
> > +static const int altera_state_to_framework_state[] = {
> > + [ALTERA_FPGAMGR_STAT_POWER_OFF] = FPGA_MGR_STATE_POWER_OFF,
> > + [ALTERA_FPGAMGR_STAT_RESET] = FPGA_MGR_STATE_RESET,
> > + [ALTERA_FPGAMGR_STAT_CFG] = FPGA_MGR_STATE_WRITE_INIT,
> > + [ALTERA_FPGAMGR_STAT_INIT] = FPGA_MGR_STATE_WRITE_INIT,
> > + [ALTERA_FPGAMGR_STAT_USER_MODE] = FPGA_MGR_STATE_OPERATING,
> > + [ALTERA_FPGAMGR_STAT_UNKNOWN] = FPGA_MGR_STATE_UNKNOWN,
> > +};
> > +
> > +static int altera_fpga_ops_state(struct fpga_manager *mgr)
>
> here should return enum - look at 5/6 comment.
>
Yes
> > +{
> > + struct altera_fpga_priv *priv = mgr->priv;
> > + u32 state;
>
> this is also int not unsigned.
>
Yes
> > + int ret;
> > +
> > + state = altera_fpga_state_get(priv);
> > +
> > + if (state < ARRAY_SIZE(altera_state_to_framework_state))
> > + ret = altera_state_to_framework_state[state];
> > + else
> > + ret = FPGA_MGR_STATE_UNKNOWN;
> > +
> > + return ret;
> > +}
> > +
> > +#if IS_ENABLED(CONFIG_REGULATOR)
>
> Instead of this look below
>
> > +static int altera_fpga_regulators_on(struct altera_fpga_priv *priv)
> > +{
> > + int i, ret;
> > +
>
> use this
>
> if (!IS_ENABLED(CONFIG_REGULATOR))
> return 0;
>
> Then you can simple compile code just once.
>
> The same change for all these functions.
>
Thanks. I only had to do it in one of the functions. If CONFIG_REGULATOR is
not enabled, regulator_enable() will return 0.
> > + for (i = 0; i < ALTERA_FPGAMGR_NUM_SUPPLIES; i++) {
> > + ret = regulator_enable(priv->fpga_supplies[i]);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static void altera_fpga_regulators_power_off(struct altera_fpga_priv *priv)
> > +{
> > + int i;
> > +
> > + for (i = ALTERA_FPGAMGR_NUM_SUPPLIES - 1; i >= 0; i--)
> > + regulator_disable(priv->fpga_supplies[i]);
> > +}
> > +
> > +static int altera_fpga_regulator_probe(struct platform_device *pdev,
> > + struct altera_fpga_priv *priv)
> > +{
> > + struct regulator *supply;
> > + unsigned int i;
> > +
> > + for (i = 0; i < ALTERA_FPGAMGR_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 altera_fpga_regulators_on(priv);
> > +}
> > +#else
> > +static int altera_fpga_regulators_on(struct altera_fpga_priv *priv)
> > +{
> > + return 0;
> > +}
> > +
> > +static void altera_fpga_regulators_power_off(struct altera_fpga_priv *priv)
> > +{
> > +}
> > +
> > +static int altera_fpga_regulator_probe(struct platform_device *pdev,
> > + struct altera_fpga_priv *priv)
> > +{
> > + return 0;
> > +}
> > +#endif
> > +
> > +static int altera_fpga_suspend(struct fpga_manager *mgr)
> > +{
> > + struct altera_fpga_priv *priv = mgr->priv;
> > +
> > + altera_fpga_regulators_power_off(priv);
> > +
> > + return 0;
> > +}
> > +
> > +static int altera_fpga_resume(struct fpga_manager *mgr)
> > +{
> > + struct altera_fpga_priv *priv = mgr->priv;
> > + u32 state;
> > + unsigned int timeout;
> > + int ret;
> > +
> > + ret = altera_fpga_regulators_on(priv);
> > + if (ret)
> > + return ret;
> > +
> > + for (timeout = 0; timeout < ALTERA_RESUME_TIMEOUT; timeout++) {
> > + state = altera_fpga_state_get(priv);
> > + if (state != ALTERA_FPGAMGR_STAT_POWER_OFF)
> > + break;
> > + msleep(20);
> > + }
> > + if (state == ALTERA_FPGAMGR_STAT_POWER_OFF)
> > + return -ETIMEDOUT;
> > +
> > + return ret;
> > +}
> > +
> > +struct fpga_manager_ops altera_altera_fpga_ops = {
>
> static here.
>
Yes
> > + .reset = altera_fpga_ops_reset,
> > + .state = altera_fpga_ops_state,
> > + .write_init = altera_fpga_ops_configure_init,
> > + .write = altera_fpga_ops_configure_write,
> > + .write_complete = altera_fpga_ops_configure_complete,
> > + .suspend = altera_fpga_suspend,
> > + .resume = altera_fpga_resume,
> > +};
> > +
> > +static int altera_fpga_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct device_node *np = pdev->dev.of_node;
> > + struct altera_fpga_priv *priv;
> > + int ret;
> > +
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + ret = altera_fpga_regulator_probe(pdev, priv);
> > + if (ret)
> > + return ret;
> > +
> > + priv->fpga_base_addr = of_iomap(np, 0);
> > + if (!priv->fpga_base_addr)
> > + return -EINVAL;
> > +
> > + priv->fpga_data_addr = of_iomap(np, 1);
> > + if (!priv->fpga_data_addr) {
> > + ret = -EINVAL;
> > + goto err_unmap_base_addr;
> > + }
> > +
> > + priv->irq = irq_of_parse_and_map(np, 0);
> > + if (priv->irq == NO_IRQ) {
>
> NO_IRQ is not defined for all archs that's why you will get compilation error.
>
> <= 0 should be fine here.
>
> > + ret = -EINVAL;
> > + goto err_unmap_data_addr;
> > + }
>
> Anyway for all of these you should be able to use
> platform_get_resource
> platform_get_irq functions
>
> then you have simplified error path here.
>
OK
>
> > +
> > + ret = request_irq(priv->irq, altera_fpga_isr, 0, "altera-fpga-mgr",
> > + priv);
> > + if (ret < 0)
> > + goto err_dispose_irq;
> > +
> > + ret = fpga_mgr_register(dev, "Altera FPGA Manager",
> > + &altera_altera_fpga_ops, priv);
> > + if (ret)
> > + goto err_free_irq;
> > +
> > + return 0;
> > +
> > +err_free_irq:
> > + free_irq(priv->irq, priv);
> > +err_dispose_irq:
> > + irq_dispose_mapping(priv->irq);
> > +err_unmap_data_addr:
> > + iounmap(priv->fpga_data_addr);
> > +err_unmap_base_addr:
> > + iounmap(priv->fpga_base_addr);
> > + return ret;
> > +}
> > +
> > +static int altera_fpga_remove(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct fpga_manager *mgr = to_fpga_manager(dev);
> > + struct altera_fpga_priv *priv = mgr->priv;
> > +
> > + fpga_mgr_remove(dev);
> > + free_irq(priv->irq, priv);
> > + irq_dispose_mapping(priv->irq);
> > + iounmap(priv->fpga_data_addr);
> > + iounmap(priv->fpga_base_addr);
> > +
> > + return 0;
> > +}
> > +
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id altera_fpga_of_match[] = {
> > + { .compatible = "altr,fpga-mgr", },
> > + {},
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, altera_fpga_of_match);
> > +#endif
> > +
> > +static struct platform_driver altera_fpga_driver = {
> > + .remove = altera_fpga_remove,
> > + .driver = {
> > + .name = "altera_fpga_manager",
> > + .owner = THIS_MODULE,
>
> This line should go away
>
Yes
> > + .of_match_table = of_match_ptr(altera_fpga_of_match),
> > + },
> > + .probe = altera_fpga_probe,
>
> I tend to have probe and remove close to each other.
>
OK
> > +};
> > +
> > +static int __init altera_fpga_init(void)
> > +{
> > + return platform_driver_register(&altera_fpga_driver);
> > +}
> > +
> > +static void __exit altera_fpga_exit(void)
> > +{
> > + platform_driver_unregister(&altera_fpga_driver);
> > +}
> > +
> > +module_init(altera_fpga_init);
> > +module_exit(altera_fpga_exit);
>
> module_platform_driver here
>
Yes
> > +
> > +MODULE_AUTHOR("Alan Tull <[email protected]>");
> > +MODULE_DESCRIPTION("Altera FPGA Manager");
> > +MODULE_LICENSE("GPL v2");
> >
>
> Thanks,
> Michal
>
Thanks for the review.
Alan