[v4,1/3] fpga: xilinx-spi: extract a common driver core

Message ID 20240221195058.1281973-2-charles.perry@savoirfairelinux.com
State New
Headers
Series fpga: xilinx-selectmap: add new driver |

Commit Message

Charles Perry Feb. 21, 2024, 7:50 p.m. UTC
  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
  

Comments

Xu Yilun Feb. 26, 2024, 9:19 a.m. UTC | #1
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
> 
>
  
kernel test robot March 1, 2024, 5:29 a.m. UTC | #2
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!
  
Charles Perry March 3, 2024, 5:20 p.m. UTC | #3
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
>>
  

Patch

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);