[PATCH v5 0/3] fpga: xilinx-selectmap: add new driver

[PATCH v5 0/3] fpga: xilinx-selectmap: add new driver

[Charles Perry]

Hello,

This patchset adds a new driver for the 7 series FPGA's SelectMAP
interface.

The SelectMAP interface shares a common GPIO protocol with the SPI
interface which is already in the kernel (drivers/fpga/xilinx-spi.c).
The approach proposed in this patchset is to refactor xilinx-spi.c into
xilinx-core.c which would handle the common GPIO protocol. This is then
used to build two drivers, the already existing xilinx-spi.c driver and
a newly added xilinx-selectmap.c driver.

The SelectMAP driver proposed only supports 8 bit mode. This is because
the 16 and 32 bits mode have limitations with regards to compressed
bitstream support as well as introducing endianness considerations.

I'm testing xilinx-selectmap.c on a custom i.MX6 board connected to an
Artix 7 FPGA. Flashing a 913K bitstream takes 0.44 seconds.

Changes since v4: (from Yilun and Krzysztof review)
 * xilinx-core: use sizeof() instead of hardcoded immediate
 * xilinx-core: fix module compilation (EXPORT_SYMBOL_GPL, MODULE_LICENSE,
   MODULE_AUTHOR, MODULE_DESCRIPTION)
 * xilinx-core: add private/public qualifiers for struct xilinx_fpga_core
 * xilinx-spi: remove struct xilinx_spi_conf. This struct isn't needed as
   the struct spi_device* can be retrieved from the struct device*.
 * dt-bindings: remove usage of "_b" and "-b" for the new driver. We
   agreed that the spi and selectmap driver will use different bindings
   which will be handled by the driver core and that the legacy names will
   be used only for the spi compatible.
 * xilinx-core: select between prog/init and prog_b/init-b

Changes since v3: (from Rob Herring review)
 * Fix an error in the DT binding example compatible.
 * Drop the renaming of "prog_b" to "prog" and "init-b" to "init".
   Patches 2 and 3 are removed.

Changes since v2:
 * Inserted patch 2 and 3 which rename "prog_b" and "init-b" into "prog"
   and "init" for the SPI driver.
 * From Krzysztof Kozlowski review's:
   * Use more specific compatible names
   * Remove other missing occurences of the slave word missed in v2.
 * From Xu Yilun review's:
   * Fix vertical whitespace in get_done_gpio().
   * Combine write() and write_one_dummy_byte() together.
   * Eliminate most of the xilinx_core_probe() arguments, the driver
     needs to populate those directly into the xilinx_fpga_core struct.
     Added some documentation to struct xilinx_fpga_core to clarify
     this.
   * Removed typedefs from xilinx-core.h.
   * Moved null checks in xilinx_core_probe() to first patch.
   * Move csi_b and rdwr_b out of xilinx_selectmap_conf as they are not
     used out of the probe function.

Changes since v1: (from Krzysztof Kozlowski review's)
  * Use more conventional names for gpio DT bindings
  * fix example in DT bindings
  * add mc-peripheral-props.yaml to DT bindings
  * fix various formatting mistakes
  * Remove all occurences of the "slave" word.

Charles Perry (3):
  fpga: xilinx-spi: extract a common driver core
  dt-bindings: fpga: xlnx,fpga-selectmap: add DT schema
  fpga: xilinx-selectmap: add new driver

 .../bindings/fpga/xlnx,fpga-selectmap.yaml    |  86 +++++++
 drivers/fpga/Kconfig                          |  12 +
 drivers/fpga/Makefile                         |   2 +
 drivers/fpga/xilinx-core.c                    | 238 ++++++++++++++++++
 drivers/fpga/xilinx-core.h                    |  30 +++
 drivers/fpga/xilinx-selectmap.c               |  97 +++++++
 drivers/fpga/xilinx-spi.c                     | 220 +---------------
 7 files changed, 478 insertions(+), 207 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/fpga/xlnx,fpga-selectmap.yaml
 create mode 100644 drivers/fpga/xilinx-core.c
 create mode 100644 drivers/fpga/xilinx-core.h
 create mode 100644 drivers/fpga/xilinx-selectmap.c

--
2.43.0

[PATCH v5 1/3] fpga: xilinx-spi: extract a common driver core

[Charles Perry]

Factor out the gpio handshaking (using PROGRAM_B, INIT_B and DONE)
protocol in xilinx-core so that it can be reused for another driver.
This commit does not change anything functionally to xilinx-spi.

xilinx-core expects drivers to provide a write(const char* buf,
size_t count) function that performs the actual write to the device,
as well as a struct device* for resource management.

Signed-off-by: Charles Perry <charles.perry@savoirfairelinux.com>
---
Changes since v4: (from Yilun review)
 * xilinx-core: use sizeof() instead of hardcoded immediate
 * xilinx-core: fix module compilation (EXPORT_SYMBOL_GPL, MODULE_LICENSE,
   MODULE_AUTHOR, MODULE_DESCRIPTION)
 * xilinx-core: add private/public qualifiers for struct xilinx_fpga_core
 * xilinx-spi: remove struct xilinx_spi_conf. This struct isn't needed as
   the struct spi_device* can be retrieved from the struct device*.

 drivers/fpga/Kconfig       |   4 +
 drivers/fpga/Makefile      |   1 +
 drivers/fpga/xilinx-core.c | 213 +++++++++++++++++++++++++++++++++++
 drivers/fpga/xilinx-core.h |  30 +++++
 drivers/fpga/xilinx-spi.c  | 220 +++----------------------------------
 5 files changed, 261 insertions(+), 207 deletions(-)
 create mode 100644 drivers/fpga/xilinx-core.c
 create mode 100644 drivers/fpga/xilinx-core.h

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 2f689ac4ba3a3..d27a1ebf40838 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -64,9 +64,13 @@ config FPGA_MGR_STRATIX10_SOC
 	help
 	  FPGA manager driver support for the Intel Stratix10 SoC.

+config FPGA_MGR_XILINX_CORE
+	tristate
+
 config FPGA_MGR_XILINX_SPI
 	tristate "Xilinx Configuration over Slave Serial (SPI)"
 	depends on SPI
+	select FPGA_MGR_XILINX_CORE
 	help
 	  FPGA manager driver support for Xilinx FPGA configuration
 	  over slave serial interface.
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index 352a2612623e0..7ec795b6a5a70 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_FPGA_MGR_SOCFPGA)		+= socfpga.o
 obj-$(CONFIG_FPGA_MGR_SOCFPGA_A10)	+= socfpga-a10.o
 obj-$(CONFIG_FPGA_MGR_STRATIX10_SOC)	+= stratix10-soc.o
 obj-$(CONFIG_FPGA_MGR_TS73XX)		+= ts73xx-fpga.o
+obj-$(CONFIG_FPGA_MGR_XILINX_CORE)	+= xilinx-core.o
 obj-$(CONFIG_FPGA_MGR_XILINX_SPI)	+= xilinx-spi.o
 obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA)	+= zynq-fpga.o
 obj-$(CONFIG_FPGA_MGR_ZYNQMP_FPGA)	+= zynqmp-fpga.o
diff --git a/drivers/fpga/xilinx-core.c b/drivers/fpga/xilinx-core.c
new file mode 100644
index 0000000000000..a35c43382dd5f
--- /dev/null
+++ b/drivers/fpga/xilinx-core.c
@@ -0,0 +1,213 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Common parts of the Xilinx Spartan6 and 7 Series FPGA manager drivers.
+ *
+ * Copyright (C) 2017 DENX Software Engineering
+ *
+ * Anatolij Gustschin <agust@denx.de>
+ */
+
+#include "xilinx-core.h"
+
+#include <linux/delay.h>
+#include <linux/fpga/fpga-mgr.h>
+#include <linux/gpio/consumer.h>
+#include <linux/of.h>
+
+static int get_done_gpio(struct fpga_manager *mgr)
+{
+	struct xilinx_fpga_core *core = mgr->priv;
+	int ret;
+
+	ret = gpiod_get_value(core->done);
+	if (ret < 0)
+		dev_err(&mgr->dev, "Error reading DONE (%d)\n", ret);
+
+	return ret;
+}
+
+static enum fpga_mgr_states xilinx_core_state(struct fpga_manager *mgr)
+{
+	if (!get_done_gpio(mgr))
+		return FPGA_MGR_STATE_RESET;
+
+	return FPGA_MGR_STATE_UNKNOWN;
+}
+
+/**
+ * wait_for_init_b - wait for the INIT_B pin to have a given state, or wait
+ * a given delay if the pin is unavailable
+ *
+ * @mgr:        The FPGA manager object
+ * @value:      Value INIT_B to wait for (1 = asserted = low)
+ * @alt_udelay: Delay to wait if the INIT_B GPIO is not available
+ *
+ * Returns 0 when the INIT_B GPIO reached the given state or -ETIMEDOUT if
+ * too much time passed waiting for that. If no INIT_B GPIO is available
+ * then always return 0.
+ */
+static int wait_for_init_b(struct fpga_manager *mgr, int value,
+			   unsigned long alt_udelay)
+{
+	struct xilinx_fpga_core *core = mgr->priv;
+	unsigned long timeout = jiffies + msecs_to_jiffies(1000);
+
+	if (core->init_b) {
+		while (time_before(jiffies, timeout)) {
+			int ret = gpiod_get_value(core->init_b);
+
+			if (ret == value)
+				return 0;
+
+			if (ret < 0) {
+				dev_err(&mgr->dev,
+					"Error reading INIT_B (%d)\n", ret);
+				return ret;
+			}
+
+			usleep_range(100, 400);
+		}
+
+		dev_err(&mgr->dev, "Timeout waiting for INIT_B to %s\n",
+			value ? "assert" : "deassert");
+		return -ETIMEDOUT;
+	}
+
+	udelay(alt_udelay);
+
+	return 0;
+}
+
+static int xilinx_core_write_init(struct fpga_manager *mgr,
+				  struct fpga_image_info *info, const char *buf,
+				  size_t count)
+{
+	struct xilinx_fpga_core *core = mgr->priv;
+	int err;
+
+	if (info->flags & FPGA_MGR_PARTIAL_RECONFIG) {
+		dev_err(&mgr->dev, "Partial reconfiguration not supported\n");
+		return -EINVAL;
+	}
+
+	gpiod_set_value(core->prog_b, 1);
+
+	err = wait_for_init_b(mgr, 1, 1); /* min is 500 ns */
+	if (err) {
+		gpiod_set_value(core->prog_b, 0);
+		return err;
+	}
+
+	gpiod_set_value(core->prog_b, 0);
+
+	err = wait_for_init_b(mgr, 0, 0);
+	if (err)
+		return err;
+
+	if (get_done_gpio(mgr)) {
+		dev_err(&mgr->dev, "Unexpected DONE pin state...\n");
+		return -EIO;
+	}
+
+	/* program latency */
+	usleep_range(7500, 7600);
+	return 0;
+}
+
+static int xilinx_core_write(struct fpga_manager *mgr, const char *buf,
+			     size_t count)
+{
+	struct xilinx_fpga_core *core = mgr->priv;
+
+	return core->write(core, buf, count);
+}
+
+static int xilinx_core_write_complete(struct fpga_manager *mgr,
+				      struct fpga_image_info *info)
+{
+	struct xilinx_fpga_core *core = mgr->priv;
+	unsigned long timeout =
+		jiffies + usecs_to_jiffies(info->config_complete_timeout_us);
+	bool expired = false;
+	int done;
+	int ret;
+	const char padding[1] = { 0xff };
+
+	/*
+	 * This loop is carefully written such that if the driver is
+	 * scheduled out for more than 'timeout', we still check for DONE
+	 * before giving up and we apply 8 extra CCLK cycles in all cases.
+	 */
+	while (!expired) {
+		expired = time_after(jiffies, timeout);
+
+		done = get_done_gpio(mgr);
+		if (done < 0)
+			return done;
+
+		ret = core->write(core, padding, sizeof(padding));
+		if (ret)
+			return ret;
+
+		if (done)
+			return 0;
+	}
+
+	if (core->init_b) {
+		ret = gpiod_get_value(core->init_b);
+
+		if (ret < 0) {
+			dev_err(&mgr->dev, "Error reading INIT_B (%d)\n", ret);
+			return ret;
+		}
+
+		dev_err(&mgr->dev,
+			ret ? "CRC error or invalid device\n" :
+			      "Missing sync word or incomplete bitstream\n");
+	} else {
+		dev_err(&mgr->dev, "Timeout after config data transfer\n");
+	}
+
+	return -ETIMEDOUT;
+}
+
+static const struct fpga_manager_ops xilinx_core_ops = {
+	.state = xilinx_core_state,
+	.write_init = xilinx_core_write_init,
+	.write = xilinx_core_write,
+	.write_complete = xilinx_core_write_complete,
+};
+
+int xilinx_core_probe(struct xilinx_fpga_core *core)
+{
+	struct fpga_manager *mgr;
+
+	if (!core || !core->dev || !core->write)
+		return -EINVAL;
+
+	/* PROGRAM_B is active low */
+	core->prog_b = devm_gpiod_get(core->dev, "prog_b", GPIOD_OUT_LOW);
+	if (IS_ERR(core->prog_b))
+		return dev_err_probe(core->dev, PTR_ERR(core->prog_b),
+				     "Failed to get PROGRAM_B gpio\n");
+
+	core->init_b = devm_gpiod_get_optional(core->dev, "init-b", GPIOD_IN);
+	if (IS_ERR(core->init_b))
+		return dev_err_probe(core->dev, PTR_ERR(core->init_b),
+				     "Failed to get INIT_B gpio\n");
+
+	core->done = devm_gpiod_get(core->dev, "done", GPIOD_IN);
+	if (IS_ERR(core->done))
+		return dev_err_probe(core->dev, PTR_ERR(core->done),
+				     "Failed to get DONE gpio\n");
+
+	mgr = devm_fpga_mgr_register(core->dev,
+				     "Xilinx Slave Serial FPGA Manager",
+				     &xilinx_core_ops, core);
+	return PTR_ERR_OR_ZERO(mgr);
+}
+EXPORT_SYMBOL_GPL(xilinx_core_probe);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Anatolij Gustschin <agust@denx.de>");
+MODULE_DESCRIPTION("Xilinx 7 Series FPGA manager core");
diff --git a/drivers/fpga/xilinx-core.h b/drivers/fpga/xilinx-core.h
new file mode 100644
index 0000000000000..2fef95b341be0
--- /dev/null
+++ b/drivers/fpga/xilinx-core.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __XILINX_CORE_H
+#define __XILINX_CORE_H
+
+#include <linux/device.h>
+
+/**
+ * struct xilinx_fpga_core - interface between the driver and the core manager
+ *                           of Xilinx 7 Series FPGA manager
+ * @dev:       device node
+ * @write:     write callback of the driver
+ * @prog_b:    PROGRAM_B gpio descriptor
+ * @init_b:    INIT_B gpio descriptor
+ * @done:      DONE gpio descriptor
+ */
+struct xilinx_fpga_core {
+/* public: */
+	struct device *dev;
+	int (*write)(struct xilinx_fpga_core *core, const char *buf,
+		     size_t count);
+/* private: handled by xilinx-core */
+	struct gpio_desc *prog_b;
+	struct gpio_desc *init_b;
+	struct gpio_desc *done;
+};
+
+int xilinx_core_probe(struct xilinx_fpga_core *core);
+
+#endif /* __XILINX_CORE_H */
diff --git a/drivers/fpga/xilinx-spi.c b/drivers/fpga/xilinx-spi.c
index e1a227e7ff2ae..ebf339cf0fd81 100644
--- a/drivers/fpga/xilinx-spi.c
+++ b/drivers/fpga/xilinx-spi.c
@@ -10,127 +10,17 @@
  * the slave serial configuration interface.
  */

-#include <linux/delay.h>
-#include <linux/device.h>
-#include <linux/fpga/fpga-mgr.h>
-#include <linux/gpio/consumer.h>
+#include "xilinx-core.h"
+
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
 #include <linux/of.h>
 #include <linux/spi/spi.h>
-#include <linux/sizes.h>
-
-struct xilinx_spi_conf {
-	struct spi_device *spi;
-	struct gpio_desc *prog_b;
-	struct gpio_desc *init_b;
-	struct gpio_desc *done;
-};
-
-static int get_done_gpio(struct fpga_manager *mgr)
-{
-	struct xilinx_spi_conf *conf = mgr->priv;
-	int ret;
-
-	ret = gpiod_get_value(conf->done);
-
-	if (ret < 0)
-		dev_err(&mgr->dev, "Error reading DONE (%d)\n", ret);
-
-	return ret;
-}
-
-static enum fpga_mgr_states xilinx_spi_state(struct fpga_manager *mgr)
-{
-	if (!get_done_gpio(mgr))
-		return FPGA_MGR_STATE_RESET;
-
-	return FPGA_MGR_STATE_UNKNOWN;
-}
-
-/**
- * wait_for_init_b - wait for the INIT_B pin to have a given state, or wait
- * a given delay if the pin is unavailable
- *
- * @mgr:        The FPGA manager object
- * @value:      Value INIT_B to wait for (1 = asserted = low)
- * @alt_udelay: Delay to wait if the INIT_B GPIO is not available
- *
- * Returns 0 when the INIT_B GPIO reached the given state or -ETIMEDOUT if
- * too much time passed waiting for that. If no INIT_B GPIO is available
- * then always return 0.
- */
-static int wait_for_init_b(struct fpga_manager *mgr, int value,
-			   unsigned long alt_udelay)
-{
-	struct xilinx_spi_conf *conf = mgr->priv;
-	unsigned long timeout = jiffies + msecs_to_jiffies(1000);
-
-	if (conf->init_b) {
-		while (time_before(jiffies, timeout)) {
-			int ret = gpiod_get_value(conf->init_b);
-
-			if (ret == value)
-				return 0;
-
-			if (ret < 0) {
-				dev_err(&mgr->dev, "Error reading INIT_B (%d)\n", ret);
-				return ret;
-			}
-
-			usleep_range(100, 400);
-		}
-
-		dev_err(&mgr->dev, "Timeout waiting for INIT_B to %s\n",
-			value ? "assert" : "deassert");
-		return -ETIMEDOUT;
-	}
-
-	udelay(alt_udelay);
-
-	return 0;
-}
-
-static int xilinx_spi_write_init(struct fpga_manager *mgr,
-				 struct fpga_image_info *info,
-				 const char *buf, size_t count)
-{
-	struct xilinx_spi_conf *conf = mgr->priv;
-	int err;
-
-	if (info->flags & FPGA_MGR_PARTIAL_RECONFIG) {
-		dev_err(&mgr->dev, "Partial reconfiguration not supported\n");
-		return -EINVAL;
-	}
-
-	gpiod_set_value(conf->prog_b, 1);
-
-	err = wait_for_init_b(mgr, 1, 1); /* min is 500 ns */
-	if (err) {
-		gpiod_set_value(conf->prog_b, 0);
-		return err;
-	}
-
-	gpiod_set_value(conf->prog_b, 0);
-
-	err = wait_for_init_b(mgr, 0, 0);
-	if (err)
-		return err;
-
-	if (get_done_gpio(mgr)) {
-		dev_err(&mgr->dev, "Unexpected DONE pin state...\n");
-		return -EIO;
-	}

-	/* program latency */
-	usleep_range(7500, 7600);
-	return 0;
-}
-
-static int xilinx_spi_write(struct fpga_manager *mgr, const char *buf,
+static int xilinx_spi_write(struct xilinx_fpga_core *core, const char *buf,
 			    size_t count)
 {
-	struct xilinx_spi_conf *conf = mgr->priv;
+	struct spi_device *spi = to_spi_device(core->dev);
 	const char *fw_data = buf;
 	const char *fw_data_end = fw_data + count;

@@ -141,9 +31,9 @@ static int xilinx_spi_write(struct fpga_manager *mgr, const char *buf,
 		remaining = fw_data_end - fw_data;
 		stride = min_t(size_t, remaining, SZ_4K);

-		ret = spi_write(conf->spi, fw_data, stride);
+		ret = spi_write(spi, fw_data, stride);
 		if (ret) {
-			dev_err(&mgr->dev, "SPI error in firmware write: %d\n",
+			dev_err(core->dev, "SPI error in firmware write: %d\n",
 				ret);
 			return ret;
 		}
@@ -153,109 +43,25 @@ static int xilinx_spi_write(struct fpga_manager *mgr, const char *buf,
 	return 0;
 }

-static int xilinx_spi_apply_cclk_cycles(struct xilinx_spi_conf *conf)
-{
-	struct spi_device *spi = conf->spi;
-	const u8 din_data[1] = { 0xff };
-	int ret;
-
-	ret = spi_write(conf->spi, din_data, sizeof(din_data));
-	if (ret)
-		dev_err(&spi->dev, "applying CCLK cycles failed: %d\n", ret);
-
-	return ret;
-}
-
-static int xilinx_spi_write_complete(struct fpga_manager *mgr,
-				     struct fpga_image_info *info)
-{
-	struct xilinx_spi_conf *conf = mgr->priv;
-	unsigned long timeout = jiffies + usecs_to_jiffies(info->config_complete_timeout_us);
-	bool expired = false;
-	int done;
-	int ret;
-
-	/*
-	 * This loop is carefully written such that if the driver is
-	 * scheduled out for more than 'timeout', we still check for DONE
-	 * before giving up and we apply 8 extra CCLK cycles in all cases.
-	 */
-	while (!expired) {
-		expired = time_after(jiffies, timeout);
-
-		done = get_done_gpio(mgr);
-		if (done < 0)
-			return done;
-
-		ret = xilinx_spi_apply_cclk_cycles(conf);
-		if (ret)
-			return ret;
-
-		if (done)
-			return 0;
-	}
-
-	if (conf->init_b) {
-		ret = gpiod_get_value(conf->init_b);
-
-		if (ret < 0) {
-			dev_err(&mgr->dev, "Error reading INIT_B (%d)\n", ret);
-			return ret;
-		}
-
-		dev_err(&mgr->dev,
-			ret ? "CRC error or invalid device\n"
-			: "Missing sync word or incomplete bitstream\n");
-	} else {
-		dev_err(&mgr->dev, "Timeout after config data transfer\n");
-	}
-
-	return -ETIMEDOUT;
-}
-
-static const struct fpga_manager_ops xilinx_spi_ops = {
-	.state = xilinx_spi_state,
-	.write_init = xilinx_spi_write_init,
-	.write = xilinx_spi_write,
-	.write_complete = xilinx_spi_write_complete,
-};
-
 static int xilinx_spi_probe(struct spi_device *spi)
 {
-	struct xilinx_spi_conf *conf;
-	struct fpga_manager *mgr;
+	struct xilinx_fpga_core *conf;

 	conf = devm_kzalloc(&spi->dev, sizeof(*conf), GFP_KERNEL);
 	if (!conf)
 		return -ENOMEM;

-	conf->spi = spi;
+	conf->dev = &spi->dev;
+	conf->write = xilinx_spi_write;

-	/* PROGRAM_B is active low */
-	conf->prog_b = devm_gpiod_get(&spi->dev, "prog_b", GPIOD_OUT_LOW);
-	if (IS_ERR(conf->prog_b))
-		return dev_err_probe(&spi->dev, PTR_ERR(conf->prog_b),
-				     "Failed to get PROGRAM_B gpio\n");
-
-	conf->init_b = devm_gpiod_get_optional(&spi->dev, "init-b", GPIOD_IN);
-	if (IS_ERR(conf->init_b))
-		return dev_err_probe(&spi->dev, PTR_ERR(conf->init_b),
-				     "Failed to get INIT_B gpio\n");
-
-	conf->done = devm_gpiod_get(&spi->dev, "done", GPIOD_IN);
-	if (IS_ERR(conf->done))
-		return dev_err_probe(&spi->dev, PTR_ERR(conf->done),
-				     "Failed to get DONE gpio\n");
-
-	mgr = devm_fpga_mgr_register(&spi->dev,
-				     "Xilinx Slave Serial FPGA Manager",
-				     &xilinx_spi_ops, conf);
-	return PTR_ERR_OR_ZERO(mgr);
+	return xilinx_core_probe(conf);
 }

 #ifdef CONFIG_OF
 static const struct of_device_id xlnx_spi_of_match[] = {
-	{ .compatible = "xlnx,fpga-slave-serial", },
+	{
+		.compatible = "xlnx,fpga-slave-serial",
+	},
 	{}
 };
 MODULE_DEVICE_TABLE(of, xlnx_spi_of_match);
--
2.43.0

Re: [PATCH v5 1/3] fpga: xilinx-spi: extract a common driver core

[Xu Yilun]

> +/**
> + * struct xilinx_fpga_core - interface between the driver and the core manager
> + *                           of Xilinx 7 Series FPGA manager
> + * @dev:       device node
> + * @write:     write callback of the driver
> + * @prog_b:    PROGRAM_B gpio descriptor
> + * @init_b:    INIT_B gpio descriptor
> + * @done:      DONE gpio descriptor

Please re-check the Documentation again:
"Structure fields that are inside a private: area are not listed in the generated output documentation"

> + */
> +struct xilinx_fpga_core {
> +/* public: */
> +	struct device *dev;
> +	int (*write)(struct xilinx_fpga_core *core, const char *buf,
> +		     size_t count);
> +/* private: handled by xilinx-core */
> +	struct gpio_desc *prog_b;
> +	struct gpio_desc *init_b;
> +	struct gpio_desc *done;
> +};
> +
[...]
> -
>  static int xilinx_spi_probe(struct spi_device *spi)
>  {
> -	struct xilinx_spi_conf *conf;
> -	struct fpga_manager *mgr;
> +	struct xilinx_fpga_core *conf;

Why do you name it conf? Maybe "core" is better?

Thanks,
Yilun

Re: [PATCH v5 1/3] fpga: xilinx-spi: extract a common driver core

[Charles Perry]

On Mar 19, 2024, at 12:20 AM, Xu Yilun yilun.xu@linux.intel.com wrote:
>> +/**
>> + * struct xilinx_fpga_core - interface between the driver and the core manager
>> + *                           of Xilinx 7 Series FPGA manager
>> + * @dev:       device node
>> + * @write:     write callback of the driver
>> + * @prog_b:    PROGRAM_B gpio descriptor
>> + * @init_b:    INIT_B gpio descriptor
>> + * @done:      DONE gpio descriptor
> 
> Please re-check the Documentation again:
> "Structure fields that are inside a private: area are not listed in the
> generated output documentation"
> 

I did generate the documentation for that struct and saw that the
private fields are indeed removed. This hinted me into thinking that
I should keep the private kernel-doc, because it's the generator job
to remove those. Looking again at the kernel-doc.rst example on that
topic, I understand that this is not the case, will fix.

>> + */
>> +struct xilinx_fpga_core {
>> +/* public: */
>> +	struct device *dev;
>> +	int (*write)(struct xilinx_fpga_core *core, const char *buf,
>> +		     size_t count);
>> +/* private: handled by xilinx-core */
>> +	struct gpio_desc *prog_b;
>> +	struct gpio_desc *init_b;
>> +	struct gpio_desc *done;
>> +};
>> +
> [...]
>> -
>>  static int xilinx_spi_probe(struct spi_device *spi)
>>  {
>> -	struct xilinx_spi_conf *conf;
>> -	struct fpga_manager *mgr;
>> +	struct xilinx_fpga_core *conf;
> 
> Why do you name it conf? Maybe "core" is better?
> 
> Thanks,
> Yilun

Yes, I changed the type but not the name.

Thank you for the review,
Charles

[PATCH v5 2/3] dt-bindings: fpga: xlnx,fpga-selectmap: add DT schema

[Charles Perry]

Document the SelectMAP interface of Xilinx 7 series FPGA.

Signed-off-by: Charles Perry <charles.perry@savoirfairelinux.com>
---
Changes since v4: (from Yilun and Krzysztof review)
 * dt-bindings: remove usage of "_b" and "-b" for the new driver. We
   agreed that the spi and selectmap driver will use different bindings
   which will be handled by the driver core and that the legacy names will
   be used only for the spi compatible.

 .../bindings/fpga/xlnx,fpga-selectmap.yaml    | 86 +++++++++++++++++++
 1 file changed, 86 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/fpga/xlnx,fpga-selectmap.yaml

diff --git a/Documentation/devicetree/bindings/fpga/xlnx,fpga-selectmap.yaml b/Documentation/devicetree/bindings/fpga/xlnx,fpga-selectmap.yaml
new file mode 100644
index 0000000000000..05775746fd706
--- /dev/null
+++ b/Documentation/devicetree/bindings/fpga/xlnx,fpga-selectmap.yaml
@@ -0,0 +1,86 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/fpga/xlnx,fpga-selectmap.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Xilinx SelectMAP FPGA interface
+
+maintainers:
+  - Charles Perry <charles.perry@savoirfairelinux.com>
+
+description: |
+  Xilinx 7 Series FPGAs support a method of loading the bitstream over a
+  parallel port named the SelectMAP interface in the documentation. Only
+  the x8 mode is supported where data is loaded at one byte per rising edge of
+  the clock, with the MSB of each byte presented to the D0 pin.
+
+  Datasheets:
+    https://www.xilinx.com/support/documentation/user_guides/ug470_7Series_Config.pdf
+
+allOf:
+  - $ref: /schemas/memory-controllers/mc-peripheral-props.yaml#
+
+properties:
+  compatible:
+    enum:
+      - xlnx,fpga-xc7s-selectmap
+      - xlnx,fpga-xc7a-selectmap
+      - xlnx,fpga-xc7k-selectmap
+      - xlnx,fpga-xc7v-selectmap
+
+  reg:
+    description:
+      At least 1 byte of memory mapped IO
+    maxItems: 1
+
+  prog-gpios:
+    description:
+      config pin (referred to as PROGRAM_B in the manual)
+    maxItems: 1
+
+  done-gpios:
+    description:
+      config status pin (referred to as DONE in the manual)
+    maxItems: 1
+
+  init-gpios:
+    description:
+      initialization status and configuration error pin
+      (referred to as INIT_B in the manual)
+    maxItems: 1
+
+  csi-gpios:
+    description:
+      chip select pin (referred to as CSI_B in the manual)
+      Optional gpio for if the bus controller does not provide a chip select.
+    maxItems: 1
+
+  rdwr-gpios:
+    description:
+      read/write select pin (referred to as RDWR_B in the manual)
+      Optional gpio for if the bus controller does not provide this pin.
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - prog-gpios
+  - done-gpios
+  - init-gpios
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    fpga-mgr@8000000 {
+      compatible = "xlnx,fpga-xc7s-selectmap";
+      reg = <0x8000000 0x4>;
+      prog-gpios = <&gpio5 5 GPIO_ACTIVE_LOW>;
+      init-gpios = <&gpio5 8 GPIO_ACTIVE_LOW>;
+      done-gpios = <&gpio2 30 GPIO_ACTIVE_HIGH>;
+      csi-gpios = <&gpio3 19 GPIO_ACTIVE_LOW>;
+      rdwr-gpios = <&gpio3 10 GPIO_ACTIVE_LOW>;
+    };
+...
--
2.43.0

Re: [PATCH v5 2/3] dt-bindings: fpga: xlnx,fpga-selectmap: add DT schema

[Krzysztof Kozlowski]

On 13/03/2024 23:57, Charles Perry wrote:
> Document the SelectMAP interface of Xilinx 7 series FPGA.
> 
> Signed-off-by: Charles Perry <charles.perry@savoirfairelinux.com>
> ---
> Changes since v4: (from Yilun and Krzysztof review)


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH v5 3/3] fpga: xilinx-selectmap: add new driver

[Charles Perry]

Xilinx 7 series FPGA can be programmed using a parallel port named
the SelectMAP interface in the datasheet. This interface is compatible
with the i.MX6 EIM bus controller but other types of external memory
mapped parallel bus might work.

xilinx-selectmap currently only supports the x8 mode where data is loaded
at one byte per rising edge of the clock, with the MSb of each byte
presented to the D0 pin.

Signed-off-by: Charles Perry <charles.perry@savoirfairelinux.com>
---
Changes since v4: (from Yilun review)
 * xilinx-core: select between prog/init and prog_b/init-b

 drivers/fpga/Kconfig            |  8 +++
 drivers/fpga/Makefile           |  1 +
 drivers/fpga/xilinx-core.c      | 29 +++++++++-
 drivers/fpga/xilinx-selectmap.c | 97 +++++++++++++++++++++++++++++++++
 4 files changed, 133 insertions(+), 2 deletions(-)
 create mode 100644 drivers/fpga/xilinx-selectmap.c

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index d27a1ebf40838..37b35f58f0dfb 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -67,6 +67,14 @@ config FPGA_MGR_STRATIX10_SOC
 config FPGA_MGR_XILINX_CORE
 	tristate

+config FPGA_MGR_XILINX_SELECTMAP
+	tristate "Xilinx Configuration over SelectMAP"
+	depends on HAS_IOMEM
+	select FPGA_MGR_XILINX_CORE
+	help
+	  FPGA manager driver support for Xilinx FPGA configuration
+	  over SelectMAP interface.
+
 config FPGA_MGR_XILINX_SPI
 	tristate "Xilinx Configuration over Slave Serial (SPI)"
 	depends on SPI
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index 7ec795b6a5a70..aeb89bb13517e 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_FPGA_MGR_SOCFPGA_A10)	+= socfpga-a10.o
 obj-$(CONFIG_FPGA_MGR_STRATIX10_SOC)	+= stratix10-soc.o
 obj-$(CONFIG_FPGA_MGR_TS73XX)		+= ts73xx-fpga.o
 obj-$(CONFIG_FPGA_MGR_XILINX_CORE)	+= xilinx-core.o
+obj-$(CONFIG_FPGA_MGR_XILINX_SELECTMAP)	+= xilinx-selectmap.o
 obj-$(CONFIG_FPGA_MGR_XILINX_SPI)	+= xilinx-spi.o
 obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA)	+= zynq-fpga.o
 obj-$(CONFIG_FPGA_MGR_ZYNQMP_FPGA)	+= zynqmp-fpga.o
diff --git a/drivers/fpga/xilinx-core.c b/drivers/fpga/xilinx-core.c
index a35c43382dd5f..ccdeb45eba4ee 100644
--- a/drivers/fpga/xilinx-core.c
+++ b/drivers/fpga/xilinx-core.c
@@ -171,6 +171,29 @@ static int xilinx_core_write_complete(struct fpga_manager *mgr,
 	return -ETIMEDOUT;
 }

+/**
+ * xilinx_core_devm_gpiod_get - Obtain a resource-managed GPIO using a
+ *                              legacy consumer name fallback.
+ *
+ * @dev:           Device managing the GPIO
+ * @con_id:        Consumer id
+ * @legacy_con_id: Legacy consumer id
+ * @flags:         optional GPIO initialization flags
+ */
+static inline struct gpio_desc *
+xilinx_core_devm_gpiod_get(struct device *dev, const char *con_id,
+			   const char *legacy_con_id, enum gpiod_flags flags)
+{
+	struct gpio_desc *desc;
+
+	desc = devm_gpiod_get(dev, con_id, flags);
+	if (IS_ERR(desc) && PTR_ERR(desc) == -ENOENT &&
+	    of_device_is_compatible(dev->of_node, "xlnx,fpga-slave-serial"))
+		desc = devm_gpiod_get(dev, legacy_con_id, flags);
+
+	return desc;
+}
+
 static const struct fpga_manager_ops xilinx_core_ops = {
 	.state = xilinx_core_state,
 	.write_init = xilinx_core_write_init,
@@ -186,12 +209,14 @@ int xilinx_core_probe(struct xilinx_fpga_core *core)
 		return -EINVAL;

 	/* PROGRAM_B is active low */
-	core->prog_b = devm_gpiod_get(core->dev, "prog_b", GPIOD_OUT_LOW);
+	core->prog_b = xilinx_core_devm_gpiod_get(core->dev, "prog", "prog_b",
+						  GPIOD_OUT_LOW);
 	if (IS_ERR(core->prog_b))
 		return dev_err_probe(core->dev, PTR_ERR(core->prog_b),
 				     "Failed to get PROGRAM_B gpio\n");

-	core->init_b = devm_gpiod_get_optional(core->dev, "init-b", GPIOD_IN);
+	core->init_b = xilinx_core_devm_gpiod_get(core->dev, "init", "init-b",
+						  GPIOD_IN);
 	if (IS_ERR(core->init_b))
 		return dev_err_probe(core->dev, PTR_ERR(core->init_b),
 				     "Failed to get INIT_B gpio\n");
diff --git a/drivers/fpga/xilinx-selectmap.c b/drivers/fpga/xilinx-selectmap.c
new file mode 100644
index 0000000000000..b63f4623f8b2c
--- /dev/null
+++ b/drivers/fpga/xilinx-selectmap.c
@@ -0,0 +1,97 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Xilinx Spartan6 and 7 Series SelectMAP interface driver
+ *
+ * (C) 2024 Charles Perry <charles.perry@savoirfairelinux.com>
+ *
+ * Manage Xilinx FPGA firmware loaded over the SelectMAP configuration
+ * interface.
+ */
+
+#include "xilinx-core.h"
+
+#include <linux/platform_device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/of.h>
+#include <linux/io.h>
+
+struct xilinx_selectmap_conf {
+	struct xilinx_fpga_core core;
+	void __iomem *base;
+};
+
+#define to_xilinx_selectmap_conf(obj) \
+	container_of(obj, struct xilinx_selectmap_conf, core)
+
+static int xilinx_selectmap_write(struct xilinx_fpga_core *core,
+				  const char *buf, size_t count)
+{
+	struct xilinx_selectmap_conf *conf = to_xilinx_selectmap_conf(core);
+	u32 i;
+
+	for (i = 0; i < count; ++i)
+		writeb(buf[i], conf->base);
+
+	return 0;
+}
+
+static int xilinx_selectmap_probe(struct platform_device *pdev)
+{
+	struct xilinx_selectmap_conf *conf;
+	struct resource *r;
+	void __iomem *base;
+	struct gpio_desc *csi_b;
+	struct gpio_desc *rdwr_b;
+
+	conf = devm_kzalloc(&pdev->dev, sizeof(*conf), GFP_KERNEL);
+	if (!conf)
+		return -ENOMEM;
+
+	conf->core.dev = &pdev->dev;
+	conf->core.write = xilinx_selectmap_write;
+
+	base = devm_platform_get_and_ioremap_resource(pdev, 0, &r);
+	if (IS_ERR(base))
+		return dev_err_probe(&pdev->dev, PTR_ERR(base),
+				     "ioremap error\n");
+	conf->base = base;
+
+	/* CSI_B is active low */
+	csi_b = devm_gpiod_get_optional(&pdev->dev, "csi", GPIOD_OUT_HIGH);
+	if (IS_ERR(csi_b))
+		return dev_err_probe(&pdev->dev, PTR_ERR(csi_b),
+				     "Failed to get CSI_B gpio\n");
+
+	/* RDWR_B is active low */
+	rdwr_b = devm_gpiod_get_optional(&pdev->dev, "rdwr", GPIOD_OUT_HIGH);
+	if (IS_ERR(rdwr_b))
+		return dev_err_probe(&pdev->dev, PTR_ERR(rdwr_b),
+				     "Failed to get RDWR_B gpio\n");
+
+	return xilinx_core_probe(&conf->core);
+}
+
+static const struct of_device_id xlnx_selectmap_of_match[] = {
+	{ .compatible = "xlnx,fpga-xc7s-selectmap", }, // Spartan-7
+	{ .compatible = "xlnx,fpga-xc7a-selectmap", }, // Artix-7
+	{ .compatible = "xlnx,fpga-xc7k-selectmap", }, // Kintex-7
+	{ .compatible = "xlnx,fpga-xc7v-selectmap", }, // Virtex-7
+	{},
+};
+MODULE_DEVICE_TABLE(of, xlnx_selectmap_of_match);
+
+static struct platform_driver xilinx_selectmap_driver = {
+	.driver = {
+		.name = "xilinx-selectmap",
+		.of_match_table = xlnx_selectmap_of_match,
+	},
+	.probe  = xilinx_selectmap_probe,
+};
+
+module_platform_driver(xilinx_selectmap_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Charles Perry <charles.perry@savoirfairelinux.com>");
+MODULE_DESCRIPTION("Load Xilinx FPGA firmware over SelectMap");
--
2.43.0

Re: [PATCH v5 3/3] fpga: xilinx-selectmap: add new driver

[Xu Yilun]

On Wed, Mar 13, 2024 at 06:57:37PM -0400, Charles Perry wrote:
> Xilinx 7 series FPGA can be programmed using a parallel port named
> the SelectMAP interface in the datasheet. This interface is compatible
> with the i.MX6 EIM bus controller but other types of external memory
> mapped parallel bus might work.
> 
> xilinx-selectmap currently only supports the x8 mode where data is loaded
> at one byte per rising edge of the clock, with the MSb of each byte
> presented to the D0 pin.
> 
> Signed-off-by: Charles Perry <charles.perry@savoirfairelinux.com>
> ---
> Changes since v4: (from Yilun review)
>  * xilinx-core: select between prog/init and prog_b/init-b
> 
>  drivers/fpga/Kconfig            |  8 +++
>  drivers/fpga/Makefile           |  1 +
>  drivers/fpga/xilinx-core.c      | 29 +++++++++-
>  drivers/fpga/xilinx-selectmap.c | 97 +++++++++++++++++++++++++++++++++
>  4 files changed, 133 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/fpga/xilinx-selectmap.c
> 
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index d27a1ebf40838..37b35f58f0dfb 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -67,6 +67,14 @@ config FPGA_MGR_STRATIX10_SOC
>  config FPGA_MGR_XILINX_CORE
>  	tristate
> 
> +config FPGA_MGR_XILINX_SELECTMAP
> +	tristate "Xilinx Configuration over SelectMAP"
> +	depends on HAS_IOMEM
> +	select FPGA_MGR_XILINX_CORE
> +	help
> +	  FPGA manager driver support for Xilinx FPGA configuration
> +	  over SelectMAP interface.
> +
>  config FPGA_MGR_XILINX_SPI
>  	tristate "Xilinx Configuration over Slave Serial (SPI)"
>  	depends on SPI
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 7ec795b6a5a70..aeb89bb13517e 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -16,6 +16,7 @@ obj-$(CONFIG_FPGA_MGR_SOCFPGA_A10)	+= socfpga-a10.o
>  obj-$(CONFIG_FPGA_MGR_STRATIX10_SOC)	+= stratix10-soc.o
>  obj-$(CONFIG_FPGA_MGR_TS73XX)		+= ts73xx-fpga.o
>  obj-$(CONFIG_FPGA_MGR_XILINX_CORE)	+= xilinx-core.o
> +obj-$(CONFIG_FPGA_MGR_XILINX_SELECTMAP)	+= xilinx-selectmap.o
>  obj-$(CONFIG_FPGA_MGR_XILINX_SPI)	+= xilinx-spi.o
>  obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA)	+= zynq-fpga.o
>  obj-$(CONFIG_FPGA_MGR_ZYNQMP_FPGA)	+= zynqmp-fpga.o
> diff --git a/drivers/fpga/xilinx-core.c b/drivers/fpga/xilinx-core.c
> index a35c43382dd5f..ccdeb45eba4ee 100644
> --- a/drivers/fpga/xilinx-core.c
> +++ b/drivers/fpga/xilinx-core.c
> @@ -171,6 +171,29 @@ static int xilinx_core_write_complete(struct fpga_manager *mgr,
>  	return -ETIMEDOUT;
>  }
> 
> +/**
> + * xilinx_core_devm_gpiod_get - Obtain a resource-managed GPIO using a
> + *                              legacy consumer name fallback.
> + *
> + * @dev:           Device managing the GPIO
> + * @con_id:        Consumer id
> + * @legacy_con_id: Legacy consumer id
> + * @flags:         optional GPIO initialization flags
> + */

No need to have kernel doc comments for internal functions.

> +static inline struct gpio_desc *
> +xilinx_core_devm_gpiod_get(struct device *dev, const char *con_id,
> +			   const char *legacy_con_id, enum gpiod_flags flags)
> +{
> +	struct gpio_desc *desc;
> +
> +	desc = devm_gpiod_get(dev, con_id, flags);
> +	if (IS_ERR(desc) && PTR_ERR(desc) == -ENOENT &&
> +	    of_device_is_compatible(dev->of_node, "xlnx,fpga-slave-serial"))
> +		desc = devm_gpiod_get(dev, legacy_con_id, flags);
> +
> +	return desc;
> +}
> +
>  static const struct fpga_manager_ops xilinx_core_ops = {
>  	.state = xilinx_core_state,
>  	.write_init = xilinx_core_write_init,
> @@ -186,12 +209,14 @@ int xilinx_core_probe(struct xilinx_fpga_core *core)
>  		return -EINVAL;
> 
>  	/* PROGRAM_B is active low */
> -	core->prog_b = devm_gpiod_get(core->dev, "prog_b", GPIOD_OUT_LOW);
> +	core->prog_b = xilinx_core_devm_gpiod_get(core->dev, "prog", "prog_b",
> +						  GPIOD_OUT_LOW);
>  	if (IS_ERR(core->prog_b))
>  		return dev_err_probe(core->dev, PTR_ERR(core->prog_b),
>  				     "Failed to get PROGRAM_B gpio\n");
> 
> -	core->init_b = devm_gpiod_get_optional(core->dev, "init-b", GPIOD_IN);
> +	core->init_b = xilinx_core_devm_gpiod_get(core->dev, "init", "init-b",
> +						  GPIOD_IN);
>  	if (IS_ERR(core->init_b))
>  		return dev_err_probe(core->dev, PTR_ERR(core->init_b),
>  				     "Failed to get INIT_B gpio\n");

Please make a separate patch for the naming change. This give a chance
to explain why the change and how to correctly use the GPIO names.

> diff --git a/drivers/fpga/xilinx-selectmap.c b/drivers/fpga/xilinx-selectmap.c
> new file mode 100644
> index 0000000000000..b63f4623f8b2c
> --- /dev/null
> +++ b/drivers/fpga/xilinx-selectmap.c
> @@ -0,0 +1,97 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Xilinx Spartan6 and 7 Series SelectMAP interface driver
> + *
> + * (C) 2024 Charles Perry <charles.perry@savoirfairelinux.com>
> + *
> + * Manage Xilinx FPGA firmware loaded over the SelectMAP configuration
> + * interface.
> + */
> +
> +#include "xilinx-core.h"
> +
> +#include <linux/platform_device.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/of.h>
> +#include <linux/io.h>

alphabetical order please.

> +
> +struct xilinx_selectmap_conf {
> +	struct xilinx_fpga_core core;
> +	void __iomem *base;
> +};
> +
> +#define to_xilinx_selectmap_conf(obj) \
> +	container_of(obj, struct xilinx_selectmap_conf, core)
> +
> +static int xilinx_selectmap_write(struct xilinx_fpga_core *core,
> +				  const char *buf, size_t count)
> +{
> +	struct xilinx_selectmap_conf *conf = to_xilinx_selectmap_conf(core);
> +	u32 i;
> +
> +	for (i = 0; i < count; ++i)
> +		writeb(buf[i], conf->base);
> +
> +	return 0;
> +}
> +
> +static int xilinx_selectmap_probe(struct platform_device *pdev)
> +{
> +	struct xilinx_selectmap_conf *conf;
> +	struct resource *r;
> +	void __iomem *base;
> +	struct gpio_desc *csi_b;
> +	struct gpio_desc *rdwr_b;

One gpio_desc is enough, is it?  We don't use these gpio_desc anywhere
else.

BTW, reverse Xmas tree is not strictly required, but please do it when it is easy to follow.

> +
> +	conf = devm_kzalloc(&pdev->dev, sizeof(*conf), GFP_KERNEL);
> +	if (!conf)
> +		return -ENOMEM;
> +
> +	conf->core.dev = &pdev->dev;
> +	conf->core.write = xilinx_selectmap_write;
> +
> +	base = devm_platform_get_and_ioremap_resource(pdev, 0, &r);

I can't find where 'r' is used.

Thanks,
Yilun

> +	if (IS_ERR(base))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(base),
> +				     "ioremap error\n");
> +	conf->base = base;
> +
> +	/* CSI_B is active low */
> +	csi_b = devm_gpiod_get_optional(&pdev->dev, "csi", GPIOD_OUT_HIGH);
> +	if (IS_ERR(csi_b))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(csi_b),
> +				     "Failed to get CSI_B gpio\n");
> +
> +	/* RDWR_B is active low */
> +	rdwr_b = devm_gpiod_get_optional(&pdev->dev, "rdwr", GPIOD_OUT_HIGH);
> +	if (IS_ERR(rdwr_b))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(rdwr_b),
> +				     "Failed to get RDWR_B gpio\n");
> +
> +	return xilinx_core_probe(&conf->core);
> +}
> +
> +static const struct of_device_id xlnx_selectmap_of_match[] = {
> +	{ .compatible = "xlnx,fpga-xc7s-selectmap", }, // Spartan-7
> +	{ .compatible = "xlnx,fpga-xc7a-selectmap", }, // Artix-7
> +	{ .compatible = "xlnx,fpga-xc7k-selectmap", }, // Kintex-7
> +	{ .compatible = "xlnx,fpga-xc7v-selectmap", }, // Virtex-7
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, xlnx_selectmap_of_match);
> +
> +static struct platform_driver xilinx_selectmap_driver = {
> +	.driver = {
> +		.name = "xilinx-selectmap",
> +		.of_match_table = xlnx_selectmap_of_match,
> +	},
> +	.probe  = xilinx_selectmap_probe,
> +};
> +
> +module_platform_driver(xilinx_selectmap_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Charles Perry <charles.perry@savoirfairelinux.com>");
> +MODULE_DESCRIPTION("Load Xilinx FPGA firmware over SelectMap");
> --
> 2.43.0
> 

Re: [PATCH v5 3/3] fpga: xilinx-selectmap: add new driver

[Charles Perry]

On Mar 19, 2024, at 2:35 AM, Xu Yilun yilun.xu@linux.intel.com wrote:
> On Wed, Mar 13, 2024 at 06:57:37PM -0400, Charles Perry wrote:
>> Xilinx 7 series FPGA can be programmed using a parallel port named
>> the SelectMAP interface in the datasheet. This interface is compatible
>> with the i.MX6 EIM bus controller but other types of external memory
>> mapped parallel bus might work.
>> 
>> xilinx-selectmap currently only supports the x8 mode where data is loaded
>> at one byte per rising edge of the clock, with the MSb of each byte
>> presented to the D0 pin.
>> 
>> Signed-off-by: Charles Perry <charles.perry@savoirfairelinux.com>
>> ---
>> Changes since v4: (from Yilun review)
>>  * xilinx-core: select between prog/init and prog_b/init-b
>> 
>>  drivers/fpga/Kconfig            |  8 +++
>>  drivers/fpga/Makefile           |  1 +
>>  drivers/fpga/xilinx-core.c      | 29 +++++++++-
>>  drivers/fpga/xilinx-selectmap.c | 97 +++++++++++++++++++++++++++++++++
>>  4 files changed, 133 insertions(+), 2 deletions(-)
>>  create mode 100644 drivers/fpga/xilinx-selectmap.c
>> 
>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
>> index d27a1ebf40838..37b35f58f0dfb 100644
>> --- a/drivers/fpga/Kconfig
>> +++ b/drivers/fpga/Kconfig
>> @@ -67,6 +67,14 @@ config FPGA_MGR_STRATIX10_SOC
>>  config FPGA_MGR_XILINX_CORE
>>  	tristate
>> 
>> +config FPGA_MGR_XILINX_SELECTMAP
>> +	tristate "Xilinx Configuration over SelectMAP"
>> +	depends on HAS_IOMEM
>> +	select FPGA_MGR_XILINX_CORE
>> +	help
>> +	  FPGA manager driver support for Xilinx FPGA configuration
>> +	  over SelectMAP interface.
>> +
>>  config FPGA_MGR_XILINX_SPI
>>  	tristate "Xilinx Configuration over Slave Serial (SPI)"
>>  	depends on SPI
>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
>> index 7ec795b6a5a70..aeb89bb13517e 100644
>> --- a/drivers/fpga/Makefile
>> +++ b/drivers/fpga/Makefile
>> @@ -16,6 +16,7 @@ obj-$(CONFIG_FPGA_MGR_SOCFPGA_A10)	+= socfpga-a10.o
>>  obj-$(CONFIG_FPGA_MGR_STRATIX10_SOC)	+= stratix10-soc.o
>>  obj-$(CONFIG_FPGA_MGR_TS73XX)		+= ts73xx-fpga.o
>>  obj-$(CONFIG_FPGA_MGR_XILINX_CORE)	+= xilinx-core.o
>> +obj-$(CONFIG_FPGA_MGR_XILINX_SELECTMAP)	+= xilinx-selectmap.o
>>  obj-$(CONFIG_FPGA_MGR_XILINX_SPI)	+= xilinx-spi.o
>>  obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA)	+= zynq-fpga.o
>>  obj-$(CONFIG_FPGA_MGR_ZYNQMP_FPGA)	+= zynqmp-fpga.o
>> diff --git a/drivers/fpga/xilinx-core.c b/drivers/fpga/xilinx-core.c
>> index a35c43382dd5f..ccdeb45eba4ee 100644
>> --- a/drivers/fpga/xilinx-core.c
>> +++ b/drivers/fpga/xilinx-core.c
>> @@ -171,6 +171,29 @@ static int xilinx_core_write_complete(struct fpga_manager
>> *mgr,
>>  	return -ETIMEDOUT;
>>  }
>> 
>> +/**
>> + * xilinx_core_devm_gpiod_get - Obtain a resource-managed GPIO using a
>> + *                              legacy consumer name fallback.
>> + *
>> + * @dev:           Device managing the GPIO
>> + * @con_id:        Consumer id
>> + * @legacy_con_id: Legacy consumer id
>> + * @flags:         optional GPIO initialization flags
>> + */
> 
> No need to have kernel doc comments for internal functions.
> 

Ok, will remove.

>> +static inline struct gpio_desc *
>> +xilinx_core_devm_gpiod_get(struct device *dev, const char *con_id,
>> +			   const char *legacy_con_id, enum gpiod_flags flags)
>> +{
>> +	struct gpio_desc *desc;
>> +
>> +	desc = devm_gpiod_get(dev, con_id, flags);
>> +	if (IS_ERR(desc) && PTR_ERR(desc) == -ENOENT &&
>> +	    of_device_is_compatible(dev->of_node, "xlnx,fpga-slave-serial"))
>> +		desc = devm_gpiod_get(dev, legacy_con_id, flags);
>> +
>> +	return desc;
>> +}
>> +
>>  static const struct fpga_manager_ops xilinx_core_ops = {
>>  	.state = xilinx_core_state,
>>  	.write_init = xilinx_core_write_init,
>> @@ -186,12 +209,14 @@ int xilinx_core_probe(struct xilinx_fpga_core *core)
>>  		return -EINVAL;
>> 
>>  	/* PROGRAM_B is active low */
>> -	core->prog_b = devm_gpiod_get(core->dev, "prog_b", GPIOD_OUT_LOW);
>> +	core->prog_b = xilinx_core_devm_gpiod_get(core->dev, "prog", "prog_b",
>> +						  GPIOD_OUT_LOW);
>>  	if (IS_ERR(core->prog_b))
>>  		return dev_err_probe(core->dev, PTR_ERR(core->prog_b),
>>  				     "Failed to get PROGRAM_B gpio\n");
>> 
>> -	core->init_b = devm_gpiod_get_optional(core->dev, "init-b", GPIOD_IN);
>> +	core->init_b = xilinx_core_devm_gpiod_get(core->dev, "init", "init-b",
>> +						  GPIOD_IN);
>>  	if (IS_ERR(core->init_b))
>>  		return dev_err_probe(core->dev, PTR_ERR(core->init_b),
>>  				     "Failed to get INIT_B gpio\n");
> 
> Please make a separate patch for the naming change. This give a chance
> to explain why the change and how to correctly use the GPIO names.
> 

Ok

>> diff --git a/drivers/fpga/xilinx-selectmap.c b/drivers/fpga/xilinx-selectmap.c
>> new file mode 100644
>> index 0000000000000..b63f4623f8b2c
>> --- /dev/null
>> +++ b/drivers/fpga/xilinx-selectmap.c
>> @@ -0,0 +1,97 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Xilinx Spartan6 and 7 Series SelectMAP interface driver
>> + *
>> + * (C) 2024 Charles Perry <charles.perry@savoirfairelinux.com>
>> + *
>> + * Manage Xilinx FPGA firmware loaded over the SelectMAP configuration
>> + * interface.
>> + */
>> +
>> +#include "xilinx-core.h"
>> +
>> +#include <linux/platform_device.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/module.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/of.h>
>> +#include <linux/io.h>
> 
> alphabetical order please.
> 

Ok

>> +
>> +struct xilinx_selectmap_conf {
>> +	struct xilinx_fpga_core core;
>> +	void __iomem *base;
>> +};
>> +
>> +#define to_xilinx_selectmap_conf(obj) \
>> +	container_of(obj, struct xilinx_selectmap_conf, core)
>> +
>> +static int xilinx_selectmap_write(struct xilinx_fpga_core *core,
>> +				  const char *buf, size_t count)
>> +{
>> +	struct xilinx_selectmap_conf *conf = to_xilinx_selectmap_conf(core);
>> +	u32 i;
>> +
>> +	for (i = 0; i < count; ++i)
>> +		writeb(buf[i], conf->base);
>> +
>> +	return 0;
>> +}
>> +
>> +static int xilinx_selectmap_probe(struct platform_device *pdev)
>> +{
>> +	struct xilinx_selectmap_conf *conf;
>> +	struct resource *r;
>> +	void __iomem *base;
>> +	struct gpio_desc *csi_b;
>> +	struct gpio_desc *rdwr_b;
> 
> One gpio_desc is enough, is it?  We don't use these gpio_desc anywhere
> else.
> 

Ok

> BTW, reverse Xmas tree is not strictly required, but please do it when it is
> easy to follow.
>

Ok
 
>> +
>> +	conf = devm_kzalloc(&pdev->dev, sizeof(*conf), GFP_KERNEL);
>> +	if (!conf)
>> +		return -ENOMEM;
>> +
>> +	conf->core.dev = &pdev->dev;
>> +	conf->core.write = xilinx_selectmap_write;
>> +
>> +	base = devm_platform_get_and_ioremap_resource(pdev, 0, &r);
> 
> I can't find where 'r' is used.
> 
> Thanks,
> Yilun
> 

I'm guilty of keeping things around in case I need them in the future
here! Ok, will use NULL.

Regards,
Charles

>> +	if (IS_ERR(base))
>> +		return dev_err_probe(&pdev->dev, PTR_ERR(base),
>> +				     "ioremap error\n");
>> +	conf->base = base;
>> +
>> +	/* CSI_B is active low */
>> +	csi_b = devm_gpiod_get_optional(&pdev->dev, "csi", GPIOD_OUT_HIGH);
>> +	if (IS_ERR(csi_b))
>> +		return dev_err_probe(&pdev->dev, PTR_ERR(csi_b),
>> +				     "Failed to get CSI_B gpio\n");
>> +
>> +	/* RDWR_B is active low */
>> +	rdwr_b = devm_gpiod_get_optional(&pdev->dev, "rdwr", GPIOD_OUT_HIGH);
>> +	if (IS_ERR(rdwr_b))
>> +		return dev_err_probe(&pdev->dev, PTR_ERR(rdwr_b),
>> +				     "Failed to get RDWR_B gpio\n");
>> +
>> +	return xilinx_core_probe(&conf->core);
>> +}
>> +
>> +static const struct of_device_id xlnx_selectmap_of_match[] = {
>> +	{ .compatible = "xlnx,fpga-xc7s-selectmap", }, // Spartan-7
>> +	{ .compatible = "xlnx,fpga-xc7a-selectmap", }, // Artix-7
>> +	{ .compatible = "xlnx,fpga-xc7k-selectmap", }, // Kintex-7
>> +	{ .compatible = "xlnx,fpga-xc7v-selectmap", }, // Virtex-7
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, xlnx_selectmap_of_match);
>> +
>> +static struct platform_driver xilinx_selectmap_driver = {
>> +	.driver = {
>> +		.name = "xilinx-selectmap",
>> +		.of_match_table = xlnx_selectmap_of_match,
>> +	},
>> +	.probe  = xilinx_selectmap_probe,
>> +};
>> +
>> +module_platform_driver(xilinx_selectmap_driver);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("Charles Perry <charles.perry@savoirfairelinux.com>");
>> +MODULE_DESCRIPTION("Load Xilinx FPGA firmware over SelectMap");
>> --
>> 2.43.0