diff mbox series

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

Message ID 20240129225602.3832449-1-charles.perry@savoirfairelinux.com
State Accepted
Commit a52e3a9dba347134ee53ebfe68b7b22548a387b0
Headers show
Series [1/3] fpga: xilinx-spi: extract a common driver core | expand

Commit Message

Charles Perry Jan. 29, 2024, 10:56 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 two operations:

 * ->write(const char* buf, size_t count): write to the device
 * ->write_one_dummy_byte(): write 0xFF to the device

Signed-off-by: Charles Perry <charles.perry@savoirfairelinux.com>
---
 drivers/fpga/Kconfig       |   4 +
 drivers/fpga/Makefile      |   1 +
 drivers/fpga/xilinx-core.c | 210 +++++++++++++++++++++++++++++++++++++
 drivers/fpga/xilinx-core.h |  27 +++++
 drivers/fpga/xilinx-spi.c  | 202 +++--------------------------------
 5 files changed, 256 insertions(+), 188 deletions(-)
 create mode 100644 drivers/fpga/xilinx-core.c
 create mode 100644 drivers/fpga/xilinx-core.h

Comments

Rob Herring (Arm) Jan. 30, 2024, 12:21 a.m. UTC | #1
On Mon, 29 Jan 2024 17:56:01 -0500, Charles Perry wrote:
> Document the slave SelectMAP interface of Xilinx 7 series FPGA.
> 
> Signed-off-by: Charles Perry <charles.perry@savoirfairelinux.com>
> ---
>  .../fpga/xlnx,fpga-slave-selectmap.yaml       | 85 +++++++++++++++++++
>  1 file changed, 85 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/fpga/xlnx,fpga-slave-selectmap.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/fpga/xlnx,fpga-slave-selectmap.yaml: 'maintainers' is a required property
	hint: Metaschema for devicetree binding documentation
	from schema $id: http://devicetree.org/meta-schemas/base.yaml#
Error: Documentation/devicetree/bindings/fpga/xlnx,fpga-slave-selectmap.example.dts:19.9-14 syntax error
FATAL ERROR: Unable to parse input tree
make[2]: *** [scripts/Makefile.lib:419: Documentation/devicetree/bindings/fpga/xlnx,fpga-slave-selectmap.example.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1428: dt_binding_check] Error 2
make: *** [Makefile:240: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240129225602.3832449-2-charles.perry@savoirfairelinux.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Krzysztof Kozlowski Jan. 30, 2024, 7:53 a.m. UTC | #2
On 30/01/2024 08:52, Krzysztof Kozlowski wrote:
> Please run your patch through Xilinx folks before sending.

Uh, you are not Xilinx folk, so disregard that. Yet still amount of
trivial issues here point that you need to start from scratch from
existing schema or example-schema.

Best regards,
Krzysztof
Krzysztof Kozlowski Jan. 30, 2024, 4:05 p.m. UTC | #3
On 30/01/2024 16:45, Charles Perry wrote:
> 
>>> +
>>> +  reg:
>>> +    description:
>>> +      At least 1 byte of memory mapped IO
>>> +    maxItems: 1
>>> +
>>> +  prog_b-gpios:
>>
>>
>> No underscores in names.
>>
> 
> This is heavily based on "xlnx,fpga-slave-serial.yaml" which uses an underscore.
> I can use a dash instead but that would make things inconsistent across the two schemas. 

Inconsistency is not a problem. Duplicating technical debt is.

> 
>>
>>> +    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:
>>
>> Is there init-a? Open other bindings and look how these are called there.
>>
> 
> No, the "-b" is there to denote that the signal is active low. I think its shorthand
> for "bar" which is the overline (‾) that electronic engineer put on top of the name of the
> signal on schematics. It comes from the datasheet.

Then just "init-gpios"

...

>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +  - prog_b-gpios
>>> +  - done-gpios
>>> +  - init-b-gpios
>>> +
>>> +additionalProperties: true
>>
>> Nope, this cannot bue true.
>>
> 
> Ok, I'll put this to false but I'm not quite sure I understand the implications.
> 
> My reasoning behind assigning this to true was that the FPGA is an external
> device on a bus that needs to be configured by a bus controller. The bus controller
> would be the parent of the fpga DT node and the later would contain properties
> parsed by the bus controller driver.

Which bus controller? MMIO bus does not parse children properties.
Anyway, if that's the case you miss $ref to respective
peripheral-props.yaml matching your bus and then "unevaluatedProperties:
false".

Best regards,
Krzysztof
Charles Perry Jan. 30, 2024, 5:05 p.m. UTC | #4
----- On Jan 30, 2024, at 11:05 AM, Krzysztof Kozlowski krzysztof.kozlowski@linaro.org wrote:

> On 30/01/2024 16:45, Charles Perry wrote:
>> 
>>>> +
>>>> +  reg:
>>>> +    description:
>>>> +      At least 1 byte of memory mapped IO
>>>> +    maxItems: 1
>>>> +
>>>> +  prog_b-gpios:
>>>
>>>
>>> No underscores in names.
>>>
>> 
>> This is heavily based on "xlnx,fpga-slave-serial.yaml" which uses an underscore.
>> I can use a dash instead but that would make things inconsistent across the two
>> schemas.
> 
> Inconsistency is not a problem. Duplicating technical debt is.
> 
>> 
>>>
>>>> +    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:
>>>
>>> Is there init-a? Open other bindings and look how these are called there.
>>>
>> 
>> No, the "-b" is there to denote that the signal is active low. I think its
>> shorthand
>> for "bar" which is the overline (‾) that electronic engineer put on top of the
>> name of the
>> signal on schematics. It comes from the datasheet.
> 
> Then just "init-gpios"
> 
> ...
> 
>>>> +required:
>>>> +  - compatible
>>>> +  - reg
>>>> +  - prog_b-gpios
>>>> +  - done-gpios
>>>> +  - init-b-gpios
>>>> +
>>>> +additionalProperties: true
>>>
>>> Nope, this cannot bue true.
>>>
>> 
>> Ok, I'll put this to false but I'm not quite sure I understand the implications.
>> 
>> My reasoning behind assigning this to true was that the FPGA is an external
>> device on a bus that needs to be configured by a bus controller. The bus
>> controller
>> would be the parent of the fpga DT node and the later would contain properties
>> parsed by the bus controller driver.
> 
> Which bus controller? MMIO bus does not parse children properties.
> Anyway, if that's the case you miss $ref to respective
> peripheral-props.yaml matching your bus and then "unevaluatedProperties:
> false".

This one: https://elixir.bootlin.com/linux/v6.8-rc2/source/Documentation/devicetree/bindings/bus/imx-weim.txt#L56

> 
> Best regards,
> Krzysztof

Regards,
Charles
Charles Perry Jan. 30, 2024, 11:32 p.m. UTC | #5
----- On Jan 30, 2024, at 12:58 PM, Krzysztof Kozlowski krzysztof.kozlowski@linaro.org wrote:

> On 30/01/2024 18:05, Charles Perry wrote:
>> 
>> 
>> ----- On Jan 30, 2024, at 11:05 AM, Krzysztof Kozlowski
>> krzysztof.kozlowski@linaro.org wrote:
>> 
>>> On 30/01/2024 16:45, Charles Perry wrote:
>>>>
>>>>>> +
>>>>>> +  reg:
>>>>>> +    description:
>>>>>> +      At least 1 byte of memory mapped IO
>>>>>> +    maxItems: 1
>>>>>> +
>>>>>> +  prog_b-gpios:
>>>>>
>>>>>
>>>>> No underscores in names.
>>>>>
>>>>
>>>> This is heavily based on "xlnx,fpga-slave-serial.yaml" which uses an underscore.
>>>> I can use a dash instead but that would make things inconsistent across the two
>>>> schemas.
>>>
>>> Inconsistency is not a problem. Duplicating technical debt is.
>>>
>>>>
>>>>>
>>>>>> +    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:
>>>>>
>>>>> Is there init-a? Open other bindings and look how these are called there.
>>>>>
>>>>
>>>> No, the "-b" is there to denote that the signal is active low. I think its
>>>> shorthand
>>>> for "bar" which is the overline (‾) that electronic engineer put on top of the
>>>> name of the
>>>> signal on schematics. It comes from the datasheet.
>>>
>>> Then just "init-gpios"
>>>
>>> ...
>>>
>>>>>> +required:
>>>>>> +  - compatible
>>>>>> +  - reg
>>>>>> +  - prog_b-gpios
>>>>>> +  - done-gpios
>>>>>> +  - init-b-gpios
>>>>>> +
>>>>>> +additionalProperties: true
>>>>>
>>>>> Nope, this cannot bue true.
>>>>>
>>>>
>>>> Ok, I'll put this to false but I'm not quite sure I understand the implications.
>>>>
>>>> My reasoning behind assigning this to true was that the FPGA is an external
>>>> device on a bus that needs to be configured by a bus controller. The bus
>>>> controller
>>>> would be the parent of the fpga DT node and the later would contain properties
>>>> parsed by the bus controller driver.
>>>
>>> Which bus controller? MMIO bus does not parse children properties.
>>> Anyway, if that's the case you miss $ref to respective
>>> peripheral-props.yaml matching your bus and then "unevaluatedProperties:
>>> false".
>> 
>> This one:
>> https://elixir.bootlin.com/linux/v6.8-rc2/source/Documentation/devicetree/bindings/bus/imx-weim.txt#L56
> 
> Eh, ok, so after fast check WEIM looks like some memory interface bus,
> so the bus bindings should be moved to memory-controllers and converted
> to YAML. Then you add child node properties to own schema and reference
> in mc-peripheral-props, which is then referenced in your binding here,
> as I mentioned.
> 
> Best regards,
> Krzysztof

Thank you for pointing that out, mc-peripheral-props.yaml seems to be
exactly what I was looking for.

Regards,
Charles
diff mbox series

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..47839b5541ddd
--- /dev/null
+++ b/drivers/fpga/xilinx-core.c
@@ -0,0 +1,210 @@ 
+// 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;
+
+	/*
+	 * 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_one_dummy_byte(core);
+		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 device *dev,
+					  xilinx_write_func write,
+					  xilinx_write_one_dummy_byte_func write_one_dummy_byte)
+{
+	struct fpga_manager *mgr;
+
+	core->dev = dev;
+	core->write = write;
+	core->write_one_dummy_byte = write_one_dummy_byte;
+
+	/* PROGRAM_B is active low */
+	core->prog_b = devm_gpiod_get(dev, "prog_b", GPIOD_OUT_LOW);
+	if (IS_ERR(core->prog_b))
+		return dev_err_probe(dev, PTR_ERR(core->prog_b),
+				     "Failed to get PROGRAM_B gpio\n");
+
+	core->init_b = devm_gpiod_get_optional(dev, "init-b", GPIOD_IN);
+	if (IS_ERR(core->init_b))
+		return dev_err_probe(dev, PTR_ERR(core->init_b),
+				     "Failed to get INIT_B gpio\n");
+
+	core->done = devm_gpiod_get(dev, "done", GPIOD_IN);
+	if (IS_ERR(core->done))
+		return dev_err_probe(dev, PTR_ERR(core->done),
+				     "Failed to get DONE gpio\n");
+
+	mgr = devm_fpga_mgr_register(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..2e6ce2235531f
--- /dev/null
+++ b/drivers/fpga/xilinx-core.h
@@ -0,0 +1,27 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef	__XILINX_CORE_H
+#define	__XILINX_CORE_H
+
+#include <linux/device.h>
+
+struct xilinx_fpga_core;
+
+typedef int (*xilinx_write_func)(struct xilinx_fpga_core *core, const char *buf,
+			    size_t count);
+typedef int (*xilinx_write_one_dummy_byte_func)(struct xilinx_fpga_core *core);
+
+struct xilinx_fpga_core {
+	struct device *dev;
+	xilinx_write_func write;
+	xilinx_write_one_dummy_byte_func write_one_dummy_byte;
+	struct gpio_desc *prog_b;
+	struct gpio_desc *init_b;
+	struct gpio_desc *done;
+};
+
+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);
+
+#endif /* __XILINX_CORE_H */
diff --git a/drivers/fpga/xilinx-spi.c b/drivers/fpga/xilinx-spi.c
index e1a227e7ff2ae..71d8d421f3c63 100644
--- a/drivers/fpga/xilinx-spi.c
+++ b/drivers/fpga/xilinx-spi.c
@@ -10,127 +10,25 @@ 
  * 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);
-
-	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;
-}
+#define to_xilinx_spi_conf(obj) \
+	container_of(obj, struct xilinx_spi_conf, core)
 
-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 +41,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,77 +51,22 @@  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)
+static int xilinx_spi_apply_cclk_cycles(struct xilinx_fpga_core *core)
 {
-	struct spi_device *spi = conf->spi;
+	struct xilinx_spi_conf *conf = to_xilinx_spi_conf(core);
 	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);
+		dev_err(core->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)
@@ -231,26 +74,9 @@  static int xilinx_spi_probe(struct spi_device *spi)
 
 	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, &spi->dev,
+							&xilinx_spi_write,
+							&xilinx_spi_apply_cclk_cycles);
 }
 
 #ifdef CONFIG_OF