* [PATCH v4 0/3] fpga: xilinx-selectmap: add new driver
@ 2024-02-21 19:50 Charles Perry
2024-02-21 19:50 ` [PATCH v4 1/3] fpga: xilinx-spi: extract a common driver core Charles Perry
` (2 more replies)
0 siblings, 3 replies; 18+ messages in thread
From: Charles Perry @ 2024-02-21 19:50 UTC (permalink / raw)
To: mdf
Cc: avandiver, bcody, Charles Perry, Wu Hao, Xu Yilun, Tom Rix,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Michal Simek,
linux-fpga, devicetree, linux-kernel, linux-arm-kernel
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 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 | 208 +++++++++++++++++
drivers/fpga/xilinx-core.h | 28 +++
drivers/fpga/xilinx-selectmap.c | 97 ++++++++
drivers/fpga/xilinx-spi.c | 212 ++----------------
7 files changed, 446 insertions(+), 199 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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v4 1/3] fpga: xilinx-spi: extract a common driver core
2024-02-21 19:50 [PATCH v4 0/3] fpga: xilinx-selectmap: add new driver Charles Perry
@ 2024-02-21 19:50 ` Charles Perry
2024-02-26 9:19 ` Xu Yilun
2024-03-01 5:29 ` kernel test robot
2024-02-21 19:50 ` [PATCH v4 2/3] dt-bindings: fpga: xlnx,fpga-selectmap: add DT schema Charles Perry
2024-02-21 19:50 ` [PATCH v4 3/3] fpga: xilinx-selectmap: add new driver Charles Perry
2 siblings, 2 replies; 18+ messages in thread
From: Charles Perry @ 2024-02-21 19:50 UTC (permalink / raw)
To: mdf
Cc: avandiver, bcody, Charles Perry, Wu Hao, Xu Yilun, Tom Rix,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Michal Simek,
linux-fpga, devicetree, linux-kernel, linux-arm-kernel
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 single operation:
* ->write(const char* buf, size_t count): write to the device
As well as a struct device* for resource management.
Signed-off-by: Charles Perry <charles.perry@savoirfairelinux.com>
---
drivers/fpga/Kconfig | 4 +
drivers/fpga/Makefile | 1 +
drivers/fpga/xilinx-core.c | 208 ++++++++++++++++++++++++++++++++++++
drivers/fpga/xilinx-core.h | 28 +++++
drivers/fpga/xilinx-spi.c | 212 +++----------------------------------
5 files changed, 254 insertions(+), 199 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..597e8b7a530b7
--- /dev/null
+++ b/drivers/fpga/xilinx-core.c
@@ -0,0 +1,208 @@
+// 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, 1);
+ 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);
+}
diff --git a/drivers/fpga/xilinx-core.h b/drivers/fpga/xilinx-core.h
new file mode 100644
index 0000000000000..bea190287b403
--- /dev/null
+++ b/drivers/fpga/xilinx-core.h
@@ -0,0 +1,28 @@
+/* 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, must be set by the driver
+ * @write: write callback of the driver, must be set by the driver
+ * @prog_b: PROGRAM_B gpio, handled by the core manager
+ * @init_b: INIT_B gpio, handled by the core manager
+ * @done: DONE gpio, handled by the core manager
+ */
+struct xilinx_fpga_core {
+ struct device *dev;
+ int (*write)(struct xilinx_fpga_core *core, const char *buf,
+ size_t count);
+ 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..12f401502a53a 100644
--- a/drivers/fpga/xilinx-spi.c
+++ b/drivers/fpga/xilinx-spi.c
@@ -10,127 +10,24 @@
* 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 xilinx_fpga_core core;
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);
+#define to_xilinx_spi_conf(obj) container_of(obj, struct xilinx_spi_conf, core)
- 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 xilinx_spi_conf *conf = to_xilinx_spi_conf(core);
const char *fw_data = buf;
const char *fw_data_end = fw_data + count;
@@ -143,7 +40,7 @@ static int xilinx_spi_write(struct fpga_manager *mgr, const char *buf,
ret = spi_write(conf->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 +50,26 @@ 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;
conf = devm_kzalloc(&spi->dev, sizeof(*conf), GFP_KERNEL);
if (!conf)
return -ENOMEM;
+ conf->core.dev = &spi->dev;
+ conf->core.write = xilinx_spi_write;
conf->spi = spi;
- /* 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->core);
}
#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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 2/3] dt-bindings: fpga: xlnx,fpga-selectmap: add DT schema
2024-02-21 19:50 [PATCH v4 0/3] fpga: xilinx-selectmap: add new driver Charles Perry
2024-02-21 19:50 ` [PATCH v4 1/3] fpga: xilinx-spi: extract a common driver core Charles Perry
@ 2024-02-21 19:50 ` Charles Perry
2024-02-27 10:10 ` Krzysztof Kozlowski
2024-02-21 19:50 ` [PATCH v4 3/3] fpga: xilinx-selectmap: add new driver Charles Perry
2 siblings, 1 reply; 18+ messages in thread
From: Charles Perry @ 2024-02-21 19:50 UTC (permalink / raw)
To: mdf
Cc: avandiver, bcody, Charles Perry, Wu Hao, Xu Yilun, Tom Rix,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Michal Simek,
linux-fpga, devicetree, linux-kernel, linux-arm-kernel
Document the SelectMAP interface of Xilinx 7 series FPGA.
Signed-off-by: Charles Perry <charles.perry@savoirfairelinux.com>
---
.../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..08a5e92781657
--- /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_b-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-b-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_b-gpios
+ - done-gpios
+ - init-b-gpios
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+ fpga-mgr@8000000 {
+ compatible = "xlnx,fpga-xc7s-selectmap";
+ reg = <0x8000000 0x4>;
+ prog_b-gpios = <&gpio5 5 GPIO_ACTIVE_LOW>;
+ init-b-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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v4 3/3] fpga: xilinx-selectmap: add new driver
2024-02-21 19:50 [PATCH v4 0/3] fpga: xilinx-selectmap: add new driver Charles Perry
2024-02-21 19:50 ` [PATCH v4 1/3] fpga: xilinx-spi: extract a common driver core Charles Perry
2024-02-21 19:50 ` [PATCH v4 2/3] dt-bindings: fpga: xlnx,fpga-selectmap: add DT schema Charles Perry
@ 2024-02-21 19:50 ` Charles Perry
2024-02-26 9:50 ` Xu Yilun
2 siblings, 1 reply; 18+ messages in thread
From: Charles Perry @ 2024-02-21 19:50 UTC (permalink / raw)
To: mdf
Cc: avandiver, bcody, Charles Perry, Wu Hao, Xu Yilun, Tom Rix,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Michal Simek,
linux-fpga, devicetree, linux-kernel, linux-arm-kernel
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>
---
drivers/fpga/Kconfig | 8 +++
drivers/fpga/Makefile | 1 +
drivers/fpga/xilinx-selectmap.c | 97 +++++++++++++++++++++++++++++++++
3 files changed, 106 insertions(+)
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-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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/3] fpga: xilinx-spi: extract a common driver core
2024-02-21 19:50 ` [PATCH v4 1/3] fpga: xilinx-spi: extract a common driver core Charles Perry
@ 2024-02-26 9:19 ` Xu Yilun
2024-03-03 17:20 ` Charles Perry
2024-03-01 5:29 ` kernel test robot
1 sibling, 1 reply; 18+ messages in thread
From: Xu Yilun @ 2024-02-26 9:19 UTC (permalink / raw)
To: Charles Perry
Cc: mdf, avandiver, bcody, Wu Hao, Xu Yilun, Tom Rix, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Michal Simek, linux-fpga,
devicetree, linux-kernel, linux-arm-kernel
On Wed, Feb 21, 2024 at 02:50:47PM -0500, Charles Perry wrote:
> 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 single operation:
>
> * ->write(const char* buf, size_t count): write to the device
>
> As well as a struct device* for resource management.
>
> Signed-off-by: Charles Perry <charles.perry@savoirfairelinux.com>
> ---
> drivers/fpga/Kconfig | 4 +
> drivers/fpga/Makefile | 1 +
> drivers/fpga/xilinx-core.c | 208 ++++++++++++++++++++++++++++++++++++
> drivers/fpga/xilinx-core.h | 28 +++++
> drivers/fpga/xilinx-spi.c | 212 +++----------------------------------
> 5 files changed, 254 insertions(+), 199 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..597e8b7a530b7
> --- /dev/null
> +++ b/drivers/fpga/xilinx-core.c
> @@ -0,0 +1,208 @@
> +// 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, 1);
^
sizeof(padding), Use as less immediate numbers as possible.
> + 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() is needed? Have you ever built them as modules?
Try to follow Documentation/process/submit-checklist.rst as much as you can.
And MODULE_LICENSE() & MODULE_DESCRIPTION() please.
> diff --git a/drivers/fpga/xilinx-core.h b/drivers/fpga/xilinx-core.h
> new file mode 100644
> index 0000000000000..bea190287b403
> --- /dev/null
> +++ b/drivers/fpga/xilinx-core.h
> @@ -0,0 +1,28 @@
> +/* 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, must be set by the driver
> + * @write: write callback of the driver, must be set by the driver
> + * @prog_b: PROGRAM_B gpio, handled by the core manager
> + * @init_b: INIT_B gpio, handled by the core manager
> + * @done: DONE gpio, handled by the core manager
Internal fields are not listed in the generated output documentation.
> + */
> +struct xilinx_fpga_core {
> + struct device *dev;
> + int (*write)(struct xilinx_fpga_core *core, const char *buf,
> + size_t count);
> + struct gpio_desc *prog_b;
> + struct gpio_desc *init_b;
> + struct gpio_desc *done;
Use private tag here.
Please follow Documentation/doc-guide/kernel-doc.rst
> +};
> +
> +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..12f401502a53a 100644
> --- a/drivers/fpga/xilinx-spi.c
> +++ b/drivers/fpga/xilinx-spi.c
> @@ -10,127 +10,24 @@
> * 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 xilinx_fpga_core core;
> 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);
> +#define to_xilinx_spi_conf(obj) container_of(obj, struct xilinx_spi_conf, core)
Use to_spi_device(), then no need the MACRO or struct xilinx_spi_conf
Thanks,
Yilun
>
> - 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 xilinx_spi_conf *conf = to_xilinx_spi_conf(core);
> const char *fw_data = buf;
> const char *fw_data_end = fw_data + count;
>
> @@ -143,7 +40,7 @@ static int xilinx_spi_write(struct fpga_manager *mgr, const char *buf,
>
> ret = spi_write(conf->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 +50,26 @@ 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;
>
> conf = devm_kzalloc(&spi->dev, sizeof(*conf), GFP_KERNEL);
> if (!conf)
> return -ENOMEM;
>
> + conf->core.dev = &spi->dev;
> + conf->core.write = xilinx_spi_write;
> conf->spi = spi;
>
> - /* 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->core);
> }
>
> #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
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 3/3] fpga: xilinx-selectmap: add new driver
2024-02-21 19:50 ` [PATCH v4 3/3] fpga: xilinx-selectmap: add new driver Charles Perry
@ 2024-02-26 9:50 ` Xu Yilun
2024-03-03 17:22 ` Charles Perry
0 siblings, 1 reply; 18+ messages in thread
From: Xu Yilun @ 2024-02-26 9:50 UTC (permalink / raw)
To: Charles Perry
Cc: mdf, avandiver, bcody, Wu Hao, Xu Yilun, Tom Rix, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Michal Simek, linux-fpga,
devicetree, linux-kernel, linux-arm-kernel
On Wed, Feb 21, 2024 at 02:50:49PM -0500, 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>
> ---
> drivers/fpga/Kconfig | 8 +++
> drivers/fpga/Makefile | 1 +
> drivers/fpga/xilinx-selectmap.c | 97 +++++++++++++++++++++++++++++++++
> 3 files changed, 106 insertions(+)
> 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-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);
Does the driver have to be used with OF or not?
If yes, please specify the reason and enforce in Kconfig.
If no, please ensure it decently compiles without CONFIG_OF.
Thanks,
Yilun
> +
> +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
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 2/3] dt-bindings: fpga: xlnx,fpga-selectmap: add DT schema
2024-02-21 19:50 ` [PATCH v4 2/3] dt-bindings: fpga: xlnx,fpga-selectmap: add DT schema Charles Perry
@ 2024-02-27 10:10 ` Krzysztof Kozlowski
2024-03-03 17:21 ` Charles Perry
0 siblings, 1 reply; 18+ messages in thread
From: Krzysztof Kozlowski @ 2024-02-27 10:10 UTC (permalink / raw)
To: Charles Perry, mdf
Cc: avandiver, bcody, Wu Hao, Xu Yilun, Tom Rix, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Michal Simek, linux-fpga,
devicetree, linux-kernel, linux-arm-kernel
On 21/02/2024 20:50, Charles Perry wrote:
> Document the SelectMAP interface of Xilinx 7 series FPGA.
>
> Signed-off-by: Charles Perry <charles.perry@savoirfairelinux.com>
> ---
> .../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..08a5e92781657
> --- /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_b-gpios:
I commented on this and still see underscore. Nothing in commit msg
explains why this should have underscore. Changelog is also vague -
describes that you brought back underscores, instead of explaining why
you did it.
So the same comments as usual:
No underscores in names.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/3] fpga: xilinx-spi: extract a common driver core
2024-02-21 19:50 ` [PATCH v4 1/3] fpga: xilinx-spi: extract a common driver core Charles Perry
2024-02-26 9:19 ` Xu Yilun
@ 2024-03-01 5:29 ` kernel test robot
1 sibling, 0 replies; 18+ messages in thread
From: kernel test robot @ 2024-03-01 5:29 UTC (permalink / raw)
To: Charles Perry, mdf
Cc: oe-kbuild-all, avandiver, bcody, Charles Perry, Wu Hao, Xu Yilun,
Tom Rix, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Michal Simek, linux-fpga, devicetree, linux-kernel,
linux-arm-kernel
Hi Charles,
kernel test robot noticed the following build warnings:
[auto build test WARNING on robh/for-next]
[also build test WARNING on linus/master xilinx-xlnx/master v6.8-rc6 next-20240229]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Charles-Perry/fpga-xilinx-spi-extract-a-common-driver-core/20240222-040453
base: https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link: https://lore.kernel.org/r/20240221195058.1281973-2-charles.perry%40savoirfairelinux.com
patch subject: [PATCH v4 1/3] fpga: xilinx-spi: extract a common driver core
config: hexagon-randconfig-r063-20240229 (https://download.01.org/0day-ci/archive/20240301/202403011334.uEOiA7Q0-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project 325f51237252e6dab8e4e1ea1fa7acbb4faee1cd)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240301/202403011334.uEOiA7Q0-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202403011334.uEOiA7Q0-lkp@intel.com/
All warnings (new ones prefixed by >>, old ones prefixed by <<):
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/base/regmap/regmap-spmi.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/block/brd.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/block/ublk_drv.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/scsi/scsi_common.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/nvme/host/nvme-core.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/nvme/host/nvme-fabrics.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/nvme/target/nvmet.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/nvme/target/nvme-loop.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/nvme/target/nvmet-fc.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/spi/spi-fsl-lib.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/firewire/firewire-uapi-test.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/phy/phy-am335x-control.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/phy/phy-am335x.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/misc/ezusb.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/chipidea/ci_hdrc_msm.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/libcomposite.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/usb_f_acm.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/u_serial.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/usb_f_serial.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/usb_f_obex.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/u_ether.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/usb_f_ncm.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/usb_f_ecm.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/usb_f_ecm_subset.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/usb_f_rndis.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/usb_f_mass_storage.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/usb_f_fs.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/usb_f_hid.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/usb/gadget/function/usb_f_tcm.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/input/touchscreen/cyttsp_i2c_common.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/input/tests/input_test.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/i2c/busses/i2c-qup.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/media/rc/rc-core.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/watchdog/twl4030_wdt.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/watchdog/ts4800_wdt.o
WARNING: modpost: drivers/mmc/host/davinci_mmc: section mismatch in reference: davinci_mmcsd_driver+0x8 (section: .data) -> davinci_mmcsd_remove (section: .exit.text)
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mmc/host/of_mmc_spi.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mmc/host/tmio_mmc_core.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mmc/host/renesas_sdhi_core.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mmc/core/mmc_core.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mmc/core/pwrseq_sd8787.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mmc/core/pwrseq_emmc.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mmc/core/sdio_uart.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/leds/blink/leds-bcm63138.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-aureal.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-betopff.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-cherry.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-dr.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-emsff.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-elecom.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-elo.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-gyration.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-lcpower.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-letsketch.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-logitech.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-lg-g15.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-logitech-dj.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-logitech-hidpp.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-maltron.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-ntrig.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-razer.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-saitek.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-samsung.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-semitek.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-sony.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-steelseries.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-sunplus.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-tmff.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-twinhan.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hid/hid-uclogic.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/staging/greybus/gb-power-supply.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/staging/greybus/gb-pwm.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/staging/greybus/gb-sdio.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/staging/greybus/gb-spi.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/staging/fbtft/fbtft.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/devfreq/governor_performance.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/devfreq/governor_userspace.o
WARNING: modpost: drivers/memory/emif: section mismatch in reference: emif_driver+0x8 (section: .data) -> emif_remove (section: .exit.text)
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/nvmem/nvmem_brcm_nvram.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/nvmem/nvmem_u-boot-env.o
ERROR: modpost: missing MODULE_LICENSE() in drivers/fpga/xilinx-core.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fpga/xilinx-core.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/parport/parport.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mtd/chips/cfi_util.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mtd/chips/cfi_cmdset_0020.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/mtd/maps/map_funcs.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/spmi/hisi-spmi-controller.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/spmi/spmi-pmic-arb.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/uio/uio.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/uio/uio_pruss.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hwmon/corsair-cpro.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/hwmon/mr75203.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/iio/adc/ingenic-adc.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/iio/adc/xilinx-ams.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/iio/buffer/kfifo_buf.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fsi/fsi-core.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fsi/fsi-master-hub.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fsi/fsi-master-gpio.o
WARNING: modpost: missing MODULE_DESCRIPTION() in drivers/fsi/fsi-scom.o
WARNING: modpost: missing MODULE_DESCRIPTION() in net/sched/em_text.o
>> WARNING: modpost: "xilinx_core_probe" [drivers/fpga/xilinx-spi.ko] undefined!
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 1/3] fpga: xilinx-spi: extract a common driver core
2024-02-26 9:19 ` Xu Yilun
@ 2024-03-03 17:20 ` Charles Perry
0 siblings, 0 replies; 18+ messages in thread
From: Charles Perry @ 2024-03-03 17:20 UTC (permalink / raw)
To: Xu Yilun
Cc: mdf, Allen VANDIVER, Brian CODY, hao wu, yilun xu, Tom Rix,
Rob Herring, krzysztof kozlowski+dt, Conor Dooley, Michal Simek,
linux-fpga, devicetree, linux-kernel, linux-arm-kernel
On Feb 26, 2024, at 2:19 AM, Xu Yilun yilun.xu@linux.intel.com wrote:
> On Wed, Feb 21, 2024 at 02:50:47PM -0500, Charles Perry wrote:
>> 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 single operation:
>>
>> * ->write(const char* buf, size_t count): write to the device
>>
>> As well as a struct device* for resource management.
>>
>> Signed-off-by: Charles Perry <charles.perry@savoirfairelinux.com>
>> ---
>> drivers/fpga/Kconfig | 4 +
>> drivers/fpga/Makefile | 1 +
>> drivers/fpga/xilinx-core.c | 208 ++++++++++++++++++++++++++++++++++++
>> drivers/fpga/xilinx-core.h | 28 +++++
>> drivers/fpga/xilinx-spi.c | 212 +++----------------------------------
>> 5 files changed, 254 insertions(+), 199 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..597e8b7a530b7
>> --- /dev/null
>> +++ b/drivers/fpga/xilinx-core.c
>> @@ -0,0 +1,208 @@
>> +// 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, 1);
> ^
>
> sizeof(padding), Use as less immediate numbers as possible.
>
Ok
>> + 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() is needed? Have you ever built them as modules?
> Try to follow Documentation/process/submit-checklist.rst as much as you can.
>
>
> And MODULE_LICENSE() & MODULE_DESCRIPTION() please.
>
Yes, there are some missing symbol info when building as a module. Will fix.
>> diff --git a/drivers/fpga/xilinx-core.h b/drivers/fpga/xilinx-core.h
>> new file mode 100644
>> index 0000000000000..bea190287b403
>> --- /dev/null
>> +++ b/drivers/fpga/xilinx-core.h
>> @@ -0,0 +1,28 @@
>> +/* 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, must be set by the driver
>> + * @write: write callback of the driver, must be set by the driver
>> + * @prog_b: PROGRAM_B gpio, handled by the core manager
>> + * @init_b: INIT_B gpio, handled by the core manager
>> + * @done: DONE gpio, handled by the core manager
>
> Internal fields are not listed in the generated output documentation.
>
Ok, will remove.
>> + */
>> +struct xilinx_fpga_core {
>> + struct device *dev;
>> + int (*write)(struct xilinx_fpga_core *core, const char *buf,
>> + size_t count);
>> + struct gpio_desc *prog_b;
>> + struct gpio_desc *init_b;
>> + struct gpio_desc *done;
>
> Use private tag here.
>
> Please follow Documentation/doc-guide/kernel-doc.rst
>
Didn't knew about that, thank you!
>> +};
>> +
>> +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..12f401502a53a 100644
>> --- a/drivers/fpga/xilinx-spi.c
>> +++ b/drivers/fpga/xilinx-spi.c
>> @@ -10,127 +10,24 @@
>> * 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 xilinx_fpga_core core;
>> 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);
>> +#define to_xilinx_spi_conf(obj) container_of(obj, struct xilinx_spi_conf, core)
>
> Use to_spi_device(), then no need the MACRO or struct xilinx_spi_conf
>
> Thanks,
> Yilun
>
Ok, good idea.
Thank you for the review, I'll send a v5 in ~2 weeks as I don't have access to my hw
right now.
Regards,
Charles
>>
>> - 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 xilinx_spi_conf *conf = to_xilinx_spi_conf(core);
>> const char *fw_data = buf;
>> const char *fw_data_end = fw_data + count;
>>
>> @@ -143,7 +40,7 @@ static int xilinx_spi_write(struct fpga_manager *mgr, const
>> char *buf,
>>
>> ret = spi_write(conf->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 +50,26 @@ 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;
>>
>> conf = devm_kzalloc(&spi->dev, sizeof(*conf), GFP_KERNEL);
>> if (!conf)
>> return -ENOMEM;
>>
>> + conf->core.dev = &spi->dev;
>> + conf->core.write = xilinx_spi_write;
>> conf->spi = spi;
>>
>> - /* 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->core);
>> }
>>
>> #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
>>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 2/3] dt-bindings: fpga: xlnx,fpga-selectmap: add DT schema
2024-02-27 10:10 ` Krzysztof Kozlowski
@ 2024-03-03 17:21 ` Charles Perry
2024-03-04 7:30 ` Krzysztof Kozlowski
0 siblings, 1 reply; 18+ messages in thread
From: Charles Perry @ 2024-03-03 17:21 UTC (permalink / raw)
To: Krzysztof Kozlowski, Rob Herring, yilun xu
Cc: mdf, Allen VANDIVER, Brian CODY, hao wu, Tom Rix,
krzysztof kozlowski+dt, Conor Dooley, Michal Simek, linux-fpga,
devicetree, linux-kernel, linux-arm-kernel
On Feb 27, 2024, at 3:10 AM, Krzysztof Kozlowski krzysztof.kozlowski@linaro.org wrote:
> On 21/02/2024 20:50, Charles Perry wrote:
>> Document the SelectMAP interface of Xilinx 7 series FPGA.
>>
>> Signed-off-by: Charles Perry <charles.perry@savoirfairelinux.com>
>> ---
>> .../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..08a5e92781657
>> --- /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_b-gpios:
>
> I commented on this and still see underscore. Nothing in commit msg
> explains why this should have underscore. Changelog is also vague -
> describes that you brought back underscores, instead of explaining why
> you did it.
>
> So the same comments as usual:
>
> No underscores in names.
>
> Best regards,
> Krzysztof
Hello Krzysztof,
Yes, I've gone full circle on that issue. Here's what I tried so far:
1) Reuse the same gpio names: Duplicates errors of the past, Krzysztof
doesn't like it.
2) Different gpio names for new driver only: Makes the driver code
overly complicated, Yilun doesn't like it.
3) Change gpio names for both drivers, deprecate the old names: Makes
the DT binding and the driver code overly complicated, Rob doesn't
like it.
I think that while the driver code shouldn't be the driving force for
the DT spec, it can be a good indication that the spec is unpractical to
implement.
In this case, there are two interfaces on a chip that uses the same GPIO
protocol, it would only make sense that they use the same names, this
discards solution #2.
That leaves us with #1 or #3, which is to ask if the added complexity to
the driver code and DT binding is worth it for a gain in naming
convention.
There might also be another solution that I haven't seen.
Regards,
Charles
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 3/3] fpga: xilinx-selectmap: add new driver
2024-02-26 9:50 ` Xu Yilun
@ 2024-03-03 17:22 ` Charles Perry
2024-03-13 22:47 ` Charles Perry
0 siblings, 1 reply; 18+ messages in thread
From: Charles Perry @ 2024-03-03 17:22 UTC (permalink / raw)
To: Xu Yilun
Cc: mdf, Allen VANDIVER, Brian CODY, hao wu, yilun xu, Tom Rix,
Rob Herring, krzysztof kozlowski+dt, Conor Dooley, Michal Simek,
linux-fpga, devicetree, linux-kernel, linux-arm-kernel
On Feb 26, 2024, at 2:50 AM, Xu Yilun yilun.xu@linux.intel.com wrote:
> On Wed, Feb 21, 2024 at 02:50:49PM -0500, 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>
>> ---
>> drivers/fpga/Kconfig | 8 +++
>> drivers/fpga/Makefile | 1 +
>> drivers/fpga/xilinx-selectmap.c | 97 +++++++++++++++++++++++++++++++++
>> 3 files changed, 106 insertions(+)
>> 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-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);
>
> Does the driver have to be used with OF or not?
>
> If yes, please specify the reason and enforce in Kconfig.
> If no, please ensure it decently compiles without CONFIG_OF.
>
> Thanks,
> Yilun
>
No, it doesn't need OF explicitly as it only needs a few GPIO and a
memory mapped IO region. It would be possible to get this info from
platform data.
I'll fix the compilation without CONFIG_OF.
Regards,
Charles
>> +
>> +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
>>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 2/3] dt-bindings: fpga: xlnx,fpga-selectmap: add DT schema
2024-03-03 17:21 ` Charles Perry
@ 2024-03-04 7:30 ` Krzysztof Kozlowski
2024-03-04 7:31 ` Krzysztof Kozlowski
0 siblings, 1 reply; 18+ messages in thread
From: Krzysztof Kozlowski @ 2024-03-04 7:30 UTC (permalink / raw)
To: Charles Perry, Rob Herring, yilun xu
Cc: mdf, Allen VANDIVER, Brian CODY, hao wu, Tom Rix,
krzysztof kozlowski+dt, Conor Dooley, Michal Simek, linux-fpga,
devicetree, linux-kernel, linux-arm-kernel
On 03/03/2024 18:21, Charles Perry wrote:
> On Feb 27, 2024, at 3:10 AM, Krzysztof Kozlowski krzysztof.kozlowski@linaro.org wrote:
>
>> On 21/02/2024 20:50, Charles Perry wrote:
>>> Document the SelectMAP interface of Xilinx 7 series FPGA.
>>>
>>> Signed-off-by: Charles Perry <charles.perry@savoirfairelinux.com>
>>> ---
>>> .../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..08a5e92781657
>>> --- /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_b-gpios:
>>
>> I commented on this and still see underscore. Nothing in commit msg
>> explains why this should have underscore. Changelog is also vague -
>> describes that you brought back underscores, instead of explaining why
>> you did it.
>>
>> So the same comments as usual:
>>
>> No underscores in names.
>>
>> Best regards,
>> Krzysztof
>
> Hello Krzysztof,
>
> Yes, I've gone full circle on that issue. Here's what I tried so far:
And what part of the commit description allows me to understand this?
>
> 1) Reuse the same gpio names: Duplicates errors of the past, Krzysztof
> doesn't like it.
> 2) Different gpio names for new driver only: Makes the driver code
> overly complicated, Yilun doesn't like it.
That's a new driver, right? So what is complicated here? You have new
code and you take prog-b or prog_b?
> 3) Change gpio names for both drivers, deprecate the old names: Makes
> the DT binding and the driver code overly complicated, Rob doesn't
> like it.
I don't think I proposed changing existing bindings.
>
> I think that while the driver code shouldn't be the driving force for
> the DT spec, it can be a good indication that the spec is unpractical to
> implement.
What is impractical in implementing this? You just pass either A or B to
function requesting GPIO. Just choose proper name.
>
> In this case, there are two interfaces on a chip that uses the same GPIO
> protocol, it would only make sense that they use the same names, this
> discards solution #2.
I don't understand this. You have devm_gpiod_get() in your new code. Why
is it difficult to use different name?
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 2/3] dt-bindings: fpga: xlnx,fpga-selectmap: add DT schema
2024-03-04 7:30 ` Krzysztof Kozlowski
@ 2024-03-04 7:31 ` Krzysztof Kozlowski
2024-03-05 2:27 ` Charles Perry
0 siblings, 1 reply; 18+ messages in thread
From: Krzysztof Kozlowski @ 2024-03-04 7:31 UTC (permalink / raw)
To: Charles Perry, Rob Herring, yilun xu
Cc: mdf, Allen VANDIVER, Brian CODY, hao wu, Tom Rix,
krzysztof kozlowski+dt, Conor Dooley, Michal Simek, linux-fpga,
devicetree, linux-kernel, linux-arm-kernel
On 04/03/2024 08:30, Krzysztof Kozlowski wrote:
> On 03/03/2024 18:21, Charles Perry wrote:
>> On Feb 27, 2024, at 3:10 AM, Krzysztof Kozlowski krzysztof.kozlowski@linaro.org wrote:
>>
>>> On 21/02/2024 20:50, Charles Perry wrote:
>>>> Document the SelectMAP interface of Xilinx 7 series FPGA.
>>>>
>>>> Signed-off-by: Charles Perry <charles.perry@savoirfairelinux.com>
>>>> ---
>>>> .../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..08a5e92781657
>>>> --- /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_b-gpios:
>>>
>>> I commented on this and still see underscore. Nothing in commit msg
>>> explains why this should have underscore. Changelog is also vague -
>>> describes that you brought back underscores, instead of explaining why
>>> you did it.
>>>
>>> So the same comments as usual:
>>>
>>> No underscores in names.
>>>
>>> Best regards,
>>> Krzysztof
>>
>> Hello Krzysztof,
>>
>> Yes, I've gone full circle on that issue. Here's what I tried so far:
>
> And what part of the commit description allows me to understand this?
>
>>
>> 1) Reuse the same gpio names: Duplicates errors of the past, Krzysztof
>> doesn't like it.
>> 2) Different gpio names for new driver only: Makes the driver code
>> overly complicated, Yilun doesn't like it.
>
> That's a new driver, right? So what is complicated here? You have new
> code and you take prog-b or prog_b?
>
>> 3) Change gpio names for both drivers, deprecate the old names: Makes
>> the DT binding and the driver code overly complicated, Rob doesn't
>> like it.
>
> I don't think I proposed changing existing bindings.
>
>>
>> I think that while the driver code shouldn't be the driving force for
>> the DT spec, it can be a good indication that the spec is unpractical to
>> implement.
>
> What is impractical in implementing this? You just pass either A or B to
> function requesting GPIO. Just choose proper name.
>
>>
>> In this case, there are two interfaces on a chip that uses the same GPIO
>> protocol, it would only make sense that they use the same names, this
>> discards solution #2.
>
> I don't understand this. You have devm_gpiod_get() in your new code. Why
> is it difficult to use different name?
And I forgot to emphasize: none of these is mentioned in commit msg, so
for v5 you will get exactly the same complains. And for every other
patch which repeats the same and does not clarify caveats or exceptions.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 2/3] dt-bindings: fpga: xlnx,fpga-selectmap: add DT schema
2024-03-04 7:31 ` Krzysztof Kozlowski
@ 2024-03-05 2:27 ` Charles Perry
2024-03-05 7:27 ` Krzysztof Kozlowski
2024-03-06 7:10 ` Xu Yilun
0 siblings, 2 replies; 18+ messages in thread
From: Charles Perry @ 2024-03-05 2:27 UTC (permalink / raw)
To: Krzysztof Kozlowski, yilun xu
Cc: Rob Herring, mdf, Allen VANDIVER, Brian CODY, hao wu, Tom Rix,
krzysztof kozlowski+dt, Conor Dooley, Michal Simek, linux-fpga,
devicetree, linux-kernel, linux-arm-kernel
On Mar 4, 2024, at 12:31 AM, Krzysztof Kozlowski krzysztof.kozlowski@linaro.org wrote:
> On 04/03/2024 08:30, Krzysztof Kozlowski wrote:
>> On 03/03/2024 18:21, Charles Perry wrote:
>>> On Feb 27, 2024, at 3:10 AM, Krzysztof Kozlowski krzysztof.kozlowski@linaro.org
>>> wrote:
>>>
>>>> On 21/02/2024 20:50, Charles Perry wrote:
>>>>> Document the SelectMAP interface of Xilinx 7 series FPGA.
>>>>>
>>>>> Signed-off-by: Charles Perry <charles.perry@savoirfairelinux.com>
>>>>> ---
>>>>> .../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..08a5e92781657
>>>>> --- /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_b-gpios:
>>>>
>>>> I commented on this and still see underscore. Nothing in commit msg
>>>> explains why this should have underscore. Changelog is also vague -
>>>> describes that you brought back underscores, instead of explaining why
>>>> you did it.
>>>>
>>>> So the same comments as usual:
>>>>
>>>> No underscores in names.
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>
>>> Hello Krzysztof,
>>>
>>> Yes, I've gone full circle on that issue. Here's what I tried so far:
>>
>> And what part of the commit description allows me to understand this?
>>
I have a changelog in the cover letter:
https://lore.kernel.org/all/20240221195058.1281973-1-charles.perry@savoirfairelinux.com/
>>>
>>> 1) Reuse the same gpio names: Duplicates errors of the past, Krzysztof
>>> doesn't like it.
>>> 2) Different gpio names for new driver only: Makes the driver code
>>> overly complicated, Yilun doesn't like it.
>>
>> That's a new driver, right? So what is complicated here? You have new
>> code and you take prog-b or prog_b?
>>
>>> 3) Change gpio names for both drivers, deprecate the old names: Makes
>>> the DT binding and the driver code overly complicated, Rob doesn't
>>> like it.
>>
>> I don't think I proposed changing existing bindings.
>>
>>>
>>> I think that while the driver code shouldn't be the driving force for
>>> the DT spec, it can be a good indication that the spec is unpractical to
>>> implement.
>>
>> What is impractical in implementing this? You just pass either A or B to
>> function requesting GPIO. Just choose proper name.
>>
It's not complicated but it requires more code than if "prog_b" had been
used.
>>>
>>> In this case, there are two interfaces on a chip that uses the same GPIO
>>> protocol, it would only make sense that they use the same names, this
>>> discards solution #2.
>>
>> I don't understand this. You have devm_gpiod_get() in your new code. Why
>> is it difficult to use different name?
Yilun asked to avoid changing the names between the two drivers.
First comment in this mail:
https://lore.kernel.org/all/Zb9GkY6cMtR+4xOX@yilunxu-OptiPlex-7050/
Yilun, let me know if this is something you'd accept as this is a concern
for the device tree maintainers.
>
> And I forgot to emphasize: none of these is mentioned in commit msg, so
> for v5 you will get exactly the same complains. And for every other
> patch which repeats the same and does not clarify caveats or exceptions.
>
> Best regards,
> Krzysztof
Should I keep my changelog in the individual commits? I thought the norm
was to put this the cover letter.
Regards,
Charles
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 2/3] dt-bindings: fpga: xlnx,fpga-selectmap: add DT schema
2024-03-05 2:27 ` Charles Perry
@ 2024-03-05 7:27 ` Krzysztof Kozlowski
2024-03-06 7:10 ` Xu Yilun
1 sibling, 0 replies; 18+ messages in thread
From: Krzysztof Kozlowski @ 2024-03-05 7:27 UTC (permalink / raw)
To: Charles Perry, yilun xu
Cc: Rob Herring, mdf, Allen VANDIVER, Brian CODY, hao wu, Tom Rix,
krzysztof kozlowski+dt, Conor Dooley, Michal Simek, linux-fpga,
devicetree, linux-kernel, linux-arm-kernel
On 05/03/2024 03:27, Charles Perry wrote:
>
>
> On Mar 4, 2024, at 12:31 AM, Krzysztof Kozlowski krzysztof.kozlowski@linaro.org wrote:
>
>> On 04/03/2024 08:30, Krzysztof Kozlowski wrote:
>>> On 03/03/2024 18:21, Charles Perry wrote:
>>>> On Feb 27, 2024, at 3:10 AM, Krzysztof Kozlowski krzysztof.kozlowski@linaro.org
>>>> wrote:
>>>>
>>>>> On 21/02/2024 20:50, Charles Perry wrote:
>>>>>> Document the SelectMAP interface of Xilinx 7 series FPGA.
>>>>>>
>>>>>> Signed-off-by: Charles Perry <charles.perry@savoirfairelinux.com>
>>>>>> ---
>>>>>> .../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..08a5e92781657
>>>>>> --- /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_b-gpios:
>>>>>
>>>>> I commented on this and still see underscore. Nothing in commit msg
>>>>> explains why this should have underscore. Changelog is also vague -
>>>>> describes that you brought back underscores, instead of explaining why
>>>>> you did it.
>>>>>
>>>>> So the same comments as usual:
>>>>>
>>>>> No underscores in names.
>>>>>
>>>>> Best regards,
>>>>> Krzysztof
>>>>
>>>> Hello Krzysztof,
>>>>
>>>> Yes, I've gone full circle on that issue. Here's what I tried so far:
>>>
>>> And what part of the commit description allows me to understand this?
>>>
>
> I have a changelog in the cover letter:
> https://lore.kernel.org/all/20240221195058.1281973-1-charles.perry@savoirfairelinux.com/
Commit should stand on its own. Many reviewers do not read cover letter,
I mostly ignore it and only check the changelog part.
>
>>>>
>>>> 1) Reuse the same gpio names: Duplicates errors of the past, Krzysztof
>>>> doesn't like it.
>>>> 2) Different gpio names for new driver only: Makes the driver code
>>>> overly complicated, Yilun doesn't like it.
>>>
>>> That's a new driver, right? So what is complicated here? You have new
>>> code and you take prog-b or prog_b?
>>>
>>>> 3) Change gpio names for both drivers, deprecate the old names: Makes
>>>> the DT binding and the driver code overly complicated, Rob doesn't
>>>> like it.
>>>
>>> I don't think I proposed changing existing bindings.
>>>
>>>>
>>>> I think that while the driver code shouldn't be the driving force for
>>>> the DT spec, it can be a good indication that the spec is unpractical to
>>>> implement.
>>>
>>> What is impractical in implementing this? You just pass either A or B to
>>> function requesting GPIO. Just choose proper name.
>>>
>
> It's not complicated but it requires more code than if "prog_b" had been
> used.
>
>>>>
>>>> In this case, there are two interfaces on a chip that uses the same GPIO
>>>> protocol, it would only make sense that they use the same names, this
>>>> discards solution #2.
>>>
>>> I don't understand this. You have devm_gpiod_get() in your new code. Why
>>> is it difficult to use different name?
>
> Yilun asked to avoid changing the names between the two drivers.
> First comment in this mail:
> https://lore.kernel.org/all/Zb9GkY6cMtR+4xOX@yilunxu-OptiPlex-7050/
>
> Yilun, let me know if this is something you'd accept as this is a concern
> for the device tree maintainers.
>
>>
>> And I forgot to emphasize: none of these is mentioned in commit msg, so
>> for v5 you will get exactly the same complains. And for every other
>> patch which repeats the same and does not clarify caveats or exceptions.
>>
>> Best regards,
>> Krzysztof
>
> Should I keep my changelog in the individual commits? I thought the norm
> was to put this the cover letter.
I speak about the commit msg, not changelog. According to this commit,
there is absolutely no reason to keep prog_b name. This goes to git
stays there and later someone will check the history and will have the
same concerns: why the hell new binding was allowed to use underscore?
Why DT maintainers gave here exception? If they gave here exception, I
deserve exception as well. Such arguments are brought at least once per
month.
Your commit must stand on its own.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 2/3] dt-bindings: fpga: xlnx,fpga-selectmap: add DT schema
2024-03-05 2:27 ` Charles Perry
2024-03-05 7:27 ` Krzysztof Kozlowski
@ 2024-03-06 7:10 ` Xu Yilun
2024-03-06 14:29 ` Charles Perry
1 sibling, 1 reply; 18+ messages in thread
From: Xu Yilun @ 2024-03-06 7:10 UTC (permalink / raw)
To: Charles Perry
Cc: Krzysztof Kozlowski, yilun xu, Rob Herring, mdf, Allen VANDIVER,
Brian CODY, hao wu, Tom Rix, krzysztof kozlowski+dt,
Conor Dooley, Michal Simek, linux-fpga, devicetree, linux-kernel,
linux-arm-kernel
On Mon, Mar 04, 2024 at 09:27:04PM -0500, Charles Perry wrote:
>
>
> On Mar 4, 2024, at 12:31 AM, Krzysztof Kozlowski krzysztof.kozlowski@linaro.org wrote:
>
> > On 04/03/2024 08:30, Krzysztof Kozlowski wrote:
> >> On 03/03/2024 18:21, Charles Perry wrote:
> >>> On Feb 27, 2024, at 3:10 AM, Krzysztof Kozlowski krzysztof.kozlowski@linaro.org
> >>> wrote:
> >>>
> >>>> On 21/02/2024 20:50, Charles Perry wrote:
> >>>>> Document the SelectMAP interface of Xilinx 7 series FPGA.
> >>>>>
> >>>>> Signed-off-by: Charles Perry <charles.perry@savoirfairelinux.com>
> >>>>> ---
> >>>>> .../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..08a5e92781657
> >>>>> --- /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_b-gpios:
> >>>>
> >>>> I commented on this and still see underscore. Nothing in commit msg
> >>>> explains why this should have underscore. Changelog is also vague -
> >>>> describes that you brought back underscores, instead of explaining why
> >>>> you did it.
> >>>>
> >>>> So the same comments as usual:
> >>>>
> >>>> No underscores in names.
> >>>>
> >>>> Best regards,
> >>>> Krzysztof
> >>>
> >>> Hello Krzysztof,
> >>>
> >>> Yes, I've gone full circle on that issue. Here's what I tried so far:
> >>
> >> And what part of the commit description allows me to understand this?
> >>
>
> I have a changelog in the cover letter:
> https://lore.kernel.org/all/20240221195058.1281973-1-charles.perry@savoirfairelinux.com/
>
> >>>
> >>> 1) Reuse the same gpio names: Duplicates errors of the past, Krzysztof
> >>> doesn't like it.
> >>> 2) Different gpio names for new driver only: Makes the driver code
> >>> overly complicated, Yilun doesn't like it.
> >>
> >> That's a new driver, right? So what is complicated here? You have new
> >> code and you take prog-b or prog_b?
> >>
> >>> 3) Change gpio names for both drivers, deprecate the old names: Makes
> >>> the DT binding and the driver code overly complicated, Rob doesn't
> >>> like it.
> >>
> >> I don't think I proposed changing existing bindings.
> >>
> >>>
> >>> I think that while the driver code shouldn't be the driving force for
> >>> the DT spec, it can be a good indication that the spec is unpractical to
> >>> implement.
> >>
> >> What is impractical in implementing this? You just pass either A or B to
> >> function requesting GPIO. Just choose proper name.
> >>
>
> It's not complicated but it requires more code than if "prog_b" had been
> used.
>
> >>>
> >>> In this case, there are two interfaces on a chip that uses the same GPIO
> >>> protocol, it would only make sense that they use the same names, this
> >>> discards solution #2.
> >>
> >> I don't understand this. You have devm_gpiod_get() in your new code. Why
> >> is it difficult to use different name?
>
> Yilun asked to avoid changing the names between the two drivers.
> First comment in this mail:
> https://lore.kernel.org/all/Zb9GkY6cMtR+4xOX@yilunxu-OptiPlex-7050/
>
> Yilun, let me know if this is something you'd accept as this is a concern
> for the device tree maintainers.
I agree that deprecated names should not be used for new DT bindings, while
keeping backward compatibility to exsiting ones, unless there is other
DT side concern.
I'm also good that the driver adapts to the DT binding change.
What I'm concerned is the driver API:
int xilinx_core_probe(struct xilinx_fpga_core *core, struct device *dev,
xilinx_write_func write,
- xilinx_write_one_dummy_byte_func write_one_dummy_byte)
+ xilinx_write_one_dummy_byte_func write_one_dummy_byte,
+ const char *prog_con_id, const char *init_con_id)
You don't have to make every bus driver input the gpio names. The core
falls back to use old gpio names only for existing devices
(.compatible = "xlnx,fpga-slave-serial"). Then the issue could be
solved?
Thanks,
Yilun
>
> >
> > And I forgot to emphasize: none of these is mentioned in commit msg, so
> > for v5 you will get exactly the same complains. And for every other
> > patch which repeats the same and does not clarify caveats or exceptions.
> >
> > Best regards,
> > Krzysztof
>
> Should I keep my changelog in the individual commits? I thought the norm
> was to put this the cover letter.
>
> Regards,
> Charles
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 2/3] dt-bindings: fpga: xlnx,fpga-selectmap: add DT schema
2024-03-06 7:10 ` Xu Yilun
@ 2024-03-06 14:29 ` Charles Perry
0 siblings, 0 replies; 18+ messages in thread
From: Charles Perry @ 2024-03-06 14:29 UTC (permalink / raw)
To: Xu Yilun
Cc: Krzysztof Kozlowski, yilun xu, Rob Herring, mdf, Allen VANDIVER,
Brian CODY, hao wu, Tom Rix, krzysztof kozlowski+dt,
Conor Dooley, Michal Simek, linux-fpga, devicetree, linux-kernel,
linux-arm-kernel
----- On Mar 6, 2024, at 12:10 AM, Xu Yilun yilun.xu@linux.intel.com wrote:
> On Mon, Mar 04, 2024 at 09:27:04PM -0500, Charles Perry wrote:
>>
>>
>> On Mar 4, 2024, at 12:31 AM, Krzysztof Kozlowski krzysztof.kozlowski@linaro.org
>> wrote:
>>
>> > On 04/03/2024 08:30, Krzysztof Kozlowski wrote:
>> >> On 03/03/2024 18:21, Charles Perry wrote:
>> >>> On Feb 27, 2024, at 3:10 AM, Krzysztof Kozlowski krzysztof.kozlowski@linaro.org
>> >>> wrote:
>> >>>
>> >>>> On 21/02/2024 20:50, Charles Perry wrote:
>> >>>>> Document the SelectMAP interface of Xilinx 7 series FPGA.
>> >>>>>
>> >>>>> Signed-off-by: Charles Perry <charles.perry@savoirfairelinux.com>
>> >>>>> ---
>> >>>>> .../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..08a5e92781657
>> >>>>> --- /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_b-gpios:
>> >>>>
>> >>>> I commented on this and still see underscore. Nothing in commit msg
>> >>>> explains why this should have underscore. Changelog is also vague -
>> >>>> describes that you brought back underscores, instead of explaining why
>> >>>> you did it.
>> >>>>
>> >>>> So the same comments as usual:
>> >>>>
>> >>>> No underscores in names.
>> >>>>
>> >>>> Best regards,
>> >>>> Krzysztof
>> >>>
>> >>> Hello Krzysztof,
>> >>>
>> >>> Yes, I've gone full circle on that issue. Here's what I tried so far:
>> >>
>> >> And what part of the commit description allows me to understand this?
>> >>
>>
>> I have a changelog in the cover letter:
>> https://lore.kernel.org/all/20240221195058.1281973-1-charles.perry@savoirfairelinux.com/
>>
>> >>>
>> >>> 1) Reuse the same gpio names: Duplicates errors of the past, Krzysztof
>> >>> doesn't like it.
>> >>> 2) Different gpio names for new driver only: Makes the driver code
>> >>> overly complicated, Yilun doesn't like it.
>> >>
>> >> That's a new driver, right? So what is complicated here? You have new
>> >> code and you take prog-b or prog_b?
>> >>
>> >>> 3) Change gpio names for both drivers, deprecate the old names: Makes
>> >>> the DT binding and the driver code overly complicated, Rob doesn't
>> >>> like it.
>> >>
>> >> I don't think I proposed changing existing bindings.
>> >>
>> >>>
>> >>> I think that while the driver code shouldn't be the driving force for
>> >>> the DT spec, it can be a good indication that the spec is unpractical to
>> >>> implement.
>> >>
>> >> What is impractical in implementing this? You just pass either A or B to
>> >> function requesting GPIO. Just choose proper name.
>> >>
>>
>> It's not complicated but it requires more code than if "prog_b" had been
>> used.
>>
>> >>>
>> >>> In this case, there are two interfaces on a chip that uses the same GPIO
>> >>> protocol, it would only make sense that they use the same names, this
>> >>> discards solution #2.
>> >>
>> >> I don't understand this. You have devm_gpiod_get() in your new code. Why
>> >> is it difficult to use different name?
>>
>> Yilun asked to avoid changing the names between the two drivers.
>> First comment in this mail:
>> https://lore.kernel.org/all/Zb9GkY6cMtR+4xOX@yilunxu-OptiPlex-7050/
>>
>> Yilun, let me know if this is something you'd accept as this is a concern
>> for the device tree maintainers.
>
> I agree that deprecated names should not be used for new DT bindings, while
> keeping backward compatibility to exsiting ones, unless there is other
> DT side concern.
>
> I'm also good that the driver adapts to the DT binding change.
>
> What I'm concerned is the driver API:
>
> int xilinx_core_probe(struct xilinx_fpga_core *core, struct device *dev,
> xilinx_write_func write,
> - xilinx_write_one_dummy_byte_func write_one_dummy_byte)
> + xilinx_write_one_dummy_byte_func write_one_dummy_byte,
> + const char *prog_con_id, const char *init_con_id)
>
> You don't have to make every bus driver input the gpio names. The core
> falls back to use old gpio names only for existing devices
> (.compatible = "xlnx,fpga-slave-serial"). Then the issue could be
> solved?
>
> Thanks,
> Yilun
>
Ok, thank you for the guidance.
Regards,
Charles
>>
>> >
>> > And I forgot to emphasize: none of these is mentioned in commit msg, so
>> > for v5 you will get exactly the same complains. And for every other
>> > patch which repeats the same and does not clarify caveats or exceptions.
>> >
>> > Best regards,
>> > Krzysztof
>>
>> Should I keep my changelog in the individual commits? I thought the norm
>> was to put this the cover letter.
>>
>> Regards,
>> Charles
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v4 3/3] fpga: xilinx-selectmap: add new driver
2024-03-03 17:22 ` Charles Perry
@ 2024-03-13 22:47 ` Charles Perry
0 siblings, 0 replies; 18+ messages in thread
From: Charles Perry @ 2024-03-13 22:47 UTC (permalink / raw)
To: Xu Yilun
Cc: mdf, Allen VANDIVER, Brian CODY, hao wu, yilun xu, Tom Rix,
Rob Herring, krzysztof kozlowski+dt, Conor Dooley, Michal Simek,
linux-fpga, devicetree, linux-kernel, linux-arm-kernel
On Mar 3, 2024, at 12:22 PM, Charles Perry charles.perry@savoirfairelinux.com wrote:
> On Feb 26, 2024, at 2:50 AM, Xu Yilun yilun.xu@linux.intel.com wrote:
>
>> On Wed, Feb 21, 2024 at 02:50:49PM -0500, 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>
>>> ---
>>> drivers/fpga/Kconfig | 8 +++
>>> drivers/fpga/Makefile | 1 +
>>> drivers/fpga/xilinx-selectmap.c | 97 +++++++++++++++++++++++++++++++++
>>> 3 files changed, 106 insertions(+)
>>> 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-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);
>>
>> Does the driver have to be used with OF or not?
>>
>> If yes, please specify the reason and enforce in Kconfig.
>> If no, please ensure it decently compiles without CONFIG_OF.
>>
>> Thanks,
>> Yilun
>>
>
> No, it doesn't need OF explicitly as it only needs a few GPIO and a
> memory mapped IO region. It would be possible to get this info from
> platform data.
>
> I'll fix the compilation without CONFIG_OF.
It does compile without CONFIG_OF in its current form as I can compile
it for x86_64 without warnings.
One of the reason why xilinx-spi.c needs an "#ifdef CONFIG_OF" wrapper
around the of_device_id table is because it uses of_match_ptr() which
resolves to NULL when CONFIG_OF is not set, which triggers an unused
variable warning. This is not the case in xilinx-selectmap.c because
it's not using of_match_ptr().
Regards,
Charles
>
> Regards,
> Charles
>
>>> +
>>> +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
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2024-03-13 22:48 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-21 19:50 [PATCH v4 0/3] fpga: xilinx-selectmap: add new driver Charles Perry
2024-02-21 19:50 ` [PATCH v4 1/3] fpga: xilinx-spi: extract a common driver core Charles Perry
2024-02-26 9:19 ` Xu Yilun
2024-03-03 17:20 ` Charles Perry
2024-03-01 5:29 ` kernel test robot
2024-02-21 19:50 ` [PATCH v4 2/3] dt-bindings: fpga: xlnx,fpga-selectmap: add DT schema Charles Perry
2024-02-27 10:10 ` Krzysztof Kozlowski
2024-03-03 17:21 ` Charles Perry
2024-03-04 7:30 ` Krzysztof Kozlowski
2024-03-04 7:31 ` Krzysztof Kozlowski
2024-03-05 2:27 ` Charles Perry
2024-03-05 7:27 ` Krzysztof Kozlowski
2024-03-06 7:10 ` Xu Yilun
2024-03-06 14:29 ` Charles Perry
2024-02-21 19:50 ` [PATCH v4 3/3] fpga: xilinx-selectmap: add new driver Charles Perry
2024-02-26 9:50 ` Xu Yilun
2024-03-03 17:22 ` Charles Perry
2024-03-13 22:47 ` Charles Perry
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).