| Message ID | 20240221195058.1281973-2-charles.perry@savoirfairelinux.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fpga: xilinx-selectmap: add new driver | expand |
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 > >
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!
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 >>
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);
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