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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Feb 4, 2024, at 3:22 AM, Xu Yilun yilun.xu@xxxxxxxxxxxxxxx wrote:
> On Wed, Jan 31, 2024 at 06:05:31PM -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 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@xxxxxxxxxxxxxxxxxxxx>
>> ---
>>  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  | 204 +++--------------------------------
>>  5 files changed, 257 insertions(+), 189 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..aff40e9394085
>> --- /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@xxxxxxx>
>> + */
>> +
>> +#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);
>> +
> 
> remove the line.
> 

Ok

>> +	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)
> 
> I prefer to move these parameters in xilinx_fpga_core structure. I don't
> want the param list becomes longer and longer when we have more inputs.
> 

Ok

>> +{
>> +	struct fpga_manager *mgr;
>> +
>> +	core->dev = dev;
>> +	core->write = write;
>> +	core->write_one_dummy_byte = write_one_dummy_byte;
> 
> Could the write() writes one dummy byte? Maybe one write callback is enough.
> 

Yes, good idea.

>> +
>> +	/* 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..40e120945ba70
>> --- /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);
> 
> No need to typedef.
> 
> Thanks,
> Yilun
> 

Ok

Thank you for the review,
Charles

>> +
>> +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..ec128dee97312 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);
>> +#define to_xilinx_spi_conf(obj) container_of(obj, struct xilinx_spi_conf, core)
>>  
>> -	err = wait_for_init_b(mgr, 1, 1); /* min is 500 ns */
>> -	if (err) {
>> -		gpiod_set_value(conf->prog_b, 0);
>> -		return err;
>> -	}
>> -
>> -	gpiod_set_value(conf->prog_b, 0);
>> -
>> -	err = wait_for_init_b(mgr, 0, 0);
>> -	if (err)
>> -		return err;
>> -
>> -	if (get_done_gpio(mgr)) {
>> -		dev_err(&mgr->dev, "Unexpected DONE pin state...\n");
>> -		return -EIO;
>> -	}
>> -
>> -	/* program latency */
>> -	usleep_range(7500, 7600);
>> -	return 0;
>> -}
>> -
>> -static int xilinx_spi_write(struct fpga_manager *mgr, const char *buf,
>> +static int xilinx_spi_write(struct xilinx_fpga_core *core, const char *buf,
>>  			    size_t count)
>>  {
>> -	struct xilinx_spi_conf *conf = mgr->priv;
>> +	struct 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,77 +50,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,31 +73,15 @@ 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
>>  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
>> 




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux