Factor out the gpio handshaking (using PROGRAM_B, INIT_B and DONE)
protocol in xilinx-core so that it can be reused for another driver.
This commit does not change anything functionally to xilinx-spi.
xilinx-core expects drivers to provide a write(const char* buf,
size_t count) function that performs the actual write to the device,
as well as a struct device* for resource management.
Signed-off-by: Charles Perry <[email protected]>
---
Changes since v4: (from Yilun review)
* xilinx-core: use sizeof() instead of hardcoded immediate
* xilinx-core: fix module compilation (EXPORT_SYMBOL_GPL, MODULE_LICENSE,
MODULE_AUTHOR, MODULE_DESCRIPTION)
* xilinx-core: add private/public qualifiers for struct xilinx_fpga_core
* xilinx-spi: remove struct xilinx_spi_conf. This struct isn't needed as
the struct spi_device* can be retrieved from the struct device*.
drivers/fpga/Kconfig | 4 +
drivers/fpga/Makefile | 1 +
drivers/fpga/xilinx-core.c | 213 +++++++++++++++++++++++++++++++++++
drivers/fpga/xilinx-core.h | 30 +++++
drivers/fpga/xilinx-spi.c | 220 +++----------------------------------
5 files changed, 261 insertions(+), 207 deletions(-)
create mode 100644 drivers/fpga/xilinx-core.c
create mode 100644 drivers/fpga/xilinx-core.h
diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index 2f689ac4ba3a3..d27a1ebf40838 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -64,9 +64,13 @@ config FPGA_MGR_STRATIX10_SOC
help
FPGA manager driver support for the Intel Stratix10 SoC.
+config FPGA_MGR_XILINX_CORE
+ tristate
+
config FPGA_MGR_XILINX_SPI
tristate "Xilinx Configuration over Slave Serial (SPI)"
depends on SPI
+ select FPGA_MGR_XILINX_CORE
help
FPGA manager driver support for Xilinx FPGA configuration
over slave serial interface.
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index 352a2612623e0..7ec795b6a5a70 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_FPGA_MGR_SOCFPGA) += socfpga.o
obj-$(CONFIG_FPGA_MGR_SOCFPGA_A10) += socfpga-a10.o
obj-$(CONFIG_FPGA_MGR_STRATIX10_SOC) += stratix10-soc.o
obj-$(CONFIG_FPGA_MGR_TS73XX) += ts73xx-fpga.o
+obj-$(CONFIG_FPGA_MGR_XILINX_CORE) += xilinx-core.o
obj-$(CONFIG_FPGA_MGR_XILINX_SPI) += xilinx-spi.o
obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA) += zynq-fpga.o
obj-$(CONFIG_FPGA_MGR_ZYNQMP_FPGA) += zynqmp-fpga.o
diff --git a/drivers/fpga/xilinx-core.c b/drivers/fpga/xilinx-core.c
new file mode 100644
index 0000000000000..a35c43382dd5f
--- /dev/null
+++ b/drivers/fpga/xilinx-core.c
@@ -0,0 +1,213 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Common parts of the Xilinx Spartan6 and 7 Series FPGA manager drivers.
+ *
+ * Copyright (C) 2017 DENX Software Engineering
+ *
+ * Anatolij Gustschin <[email protected]>
+ */
+
+#include "xilinx-core.h"
+
+#include <linux/delay.h>
+#include <linux/fpga/fpga-mgr.h>
+#include <linux/gpio/consumer.h>
+#include <linux/of.h>
+
+static int get_done_gpio(struct fpga_manager *mgr)
+{
+ struct xilinx_fpga_core *core = mgr->priv;
+ int ret;
+
+ ret = gpiod_get_value(core->done);
+ if (ret < 0)
+ dev_err(&mgr->dev, "Error reading DONE (%d)\n", ret);
+
+ return ret;
+}
+
+static enum fpga_mgr_states xilinx_core_state(struct fpga_manager *mgr)
+{
+ if (!get_done_gpio(mgr))
+ return FPGA_MGR_STATE_RESET;
+
+ return FPGA_MGR_STATE_UNKNOWN;
+}
+
+/**
+ * wait_for_init_b - wait for the INIT_B pin to have a given state, or wait
+ * a given delay if the pin is unavailable
+ *
+ * @mgr: The FPGA manager object
+ * @value: Value INIT_B to wait for (1 = asserted = low)
+ * @alt_udelay: Delay to wait if the INIT_B GPIO is not available
+ *
+ * Returns 0 when the INIT_B GPIO reached the given state or -ETIMEDOUT if
+ * too much time passed waiting for that. If no INIT_B GPIO is available
+ * then always return 0.
+ */
+static int wait_for_init_b(struct fpga_manager *mgr, int value,
+ unsigned long alt_udelay)
+{
+ struct xilinx_fpga_core *core = mgr->priv;
+ unsigned long timeout = jiffies + msecs_to_jiffies(1000);
+
+ if (core->init_b) {
+ while (time_before(jiffies, timeout)) {
+ int ret = gpiod_get_value(core->init_b);
+
+ if (ret == value)
+ return 0;
+
+ if (ret < 0) {
+ dev_err(&mgr->dev,
+ "Error reading INIT_B (%d)\n", ret);
+ return ret;
+ }
+
+ usleep_range(100, 400);
+ }
+
+ dev_err(&mgr->dev, "Timeout waiting for INIT_B to %s\n",
+ value ? "assert" : "deassert");
+ return -ETIMEDOUT;
+ }
+
+ udelay(alt_udelay);
+
+ return 0;
+}
+
+static int xilinx_core_write_init(struct fpga_manager *mgr,
+ struct fpga_image_info *info, const char *buf,
+ size_t count)
+{
+ struct xilinx_fpga_core *core = mgr->priv;
+ int err;
+
+ if (info->flags & FPGA_MGR_PARTIAL_RECONFIG) {
+ dev_err(&mgr->dev, "Partial reconfiguration not supported\n");
+ return -EINVAL;
+ }
+
+ gpiod_set_value(core->prog_b, 1);
+
+ err = wait_for_init_b(mgr, 1, 1); /* min is 500 ns */
+ if (err) {
+ gpiod_set_value(core->prog_b, 0);
+ return err;
+ }
+
+ gpiod_set_value(core->prog_b, 0);
+
+ err = wait_for_init_b(mgr, 0, 0);
+ if (err)
+ return err;
+
+ if (get_done_gpio(mgr)) {
+ dev_err(&mgr->dev, "Unexpected DONE pin state...\n");
+ return -EIO;
+ }
+
+ /* program latency */
+ usleep_range(7500, 7600);
+ return 0;
+}
+
+static int xilinx_core_write(struct fpga_manager *mgr, const char *buf,
+ size_t count)
+{
+ struct xilinx_fpga_core *core = mgr->priv;
+
+ return core->write(core, buf, count);
+}
+
+static int xilinx_core_write_complete(struct fpga_manager *mgr,
+ struct fpga_image_info *info)
+{
+ struct xilinx_fpga_core *core = mgr->priv;
+ unsigned long timeout =
+ jiffies + usecs_to_jiffies(info->config_complete_timeout_us);
+ bool expired = false;
+ int done;
+ int ret;
+ const char padding[1] = { 0xff };
+
+ /*
+ * This loop is carefully written such that if the driver is
+ * scheduled out for more than 'timeout', we still check for DONE
+ * before giving up and we apply 8 extra CCLK cycles in all cases.
+ */
+ while (!expired) {
+ expired = time_after(jiffies, timeout);
+
+ done = get_done_gpio(mgr);
+ if (done < 0)
+ return done;
+
+ ret = core->write(core, padding, sizeof(padding));
+ if (ret)
+ return ret;
+
+ if (done)
+ return 0;
+ }
+
+ if (core->init_b) {
+ ret = gpiod_get_value(core->init_b);
+
+ if (ret < 0) {
+ dev_err(&mgr->dev, "Error reading INIT_B (%d)\n", ret);
+ return ret;
+ }
+
+ dev_err(&mgr->dev,
+ ret ? "CRC error or invalid device\n" :
+ "Missing sync word or incomplete bitstream\n");
+ } else {
+ dev_err(&mgr->dev, "Timeout after config data transfer\n");
+ }
+
+ return -ETIMEDOUT;
+}
+
+static const struct fpga_manager_ops xilinx_core_ops = {
+ .state = xilinx_core_state,
+ .write_init = xilinx_core_write_init,
+ .write = xilinx_core_write,
+ .write_complete = xilinx_core_write_complete,
+};
+
+int xilinx_core_probe(struct xilinx_fpga_core *core)
+{
+ struct fpga_manager *mgr;
+
+ if (!core || !core->dev || !core->write)
+ return -EINVAL;
+
+ /* PROGRAM_B is active low */
+ core->prog_b = devm_gpiod_get(core->dev, "prog_b", GPIOD_OUT_LOW);
+ if (IS_ERR(core->prog_b))
+ return dev_err_probe(core->dev, PTR_ERR(core->prog_b),
+ "Failed to get PROGRAM_B gpio\n");
+
+ core->init_b = devm_gpiod_get_optional(core->dev, "init-b", GPIOD_IN);
+ if (IS_ERR(core->init_b))
+ return dev_err_probe(core->dev, PTR_ERR(core->init_b),
+ "Failed to get INIT_B gpio\n");
+
+ core->done = devm_gpiod_get(core->dev, "done", GPIOD_IN);
+ if (IS_ERR(core->done))
+ return dev_err_probe(core->dev, PTR_ERR(core->done),
+ "Failed to get DONE gpio\n");
+
+ mgr = devm_fpga_mgr_register(core->dev,
+ "Xilinx Slave Serial FPGA Manager",
+ &xilinx_core_ops, core);
+ return PTR_ERR_OR_ZERO(mgr);
+}
+EXPORT_SYMBOL_GPL(xilinx_core_probe);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Anatolij Gustschin <[email protected]>");
+MODULE_DESCRIPTION("Xilinx 7 Series FPGA manager core");
diff --git a/drivers/fpga/xilinx-core.h b/drivers/fpga/xilinx-core.h
new file mode 100644
index 0000000000000..2fef95b341be0
--- /dev/null
+++ b/drivers/fpga/xilinx-core.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __XILINX_CORE_H
+#define __XILINX_CORE_H
+
+#include <linux/device.h>
+
+/**
+ * struct xilinx_fpga_core - interface between the driver and the core manager
+ * of Xilinx 7 Series FPGA manager
+ * @dev: device node
+ * @write: write callback of the driver
+ * @prog_b: PROGRAM_B gpio descriptor
+ * @init_b: INIT_B gpio descriptor
+ * @done: DONE gpio descriptor
+ */
+struct xilinx_fpga_core {
+/* public: */
+ struct device *dev;
+ int (*write)(struct xilinx_fpga_core *core, const char *buf,
+ size_t count);
+/* private: handled by xilinx-core */
+ struct gpio_desc *prog_b;
+ struct gpio_desc *init_b;
+ struct gpio_desc *done;
+};
+
+int xilinx_core_probe(struct xilinx_fpga_core *core);
+
+#endif /* __XILINX_CORE_H */
diff --git a/drivers/fpga/xilinx-spi.c b/drivers/fpga/xilinx-spi.c
index e1a227e7ff2ae..ebf339cf0fd81 100644
--- a/drivers/fpga/xilinx-spi.c
+++ b/drivers/fpga/xilinx-spi.c
@@ -10,127 +10,17 @@
* the slave serial configuration interface.
*/
-#include <linux/delay.h>
-#include <linux/device.h>
-#include <linux/fpga/fpga-mgr.h>
-#include <linux/gpio/consumer.h>
+#include "xilinx-core.h"
+
#include <linux/module.h>
#include <linux/mod_devicetable.h>
#include <linux/of.h>
#include <linux/spi/spi.h>
-#include <linux/sizes.h>
-
-struct xilinx_spi_conf {
- struct spi_device *spi;
- struct gpio_desc *prog_b;
- struct gpio_desc *init_b;
- struct gpio_desc *done;
-};
-
-static int get_done_gpio(struct fpga_manager *mgr)
-{
- struct xilinx_spi_conf *conf = mgr->priv;
- int ret;
-
- ret = gpiod_get_value(conf->done);
-
- if (ret < 0)
- dev_err(&mgr->dev, "Error reading DONE (%d)\n", ret);
-
- return ret;
-}
-
-static enum fpga_mgr_states xilinx_spi_state(struct fpga_manager *mgr)
-{
- if (!get_done_gpio(mgr))
- return FPGA_MGR_STATE_RESET;
-
- return FPGA_MGR_STATE_UNKNOWN;
-}
-
-/**
- * wait_for_init_b - wait for the INIT_B pin to have a given state, or wait
- * a given delay if the pin is unavailable
- *
- * @mgr: The FPGA manager object
- * @value: Value INIT_B to wait for (1 = asserted = low)
- * @alt_udelay: Delay to wait if the INIT_B GPIO is not available
- *
- * Returns 0 when the INIT_B GPIO reached the given state or -ETIMEDOUT if
- * too much time passed waiting for that. If no INIT_B GPIO is available
- * then always return 0.
- */
-static int wait_for_init_b(struct fpga_manager *mgr, int value,
- unsigned long alt_udelay)
-{
- struct xilinx_spi_conf *conf = mgr->priv;
- unsigned long timeout = jiffies + msecs_to_jiffies(1000);
-
- if (conf->init_b) {
- while (time_before(jiffies, timeout)) {
- int ret = gpiod_get_value(conf->init_b);
-
- if (ret == value)
- return 0;
-
- if (ret < 0) {
- dev_err(&mgr->dev, "Error reading INIT_B (%d)\n", ret);
- return ret;
- }
-
- usleep_range(100, 400);
- }
-
- dev_err(&mgr->dev, "Timeout waiting for INIT_B to %s\n",
- value ? "assert" : "deassert");
- return -ETIMEDOUT;
- }
-
- udelay(alt_udelay);
-
- return 0;
-}
-
-static int xilinx_spi_write_init(struct fpga_manager *mgr,
- struct fpga_image_info *info,
- const char *buf, size_t count)
-{
- struct xilinx_spi_conf *conf = mgr->priv;
- int err;
-
- if (info->flags & FPGA_MGR_PARTIAL_RECONFIG) {
- dev_err(&mgr->dev, "Partial reconfiguration not supported\n");
- return -EINVAL;
- }
-
- gpiod_set_value(conf->prog_b, 1);
-
- err = wait_for_init_b(mgr, 1, 1); /* min is 500 ns */
- if (err) {
- gpiod_set_value(conf->prog_b, 0);
- return err;
- }
-
- gpiod_set_value(conf->prog_b, 0);
-
- err = wait_for_init_b(mgr, 0, 0);
- if (err)
- return err;
-
- if (get_done_gpio(mgr)) {
- dev_err(&mgr->dev, "Unexpected DONE pin state...\n");
- return -EIO;
- }
- /* program latency */
- usleep_range(7500, 7600);
- return 0;
-}
-
-static int xilinx_spi_write(struct fpga_manager *mgr, const char *buf,
+static int xilinx_spi_write(struct xilinx_fpga_core *core, const char *buf,
size_t count)
{
- struct xilinx_spi_conf *conf = mgr->priv;
+ struct spi_device *spi = to_spi_device(core->dev);
const char *fw_data = buf;
const char *fw_data_end = fw_data + count;
@@ -141,9 +31,9 @@ static int xilinx_spi_write(struct fpga_manager *mgr, const char *buf,
remaining = fw_data_end - fw_data;
stride = min_t(size_t, remaining, SZ_4K);
- ret = spi_write(conf->spi, fw_data, stride);
+ ret = spi_write(spi, fw_data, stride);
if (ret) {
- dev_err(&mgr->dev, "SPI error in firmware write: %d\n",
+ dev_err(core->dev, "SPI error in firmware write: %d\n",
ret);
return ret;
}
@@ -153,109 +43,25 @@ static int xilinx_spi_write(struct fpga_manager *mgr, const char *buf,
return 0;
}
-static int xilinx_spi_apply_cclk_cycles(struct xilinx_spi_conf *conf)
-{
- struct spi_device *spi = conf->spi;
- const u8 din_data[1] = { 0xff };
- int ret;
-
- ret = spi_write(conf->spi, din_data, sizeof(din_data));
- if (ret)
- dev_err(&spi->dev, "applying CCLK cycles failed: %d\n", ret);
-
- return ret;
-}
-
-static int xilinx_spi_write_complete(struct fpga_manager *mgr,
- struct fpga_image_info *info)
-{
- struct xilinx_spi_conf *conf = mgr->priv;
- unsigned long timeout = jiffies + usecs_to_jiffies(info->config_complete_timeout_us);
- bool expired = false;
- int done;
- int ret;
-
- /*
- * This loop is carefully written such that if the driver is
- * scheduled out for more than 'timeout', we still check for DONE
- * before giving up and we apply 8 extra CCLK cycles in all cases.
- */
- while (!expired) {
- expired = time_after(jiffies, timeout);
-
- done = get_done_gpio(mgr);
- if (done < 0)
- return done;
-
- ret = xilinx_spi_apply_cclk_cycles(conf);
- if (ret)
- return ret;
-
- if (done)
- return 0;
- }
-
- if (conf->init_b) {
- ret = gpiod_get_value(conf->init_b);
-
- if (ret < 0) {
- dev_err(&mgr->dev, "Error reading INIT_B (%d)\n", ret);
- return ret;
- }
-
- dev_err(&mgr->dev,
- ret ? "CRC error or invalid device\n"
- : "Missing sync word or incomplete bitstream\n");
- } else {
- dev_err(&mgr->dev, "Timeout after config data transfer\n");
- }
-
- return -ETIMEDOUT;
-}
-
-static const struct fpga_manager_ops xilinx_spi_ops = {
- .state = xilinx_spi_state,
- .write_init = xilinx_spi_write_init,
- .write = xilinx_spi_write,
- .write_complete = xilinx_spi_write_complete,
-};
-
static int xilinx_spi_probe(struct spi_device *spi)
{
- struct xilinx_spi_conf *conf;
- struct fpga_manager *mgr;
+ struct xilinx_fpga_core *conf;
conf = devm_kzalloc(&spi->dev, sizeof(*conf), GFP_KERNEL);
if (!conf)
return -ENOMEM;
- conf->spi = spi;
+ conf->dev = &spi->dev;
+ conf->write = xilinx_spi_write;
- /* PROGRAM_B is active low */
- conf->prog_b = devm_gpiod_get(&spi->dev, "prog_b", GPIOD_OUT_LOW);
- if (IS_ERR(conf->prog_b))
- return dev_err_probe(&spi->dev, PTR_ERR(conf->prog_b),
- "Failed to get PROGRAM_B gpio\n");
-
- conf->init_b = devm_gpiod_get_optional(&spi->dev, "init-b", GPIOD_IN);
- if (IS_ERR(conf->init_b))
- return dev_err_probe(&spi->dev, PTR_ERR(conf->init_b),
- "Failed to get INIT_B gpio\n");
-
- conf->done = devm_gpiod_get(&spi->dev, "done", GPIOD_IN);
- if (IS_ERR(conf->done))
- return dev_err_probe(&spi->dev, PTR_ERR(conf->done),
- "Failed to get DONE gpio\n");
-
- mgr = devm_fpga_mgr_register(&spi->dev,
- "Xilinx Slave Serial FPGA Manager",
- &xilinx_spi_ops, conf);
- return PTR_ERR_OR_ZERO(mgr);
+ return xilinx_core_probe(conf);
}
#ifdef CONFIG_OF
static const struct of_device_id xlnx_spi_of_match[] = {
- { .compatible = "xlnx,fpga-slave-serial", },
+ {
+ .compatible = "xlnx,fpga-slave-serial",
+ },
{}
};
MODULE_DEVICE_TABLE(of, xlnx_spi_of_match);
--
2.43.0
> +/**
> + * struct xilinx_fpga_core - interface between the driver and the core manager
> + * of Xilinx 7 Series FPGA manager
> + * @dev: device node
> + * @write: write callback of the driver
> + * @prog_b: PROGRAM_B gpio descriptor
> + * @init_b: INIT_B gpio descriptor
> + * @done: DONE gpio descriptor
Please re-check the Documentation again:
"Structure fields that are inside a private: area are not listed in the generated output documentation"
> + */
> +struct xilinx_fpga_core {
> +/* public: */
> + struct device *dev;
> + int (*write)(struct xilinx_fpga_core *core, const char *buf,
> + size_t count);
> +/* private: handled by xilinx-core */
> + struct gpio_desc *prog_b;
> + struct gpio_desc *init_b;
> + struct gpio_desc *done;
> +};
> +
[...]
> -
> static int xilinx_spi_probe(struct spi_device *spi)
> {
> - struct xilinx_spi_conf *conf;
> - struct fpga_manager *mgr;
> + struct xilinx_fpga_core *conf;
Why do you name it conf? Maybe "core" is better?
Thanks,
Yilun
On Mar 19, 2024, at 12:20 AM, Xu Yilun [email protected] wrote:
>> +/**
>> + * struct xilinx_fpga_core - interface between the driver and the core manager
>> + * of Xilinx 7 Series FPGA manager
>> + * @dev: device node
>> + * @write: write callback of the driver
>> + * @prog_b: PROGRAM_B gpio descriptor
>> + * @init_b: INIT_B gpio descriptor
>> + * @done: DONE gpio descriptor
>
> Please re-check the Documentation again:
> "Structure fields that are inside a private: area are not listed in the
> generated output documentation"
>
I did generate the documentation for that struct and saw that the
private fields are indeed removed. This hinted me into thinking that
I should keep the private kernel-doc, because it's the generator job
to remove those. Looking again at the kernel-doc.rst example on that
topic, I understand that this is not the case, will fix.
>> + */
>> +struct xilinx_fpga_core {
>> +/* public: */
>> + struct device *dev;
>> + int (*write)(struct xilinx_fpga_core *core, const char *buf,
>> + size_t count);
>> +/* private: handled by xilinx-core */
>> + struct gpio_desc *prog_b;
>> + struct gpio_desc *init_b;
>> + struct gpio_desc *done;
>> +};
>> +
> [...]
>> -
>> static int xilinx_spi_probe(struct spi_device *spi)
>> {
>> - struct xilinx_spi_conf *conf;
>> - struct fpga_manager *mgr;
>> + struct xilinx_fpga_core *conf;
>
> Why do you name it conf? Maybe "core" is better?
>
> Thanks,
> Yilun
Yes, I changed the type but not the name.
Thank you for the review,
Charles