fpga manager: Add Stratix V CvP driver

Commit Message

Anatolij Gustschin Feb. 13, 2017, 7:17 p.m. UTC

Add FPGA manager driver for loading Stratix V FPGA using CvP.

Signed-off-by: Anatolij Gustschin <[e-mail]>
---
 drivers/fpga/Kconfig       |   7 +
 drivers/fpga/Makefile      |   1 +
 drivers/fpga/stratix-cvp.c | 529 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 537 insertions(+)
 create mode 100644 drivers/fpga/stratix-cvp.c

Comments

Matthew Gerlach Feb. 14, 2017, 12:49 a.m. UTC | #1

Hi Anatolij,



On Mon, 13 Feb 2017, Anatolij Gustschin wrote:

> Add FPGA manager driver for loading Stratix V FPGA using CvP.

Are you envisioning a flow where this module loads the fpga, and
a separate module would actually make use of the fully configured
fpga?

Do happen to have a similar driver for the Cyclone5 FPGA using CvP
in the works?

>
> Signed-off-by: Anatolij Gustschin <[e-mail]>
> ---
> drivers/fpga/Kconfig       |   7 +
> drivers/fpga/Makefile      |   1 +
> drivers/fpga/stratix-cvp.c | 529 +++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 537 insertions(+)
> create mode 100644 drivers/fpga/stratix-cvp.c
>
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index ce861a2..7ef5b06 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -33,6 +33,13 @@ config FPGA_MGR_SOCFPGA_A10
> 	help
> 	  FPGA manager driver support for Altera Arria10 SoCFPGA.
>
> +config FPGA_MGR_STRATIX_CVP
> +	tristate "Altera Stratix V CvP"
> +	depends on PCI
> +	help
> +	  FPGA manager driver support for Altera Stratix V using the
> +	  CvP interface over PCIe
> +
> config FPGA_MGR_ZYNQ_FPGA
> 	tristate "Xilinx Zynq FPGA"
> 	depends on ARCH_ZYNQ || COMPILE_TEST
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 8df07bc..0444c78 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_FPGA)			+= fpga-mgr.o
> # FPGA Manager Drivers
> obj-$(CONFIG_FPGA_MGR_SOCFPGA)		+= socfpga.o
> obj-$(CONFIG_FPGA_MGR_SOCFPGA_A10)	+= socfpga-a10.o
> +obj-$(CONFIG_FPGA_MGR_STRATIX_CVP)	+= stratix-cvp.o
> obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA)	+= zynq-fpga.o
>
> # FPGA Bridge Drivers
> diff --git a/drivers/fpga/stratix-cvp.c b/drivers/fpga/stratix-cvp.c
> new file mode 100644
> index 0000000..5ab3499
> --- /dev/null
> +++ b/drivers/fpga/stratix-cvp.c
> @@ -0,0 +1,529 @@
> +/*
> + * Copyright (C) 2017 DENX Software Engineering
> + *
> + * Anatolij Gustschin <[e-mail]>
> + *
> + * Manage Altera Statix V FPGA firmware using PCIe CvP.
> + * Firmware must be in binary "rbf" format.
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/fpga/fpga-mgr.h>
> +#include <linux/module.h>
> +#include <linux/sizes.h>
> +#include <linux/pci.h>
> +
> +/* Vendor Specific Extended Capability Offset */
> +#define VSEC_OFFSET			0x200
> +#define VSEC_PCIE_EXT_CAP_ID_VAL	0x000b
> +
> +#define FNC		0	/* PCIe device function ID */
> +#define CVP_BAR		0	/* BAR used for data transfer in memory mode */
> +#define CVP_DUMMY_WR	244	/* dummy writes to clear CvP state machine */
> +#define TIMEOUT_IN_US	10000
> +
> +/* offsets from VSEC_OFFSET */
> +#define VSEC_PCIE_EXT_CAP_ID		(VSEC_OFFSET + 0x00)	/* 16bit */
> +#define VSEC_VERSION			0x02		/* 8bit */
> +#define VSEC_NEXT_CAP_OFF		0x03		/* 8bit */
> +#define VSEC_ID				0x04		/* 16bit */
> +#define VSEC_REV			0x06		/* 8bit */
> +#define VSEC_LENGTH			0x07		/* 8bit */
> +#define VSEC_ALTERA_MARKER		0x08		/* 32bit */
> +
> +#define VSEC_CVP_STATUS			(VSEC_OFFSET + 0x1e)	/* 16bit */
> +#define VSEC_CVP_STATUS_DATA_ENC	(1<<0)	/* is treated as encrypted */
> +#define VSEC_CVP_STATUS_DATA_COMP	(1<<1)	/* is treated as compressed */
> +#define VSEC_CVP_STATUS_CFG_RDY		(1<<2)	/* CVP_CONFIG_READY */
> +#define VSEC_CVP_STATUS_CFG_ERR		(1<<3)	/* CVP_CONFIG_ERROR */
> +#define VSEC_CVP_STATUS_CVP_EN		(1<<4)	/* ctrl block is enabling CVP */
> +#define VSEC_CVP_STATUS_USERMODE	(1<<5)	/* USERMODE */
> +#define VSEC_CVP_STATUS_CFG_DONE	(1<<7)	/* CVP_CONFIG_DONE */
> +#define VSEC_CVP_STATUS_PLD_CLK_IN_USE	(1<<8)	/* PLD_CLK_IN_USE */
> +
> +#define VSEC_CVP_MODE_CTRL		(VSEC_OFFSET + 0x20)	/* 32bit */
> +#define VSEC_CVP_MODE_CTRL_CVPMODE	(1<<0) /* CVP (1) or normal mode (0) */
> +#define VSEC_CVP_MODE_CTRL_HIP_CLK_SEL	(1<<1) /* PMA (1) or fabric clock (0) */
> +#define VSEC_CVP_MODE_CTRL_FULL_CFG	(1<<2) /* CVP_FULLCONFIG */
> +#define VSEC_CVP_MODE_CTRL_NUMCLKS	(0xff<<8) /* CVP_NUMCLKS */
> +
> +#define VSEC_CVP_DATA			(VSEC_OFFSET + 0x28)	/* 32bit */
> +#define VSEC_CVP_PROG_CTRL		(VSEC_OFFSET + 0x2c)	/* 32bit */
> +#define VSEC_CVP_PROG_CTRL_CONFIG	(1<<0)
> +#define VSEC_CVP_PROG_CTRL_START_XFER	(1<<1)
> +
> +#define VSEC_UNCOR_ERR_STATUS		(VSEC_OFFSET + 0x34)	/* 32bit */
> +#define VSEC_UNCOR_ERR_MASK		(VSEC_OFFSET + 0x38)	/* 32bit */
> +#define	VSEC_UNCOR_ERR_CVP_CFG_ERR	(1<<5)	/* CVP_CONFIG_ERROR_LATCHED */

Using the BIT macro is preferred to (1<<X)


> +
> +#define	DRV_NAME		"stratix-cvp"
> +#define	FIRMWARE_STR		"stratixv_cvp.rbf"
> +#define STRATIX_V_CVP_MGR_NAME	"Stratix V CvP FPGA Manager"
> +
> +static char *firmware = FIRMWARE_STR;
> +module_param(firmware, charp, 0664);
> +MODULE_PARM_DESC(firmware, "RBF file under /lib/firmware for loading via CvP.");
> +
> +static int chkcfg; /* use value 1 for debugging only */
> +module_param(chkcfg, int, 0664);
> +MODULE_PARM_DESC(chkcfg, "1 - check CvP status, 0 - skip checking (default 0)");
> +
> +struct stratix_cvp_conf {
> +	struct fpga_manager	*mgr;
> +	struct pci_dev		*pci_dev;
> +	void __iomem		*map;
> +	resource_size_t		map_base;
> +	resource_size_t		map_len;
> +};
> +
> +static enum fpga_mgr_states stratix_cvp_state(struct fpga_manager *mgr)
> +{
> +	return mgr->state;
> +}

The state routine usually reads a status register and maps
the value to an appropriate enum fpga_mgr_states.

> +
> +static int stratix_cvp_write_init(struct fpga_manager *mgr,
> +				  struct fpga_image_info *info,
> +				  const char *buf, size_t count)
> +{
> +	struct stratix_cvp_conf *conf = mgr->priv;
> +	struct pci_bus *bus = conf->pci_dev->bus;
> +	struct pci_dev *pdev = conf->pci_dev;
> +	int delay_us, i;
> +	u32 val32;
> +	u16 val16;
> +	u32 data;
> +
> +	if (info && info->flags & FPGA_MGR_PARTIAL_RECONFIG) {
> +		dev_err(&mgr->dev, "Partial reconfiguration not supported.\n");
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * STEP 1 - read CVP status and check CVP_EN flag
> +	 */
> +	pci_bus_read_config_word(bus, FNC, VSEC_CVP_STATUS, &val16);
> +	if (!(val16 & VSEC_CVP_STATUS_CVP_EN)) {
> +		dev_err(&mgr->dev, "CVP mode off: 0x%04x\n", val16);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * STEP 2
> +	 * - set HIP_CLK_SEL and CVP_MODE (must be set in the order mentioned)
> +	 */
> +	/* switch from fabric to PMA clock */
> +	pci_read_config_dword(pdev, VSEC_CVP_MODE_CTRL, &val32);
> +	val32 |= VSEC_CVP_MODE_CTRL_HIP_CLK_SEL;
> +	pci_write_config_dword(pdev, VSEC_CVP_MODE_CTRL, val32);
> +
> +	/* set CVP mode */
> +	pci_read_config_dword(pdev, VSEC_CVP_MODE_CTRL, &val32);
> +	val32 |= VSEC_CVP_MODE_CTRL_CVPMODE;
> +	pci_write_config_dword(pdev, VSEC_CVP_MODE_CTRL, val32);
> +
> +	/*
> +	 * STEP 3
> +	 * - set CVP_NUMCLKS to 1 and issue CVP_DUMMY_WR dummy writes to the HIP
> +	 */
> +	pci_read_config_dword(pdev, VSEC_CVP_MODE_CTRL, &val32);
> +	val32 &= ~VSEC_CVP_MODE_CTRL_NUMCLKS;
> +	val32 |= 0x01 << 8;	/* 1 clock */
> +	pci_write_config_dword(pdev, VSEC_CVP_MODE_CTRL, val32);
> +
> +	/* dummy memory write to switch from internal clock to CVP clock */
> +	val32 = 0xdeadbeef;
> +	if (conf->map) {
> +		u32 *map = conf->map;
> +
> +		for (i = 0; i < CVP_DUMMY_WR; i++)
> +			*map++ = val32; /* use MemoryWrite */
> +	} else {
> +		for (i = 0; i < CVP_DUMMY_WR; i++)
> +			pci_write_config_dword(pdev, VSEC_CVP_DATA, val32);
> +	}
> +
> +	/*
> +	 * STEP 4 - set CVP_CONFIG bit
> +	 */
> +	pci_read_config_dword(pdev, VSEC_CVP_PROG_CTRL, &val32);
> +	/* request control block to begin transfer using CVP */
> +	val32 |= VSEC_CVP_PROG_CTRL_CONFIG;
> +	pci_write_config_dword(pdev, VSEC_CVP_PROG_CTRL, val32);
> +
> +	/*
> +	 * STEP 5 - poll CVP_CONFIG READY
> +	 */
> +	delay_us = 0;
> +	while (1) {
> +		pci_read_config_word(pdev, VSEC_CVP_STATUS, &val16);
> +		if ((val16 & VSEC_CVP_STATUS_CFG_RDY) ==
> +		    VSEC_CVP_STATUS_CFG_RDY)
> +			break;
> +
> +		udelay(1); /* wait 1us */
> +
> +		if (delay_us++ > TIMEOUT_IN_US) {
> +			dev_warn(&mgr->dev, "CVP_CONFIG_READY == 1 timeout\n");
> +			return -ETIMEDOUT;
> +		}
> +	}
> +
> +	/*
> +	 * STEP 6
> +	 * - set CVP_NUMCLKS to 1 and issue CVP_DUMMY_WR dummy writes to the HIP
> +	 */
> +	pci_read_config_dword(pdev, VSEC_CVP_MODE_CTRL, &val32);
> +	val32 &= ~VSEC_CVP_MODE_CTRL_NUMCLKS;
> +	val32 |= 0x01 << 8; /* 1 clock */
> +	pci_write_config_dword(pdev, VSEC_CVP_MODE_CTRL, val32);
> +
> +	/* dummy memory write to switch from internal clock to CVP clock */
> +	val32 = 0xdeadbeef;
> +	if (conf->map) {
> +		u32 *map = conf->map;
> +
> +		for (i = 0; i < CVP_DUMMY_WR; i++)
> +			*map++ = val32; /* use MemoryWrite */
> +	} else {
> +		for (i = 0; i < CVP_DUMMY_WR; i++)
> +			pci_write_config_dword(pdev, VSEC_CVP_DATA, val32);
> +	}
> +
> +	/*
> +	 * STEP 7 - set START_XFER
> +	 */
> +	pci_read_config_dword(pdev, VSEC_CVP_PROG_CTRL, &val32);
> +	val32 |= VSEC_CVP_PROG_CTRL_START_XFER;
> +	pci_write_config_dword(pdev, VSEC_CVP_PROG_CTRL, val32);
> +
> +	/*
> +	 * STEP 8 (start transfer)
> +	 * check required bitstream type - read CVP status and set CVP_NUMCLKS
> +	 * accordingly
> +	 *	Compressed	Encrypted	CVP_NUMCLKS
> +	 *	No		No		1
> +	 *	Yes		No		8
> +	 *	No		Yes		4
> +	 *	Yes		Yes		8
> +	 */
> +	pci_read_config_word(pdev, VSEC_CVP_STATUS, &val16);
> +	val16 &= (VSEC_CVP_STATUS_DATA_ENC | VSEC_CVP_STATUS_DATA_COMP);
> +	switch (val16) {
> +	case VSEC_CVP_STATUS_DATA_ENC:
> +		data = 4;
> +		break;
> +	case VSEC_CVP_STATUS_DATA_COMP:
> +	case (VSEC_CVP_STATUS_DATA_ENC | VSEC_CVP_STATUS_DATA_COMP):
> +		data = 8;
> +		break;
> +	default:
> +		data = 1;
> +		break;
> +	}
> +
> +	/* write number of CVP clock cycles according to bitstream type */
> +	pci_read_config_dword(pdev, VSEC_CVP_MODE_CTRL, &val32);
> +	val32 &= ~VSEC_CVP_MODE_CTRL_NUMCLKS;
> +	val32 |= data << 8;	/* bitstream specific clock count */
> +	pci_write_config_dword(pdev, VSEC_CVP_MODE_CTRL, val32);
> +
> +	return 0;
> +}
> +
> +static inline int stratix_cvp_chk_cfg_error(struct fpga_manager *mgr,
> +					    size_t bytes)
> +{
> +	struct stratix_cvp_conf *conf = mgr->priv;
> +	u16 val16;
> +
> +	/*
> +	 * STEP 10 (optional) - check CVP_CONFIG_ERROR flag
> +	 */
> +	pci_read_config_word(conf->pci_dev, VSEC_CVP_STATUS, &val16);
> +	if (val16 & VSEC_CVP_STATUS_CFG_ERR) {
> +		dev_err(&mgr->dev, " CVP_CONFIG_ERROR after %d bytes!\n",
> +			bytes);
> +		return -EPROTO;
> +	}
> +	return 0;
> +}
> +
> +static int stratix_cvp_write(struct fpga_manager *mgr, const char *buf,
> +			     size_t count)
> +{
> +	struct stratix_cvp_conf *conf = mgr->priv;
> +	struct pci_dev *pdev = conf->pci_dev;
> +	const u32 *data, *data_end;
> +	size_t bytes = 0;
> +	int status = 0;
> +
> +	/*
> +	 * STEP 9
> +	 * - write 32-bit configuration data from POF file to CVP data register
> +	 */
> +	data = (u32 *)buf;
> +	data_end = (u32 *)((char *)buf + count);
> +
> +	if (conf->map) {
> +		u32 *map = conf->map;
> +
> +		while (data < data_end) {
> +			*map = *data++;
> +			bytes += 4;
> +
> +			/*
> +			 * STEP 10 (optional) and STEP 11
> +			 * - check error flag
> +			 * - loop until data transfer completed
> +			 */
> +			if (chkcfg) {
> +				status = stratix_cvp_chk_cfg_error(mgr, bytes);
> +				if (status < 0)
> +					break;
> +			}
> +		}
> +		return status;
> +	}
> +
> +	while (data < data_end) {
> +		pci_write_config_dword(pdev, VSEC_CVP_DATA, *data++);
> +		bytes += 4;
> +
> +		/*
> +		 * STEP 10 (optional) and STEP 11
> +		 * - check error flag
> +		 * - loop until data transfer completed
> +		 */
> +		if (chkcfg) {
> +			status = stratix_cvp_chk_cfg_error(mgr, bytes);
> +			if (status < 0)
> +				break;
> +		}
> +	}
> +
> +	return status;
> +}
> +
> +static int stratix_cvp_write_complete(struct fpga_manager *mgr,
> +				      struct fpga_image_info *info)
> +{
> +	struct stratix_cvp_conf *conf = mgr->priv;
> +	struct pci_dev *pdev = conf->pci_dev;
> +	int status = 0;
> +	int delay_us, i;
> +	u32 val32;
> +	u16 status_msk;
> +	u16 val16;
> +
> +	/*
> +	 * STEP 12 - reset START_XFER bit
> +	 */
> +	pci_read_config_dword(pdev, VSEC_CVP_PROG_CTRL, &val32);
> +	val32 &= ~VSEC_CVP_PROG_CTRL_START_XFER;
> +	pci_write_config_dword(pdev, VSEC_CVP_PROG_CTRL, val32);
> +
> +	/*
> +	 * STEP 13 - reset CVP_CONFIG bit
> +	 */
> +	val32 &= ~VSEC_CVP_PROG_CTRL_CONFIG;
> +	pci_write_config_dword(pdev, VSEC_CVP_PROG_CTRL, val32);
> +
> +	/*
> +	 * STEP 14
> +	 * - set CVP_NUMCLKS to 0x01 and then issue CVP_DUMMY_WR dummy
> +	 *   writes to the HIP
> +	 */
> +	pci_read_config_dword(pdev, VSEC_CVP_MODE_CTRL, &val32);
> +	val32 &= ~VSEC_CVP_MODE_CTRL_NUMCLKS;
> +	/* set number of CVP clock cycles for every CVP Data Register Write */
> +	val32 |= 0x01 << 8;	/* 1 clock */
> +	pci_write_config_dword(pdev, VSEC_CVP_MODE_CTRL, val32);
> +
> +	/* dummy memory write to switch from CVP clock to internal clock */
> +	val32 = 0xdeadbeef;
> +	if (conf->map) {
> +		u32 *map = conf->map;
> +
> +		for (i = 0; i < CVP_DUMMY_WR; i++)
> +			*map++ = val32; /* use MemoryWrite */
> +	} else {
> +		for (i = 0; i < CVP_DUMMY_WR; i++)
> +			pci_write_config_dword(pdev, VSEC_CVP_DATA, val32);
> +	}
> +
> +	/*
> +	 * STEP 15 - poll CVP_CONFIG_READY bit
> +	 */
> +	delay_us = 0;
> +	while (1) {
> +		pci_read_config_word(pdev, VSEC_CVP_STATUS, &val16);
> +		if ((val16 & VSEC_CVP_STATUS_CFG_RDY) == 0)
> +			break;
> +
> +		udelay(1);	/* wait 1us */
> +
> +		if (delay_us++ > TIMEOUT_IN_US) {

Alan Tull has a patch where this timeout is passed in as part of the 
struct fpga_image_info.  Would that help here?

> +			dev_warn(&mgr->dev, "CVP_CONFIG_READY == 0 timeout\n");
> +			status = -ETIME;
> +			break;
> +		}
> +	}
> +
> +	if (status < 0)
> +		return status;
> +
> +	/*
> +	 * STEP 16 - check CVP_CONFIG_ERROR_LATCHED bit
> +	 */
> +	pci_read_config_dword(pdev, VSEC_UNCOR_ERR_STATUS, &val32);
> +	if (val32 & VSEC_UNCOR_ERR_CVP_CFG_ERR) {
> +		dev_err(&mgr->dev, " detected CVP_CONFIG_ERROR_LATCHED!\n");
> +		return -EFAULT;
> +	}
> +
> +	/*
> +	 * STEP 17 - reset CVP_MODE and HIP_CLK_SEL bit
> +	 */
> +	pci_read_config_dword(pdev, VSEC_CVP_MODE_CTRL, &val32);
> +	val32 &= ~VSEC_CVP_MODE_CTRL_HIP_CLK_SEL;
> +	val32 &= ~VSEC_CVP_MODE_CTRL_CVPMODE;
> +	pci_write_config_dword(pdev, VSEC_CVP_MODE_CTRL, val32);
> +
> +	/*
> +	 * STEP 18 - poll PLD_CLK_IN_USE and USER_MODE bits
> +	 */
> +	delay_us = 0;
> +	status_msk = VSEC_CVP_STATUS_PLD_CLK_IN_USE | VSEC_CVP_STATUS_USERMODE;
> +	while (1) {
> +		pci_read_config_word(pdev, VSEC_CVP_STATUS, &val16);
> +		if ((val16 & status_msk) == status_msk)
> +			break;
> +
> +		udelay(1); /* wait 1us */
> +
> +		if (delay_us++ > TIMEOUT_IN_US) {
> +			dev_warn(&mgr->dev, "PLD_CLK_IN_USE|USERMODE timeout\n");
> +			status = -ETIME;
> +			break;
> +		}
> +	}
> +
> +	return status;
> +}
> +
> +static const struct fpga_manager_ops stratix_cvp_ops = {
> +	.state		= stratix_cvp_state,
> +	.write_init	= stratix_cvp_write_init,
> +	.write		= stratix_cvp_write,
> +	.write_complete	= stratix_cvp_write_complete,
> +};
> +
> +static int stratix_cvp_probe(struct pci_dev *pdev,
> +			     const struct pci_device_id *dev_id)
> +{
> +	struct stratix_cvp_conf *conf;
> +	u16 cmd, val16;
> +	int ret;
> +
> +	/* Enable memory BAR access */
> +	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> +	if (!(cmd & PCI_COMMAND_MEMORY)) {
> +		cmd |= PCI_COMMAND_MEMORY;
> +		pci_write_config_word(pdev, PCI_COMMAND, cmd);
> +	}
> +
> +	conf = devm_kzalloc(&pdev->dev, sizeof(*conf), GFP_KERNEL);
> +	if (!conf) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +
> +	pci_bus_read_config_word(pdev->bus, FNC, VSEC_PCIE_EXT_CAP_ID, &val16);
> +	if (val16 != VSEC_PCIE_EXT_CAP_ID_VAL) {
> +		dev_err(&pdev->dev, "Wrong EXT_CAP_ID value 0x%x\n", val16);
> +		ret = -ENODEV;
> +		goto err;
> +	}
> +
> +	conf->pci_dev = pdev;
> +	conf->map_base = pci_resource_start(pdev, CVP_BAR);
> +	conf->map_len = pci_resource_len(pdev, CVP_BAR);
> +
> +	if (pci_request_region(pdev, CVP_BAR, "CVP") < 0) {
> +		dev_err(&pdev->dev, "Requesting CVP BAR region failed\n");
> +		ret = -ENODEV;
> +		goto err;
> +	}
> +
> +	conf->map = ioremap_nocache(conf->map_base, conf->map_len);
> +	if (!conf->map)
> +		dev_warn(&pdev->dev, "Mapping CVP BAR failed\n");
> +
> +	ret = fpga_mgr_register(&pdev->dev, STRATIX_V_CVP_MGR_NAME,
> +				&stratix_cvp_ops, conf);

I believe there may be a problem using a fixed name when there are more 
than one of the cards plugged into the system.  A unique name may 
be required here.

> +	if (ret)
> +		goto err_unmap;
> +
> +	conf->mgr = fpga_mgr_get(&pdev->dev);
> +	if (IS_ERR(conf->mgr)) {
> +		dev_err(&pdev->dev, "Getting fpga mgr reference failed\n");
> +		ret = -ENODEV;
> +		goto err_unmap;
> +	}
> +
> +	ret = fpga_mgr_firmware_load(conf->mgr, NULL, firmware);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to load fpga image\n");
> +		goto err_mgr;
> +	}
> +
> +	return 0;
> +
> +err_mgr:
> +	fpga_mgr_put(conf->mgr);
> +	fpga_mgr_unregister(&pdev->dev);
> +err_unmap:
> +	iounmap(conf->map);
> +	pci_release_region(pdev, CVP_BAR);
> +err:
> +	cmd &= ~PCI_COMMAND_MEMORY;
> +	pci_write_config_word(pdev, PCI_COMMAND, cmd);
> +	return ret;
> +}
> +
> +static void stratix_cvp_remove(struct pci_dev *pdev)
> +{
> +	struct fpga_manager *mgr = pci_get_drvdata(pdev);
> +	struct stratix_cvp_conf *conf = mgr->priv;
> +	u16 cmd;
> +
> +	fpga_mgr_put(conf->mgr);
> +	fpga_mgr_unregister(&pdev->dev);
> +	iounmap(conf->map);
> +	pci_release_region(pdev, CVP_BAR);
> +	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> +	cmd &= ~PCI_COMMAND_MEMORY;
> +	pci_write_config_word(pdev, PCI_COMMAND, cmd);
> +}
> +
> +#define PCI_VENDOR_ID_ALTERA	0x1172
> +#define PCI_DEVICE_ID_FPGA	0xe001
> +
> +static struct pci_device_id stratix_cvp_id_tbl[] = {
> +	{ PCI_VDEVICE(ALTERA, PCI_DEVICE_ID_FPGA) },
> +	{ }
> +};
> +
> +static struct pci_driver stratix_cvp_driver = {
> +	.name   = DRV_NAME,
> +	.id_table = stratix_cvp_id_tbl,
> +	.probe  = stratix_cvp_probe,
> +	.remove = stratix_cvp_remove,
> +};
> +
> +module_pci_driver(stratix_cvp_driver)
> +
> +MODULE_DEVICE_TABLE(pci, stratix_cvp_id_tbl);
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Anatolij Gustschin <[e-mail]>");
> +MODULE_DESCRIPTION("Module to load Altera FPGA over CvP");
> -- 
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Anatolij Gustschin Feb. 14, 2017, 5:07 p.m. UTC | #2

Hi Matthew,

On Mon, 13 Feb 2017 16:49:24 -0800 (PST)
[e-mail] [e-mail] wrote:

>Hi Anatolij,
>
>
>
>On Mon, 13 Feb 2017, Anatolij Gustschin wrote:
>
>> Add FPGA manager driver for loading Stratix V FPGA using CvP.  
>
>Are you envisioning a flow where this module loads the fpga, and
>a separate module would actually make use of the fully configured
>fpga?

yes, this should work. I can trigger the unbinding in the CvP driver
and then bind another driver module for configured fpga, e.g.:

  echo 0000:06:00.0  > /sys/bus/pci/drivers/stratix-cvp/unbind

This will detach the fpga manager. Now I can bind another driver:

  echo 0000:06:00.0  > /sys/bus/pci/drivers/fpga_test/bind

And for a new configuration just trigger the unbinding by

  echo 0000:06:00.0  > /sys/bus/pci/drivers/fpga_test/unbind

and bind the fpga manager again:

  echo 0000:06:00.0  > /sys/bus/pci/drivers/stratix-cvp/bind


>Do happen to have a similar driver for the Cyclone5 FPGA using CvP
>in the works?

No, currently there is no plan vor Cyclone5 CvP.

...
>> +#define VSEC_UNCOR_ERR_STATUS		(VSEC_OFFSET + 0x34)	/* 32bit */
>> +#define VSEC_UNCOR_ERR_MASK		(VSEC_OFFSET + 0x38)	/* 32bit */
>> +#define	VSEC_UNCOR_ERR_CVP_CFG_ERR	(1<<5)	/* CVP_CONFIG_ERROR_LATCHED */  
>
>Using the BIT macro is preferred to (1<<X)

Okay, I'll fix it.

...
>> +static enum fpga_mgr_states stratix_cvp_state(struct fpga_manager *mgr)
>> +{
>> +	return mgr->state;
>> +}  
>
>The state routine usually reads a status register and maps
>the value to an appropriate enum fpga_mgr_states.

Okay, I'll rework here.

...
>> +	delay_us = 0;
>> +	while (1) {
>> +		pci_read_config_word(pdev, VSEC_CVP_STATUS, &val16);
>> +		if ((val16 & VSEC_CVP_STATUS_CFG_RDY) == 0)
>> +			break;
>> +
>> +		udelay(1);	/* wait 1us */
>> +
>> +		if (delay_us++ > TIMEOUT_IN_US) {  
>
>Alan Tull has a patch where this timeout is passed in as part of the 
>struct fpga_image_info.  Would that help here?

yes, this makes sense.

...
>> +	ret = fpga_mgr_register(&pdev->dev, STRATIX_V_CVP_MGR_NAME,
>> +				&stratix_cvp_ops, conf);  
>
>I believe there may be a problem using a fixed name when there are more 
>than one of the cards plugged into the system.  A unique name may 
>be required here.

Good point, I already thought about this, but I'm not sure what is the
best way to set a unique name here.

Thanks for review and comments!

Best regards,

Anatolij
--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Alan Tull Feb. 14, 2017, 5:38 p.m. UTC | #3

On Mon, Feb 13, 2017 at 1:17 PM, Anatolij Gustschin <[e-mail]> wrote:
> Add FPGA manager driver for loading Stratix V FPGA using CvP.

Hi Anatolij,

> +
> +static enum fpga_mgr_states stratix_cvp_state(struct fpga_manager *mgr)
> +{
> +       return mgr->state;
> +}

fpga_mgr_register calls this function to initialize mgr->state.  So
this function needs to either figure out the state of the part or
return FPGA_MGR_STATE_UNKNOWN.

> +
> +#define PCI_VENDOR_ID_ALTERA   0x1172
> +#define PCI_DEVICE_ID_FPGA     0xe001
> +

The DID may be different, so we probably need some way of passing it in.

We were working on a Cyclone V CVP driver; it may be quite similar, so
once we've had more time to look at this, perhaps we can have one
driver for both.

I'll spend some more time looking at your patches.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Li, Yi Feb. 14, 2017, 7:38 p.m. UTC | #4

On 2/13/2017 6:49 PM, [e-mail] wrote:
>
> Hi Anatolij,
>
>
>
> On Mon, 13 Feb 2017, Anatolij Gustschin wrote:
>
>> Add FPGA manager driver for loading Stratix V FPGA using CvP.
>
> Are you envisioning a flow where this module loads the fpga, and
> a separate module would actually make use of the fully configured
> fpga?
>
> Do happen to have a similar driver for the Cyclone5 FPGA using CvP
> in the works?
>
Hi Anatolij

I am currently working on the Cyclone V CvP. Based on the User Guild doc 
UG-01101
https://www.altera.com/en_US/pdfs/literature/ug/ug_cvp.pdf, all the V 
series
should have the same CvP flow. If that's the case, we might want to 
change the driver/function
name to altera_v_xxx and re-use it for all V series.  I will test your 
patch on Cyclone V.
Some comments below:
>>
>> Signed-off-by: Anatolij Gustschin <[e-mail]>
>> ---
>> drivers/fpga/Kconfig       |   7 +
>> drivers/fpga/Makefile      |   1 +
>> drivers/fpga/stratix-cvp.c | 529 
>> +++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 537 insertions(+)
>> create mode 100644 drivers/fpga/stratix-cvp.c
>>
>> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
>> index ce861a2..7ef5b06 100644
>> --- a/drivers/fpga/Kconfig
>> +++ b/drivers/fpga/Kconfig
>> @@ -33,6 +33,13 @@ config FPGA_MGR_SOCFPGA_A10
>>     help
>>       FPGA manager driver support for Altera Arria10 SoCFPGA.
>>
>> +config FPGA_MGR_STRATIX_CVP
>> +    tristate "Altera Stratix V CvP"
>> +    depends on PCI
>> +    help
>> +      FPGA manager driver support for Altera Stratix V using the
>> +      CvP interface over PCIe
>> +
>> config FPGA_MGR_ZYNQ_FPGA
>>     tristate "Xilinx Zynq FPGA"
>>     depends on ARCH_ZYNQ || COMPILE_TEST
>> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
>> index 8df07bc..0444c78 100644
>> --- a/drivers/fpga/Makefile
>> +++ b/drivers/fpga/Makefile
>> @@ -8,6 +8,7 @@ obj-$(CONFIG_FPGA)            += fpga-mgr.o
>> # FPGA Manager Drivers
>> obj-$(CONFIG_FPGA_MGR_SOCFPGA)        += socfpga.o
>> obj-$(CONFIG_FPGA_MGR_SOCFPGA_A10)    += socfpga-a10.o
>> +obj-$(CONFIG_FPGA_MGR_STRATIX_CVP)    += stratix-cvp.o
>> obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA)    += zynq-fpga.o
>>
>> # FPGA Bridge Drivers
>> diff --git a/drivers/fpga/stratix-cvp.c b/drivers/fpga/stratix-cvp.c
>> new file mode 100644
>> index 0000000..5ab3499
>> --- /dev/null
>> +++ b/drivers/fpga/stratix-cvp.c
>> @@ -0,0 +1,529 @@
>> +/*
>> + * Copyright (C) 2017 DENX Software Engineering
>> + *
>> + * Anatolij Gustschin <[e-mail]>
>> + *
>> + * Manage Altera Statix V FPGA firmware using PCIe CvP.
>> + * Firmware must be in binary "rbf" format.
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/fpga/fpga-mgr.h>
>> +#include <linux/module.h>
>> +#include <linux/sizes.h>
>> +#include <linux/pci.h>
>> +
>> +/* Vendor Specific Extended Capability Offset */
>> +#define VSEC_OFFSET            0x200
>> +#define VSEC_PCIE_EXT_CAP_ID_VAL    0x000b
>> +
>> +#define FNC        0    /* PCIe device function ID */
>> +#define CVP_BAR        0    /* BAR used for data transfer in memory 
>> mode */
Do we know the BAR and FNC is always 0?
>> +#define CVP_DUMMY_WR    244    /* dummy writes to clear CvP state 
>> machine */
>> +#define TIMEOUT_IN_US    10000
>> +
>> +/* offsets from VSEC_OFFSET */
>> +#define VSEC_PCIE_EXT_CAP_ID        (VSEC_OFFSET + 0x00)    /* 16bit */
>> +#define VSEC_VERSION            0x02        /* 8bit */
>> +#define VSEC_NEXT_CAP_OFF        0x03        /* 8bit */
>> +#define VSEC_ID                0x04        /* 16bit */
>> +#define VSEC_REV            0x06        /* 8bit */
>> +#define VSEC_LENGTH            0x07        /* 8bit */
>> +#define VSEC_ALTERA_MARKER        0x08        /* 32bit */
>> +
>> +#define VSEC_CVP_STATUS            (VSEC_OFFSET + 0x1e)    /* 16bit */
>> +#define VSEC_CVP_STATUS_DATA_ENC    (1<<0)    /* is treated as 
>> encrypted */
>> +#define VSEC_CVP_STATUS_DATA_COMP    (1<<1)    /* is treated as 
>> compressed */
According to the UG-01101,the CVP_STATUS register is a 32 bits register 
offset at 21C, the bit DATA_ENC and DATA_COMP
do not exist (bit 0-17 reserved).
>> +#define VSEC_CVP_STATUS_CFG_RDY (1<<2)    /* CVP_CONFIG_READY */
>> +#define VSEC_CVP_STATUS_CFG_ERR        (1<<3)    /* CVP_CONFIG_ERROR */
>> +#define VSEC_CVP_STATUS_CVP_EN        (1<<4)    /* ctrl block is 
>> enabling CVP */
>> +#define VSEC_CVP_STATUS_USERMODE    (1<<5)    /* USERMODE */
>> +#define VSEC_CVP_STATUS_CFG_DONE    (1<<7)    /* CVP_CONFIG_DONE */
>> +#define VSEC_CVP_STATUS_PLD_CLK_IN_USE    (1<<8)    /* 
>> PLD_CLK_IN_USE */
>> +
>> +#define VSEC_CVP_MODE_CTRL        (VSEC_OFFSET + 0x20)    /* 32bit */
>> +#define VSEC_CVP_MODE_CTRL_CVPMODE    (1<<0) /* CVP (1) or normal 
>> mode (0) */
>> +#define VSEC_CVP_MODE_CTRL_HIP_CLK_SEL    (1<<1) /* PMA (1) or 
>> fabric clock (0) */
>> +#define VSEC_CVP_MODE_CTRL_FULL_CFG    (1<<2) /* CVP_FULLCONFIG */
>> +#define VSEC_CVP_MODE_CTRL_NUMCLKS    (0xff<<8) /* CVP_NUMCLKS */
>> +
>> +#define VSEC_CVP_DATA            (VSEC_OFFSET + 0x28)    /* 32bit */
>> +#define VSEC_CVP_PROG_CTRL        (VSEC_OFFSET + 0x2c)    /* 32bit */
>> +#define VSEC_CVP_PROG_CTRL_CONFIG    (1<<0)
>> +#define VSEC_CVP_PROG_CTRL_START_XFER    (1<<1)
>> +
>> +#define VSEC_UNCOR_ERR_STATUS        (VSEC_OFFSET + 0x34)    /* 
>> 32bit */
>> +#define VSEC_UNCOR_ERR_MASK        (VSEC_OFFSET + 0x38)    /* 32bit */
>> +#define    VSEC_UNCOR_ERR_CVP_CFG_ERR    (1<<5)    /* 
>> CVP_CONFIG_ERROR_LATCHED */
>
> Using the BIT macro is preferred to (1<<X)
>
>
>> +
>> +#define    DRV_NAME        "stratix-cvp"
>> +#define    FIRMWARE_STR        "stratixv_cvp.rbf"
>> +#define STRATIX_V_CVP_MGR_NAME    "Stratix V CvP FPGA Manager"
>> +
>> +static char *firmware = FIRMWARE_STR;
>> +module_param(firmware, charp, 0664);
>> +MODULE_PARM_DESC(firmware, "RBF file under /lib/firmware for loading 
>> via CvP.");
>> +
>> +static int chkcfg; /* use value 1 for debugging only */
>> +module_param(chkcfg, int, 0664);
>> +MODULE_PARM_DESC(chkcfg, "1 - check CvP status, 0 - skip checking 
>> (default 0)");
>> +
>> +struct stratix_cvp_conf {
>> +    struct fpga_manager    *mgr;
>> +    struct pci_dev        *pci_dev;
>> +    void __iomem        *map;
>> +    resource_size_t        map_base;
>> +    resource_size_t        map_len;
>> +};
>> +
>> +static enum fpga_mgr_states stratix_cvp_state(struct fpga_manager *mgr)
>> +{
>> +    return mgr->state;
>> +}
>
> The state routine usually reads a status register and maps
> the value to an appropriate enum fpga_mgr_states.
>
>> +
>> +static int stratix_cvp_write_init(struct fpga_manager *mgr,
>> +                  struct fpga_image_info *info,
>> +                  const char *buf, size_t count)
>> +{
>> +    struct stratix_cvp_conf *conf = mgr->priv;
>> +    struct pci_bus *bus = conf->pci_dev->bus;
>> +    struct pci_dev *pdev = conf->pci_dev;
>> +    int delay_us, i;
>> +    u32 val32;
>> +    u16 val16;
>> +    u32 data;
>> +
>> +    if (info && info->flags & FPGA_MGR_PARTIAL_RECONFIG) {
>> +        dev_err(&mgr->dev, "Partial reconfiguration not supported.\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    /*
>> +     * STEP 1 - read CVP status and check CVP_EN flag
>> +     */
>> +    pci_bus_read_config_word(bus, FNC, VSEC_CVP_STATUS, &val16);
>> +    if (!(val16 & VSEC_CVP_STATUS_CVP_EN)) {
>> +        dev_err(&mgr->dev, "CVP mode off: 0x%04x\n", val16);
>> +        return -EINVAL;
>> +    }
We might want to check the CONFIG_READY bit here, if it's set by BIOS 
(yes, I saw it happening on some boards) or left unclean by
previous programming, we need to tear it down first (started with your 
step 12).
Something like below:
     if (val &VSEC_CVP_STATUS_CFG_RDY ) {
         dev_info(dev, "Bios already start the CvP, tear it down first\n");
         altera_v_cvp_fpga_teardown(mgr, info);
     }
altera_v_cvp_fpga_teardown function can be re-used by cvp_write_complete 
function and this init one.

>> +
>> +    /*
>> +     * STEP 2
>> +     * - set HIP_CLK_SEL and CVP_MODE (must be set in the order 
>> mentioned)
>> +     */
>> +    /* switch from fabric to PMA clock */
>> +    pci_read_config_dword(pdev, VSEC_CVP_MODE_CTRL, &val32);
>> +    val32 |= VSEC_CVP_MODE_CTRL_HIP_CLK_SEL;
>> +    pci_write_config_dword(pdev, VSEC_CVP_MODE_CTRL, val32);
>> +
>> +    /* set CVP mode */
>> +    pci_read_config_dword(pdev, VSEC_CVP_MODE_CTRL, &val32);
>> +    val32 |= VSEC_CVP_MODE_CTRL_CVPMODE;
>> +    pci_write_config_dword(pdev, VSEC_CVP_MODE_CTRL, val32);
>> +
>> +    /*
>> +     * STEP 3
>> +     * - set CVP_NUMCLKS to 1 and issue CVP_DUMMY_WR dummy writes to 
>> the HIP
>> +     */
>> +    pci_read_config_dword(pdev, VSEC_CVP_MODE_CTRL, &val32);
>> +    val32 &= ~VSEC_CVP_MODE_CTRL_NUMCLKS;
>> +    val32 |= 0x01 << 8;    /* 1 clock */
>> +    pci_write_config_dword(pdev, VSEC_CVP_MODE_CTRL, val32);
>> +
>> +    /* dummy memory write to switch from internal clock to CVP clock */
>> +    val32 = 0xdeadbeef;
>> +    if (conf->map) {
>> +        u32 *map = conf->map;
>> +
>> +        for (i = 0; i < CVP_DUMMY_WR; i++)
>> +            *map++ = val32; /* use MemoryWrite */
>> +    } else {
>> +        for (i = 0; i < CVP_DUMMY_WR; i++)
>> +            pci_write_config_dword(pdev, VSEC_CVP_DATA, val32);
>> +    }
>> +
>> +    /*
>> +     * STEP 4 - set CVP_CONFIG bit
>> +     */
>> +    pci_read_config_dword(pdev, VSEC_CVP_PROG_CTRL, &val32);
>> +    /* request control block to begin transfer using CVP */
>> +    val32 |= VSEC_CVP_PROG_CTRL_CONFIG;
>> +    pci_write_config_dword(pdev, VSEC_CVP_PROG_CTRL, val32);
>> +
>> +    /*
>> +     * STEP 5 - poll CVP_CONFIG READY
>> +     */
>> +    delay_us = 0;
>> +    while (1) {
>> +        pci_read_config_word(pdev, VSEC_CVP_STATUS, &val16);
Again the CVP_STATUS register is 32 bits instead of 16 bit.
>> +        if ((val16 & VSEC_CVP_STATUS_CFG_RDY) ==
>> +            VSEC_CVP_STATUS_CFG_RDY)
>> +            break;
>> +
>> +        udelay(1); /* wait 1us */
>> +
>> +        if (delay_us++ > TIMEOUT_IN_US) {
>> +            dev_warn(&mgr->dev, "CVP_CONFIG_READY == 1 timeout\n");
>> +            return -ETIMEDOUT;
>> +        }
>> +    }
>> +
>> +    /*
>> +     * STEP 6
>> +     * - set CVP_NUMCLKS to 1 and issue CVP_DUMMY_WR dummy writes to 
>> the HIP
>> +     */
>> +    pci_read_config_dword(pdev, VSEC_CVP_MODE_CTRL, &val32);
>> +    val32 &= ~VSEC_CVP_MODE_CTRL_NUMCLKS;
>> +    val32 |= 0x01 << 8; /* 1 clock */
>> +    pci_write_config_dword(pdev, VSEC_CVP_MODE_CTRL, val32);
>> +
>> +    /* dummy memory write to switch from internal clock to CVP clock */
>> +    val32 = 0xdeadbeef;
>> +    if (conf->map) {
>> +        u32 *map = conf->map;
>> +
>> +        for (i = 0; i < CVP_DUMMY_WR; i++)
>> +            *map++ = val32; /* use MemoryWrite */
>> +    } else {
>> +        for (i = 0; i < CVP_DUMMY_WR; i++)
>> +            pci_write_config_dword(pdev, VSEC_CVP_DATA, val32);
>> +    }
>> +
>> +    /*
>> +     * STEP 7 - set START_XFER
>> +     */
>> +    pci_read_config_dword(pdev, VSEC_CVP_PROG_CTRL, &val32);
>> +    val32 |= VSEC_CVP_PROG_CTRL_START_XFER;
>> +    pci_write_config_dword(pdev, VSEC_CVP_PROG_CTRL, val32);
>> +
>> +    /*
>> +     * STEP 8 (start transfer)
>> +     * check required bitstream type - read CVP status and set 
>> CVP_NUMCLKS
>> +     * accordingly
>> +     *    Compressed    Encrypted    CVP_NUMCLKS
>> +     *    No        No        1
>> +     *    Yes        No        8
>> +     *    No        Yes        4
>> +     *    Yes        Yes        8
>> +     */
>> +    pci_read_config_word(pdev, VSEC_CVP_STATUS, &val16);
>> +    val16 &= (VSEC_CVP_STATUS_DATA_ENC | VSEC_CVP_STATUS_DATA_COMP);
>> +    switch (val16) {
>> +    case VSEC_CVP_STATUS_DATA_ENC:
>> +        data = 4;
>> +        break;
>> +    case VSEC_CVP_STATUS_DATA_COMP:
>> +    case (VSEC_CVP_STATUS_DATA_ENC | VSEC_CVP_STATUS_DATA_COMP):
>> +        data = 8;
>> +        break;
>> +    default:
>> +        data = 1;
>> +        break;
>> +    }
My understanding is we need to set the NUMCLKS base on RBF stream format 
instead of polling
the non-existed VSEC_CVP_STATUS_DATA_ENC and VSEC_CVP_STATUS_DATA_COMP bits.
>> +
>> +    /* write number of CVP clock cycles according to bitstream type */
>> +    pci_read_config_dword(pdev, VSEC_CVP_MODE_CTRL, &val32);
>> +    val32 &= ~VSEC_CVP_MODE_CTRL_NUMCLKS;
>> +    val32 |= data << 8;    /* bitstream specific clock count */
>> +    pci_write_config_dword(pdev, VSEC_CVP_MODE_CTRL, val32);
>> +
>> +    return 0;
>> +}
>> +
>> +static inline int stratix_cvp_chk_cfg_error(struct fpga_manager *mgr,
>> +                        size_t bytes)
>> +{
>> +    struct stratix_cvp_conf *conf = mgr->priv;
>> +    u16 val16;
>> +
>> +    /*
>> +     * STEP 10 (optional) - check CVP_CONFIG_ERROR flag
>> +     */
>> +    pci_read_config_word(conf->pci_dev, VSEC_CVP_STATUS, &val16);
>> +    if (val16 & VSEC_CVP_STATUS_CFG_ERR) {
>> +        dev_err(&mgr->dev, " CVP_CONFIG_ERROR after %d bytes!\n",
>> +            bytes);
>> +        return -EPROTO;
>> +    }
>> +    return 0;
>> +}
>> +
>> +static int stratix_cvp_write(struct fpga_manager *mgr, const char *buf,
>> +                 size_t count)
>> +{
>> +    struct stratix_cvp_conf *conf = mgr->priv;
>> +    struct pci_dev *pdev = conf->pci_dev;
>> +    const u32 *data, *data_end;
>> +    size_t bytes = 0;
>> +    int status = 0;
>> +
>> +    /*
>> +     * STEP 9
>> +     * - write 32-bit configuration data from POF file to CVP data 
>> register
>> +     */
>> +    data = (u32 *)buf;
>> +    data_end = (u32 *)((char *)buf + count);
>> +
What happens if the length of the bit stream is not dword (4 bytes) 
aligned? We need to pad the un-aligned stream
at the end.
>> +    if (conf->map) {
>> +        u32 *map = conf->map;
>> +
>> +        while (data < data_end) {
>> +            *map = *data++;
>> +            bytes += 4;
>> +
>> +            /*
>> +             * STEP 10 (optional) and STEP 11
>> +             * - check error flag
>> +             * - loop until data transfer completed
>> +             */
>> +            if (chkcfg) {
>> +                status = stratix_cvp_chk_cfg_error(mgr, bytes);
>> +                if (status < 0)
>> +                    break;
>> +            }
This might be too heavy to check every dword writing, since the error 
status bit will persist once error detected, we
might want to check periodically, say every 1K dword?.
>> +        }
>> +        return status;
>> +    }
>> +
>> +    while (data < data_end) {
>> +        pci_write_config_dword(pdev, VSEC_CVP_DATA, *data++);
>> +        bytes += 4;
>> +
>> +        /*
>> +         * STEP 10 (optional) and STEP 11
>> +         * - check error flag
>> +         * - loop until data transfer completed
>> +         */
>> +        if (chkcfg) {
>> +            status = stratix_cvp_chk_cfg_error(mgr, bytes);
>> +            if (status < 0)
>> +                break;
>> +        }
>> +    }
>> +
>> +    return status;
>> +}
>> +
>> +static int stratix_cvp_write_complete(struct fpga_manager *mgr,
>> +                      struct fpga_image_info *info)
>> +{
>> +    struct stratix_cvp_conf *conf = mgr->priv;
>> +    struct pci_dev *pdev = conf->pci_dev;
>> +    int status = 0;
>> +    int delay_us, i;
>> +    u32 val32;
>> +    u16 status_msk;
>> +    u16 val16;
>> +
>> +    /*
>> +     * STEP 12 - reset START_XFER bit
>> +     */
>> +    pci_read_config_dword(pdev, VSEC_CVP_PROG_CTRL, &val32);
>> +    val32 &= ~VSEC_CVP_PROG_CTRL_START_XFER;
>> +    pci_write_config_dword(pdev, VSEC_CVP_PROG_CTRL, val32);
>> +
>> +    /*
>> +     * STEP 13 - reset CVP_CONFIG bit
>> +     */
>> +    val32 &= ~VSEC_CVP_PROG_CTRL_CONFIG;
>> +    pci_write_config_dword(pdev, VSEC_CVP_PROG_CTRL, val32);
>> +
>> +    /*
>> +     * STEP 14
>> +     * - set CVP_NUMCLKS to 0x01 and then issue CVP_DUMMY_WR dummy
>> +     *   writes to the HIP
>> +     */
>> +    pci_read_config_dword(pdev, VSEC_CVP_MODE_CTRL, &val32);
>> +    val32 &= ~VSEC_CVP_MODE_CTRL_NUMCLKS;
>> +    /* set number of CVP clock cycles for every CVP Data Register 
>> Write */
>> +    val32 |= 0x01 << 8;    /* 1 clock */
>> +    pci_write_config_dword(pdev, VSEC_CVP_MODE_CTRL, val32);
>> +
>> +    /* dummy memory write to switch from CVP clock to internal clock */
>> +    val32 = 0xdeadbeef;
>> +    if (conf->map) {
>> +        u32 *map = conf->map;
>> +
>> +        for (i = 0; i < CVP_DUMMY_WR; i++)
>> +            *map++ = val32; /* use MemoryWrite */
>> +    } else {
>> +        for (i = 0; i < CVP_DUMMY_WR; i++)
>> +            pci_write_config_dword(pdev, VSEC_CVP_DATA, val32);
>> +    }
>> +
>> +    /*
>> +     * STEP 15 - poll CVP_CONFIG_READY bit
>> +     */
>> +    delay_us = 0;
>> +    while (1) {
>> +        pci_read_config_word(pdev, VSEC_CVP_STATUS, &val16);
>> +        if ((val16 & VSEC_CVP_STATUS_CFG_RDY) == 0)
>> +            break;
>> +
>> +        udelay(1);    /* wait 1us */
>> +
>> +        if (delay_us++ > TIMEOUT_IN_US) {
>
> Alan Tull has a patch where this timeout is passed in as part of the 
> struct fpga_image_info.  Would that help here?
>
>> +            dev_warn(&mgr->dev, "CVP_CONFIG_READY == 0 timeout\n");
>> +            status = -ETIME;
>> +            break;
>> +        }
>> +    }
>> +
>> +    if (status < 0)
>> +        return status;
>> +
>> +    /*
>> +     * STEP 16 - check CVP_CONFIG_ERROR_LATCHED bit
>> +     */
>> +    pci_read_config_dword(pdev, VSEC_UNCOR_ERR_STATUS, &val32);
>> +    if (val32 & VSEC_UNCOR_ERR_CVP_CFG_ERR) {
>> +        dev_err(&mgr->dev, " detected CVP_CONFIG_ERROR_LATCHED!\n");
>> +        return -EFAULT;
>> +    }
>> +
>> +    /*
>> +     * STEP 17 - reset CVP_MODE and HIP_CLK_SEL bit
>> +     */
>> +    pci_read_config_dword(pdev, VSEC_CVP_MODE_CTRL, &val32);
>> +    val32 &= ~VSEC_CVP_MODE_CTRL_HIP_CLK_SEL;
>> +    val32 &= ~VSEC_CVP_MODE_CTRL_CVPMODE;
>> +    pci_write_config_dword(pdev, VSEC_CVP_MODE_CTRL, val32);
>> +
>> +    /*
>> +     * STEP 18 - poll PLD_CLK_IN_USE and USER_MODE bits
>> +     */
>> +    delay_us = 0;
>> +    status_msk = VSEC_CVP_STATUS_PLD_CLK_IN_USE | 
>> VSEC_CVP_STATUS_USERMODE;
>> +    while (1) {
>> +        pci_read_config_word(pdev, VSEC_CVP_STATUS, &val16);
>> +        if ((val16 & status_msk) == status_msk)
>> +            break;
>> +
>> +        udelay(1); /* wait 1us */
>> +
>> +        if (delay_us++ > TIMEOUT_IN_US) {
>> +            dev_warn(&mgr->dev, "PLD_CLK_IN_USE|USERMODE timeout\n");
>> +            status = -ETIME;
>> +            break;
>> +        }
>> +    }
>> +
>> +    return status;
>> +}
>> +
>> +static const struct fpga_manager_ops stratix_cvp_ops = {
>> +    .state        = stratix_cvp_state,
>> +    .write_init    = stratix_cvp_write_init,
>> +    .write        = stratix_cvp_write,
>> +    .write_complete    = stratix_cvp_write_complete,
>> +};
>> +
>> +static int stratix_cvp_probe(struct pci_dev *pdev,
>> +                 const struct pci_device_id *dev_id)
>> +{
>> +    struct stratix_cvp_conf *conf;
>> +    u16 cmd, val16;
>> +    int ret;
>> +
>> +    /* Enable memory BAR access */
>> +    pci_read_config_word(pdev, PCI_COMMAND, &cmd);
>> +    if (!(cmd & PCI_COMMAND_MEMORY)) {
>> +        cmd |= PCI_COMMAND_MEMORY;
>> +        pci_write_config_word(pdev, PCI_COMMAND, cmd);
>> +    }
>> +
>> +    conf = devm_kzalloc(&pdev->dev, sizeof(*conf), GFP_KERNEL);
>> +    if (!conf) {
>> +        ret = -ENOMEM;
>> +        goto err;
>> +    }
>> +
>> +    pci_bus_read_config_word(pdev->bus, FNC, VSEC_PCIE_EXT_CAP_ID, 
>> &val16);
>> +    if (val16 != VSEC_PCIE_EXT_CAP_ID_VAL) {
>> +        dev_err(&pdev->dev, "Wrong EXT_CAP_ID value 0x%x\n", val16);
>> +        ret = -ENODEV;
>> +        goto err;
>> +    }
>> +
>> +    conf->pci_dev = pdev;
>> +    conf->map_base = pci_resource_start(pdev, CVP_BAR);
>> +    conf->map_len = pci_resource_len(pdev, CVP_BAR);
>> +
>> +    if (pci_request_region(pdev, CVP_BAR, "CVP") < 0) {
>> +        dev_err(&pdev->dev, "Requesting CVP BAR region failed\n");
>> +        ret = -ENODEV;
>> +        goto err;
>> +    }
>> +
>> +    conf->map = ioremap_nocache(conf->map_base, conf->map_len);
>> +    if (!conf->map)
>> +        dev_warn(&pdev->dev, "Mapping CVP BAR failed\n");
>> +
>> +    ret = fpga_mgr_register(&pdev->dev, STRATIX_V_CVP_MGR_NAME,
>> +                &stratix_cvp_ops, conf);
>
> I believe there may be a problem using a fixed name when there are 
> more than one of the cards plugged into the system.  A unique name may 
> be required here.
>
>> +    if (ret)
>> +        goto err_unmap;
>> +
>> +    conf->mgr = fpga_mgr_get(&pdev->dev);
>> +    if (IS_ERR(conf->mgr)) {
>> +        dev_err(&pdev->dev, "Getting fpga mgr reference failed\n");
>> +        ret = -ENODEV;
>> +        goto err_unmap;
>> +    }
>> +
>> +    ret = fpga_mgr_firmware_load(conf->mgr, NULL, firmware);
>> +    if (ret) {
>> +        dev_err(&pdev->dev, "failed to load fpga image\n");
>> +        goto err_mgr;
>> +    }
>> +
>> +    return 0;
>> +
>> +err_mgr:
>> +    fpga_mgr_put(conf->mgr);
>> +    fpga_mgr_unregister(&pdev->dev);
>> +err_unmap:
>> +    iounmap(conf->map);
>> +    pci_release_region(pdev, CVP_BAR);
>> +err:
>> +    cmd &= ~PCI_COMMAND_MEMORY;
>> +    pci_write_config_word(pdev, PCI_COMMAND, cmd);
>> +    return ret;
>> +}
>> +
>> +static void stratix_cvp_remove(struct pci_dev *pdev)
>> +{
>> +    struct fpga_manager *mgr = pci_get_drvdata(pdev);
>> +    struct stratix_cvp_conf *conf = mgr->priv;
>> +    u16 cmd;
>> +
>> +    fpga_mgr_put(conf->mgr);
>> +    fpga_mgr_unregister(&pdev->dev);
>> +    iounmap(conf->map);
>> +    pci_release_region(pdev, CVP_BAR);
>> +    pci_read_config_word(pdev, PCI_COMMAND, &cmd);
>> +    cmd &= ~PCI_COMMAND_MEMORY;
>> +    pci_write_config_word(pdev, PCI_COMMAND, cmd);
>> +}
>> +
>> +#define PCI_VENDOR_ID_ALTERA    0x1172
>> +#define PCI_DEVICE_ID_FPGA    0xe001
>> +
>> +static struct pci_device_id stratix_cvp_id_tbl[] = {
>> +    { PCI_VDEVICE(ALTERA, PCI_DEVICE_ID_FPGA) },
>> +    { }
>> +};
FPGA are flexible, different design might have different PCI device ID, 
prefer to set to
PCI_DEVICE(ALTERA, PCI_ANY_ID), or this driver can only work for this 
particular hard wired card.
>> +
>> +static struct pci_driver stratix_cvp_driver = {
>> +    .name   = DRV_NAME,
>> +    .id_table = stratix_cvp_id_tbl,
>> +    .probe  = stratix_cvp_probe,
>> +    .remove = stratix_cvp_remove,
>> +};
>> +
>> +module_pci_driver(stratix_cvp_driver)
>> +
>> +MODULE_DEVICE_TABLE(pci, stratix_cvp_id_tbl);
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("Anatolij Gustschin <[e-mail]>");
>> +MODULE_DESCRIPTION("Module to load Altera FPGA over CvP");
>> -- 
>> 2.7.4
>>
>> -- 
>> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
>> the body of a message to [e-mail]
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>>
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Anatolij Gustschin Feb. 14, 2017, 8:16 p.m. UTC | #5

Hi Alan,

On Tue, 14 Feb 2017 11:38:05 -0600
Alan Tull [e-mail] wrote:

>On Mon, Feb 13, 2017 at 1:17 PM, Anatolij Gustschin <[e-mail]> wrote:
>> Add FPGA manager driver for loading Stratix V FPGA using CvP.  
>
>Hi Anatolij,
>
>> +
>> +static enum fpga_mgr_states stratix_cvp_state(struct fpga_manager *mgr)
>> +{
>> +       return mgr->state;
>> +}  
>
>fpga_mgr_register calls this function to initialize mgr->state.  So
>this function needs to either figure out the state of the part or
>return FPGA_MGR_STATE_UNKNOWN.

Okay, I'll rework it.

>> +#define PCI_VENDOR_ID_ALTERA   0x1172
>> +#define PCI_DEVICE_ID_FPGA     0xe001
>> +  
>
>The DID may be different, so we probably need some way of passing it in.

We could use PCI_ANY_ID for DID and users can then set their device
ID by running e.g.:

  echo "1172 e001" > /sys/bus/pci/drivers/stratix-cvp/new_id

new_id is documented in Documentation/ABI/testing/sysfs-bus-pci

>We were working on a Cyclone V CVP driver; it may be quite similar, so
>once we've had more time to look at this, perhaps we can have one
>driver for both.

Yes, this would be better.

>I'll spend some more time looking at your patches.

Thanks,

Anatolij
--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Anatolij Gustschin Feb. 14, 2017, 9:31 p.m. UTC | #6

Hi Yi Li,

On Tue, 14 Feb 2017 13:38:22 -0600
Li, Yi [e-mail] wrote:
...
>I am currently working on the Cyclone V CvP. Based on the User Guild doc 
>UG-01101
>https://www.altera.com/en_US/pdfs/literature/ug/ug_cvp.pdf, all the V 
>series
>should have the same CvP flow. If that's the case, we might want to 
>change the driver/function name to altera_v_xxx and re-use it for
>all V series.

Okay!

>I will test your patch on Cyclone V.
>Some comments below:

Thanks!

...
>>> +#define FNC        0    /* PCIe device function ID */
>>> +#define CVP_BAR        0    /* BAR used for data transfer in memory 
>>> mode */  
>Do we know the BAR and FNC is always 0?

I don't know if this is always the case, maybe it can differ. Is there
a way to detect it?

...
>>> +#define VSEC_CVP_STATUS            (VSEC_OFFSET + 0x1e)    /* 16bit */
>>> +#define VSEC_CVP_STATUS_DATA_ENC    (1<<0)    /* is treated as 
>>> encrypted */
>>> +#define VSEC_CVP_STATUS_DATA_COMP    (1<<1)    /* is treated as 
>>> compressed */  
>According to the UG-01101,the CVP_STATUS register is a 32 bits register 
>offset at 21C, the bit DATA_ENC and DATA_COMP
>do not exist (bit 0-17 reserved).

We can access the upper word of this register at offset 0x21e,
the bits in the lower word are reserved. But yes, it makes sense
to read as 32-bit and move the bit offsets defined below to make
future extension easier (when the reserved bits will implement
something).

...
>>> +    /*
>>> +     * STEP 1 - read CVP status and check CVP_EN flag
>>> +     */
>>> +    pci_bus_read_config_word(bus, FNC, VSEC_CVP_STATUS, &val16);
>>> +    if (!(val16 & VSEC_CVP_STATUS_CVP_EN)) {
>>> +        dev_err(&mgr->dev, "CVP mode off: 0x%04x\n", val16);
>>> +        return -EINVAL;
>>> +    }  
>We might want to check the CONFIG_READY bit here, if it's set by BIOS 
>(yes, I saw it happening on some boards) or left unclean by
>previous programming, we need to tear it down first (started with your 
>step 12).
>Something like below:
>     if (val &VSEC_CVP_STATUS_CFG_RDY ) {
>         dev_info(dev, "Bios already start the CvP, tear it down first\n");
>         altera_v_cvp_fpga_teardown(mgr, info);
>     }
>altera_v_cvp_fpga_teardown function can be re-used by cvp_write_complete 
>function and this init one.

Okay, I can add it.

...
>>> +    /*
>>> +     * STEP 5 - poll CVP_CONFIG READY
>>> +     */
>>> +    delay_us = 0;
>>> +    while (1) {
>>> +        pci_read_config_word(pdev, VSEC_CVP_STATUS, &val16);  
>Again the CVP_STATUS register is 32 bits instead of 16 bit.

yes, it works here because of the used offset and matching bit
definitions. But we can change it to 32-bit.

...
>>> +    /*
>>> +     * STEP 7 - set START_XFER
>>> +     */
>>> +    pci_read_config_dword(pdev, VSEC_CVP_PROG_CTRL, &val32);
>>> +    val32 |= VSEC_CVP_PROG_CTRL_START_XFER;
>>> +    pci_write_config_dword(pdev, VSEC_CVP_PROG_CTRL, val32);
>>> +
>>> +    /*
>>> +     * STEP 8 (start transfer)
>>> +     * check required bitstream type - read CVP status and set 
>>> CVP_NUMCLKS
>>> +     * accordingly
>>> +     *    Compressed    Encrypted    CVP_NUMCLKS
>>> +     *    No        No        1
>>> +     *    Yes        No        8
>>> +     *    No        Yes        4
>>> +     *    Yes        Yes        8
>>> +     */
>>> +    pci_read_config_word(pdev, VSEC_CVP_STATUS, &val16);
>>> +    val16 &= (VSEC_CVP_STATUS_DATA_ENC | VSEC_CVP_STATUS_DATA_COMP);
>>> +    switch (val16) {
>>> +    case VSEC_CVP_STATUS_DATA_ENC:
>>> +        data = 4;
>>> +        break;
>>> +    case VSEC_CVP_STATUS_DATA_COMP:
>>> +    case (VSEC_CVP_STATUS_DATA_ENC | VSEC_CVP_STATUS_DATA_COMP):
>>> +        data = 8;
>>> +        break;
>>> +    default:
>>> +        data = 1;
>>> +        break;
>>> +    }  
>My understanding is we need to set the NUMCLKS base on RBF stream format 
>instead of polling
>the non-existed VSEC_CVP_STATUS_DATA_ENC and VSEC_CVP_STATUS_DATA_COMP bits.

Is this part of the stream format documented somewhere?

...
>>> +    /*
>>> +     * STEP 9
>>> +     * - write 32-bit configuration data from POF file to CVP data 
>>> register
>>> +     */
>>> +    data = (u32 *)buf;
>>> +    data_end = (u32 *)((char *)buf + count);
>>> +  
>What happens if the length of the bit stream is not dword (4 bytes) 
>aligned? We need to pad the un-aligned stream
>at the end.

Yes, agreed. The size of all my testing RBF files is a whole multiple
of 4 and these files are padded by 0xff at the end. But it would be
good to be prepared for different sizes.
 
...
>>> +            /*
>>> +             * STEP 10 (optional) and STEP 11
>>> +             * - check error flag
>>> +             * - loop until data transfer completed
>>> +             */
>>> +            if (chkcfg) {
>>> +                status = stratix_cvp_chk_cfg_error(mgr, bytes);
>>> +                if (status < 0)
>>> +                    break;
>>> +            }  
>This might be too heavy to check every dword writing, since the error 
>status bit will persist once error detected, we
>might want to check periodically, say every 1K dword?.

Okay, will rework as suggested.

...
>>> +#define PCI_VENDOR_ID_ALTERA    0x1172
>>> +#define PCI_DEVICE_ID_FPGA    0xe001
>>> +
>>> +static struct pci_device_id stratix_cvp_id_tbl[] = {
>>> +    { PCI_VDEVICE(ALTERA, PCI_DEVICE_ID_FPGA) },
>>> +    { }
>>> +};  
>FPGA are flexible, different design might have different PCI device ID, 
>prefer to set to
>PCI_DEVICE(ALTERA, PCI_ANY_ID), or this driver can only work for this 
>particular hard wired card.

Okay, I'll add PCI_ANY_ID here and will use new_id to set custom device
ID over sysfs.

Thanks,

Anatolij
--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Li, Yi Feb. 14, 2017, 11:28 p.m. UTC | #7

hi Anatolij


On 2/14/2017 3:31 PM, Anatolij Gustschin wrote:
> Hi Yi Li,
>
> On Tue, 14 Feb 2017 13:38:22 -0600
> Li, Yi [e-mail] wrote:
> ...
>> I am currently working on the Cyclone V CvP. Based on the User Guild doc
>> UG-01101
>> https://www.altera.com/en_US/pdfs/literature/ug/ug_cvp.pdf, all the V
>> series
>> should have the same CvP flow. If that's the case, we might want to
>> change the driver/function name to altera_v_xxx and re-use it for
>> all V series.
> Okay!
>
>> I will test your patch on Cyclone V.
>> Some comments below:
> Thanks!
>
> ...
>>>> +#define FNC        0    /* PCIe device function ID */
>>>> +#define CVP_BAR        0    /* BAR used for data transfer in memory
>>>> mode */
>> Do we know the BAR and FNC is always 0?
> I don't know if this is always the case, maybe it can differ. Is there
> a way to detect it?

Read the UG again, maybe it does not matter. According to the UG driver 
Flow diag, the rbf stream should be written through the data register. 
The xxx_cvp_write should not use the conf->map then. The two dummy write 
can be address to memory of any BAR.
One more comment, Alan added the debug_fs support for the FPGA manager. 
It will help to select different firmware file instead of hard-coding 
the FIRMWARE_STR, which will not work for multiple cards, multiple 
firmware case.

Yi
>
> ...
>>>> +#define VSEC_CVP_STATUS            (VSEC_OFFSET + 0x1e)    /* 16bit */
>>>> +#define VSEC_CVP_STATUS_DATA_ENC    (1<<0)    /* is treated as
>>>> encrypted */
>>>> +#define VSEC_CVP_STATUS_DATA_COMP    (1<<1)    /* is treated as
>>>> compressed */
>> According to the UG-01101,the CVP_STATUS register is a 32 bits register
>> offset at 21C, the bit DATA_ENC and DATA_COMP
>> do not exist (bit 0-17 reserved).
> We can access the upper word of this register at offset 0x21e,
> the bits in the lower word are reserved. But yes, it makes sense
> to read as 32-bit and move the bit offsets defined below to make
> future extension easier (when the reserved bits will implement
> something).
>
> ...
>>>> +    /*
>>>> +     * STEP 1 - read CVP status and check CVP_EN flag
>>>> +     */
>>>> +    pci_bus_read_config_word(bus, FNC, VSEC_CVP_STATUS, &val16);
>>>> +    if (!(val16 & VSEC_CVP_STATUS_CVP_EN)) {
>>>> +        dev_err(&mgr->dev, "CVP mode off: 0x%04x\n", val16);
>>>> +        return -EINVAL;
>>>> +    }
>> We might want to check the CONFIG_READY bit here, if it's set by BIOS
>> (yes, I saw it happening on some boards) or left unclean by
>> previous programming, we need to tear it down first (started with your
>> step 12).
>> Something like below:
>>      if (val &VSEC_CVP_STATUS_CFG_RDY ) {
>>          dev_info(dev, "Bios already start the CvP, tear it down first\n");
>>          altera_v_cvp_fpga_teardown(mgr, info);
>>      }
>> altera_v_cvp_fpga_teardown function can be re-used by cvp_write_complete
>> function and this init one.
> Okay, I can add it.
>
> ...
>>>> +    /*
>>>> +     * STEP 5 - poll CVP_CONFIG READY
>>>> +     */
>>>> +    delay_us = 0;
>>>> +    while (1) {
>>>> +        pci_read_config_word(pdev, VSEC_CVP_STATUS, &val16);
>> Again the CVP_STATUS register is 32 bits instead of 16 bit.
> yes, it works here because of the used offset and matching bit
> definitions. But we can change it to 32-bit.
>
> ...
>>>> +    /*
>>>> +     * STEP 7 - set START_XFER
>>>> +     */
>>>> +    pci_read_config_dword(pdev, VSEC_CVP_PROG_CTRL, &val32);
>>>> +    val32 |= VSEC_CVP_PROG_CTRL_START_XFER;
>>>> +    pci_write_config_dword(pdev, VSEC_CVP_PROG_CTRL, val32);
>>>> +
>>>> +    /*
>>>> +     * STEP 8 (start transfer)
>>>> +     * check required bitstream type - read CVP status and set
>>>> CVP_NUMCLKS
>>>> +     * accordingly
>>>> +     *    Compressed    Encrypted    CVP_NUMCLKS
>>>> +     *    No        No        1
>>>> +     *    Yes        No        8
>>>> +     *    No        Yes        4
>>>> +     *    Yes        Yes        8
>>>> +     */
>>>> +    pci_read_config_word(pdev, VSEC_CVP_STATUS, &val16);
>>>> +    val16 &= (VSEC_CVP_STATUS_DATA_ENC | VSEC_CVP_STATUS_DATA_COMP);
>>>> +    switch (val16) {
>>>> +    case VSEC_CVP_STATUS_DATA_ENC:
>>>> +        data = 4;
>>>> +        break;
>>>> +    case VSEC_CVP_STATUS_DATA_COMP:
>>>> +    case (VSEC_CVP_STATUS_DATA_ENC | VSEC_CVP_STATUS_DATA_COMP):
>>>> +        data = 8;
>>>> +        break;
>>>> +    default:
>>>> +        data = 1;
>>>> +        break;
>>>> +    }
>> My understanding is we need to set the NUMCLKS base on RBF stream format
>> instead of polling
>> the non-existed VSEC_CVP_STATUS_DATA_ENC and VSEC_CVP_STATUS_DATA_COMP bits.
> Is this part of the stream format documented somewhere?

  Will need to find out how to do this, or maybe add a pass in parameter 
for the stream format?

>
> ...
>>>> +    /*
>>>> +     * STEP 9
>>>> +     * - write 32-bit configuration data from POF file to CVP data
>>>> register
>>>> +     */
>>>> +    data = (u32 *)buf;
>>>> +    data_end = (u32 *)((char *)buf + count);
>>>> +
>> What happens if the length of the bit stream is not dword (4 bytes)
>> aligned? We need to pad the un-aligned stream
>> at the end.
> Yes, agreed. The size of all my testing RBF files is a whole multiple
> of 4 and these files are padded by 0xff at the end. But it would be
> good to be prepared for different sizes.
>   
> ...
>>>> +            /*
>>>> +             * STEP 10 (optional) and STEP 11
>>>> +             * - check error flag
>>>> +             * - loop until data transfer completed
>>>> +             */
>>>> +            if (chkcfg) {
>>>> +                status = stratix_cvp_chk_cfg_error(mgr, bytes);
>>>> +                if (status < 0)
>>>> +                    break;
>>>> +            }
>> This might be too heavy to check every dword writing, since the error
>> status bit will persist once error detected, we
>> might want to check periodically, say every 1K dword?.
> Okay, will rework as suggested.
>
> ...
>>>> +#define PCI_VENDOR_ID_ALTERA    0x1172
>>>> +#define PCI_DEVICE_ID_FPGA    0xe001
>>>> +
>>>> +static struct pci_device_id stratix_cvp_id_tbl[] = {
>>>> +    { PCI_VDEVICE(ALTERA, PCI_DEVICE_ID_FPGA) },
>>>> +    { }
>>>> +};
>> FPGA are flexible, different design might have different PCI device ID,
>> prefer to set to
>> PCI_DEVICE(ALTERA, PCI_ANY_ID), or this driver can only work for this
>> particular hard wired card.
> Okay, I'll add PCI_ANY_ID here and will use new_id to set custom device
> ID over sysfs.
>
> Thanks,
>
> Anatolij

--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Anatolij Gustschin Feb. 15, 2017, 7:49 a.m. UTC | #8

Hi Yi,

On Tue, 14 Feb 2017 17:28:46 -0600
Li, Yi [e-mail] wrote:
...
>> ...  
>>>>> +#define FNC        0    /* PCIe device function ID */
>>>>> +#define CVP_BAR        0    /* BAR used for data transfer in memory
>>>>> mode */  
>>> Do we know the BAR and FNC is always 0?  
>> I don't know if this is always the case, maybe it can differ. Is there
>> a way to detect it?  
>
>Read the UG again, maybe it does not matter. According to the UG driver 
>Flow diag, the rbf stream should be written through the data register. 
>The xxx_cvp_write should not use the conf->map then. The two dummy write 
>can be address to memory of any BAR.

writes to the data register via configuration space are already implemented
in the driver as a fallback if we fail to map the memory BAR region for
some reason. By default we write to the data register via memory BAR as
it is much faster than configuration space accesses to the data register.
The CvP Data Register description (page 6-7 in UG) states that the data
register can be accessed via any address in the memory space BAR in
CvP mode for higher throughput.

>One more comment, Alan added the debug_fs support for the FPGA manager. 
>It will help to select different firmware file instead of hard-coding 
>the FIRMWARE_STR, which will not work for multiple cards, multiple 
>firmware case.

will FPGA manager debug_fs interface be merged in mainline? I've seen
objections against debugfs interface for configuration on the mailing
list, IIRC.

...
>>>>> +    pci_read_config_word(pdev, VSEC_CVP_STATUS, &val16);
>>>>> +    val16 &= (VSEC_CVP_STATUS_DATA_ENC | VSEC_CVP_STATUS_DATA_COMP);
>>>>> +    switch (val16) {
>>>>> +    case VSEC_CVP_STATUS_DATA_ENC:
>>>>> +        data = 4;
>>>>> +        break;
>>>>> +    case VSEC_CVP_STATUS_DATA_COMP:
>>>>> +    case (VSEC_CVP_STATUS_DATA_ENC | VSEC_CVP_STATUS_DATA_COMP):
>>>>> +        data = 8;
>>>>> +        break;
>>>>> +    default:
>>>>> +        data = 1;
>>>>> +        break;
>>>>> +    }  
>>> My understanding is we need to set the NUMCLKS base on RBF stream format
>>> instead of polling
>>> the non-existed VSEC_CVP_STATUS_DATA_ENC and VSEC_CVP_STATUS_DATA_COMP bits.  
>> Is this part of the stream format documented somewhere?  
>
>  Will need to find out how to do this, or maybe add a pass in parameter 
>for the stream format?

Maybe. Thanks!

Anatolij
--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Alan Tull Feb. 15, 2017, 5:33 p.m. UTC | #9

On Wed, Feb 15, 2017 at 1:49 AM, Anatolij Gustschin <[e-mail]> wrote:
> Hi Yi,
>
> On Tue, 14 Feb 2017 17:28:46 -0600
> Li, Yi [e-mail] wrote:
> ...
>>> ...
>>>>>> +#define FNC        0    /* PCIe device function ID */
>>>>>> +#define CVP_BAR        0    /* BAR used for data transfer in memory
>>>>>> mode */
>>>> Do we know the BAR and FNC is always 0?
>>> I don't know if this is always the case, maybe it can differ. Is there
>>> a way to detect it?
>>
>>Read the UG again, maybe it does not matter. According to the UG driver
>>Flow diag, the rbf stream should be written through the data register.
>>The xxx_cvp_write should not use the conf->map then. The two dummy write
>>can be address to memory of any BAR.
>
> writes to the data register via configuration space are already implemented
> in the driver as a fallback if we fail to map the memory BAR region for
> some reason. By default we write to the data register via memory BAR as
> it is much faster than configuration space accesses to the data register.
> The CvP Data Register description (page 6-7 in UG) states that the data
> register can be accessed via any address in the memory space BAR in
> CvP mode for higher throughput.
>
>>One more comment, Alan added the debug_fs support for the FPGA manager.
>>It will help to select different firmware file instead of hard-coding
>>the FIRMWARE_STR, which will not work for multiple cards, multiple
>>firmware case.
>
> will FPGA manager debug_fs interface be merged in mainline? I've seen
> objections against debugfs interface for configuration on the mailing
> list, IIRC.

Hmmm email problems started half way through the morning.  Resending this:

The debugfs that I wrote is really only for debug, not for production use.

The individual FPGA manager drivers shouldn't be adding userspace
interfaces.  The interface should be common code, that's the point of
having a framework.  I've posted a patchset that adds a sysfs
interface to the fpga region layer that will be controlling the fpga
managers and fpga bridges.  That will probably be controversial and
will require some discussion.

Thanks,
Alan

>
> ...
>>>>>> +    pci_read_config_word(pdev, VSEC_CVP_STATUS, &val16);
>>>>>> +    val16 &= (VSEC_CVP_STATUS_DATA_ENC | VSEC_CVP_STATUS_DATA_COMP);
>>>>>> +    switch (val16) {
>>>>>> +    case VSEC_CVP_STATUS_DATA_ENC:
>>>>>> +        data = 4;
>>>>>> +        break;
>>>>>> +    case VSEC_CVP_STATUS_DATA_COMP:
>>>>>> +    case (VSEC_CVP_STATUS_DATA_ENC | VSEC_CVP_STATUS_DATA_COMP):
>>>>>> +        data = 8;
>>>>>> +        break;
>>>>>> +    default:
>>>>>> +        data = 1;
>>>>>> +        break;
>>>>>> +    }
>>>> My understanding is we need to set the NUMCLKS base on RBF stream format
>>>> instead of polling
>>>> the non-existed VSEC_CVP_STATUS_DATA_ENC and VSEC_CVP_STATUS_DATA_COMP bits.
>>> Is this part of the stream format documented somewhere?
>>
>>  Will need to find out how to do this, or maybe add a pass in parameter
>>for the stream format?
>
> Maybe. Thanks!
>
> Anatolij
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Alan Tull Feb. 15, 2017, 6:07 p.m. UTC | #10

On Tue, Feb 14, 2017 at 3:31 PM, Anatolij Gustschin <[e-mail]> wrote:

>>>> +#define PCI_VENDOR_ID_ALTERA    0x1172
>>>> +#define PCI_DEVICE_ID_FPGA    0xe001
>>>> +
>>>> +static struct pci_device_id stratix_cvp_id_tbl[] = {
>>>> +    { PCI_VDEVICE(ALTERA, PCI_DEVICE_ID_FPGA) },
>>>> +    { }
>>>> +};
>>FPGA are flexible, different design might have different PCI device ID,
>>prefer to set to
>>PCI_DEVICE(ALTERA, PCI_ANY_ID), or this driver can only work for this
>>particular hard wired card.
>
> Okay, I'll add PCI_ANY_ID here and will use new_id to set custom device
> ID over sysfs.

Would make sense to add a Kconfig for the id (for both FPGA manager drivers)?

Alan

>
> Thanks,
>
> Anatolij
--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Moritz Fischer Feb. 15, 2017, 8:58 p.m. UTC | #11

On Wed, Feb 15, 2017 at 10:07 AM, Alan Tull <[e-mail]> wrote:
> On Tue, Feb 14, 2017 at 3:31 PM, Anatolij Gustschin <[e-mail]> wrote:
>
>>>>> +#define PCI_VENDOR_ID_ALTERA    0x1172
>>>>> +#define PCI_DEVICE_ID_FPGA    0xe001
>>>>> +
>>>>> +static struct pci_device_id stratix_cvp_id_tbl[] = {
>>>>> +    { PCI_VDEVICE(ALTERA, PCI_DEVICE_ID_FPGA) },
>>>>> +    { }
>>>>> +};
>>>FPGA are flexible, different design might have different PCI device ID,
>>>prefer to set to
>>>PCI_DEVICE(ALTERA, PCI_ANY_ID), or this driver can only work for this
>>>particular hard wired card.
>>
>> Okay, I'll add PCI_ANY_ID here and will use new_id to set custom device
>> ID over sysfs.
>
> Would make sense to add a Kconfig for the id (for both FPGA manager drivers)?

Not sure about that. That makes it a compile time choice. Not really
extensible. I think
if you want to support your board you just add the ids to the driver
matches, I don't see how
having an FPGA on there makes this different from other hardware.

Cheers,

Moritz
--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Alan Tull Feb. 15, 2017, 9:07 p.m. UTC | #12

On Wed, Feb 15, 2017 at 2:58 PM, Moritz Fischer
<[e-mail]> wrote:
> On Wed, Feb 15, 2017 at 10:07 AM, Alan Tull <[e-mail]> wrote:
>> On Tue, Feb 14, 2017 at 3:31 PM, Anatolij Gustschin <[e-mail]> wrote:
>>
>>>>>> +#define PCI_VENDOR_ID_ALTERA    0x1172
>>>>>> +#define PCI_DEVICE_ID_FPGA    0xe001
>>>>>> +
>>>>>> +static struct pci_device_id stratix_cvp_id_tbl[] = {
>>>>>> +    { PCI_VDEVICE(ALTERA, PCI_DEVICE_ID_FPGA) },
>>>>>> +    { }
>>>>>> +};
>>>>FPGA are flexible, different design might have different PCI device ID,
>>>>prefer to set to
>>>>PCI_DEVICE(ALTERA, PCI_ANY_ID), or this driver can only work for this
>>>>particular hard wired card.
>>>
>>> Okay, I'll add PCI_ANY_ID here and will use new_id to set custom device
>>> ID over sysfs.
>>
>> Would make sense to add a Kconfig for the id (for both FPGA manager drivers)?
>
> Not sure about that. That makes it a compile time choice. Not really
> extensible. I think
> if you want to support your board you just add the ids to the driver
> matches, I don't see how
> having an FPGA on there makes this different from other hardware.

The id is selectable when compiling the FPGA image.  It isn't board specific.

Alan

>
> Cheers,
>
> Moritz
--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Moritz Fischer Feb. 15, 2017, 9:10 p.m. UTC | #13

Hi Alan,

On Wed, Feb 15, 2017 at 1:07 PM, Alan Tull <[e-mail]> wrote:
> On Wed, Feb 15, 2017 at 2:58 PM, Moritz Fischer
> <[e-mail]> wrote:
>> On Wed, Feb 15, 2017 at 10:07 AM, Alan Tull <[e-mail]> wrote:
>>> On Tue, Feb 14, 2017 at 3:31 PM, Anatolij Gustschin <[e-mail]> wrote:
>>>
>>>>>>> +#define PCI_VENDOR_ID_ALTERA    0x1172
>>>>>>> +#define PCI_DEVICE_ID_FPGA    0xe001
>>>>>>> +
>>>>>>> +static struct pci_device_id stratix_cvp_id_tbl[] = {
>>>>>>> +    { PCI_VDEVICE(ALTERA, PCI_DEVICE_ID_FPGA) },
>>>>>>> +    { }
>>>>>>> +};
>>>>>FPGA are flexible, different design might have different PCI device ID,
>>>>>prefer to set to
>>>>>PCI_DEVICE(ALTERA, PCI_ANY_ID), or this driver can only work for this
>>>>>particular hard wired card.
>>>>
>>>> Okay, I'll add PCI_ANY_ID here and will use new_id to set custom device
>>>> ID over sysfs.
>>>
>>> Would make sense to add a Kconfig for the id (for both FPGA manager drivers)?
>>
>> Not sure about that. That makes it a compile time choice. Not really
>> extensible. I think
>> if you want to support your board you just add the ids to the driver
>> matches, I don't see how
>> having an FPGA on there makes this different from other hardware.
>
> The id is selectable when compiling the FPGA image.  It isn't board specific.

I get that part. But isn't that the same from any manufacturer
building any piece of hardware?
At one point I'm gonna pick my vendor & product id. So I see that for
debug & devel you wanna
add new ids (via sysfs). But once you productize sending a patch to
bind a driver to a device id
seems a reasonable requirement? :)

Cheers,

Moritz
--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Alan Tull Feb. 15, 2017, 9:18 p.m. UTC | #14

On Wed, Feb 15, 2017 at 3:10 PM, Moritz Fischer
<[e-mail]> wrote:
> Hi Alan,
>
> On Wed, Feb 15, 2017 at 1:07 PM, Alan Tull <[e-mail]> wrote:
>> On Wed, Feb 15, 2017 at 2:58 PM, Moritz Fischer
>> <[e-mail]> wrote:
>>> On Wed, Feb 15, 2017 at 10:07 AM, Alan Tull <[e-mail]> wrote:
>>>> On Tue, Feb 14, 2017 at 3:31 PM, Anatolij Gustschin <[e-mail]> wrote:
>>>>
>>>>>>>> +#define PCI_VENDOR_ID_ALTERA    0x1172
>>>>>>>> +#define PCI_DEVICE_ID_FPGA    0xe001
>>>>>>>> +
>>>>>>>> +static struct pci_device_id stratix_cvp_id_tbl[] = {
>>>>>>>> +    { PCI_VDEVICE(ALTERA, PCI_DEVICE_ID_FPGA) },
>>>>>>>> +    { }
>>>>>>>> +};
>>>>>>FPGA are flexible, different design might have different PCI device ID,
>>>>>>prefer to set to
>>>>>>PCI_DEVICE(ALTERA, PCI_ANY_ID), or this driver can only work for this
>>>>>>particular hard wired card.
>>>>>
>>>>> Okay, I'll add PCI_ANY_ID here and will use new_id to set custom device
>>>>> ID over sysfs.
>>>>
>>>> Would make sense to add a Kconfig for the id (for both FPGA manager drivers)?
>>>
>>> Not sure about that. That makes it a compile time choice. Not really
>>> extensible. I think
>>> if you want to support your board you just add the ids to the driver
>>> matches, I don't see how
>>> having an FPGA on there makes this different from other hardware.
>>
>> The id is selectable when compiling the FPGA image.  It isn't board specific.
>
> I get that part. But isn't that the same from any manufacturer
> building any piece of hardware?
> At one point I'm gonna pick my vendor & product id. So I see that for
> debug & devel you wanna
> add new ids (via sysfs). But once you productize sending a patch to
> bind a driver to a device id
> seems a reasonable requirement? :)

Yes, that does sound right!  :) Thanks!

Alan

>
> Cheers,
>
> Moritz
--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Anatolij Gustschin Feb. 15, 2017, 9:59 p.m. UTC | #15

Hi Alan,

On Wed, 15 Feb 2017 11:33:09 -0600
Alan Tull [e-mail] wrote:
...
>> will FPGA manager debug_fs interface be merged in mainline? I've seen
>> objections against debugfs interface for configuration on the mailing
>> list, IIRC.  
>
>Hmmm email problems started half way through the morning.  Resending this:
>
>The debugfs that I wrote is really only for debug, not for production use.
>
>The individual FPGA manager drivers shouldn't be adding userspace
>interfaces.  The interface should be common code, that's the point of
>having a framework.  I've posted a patchset that adds a sysfs
>interface to the fpga region layer that will be controlling the fpga
>managers and fpga bridges.  That will probably be controversial and
>will require some discussion.

Okay, thanks for clarification and patches!

Anatolij
--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Li, Yi Feb. 15, 2017, 10:06 p.m. UTC | #16

hi Alan


On 2/15/2017 11:33 AM, Alan Tull wrote:
> On Wed, Feb 15, 2017 at 1:49 AM, Anatolij Gustschin <[e-mail]> wrote:
>> Hi Yi,
>>
>> On Tue, 14 Feb 2017 17:28:46 -0600
>> Li, Yi [e-mail] wrote:
>> ...
>>>> ...
>>>>>>> +#define FNC        0    /* PCIe device function ID */
>>>>>>> +#define CVP_BAR        0    /* BAR used for data transfer in memory
>>>>>>> mode */
>>>>> Do we know the BAR and FNC is always 0?
>>>> I don't know if this is always the case, maybe it can differ. Is there
>>>> a way to detect it?
>>> Read the UG again, maybe it does not matter. According to the UG driver
>>> Flow diag, the rbf stream should be written through the data register.
>>> The xxx_cvp_write should not use the conf->map then. The two dummy write
>>> can be address to memory of any BAR.
>> writes to the data register via configuration space are already implemented
>> in the driver as a fallback if we fail to map the memory BAR region for
>> some reason. By default we write to the data register via memory BAR as
>> it is much faster than configuration space accesses to the data register.
>> The CvP Data Register description (page 6-7 in UG) states that the data
>> register can be accessed via any address in the memory space BAR in
>> CvP mode for higher throughput.

You are right, the tech pub might forget to update the Figure 6-1 CvP 
Driver Flow, which is misleading. Tested on the Cyclone V, memory Bar 
seems working.

>>> One more comment, Alan added the debug_fs support for the FPGA manager.
>>> It will help to select different firmware file instead of hard-coding
>>> the FIRMWARE_STR, which will not work for multiple cards, multiple
>>> firmware case.
>> will FPGA manager debug_fs interface be merged in mainline? I've seen
>> objections against debugfs interface for configuration on the mailing
>> list, IIRC.
> Hmmm email problems started half way through the morning.  Resending this:
>
> The debugfs that I wrote is really only for debug, not for production use.
>
> The individual FPGA manager drivers shouldn't be adding userspace
> interfaces.  The interface should be common code, that's the point of
> having a framework.  I've posted a patchset that adds a sysfs
> interface to the fpga region layer that will be controlling the fpga
> managers and fpga bridges.  That will probably be controversial and
> will require some discussion.
>
> Thanks,
> Alan
>
>> ...
>>>>>>> +    pci_read_config_word(pdev, VSEC_CVP_STATUS, &val16);
>>>>>>> +    val16 &= (VSEC_CVP_STATUS_DATA_ENC | VSEC_CVP_STATUS_DATA_COMP);
>>>>>>> +    switch (val16) {
>>>>>>> +    case VSEC_CVP_STATUS_DATA_ENC:
>>>>>>> +        data = 4;
>>>>>>> +        break;
>>>>>>> +    case VSEC_CVP_STATUS_DATA_COMP:
>>>>>>> +    case (VSEC_CVP_STATUS_DATA_ENC | VSEC_CVP_STATUS_DATA_COMP):
>>>>>>> +        data = 8;
>>>>>>> +        break;
>>>>>>> +    default:
>>>>>>> +        data = 1;
>>>>>>> +        break;
>>>>>>> +    }
>>>>> My understanding is we need to set the NUMCLKS base on RBF stream format
>>>>> instead of polling
>>>>> the non-existed VSEC_CVP_STATUS_DATA_ENC and VSEC_CVP_STATUS_DATA_COMP bits.
>>>> Is this part of the stream format documented somewhere?
>>>   Will need to find out how to do this, or maybe add a pass in parameter
>>> for the stream format?
>> Maybe. Thanks!
>>
>> Anatolij
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
>> the body of a message to [e-mail]
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Li, Yi Feb. 16, 2017, 6:20 a.m. UTC | #17

hi Aantolij


On 2/14/2017 3:31 PM, Anatolij Gustschin wrote:
> Hi Yi Li,
>
> On Tue, 14 Feb 2017 13:38:22 -0600
> Li, Yi [e-mail] wrote:
> ...
>> I am currently working on the Cyclone V CvP. Based on the User Guild doc
>> UG-01101
>> https://www.altera.com/en_US/pdfs/literature/ug/ug_cvp.pdf, all the V
>> series
>> should have the same CvP flow. If that's the case, we might want to
>> change the driver/function name to altera_v_xxx and re-use it for
>> all V series.
> Okay!
>
>> I will test your patch on Cyclone V.
>> Some comments below:
> Thanks!
The firmware load is working on my Cyclone V x86 platform. But I saw 
some kernel dump when removing the module, have you seen those before?
[   48.406757] Call Trace:
[   48.409500]  [<ffffffff81568be9>] device_del+0x139/0x260
[   48.415447]  [<ffffffff81568d32>] device_unregister+0x22/0x80
[   48.421882]  [<ffffffffa000033f>] fpga_mgr_unregister+0x4f/0x60 
[fpga_mgr]
[   48.429579]  [<ffffffffa00060ad>] stratix_cvp_remove+0x8d/0xb0 
[stratix_cvp]
[   48.437470]  [<ffffffff8147bd7d>] pci_device_remove+0x2d/0x60
[   48.443903]  [<ffffffff8156dcb0>] __device_release_driver+0x80/0x120
[   48.451015]  [<ffffffff8156de68>] driver_detach+0xc8/0xd0
[   48.457058]  [<ffffffff8156cde9>] bus_remove_driver+0x59/0xe0
[   48.463491]  [<ffffffff8156e2e0>] driver_unregister+0x30/0x70
[   48.469922]  [<ffffffff8147bc9d>] pci_unregister_driver+0x2d/0x90
[   48.476746]  [<ffffffffa0006c34>] stratix_cvp_driver_exit+0x10/0x12 
[stratix_cvp]
[   48.485126]  [<ffffffff810ef138>] SyS_delete_module+0x1d8/0x230
[   48.491755]  [<ffffffff81939c17>] system_call_fastpath+0x12/0x6a
[   48.498477] Code: ca b0 ff b8 f4 ff ff ff 5d c3 0f 1f 44 00 00 55 48 
89 e5 53 48 89 fb 48 c7 c7 60 bd eb 81 48 83 ec 08 e8 03 a2 3c 00 48 8b 
43 08 <48> 8b 88 88 00 00 00 48 8d 90 88 00 00 00 48 39 ca 74 25 f6 05
[   48.520080] RIP  [<ffffffff8156d2c1>] driver_deferred_probe_del+0x21/0xa0
[   48.527683]  RSP <ffff8802757ebd28>
[   48.531632] ---[ end trace 55d124b294260fdd ]

>
> ...

--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Anatolij Gustschin Feb. 16, 2017, 7:25 a.m. UTC | #18

Hi Yi,

On Thu, 16 Feb 2017 00:20:18 -0600
Li, Yi [e-mail] wrote:
... 
>The firmware load is working on my Cyclone V x86 platform. But I saw 
>some kernel dump when removing the module, have you seen those before?
>[   48.406757] Call Trace:
>[   48.409500]  [<ffffffff81568be9>] device_del+0x139/0x260
>[   48.415447]  [<ffffffff81568d32>] device_unregister+0x22/0x80
>[   48.421882]  [<ffffffffa000033f>] fpga_mgr_unregister+0x4f/0x60 
>[fpga_mgr]
>[   48.429579]  [<ffffffffa00060ad>] stratix_cvp_remove+0x8d/0xb0 
>[stratix_cvp]
...

no, I haven't seen this. I remove the module after unbinding the
driver

  echo 0000:06:00.0  > /sys/bus/pci/drivers/stratix-cvp/unbind
  rmmod stratix_cvp

and it always works. I also tested module remove/reload cycles with
v4.10.0-rc5 kernel. Do you use local fpga manager patches that are
not in mainline kernel yet?

Thanks,
Anatolij
--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Alan Tull Feb. 16, 2017, 10:24 p.m. UTC | #19

On Tue, Feb 14, 2017 at 11:07 AM, Anatolij Gustschin <[e-mail]> wrote:

>>> +    delay_us = 0;
>>> +    while (1) {
>>> +            pci_read_config_word(pdev, VSEC_CVP_STATUS, &val16);
>>> +            if ((val16 & VSEC_CVP_STATUS_CFG_RDY) == 0)
>>> +                    break;
>>> +
>>> +            udelay(1);      /* wait 1us */
>>> +
>>> +            if (delay_us++ > TIMEOUT_IN_US) {
>>
>>Alan Tull has a patch where this timeout is passed in as part of the
>>struct fpga_image_info.  Would that help here?
>
> yes, this makes sense.

Actually on second thought, it may not be necessary or helpful to pass
in the timeout.  There are timeouts in the fpga_image_info struct, but
they are there because different FPGA designs will have longer or
shorter timeouts.  If this is really just a fixed timeout, we don't
need to do that here.

Alan
--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Anatolij Gustschin Feb. 16, 2017, 10:51 p.m. UTC | #20

Hi Alan,

On Thu, 16 Feb 2017 16:24:32 -0600
Alan Tull [e-mail] wrote:

>On Tue, Feb 14, 2017 at 11:07 AM, Anatolij Gustschin <[e-mail]> wrote:
>
>>>> +    delay_us = 0;
>>>> +    while (1) {
>>>> +            pci_read_config_word(pdev, VSEC_CVP_STATUS, &val16);
>>>> +            if ((val16 & VSEC_CVP_STATUS_CFG_RDY) == 0)
>>>> +                    break;
>>>> +
>>>> +            udelay(1);      /* wait 1us */
>>>> +
>>>> +            if (delay_us++ > TIMEOUT_IN_US) {  
>>>
>>>Alan Tull has a patch where this timeout is passed in as part of the
>>>struct fpga_image_info.  Would that help here?  
>>
>> yes, this makes sense.  
>
>Actually on second thought, it may not be necessary or helpful to pass
>in the timeout.  There are timeouts in the fpga_image_info struct, but
>they are there because different FPGA designs will have longer or
>shorter timeouts.  If this is really just a fixed timeout, we don't
>need to do that here.

The timeout in fpga_image_info is set from the DT overlay, I'm not
using overlays for CvP. It seems that this timeout is fixed, the
Driver Flow figure in UG mentions 2ms timeout. I'll stick to fixed
timeout.

Thanks,
Anatolij
--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Li, Yi Feb. 16, 2017, 11:25 p.m. UTC | #21

hi Anatolij

On 2/16/2017 1:25 AM, Anatolij Gustschin wrote:
> Hi Yi,
>
> On Thu, 16 Feb 2017 00:20:18 -0600
> Li, Yi [e-mail] wrote:
> ...
>> The firmware load is working on my Cyclone V x86 platform. But I saw
>> some kernel dump when removing the module, have you seen those before?
>> [   48.406757] Call Trace:
>> [   48.409500]  [<ffffffff81568be9>] device_del+0x139/0x260
>> [   48.415447]  [<ffffffff81568d32>] device_unregister+0x22/0x80
>> [   48.421882]  [<ffffffffa000033f>] fpga_mgr_unregister+0x4f/0x60
>> [fpga_mgr]
>> [   48.429579]  [<ffffffffa00060ad>] stratix_cvp_remove+0x8d/0xb0
>> [stratix_cvp]
> ...
>
> no, I haven't seen this. I remove the module after unbinding the
> driver
>
>    echo 0000:06:00.0  > /sys/bus/pci/drivers/stratix-cvp/unbind
>    rmmod stratix_cvp
>
> and it always works. I also tested module remove/reload cycles with
> v4.10.0-rc5 kernel. Do you use local fpga manager patches that are
> not in mainline kernel yet?

No local patches this time, but I was missing one patch 
9dce0287a60d72656a787b075f1b9162ff3cb142 and was using pci_get_drvdata 
instead of fpga_mgr_get. Now I am applying your patch on top of 
linux-next repo commit fee922e6a9938c0242fccd544f00abe9322ea578 and do 
not see the dump anymore. Sorry about the false alarm. :)

Thanks,
Yi

>
> Thanks,
> Anatolij
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Alan Tull Feb. 17, 2017, 10:05 p.m. UTC | #22

On Thu, Feb 16, 2017 at 4:51 PM, Anatolij Gustschin <[e-mail]> wrote:
> Hi Alan,
>
> On Thu, 16 Feb 2017 16:24:32 -0600
> Alan Tull [e-mail] wrote:
>
>>On Tue, Feb 14, 2017 at 11:07 AM, Anatolij Gustschin <[e-mail]> wrote:
>>
>>>>> +    delay_us = 0;
>>>>> +    while (1) {
>>>>> +            pci_read_config_word(pdev, VSEC_CVP_STATUS, &val16);
>>>>> +            if ((val16 & VSEC_CVP_STATUS_CFG_RDY) == 0)
>>>>> +                    break;
>>>>> +
>>>>> +            udelay(1);      /* wait 1us */
>>>>> +
>>>>> +            if (delay_us++ > TIMEOUT_IN_US) {
>>>>
>>>>Alan Tull has a patch where this timeout is passed in as part of the
>>>>struct fpga_image_info.  Would that help here?
>>>
>>> yes, this makes sense.
>>
>>Actually on second thought, it may not be necessary or helpful to pass
>>in the timeout.  There are timeouts in the fpga_image_info struct, but
>>they are there because different FPGA designs will have longer or
>>shorter timeouts.  If this is really just a fixed timeout, we don't
>>need to do that here.
>
> The timeout in fpga_image_info is set from the DT overlay, I'm not
> using overlays for CvP. It seems that this timeout is fixed, the
> Driver Flow figure in UG mentions 2ms timeout. I'll stick to fixed
> timeout.

Sounds good.

Alan

>
> Thanks,
> Anatolij
--
To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
index ce861a2..7ef5b06 100644
--- a/drivers/fpga/Kconfig
+++ b/drivers/fpga/Kconfig
@@ -33,6 +33,13 @@  config FPGA_MGR_SOCFPGA_A10
 	help
 	  FPGA manager driver support for Altera Arria10 SoCFPGA.
 
+config FPGA_MGR_STRATIX_CVP
+	tristate "Altera Stratix V CvP"
+	depends on PCI
+	help
+	  FPGA manager driver support for Altera Stratix V using the
+	  CvP interface over PCIe
+
 config FPGA_MGR_ZYNQ_FPGA
 	tristate "Xilinx Zynq FPGA"
 	depends on ARCH_ZYNQ || COMPILE_TEST
diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
index 8df07bc..0444c78 100644
--- a/drivers/fpga/Makefile
+++ b/drivers/fpga/Makefile
@@ -8,6 +8,7 @@  obj-$(CONFIG_FPGA)			+= fpga-mgr.o
 # FPGA Manager Drivers
 obj-$(CONFIG_FPGA_MGR_SOCFPGA)		+= socfpga.o
 obj-$(CONFIG_FPGA_MGR_SOCFPGA_A10)	+= socfpga-a10.o
+obj-$(CONFIG_FPGA_MGR_STRATIX_CVP)	+= stratix-cvp.o
 obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA)	+= zynq-fpga.o
 
 # FPGA Bridge Drivers
diff --git a/drivers/fpga/stratix-cvp.c b/drivers/fpga/stratix-cvp.c
new file mode 100644
index 0000000..5ab3499
--- /dev/null
+++ b/drivers/fpga/stratix-cvp.c
@@ -0,0 +1,529 @@ 
+/*
+ * Copyright (C) 2017 DENX Software Engineering
+ *
+ * Anatolij Gustschin <[e-mail]>
+ *
+ * Manage Altera Statix V FPGA firmware using PCIe CvP.
+ * Firmware must be in binary "rbf" format.
+ */
+
+#include <linux/delay.h>
+#include <linux/fpga/fpga-mgr.h>
+#include <linux/module.h>
+#include <linux/sizes.h>
+#include <linux/pci.h>
+
+/* Vendor Specific Extended Capability Offset */
+#define VSEC_OFFSET			0x200
+#define VSEC_PCIE_EXT_CAP_ID_VAL	0x000b
+
+#define FNC		0	/* PCIe device function ID */
+#define CVP_BAR		0	/* BAR used for data transfer in memory mode */
+#define CVP_DUMMY_WR	244	/* dummy writes to clear CvP state machine */
+#define TIMEOUT_IN_US	10000
+
+/* offsets from VSEC_OFFSET */
+#define VSEC_PCIE_EXT_CAP_ID		(VSEC_OFFSET + 0x00)	/* 16bit */
+#define VSEC_VERSION			0x02		/* 8bit */
+#define VSEC_NEXT_CAP_OFF		0x03		/* 8bit */
+#define VSEC_ID				0x04		/* 16bit */
+#define VSEC_REV			0x06		/* 8bit */
+#define VSEC_LENGTH			0x07		/* 8bit */
+#define VSEC_ALTERA_MARKER		0x08		/* 32bit */
+
+#define VSEC_CVP_STATUS			(VSEC_OFFSET + 0x1e)	/* 16bit */
+#define VSEC_CVP_STATUS_DATA_ENC	(1<<0)	/* is treated as encrypted */
+#define VSEC_CVP_STATUS_DATA_COMP	(1<<1)	/* is treated as compressed */
+#define VSEC_CVP_STATUS_CFG_RDY		(1<<2)	/* CVP_CONFIG_READY */
+#define VSEC_CVP_STATUS_CFG_ERR		(1<<3)	/* CVP_CONFIG_ERROR */
+#define VSEC_CVP_STATUS_CVP_EN		(1<<4)	/* ctrl block is enabling CVP */
+#define VSEC_CVP_STATUS_USERMODE	(1<<5)	/* USERMODE */
+#define VSEC_CVP_STATUS_CFG_DONE	(1<<7)	/* CVP_CONFIG_DONE */
+#define VSEC_CVP_STATUS_PLD_CLK_IN_USE	(1<<8)	/* PLD_CLK_IN_USE */
+
+#define VSEC_CVP_MODE_CTRL		(VSEC_OFFSET + 0x20)	/* 32bit */
+#define VSEC_CVP_MODE_CTRL_CVPMODE	(1<<0) /* CVP (1) or normal mode (0) */
+#define VSEC_CVP_MODE_CTRL_HIP_CLK_SEL	(1<<1) /* PMA (1) or fabric clock (0) */
+#define VSEC_CVP_MODE_CTRL_FULL_CFG	(1<<2) /* CVP_FULLCONFIG */
+#define VSEC_CVP_MODE_CTRL_NUMCLKS	(0xff<<8) /* CVP_NUMCLKS */
+
+#define VSEC_CVP_DATA			(VSEC_OFFSET + 0x28)	/* 32bit */
+#define VSEC_CVP_PROG_CTRL		(VSEC_OFFSET + 0x2c)	/* 32bit */
+#define VSEC_CVP_PROG_CTRL_CONFIG	(1<<0)
+#define VSEC_CVP_PROG_CTRL_START_XFER	(1<<1)
+
+#define VSEC_UNCOR_ERR_STATUS		(VSEC_OFFSET + 0x34)	/* 32bit */
+#define VSEC_UNCOR_ERR_MASK		(VSEC_OFFSET + 0x38)	/* 32bit */
+#define	VSEC_UNCOR_ERR_CVP_CFG_ERR	(1<<5)	/* CVP_CONFIG_ERROR_LATCHED */
+
+#define	DRV_NAME		"stratix-cvp"
+#define	FIRMWARE_STR		"stratixv_cvp.rbf"
+#define STRATIX_V_CVP_MGR_NAME	"Stratix V CvP FPGA Manager"
+
+static char *firmware = FIRMWARE_STR;
+module_param(firmware, charp, 0664);
+MODULE_PARM_DESC(firmware, "RBF file under /lib/firmware for loading via CvP.");
+
+static int chkcfg; /* use value 1 for debugging only */
+module_param(chkcfg, int, 0664);
+MODULE_PARM_DESC(chkcfg, "1 - check CvP status, 0 - skip checking (default 0)");
+
+struct stratix_cvp_conf {
+	struct fpga_manager	*mgr;
+	struct pci_dev		*pci_dev;
+	void __iomem		*map;
+	resource_size_t		map_base;
+	resource_size_t		map_len;
+};
+
+static enum fpga_mgr_states stratix_cvp_state(struct fpga_manager *mgr)
+{
+	return mgr->state;
+}
+
+static int stratix_cvp_write_init(struct fpga_manager *mgr,
+				  struct fpga_image_info *info,
+				  const char *buf, size_t count)
+{
+	struct stratix_cvp_conf *conf = mgr->priv;
+	struct pci_bus *bus = conf->pci_dev->bus;
+	struct pci_dev *pdev = conf->pci_dev;
+	int delay_us, i;
+	u32 val32;
+	u16 val16;
+	u32 data;
+
+	if (info && info->flags & FPGA_MGR_PARTIAL_RECONFIG) {
+		dev_err(&mgr->dev, "Partial reconfiguration not supported.\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * STEP 1 - read CVP status and check CVP_EN flag
+	 */
+	pci_bus_read_config_word(bus, FNC, VSEC_CVP_STATUS, &val16);
+	if (!(val16 & VSEC_CVP_STATUS_CVP_EN)) {
+		dev_err(&mgr->dev, "CVP mode off: 0x%04x\n", val16);
+		return -EINVAL;
+	}
+
+	/*
+	 * STEP 2
+	 * - set HIP_CLK_SEL and CVP_MODE (must be set in the order mentioned)
+	 */
+	/* switch from fabric to PMA clock */
+	pci_read_config_dword(pdev, VSEC_CVP_MODE_CTRL, &val32);
+	val32 |= VSEC_CVP_MODE_CTRL_HIP_CLK_SEL;
+	pci_write_config_dword(pdev, VSEC_CVP_MODE_CTRL, val32);
+
+	/* set CVP mode */
+	pci_read_config_dword(pdev, VSEC_CVP_MODE_CTRL, &val32);
+	val32 |= VSEC_CVP_MODE_CTRL_CVPMODE;
+	pci_write_config_dword(pdev, VSEC_CVP_MODE_CTRL, val32);
+
+	/*
+	 * STEP 3
+	 * - set CVP_NUMCLKS to 1 and issue CVP_DUMMY_WR dummy writes to the HIP
+	 */
+	pci_read_config_dword(pdev, VSEC_CVP_MODE_CTRL, &val32);
+	val32 &= ~VSEC_CVP_MODE_CTRL_NUMCLKS;
+	val32 |= 0x01 << 8;	/* 1 clock */
+	pci_write_config_dword(pdev, VSEC_CVP_MODE_CTRL, val32);
+
+	/* dummy memory write to switch from internal clock to CVP clock */
+	val32 = 0xdeadbeef;
+	if (conf->map) {
+		u32 *map = conf->map;
+
+		for (i = 0; i < CVP_DUMMY_WR; i++)
+			*map++ = val32; /* use MemoryWrite */
+	} else {
+		for (i = 0; i < CVP_DUMMY_WR; i++)
+			pci_write_config_dword(pdev, VSEC_CVP_DATA, val32);
+	}
+
+	/*
+	 * STEP 4 - set CVP_CONFIG bit
+	 */
+	pci_read_config_dword(pdev, VSEC_CVP_PROG_CTRL, &val32);
+	/* request control block to begin transfer using CVP */
+	val32 |= VSEC_CVP_PROG_CTRL_CONFIG;
+	pci_write_config_dword(pdev, VSEC_CVP_PROG_CTRL, val32);
+
+	/*
+	 * STEP 5 - poll CVP_CONFIG READY
+	 */
+	delay_us = 0;
+	while (1) {
+		pci_read_config_word(pdev, VSEC_CVP_STATUS, &val16);
+		if ((val16 & VSEC_CVP_STATUS_CFG_RDY) ==
+		    VSEC_CVP_STATUS_CFG_RDY)
+			break;
+
+		udelay(1); /* wait 1us */
+
+		if (delay_us++ > TIMEOUT_IN_US) {
+			dev_warn(&mgr->dev, "CVP_CONFIG_READY == 1 timeout\n");
+			return -ETIMEDOUT;
+		}
+	}
+
+	/*
+	 * STEP 6
+	 * - set CVP_NUMCLKS to 1 and issue CVP_DUMMY_WR dummy writes to the HIP
+	 */
+	pci_read_config_dword(pdev, VSEC_CVP_MODE_CTRL, &val32);
+	val32 &= ~VSEC_CVP_MODE_CTRL_NUMCLKS;
+	val32 |= 0x01 << 8; /* 1 clock */
+	pci_write_config_dword(pdev, VSEC_CVP_MODE_CTRL, val32);
+
+	/* dummy memory write to switch from internal clock to CVP clock */
+	val32 = 0xdeadbeef;
+	if (conf->map) {
+		u32 *map = conf->map;
+
+		for (i = 0; i < CVP_DUMMY_WR; i++)
+			*map++ = val32; /* use MemoryWrite */
+	} else {
+		for (i = 0; i < CVP_DUMMY_WR; i++)
+			pci_write_config_dword(pdev, VSEC_CVP_DATA, val32);
+	}
+
+	/*
+	 * STEP 7 - set START_XFER
+	 */
+	pci_read_config_dword(pdev, VSEC_CVP_PROG_CTRL, &val32);
+	val32 |= VSEC_CVP_PROG_CTRL_START_XFER;
+	pci_write_config_dword(pdev, VSEC_CVP_PROG_CTRL, val32);
+
+	/*
+	 * STEP 8 (start transfer)
+	 * check required bitstream type - read CVP status and set CVP_NUMCLKS
+	 * accordingly
+	 *	Compressed	Encrypted	CVP_NUMCLKS
+	 *	No		No		1
+	 *	Yes		No		8
+	 *	No		Yes		4
+	 *	Yes		Yes		8
+	 */
+	pci_read_config_word(pdev, VSEC_CVP_STATUS, &val16);
+	val16 &= (VSEC_CVP_STATUS_DATA_ENC | VSEC_CVP_STATUS_DATA_COMP);
+	switch (val16) {
+	case VSEC_CVP_STATUS_DATA_ENC:
+		data = 4;
+		break;
+	case VSEC_CVP_STATUS_DATA_COMP:
+	case (VSEC_CVP_STATUS_DATA_ENC | VSEC_CVP_STATUS_DATA_COMP):
+		data = 8;
+		break;
+	default:
+		data = 1;
+		break;
+	}
+
+	/* write number of CVP clock cycles according to bitstream type */
+	pci_read_config_dword(pdev, VSEC_CVP_MODE_CTRL, &val32);
+	val32 &= ~VSEC_CVP_MODE_CTRL_NUMCLKS;
+	val32 |= data << 8;	/* bitstream specific clock count */
+	pci_write_config_dword(pdev, VSEC_CVP_MODE_CTRL, val32);
+
+	return 0;
+}
+
+static inline int stratix_cvp_chk_cfg_error(struct fpga_manager *mgr,
+					    size_t bytes)
+{
+	struct stratix_cvp_conf *conf = mgr->priv;
+	u16 val16;
+
+	/*
+	 * STEP 10 (optional) - check CVP_CONFIG_ERROR flag
+	 */
+	pci_read_config_word(conf->pci_dev, VSEC_CVP_STATUS, &val16);
+	if (val16 & VSEC_CVP_STATUS_CFG_ERR) {
+		dev_err(&mgr->dev, " CVP_CONFIG_ERROR after %d bytes!\n",
+			bytes);
+		return -EPROTO;
+	}
+	return 0;
+}
+
+static int stratix_cvp_write(struct fpga_manager *mgr, const char *buf,
+			     size_t count)
+{
+	struct stratix_cvp_conf *conf = mgr->priv;
+	struct pci_dev *pdev = conf->pci_dev;
+	const u32 *data, *data_end;
+	size_t bytes = 0;
+	int status = 0;
+
+	/*
+	 * STEP 9
+	 * - write 32-bit configuration data from POF file to CVP data register
+	 */
+	data = (u32 *)buf;
+	data_end = (u32 *)((char *)buf + count);
+
+	if (conf->map) {
+		u32 *map = conf->map;
+
+		while (data < data_end) {
+			*map = *data++;
+			bytes += 4;
+
+			/*
+			 * STEP 10 (optional) and STEP 11
+			 * - check error flag
+			 * - loop until data transfer completed
+			 */
+			if (chkcfg) {
+				status = stratix_cvp_chk_cfg_error(mgr, bytes);
+				if (status < 0)
+					break;
+			}
+		}
+		return status;
+	}
+
+	while (data < data_end) {
+		pci_write_config_dword(pdev, VSEC_CVP_DATA, *data++);
+		bytes += 4;
+
+		/*
+		 * STEP 10 (optional) and STEP 11
+		 * - check error flag
+		 * - loop until data transfer completed
+		 */
+		if (chkcfg) {
+			status = stratix_cvp_chk_cfg_error(mgr, bytes);
+			if (status < 0)
+				break;
+		}
+	}
+
+	return status;
+}
+
+static int stratix_cvp_write_complete(struct fpga_manager *mgr,
+				      struct fpga_image_info *info)
+{
+	struct stratix_cvp_conf *conf = mgr->priv;
+	struct pci_dev *pdev = conf->pci_dev;
+	int status = 0;
+	int delay_us, i;
+	u32 val32;
+	u16 status_msk;
+	u16 val16;
+
+	/*
+	 * STEP 12 - reset START_XFER bit
+	 */
+	pci_read_config_dword(pdev, VSEC_CVP_PROG_CTRL, &val32);
+	val32 &= ~VSEC_CVP_PROG_CTRL_START_XFER;
+	pci_write_config_dword(pdev, VSEC_CVP_PROG_CTRL, val32);
+
+	/*
+	 * STEP 13 - reset CVP_CONFIG bit
+	 */
+	val32 &= ~VSEC_CVP_PROG_CTRL_CONFIG;
+	pci_write_config_dword(pdev, VSEC_CVP_PROG_CTRL, val32);
+
+	/*
+	 * STEP 14
+	 * - set CVP_NUMCLKS to 0x01 and then issue CVP_DUMMY_WR dummy
+	 *   writes to the HIP
+	 */
+	pci_read_config_dword(pdev, VSEC_CVP_MODE_CTRL, &val32);
+	val32 &= ~VSEC_CVP_MODE_CTRL_NUMCLKS;
+	/* set number of CVP clock cycles for every CVP Data Register Write */
+	val32 |= 0x01 << 8;	/* 1 clock */
+	pci_write_config_dword(pdev, VSEC_CVP_MODE_CTRL, val32);
+
+	/* dummy memory write to switch from CVP clock to internal clock */
+	val32 = 0xdeadbeef;
+	if (conf->map) {
+		u32 *map = conf->map;
+
+		for (i = 0; i < CVP_DUMMY_WR; i++)
+			*map++ = val32; /* use MemoryWrite */
+	} else {
+		for (i = 0; i < CVP_DUMMY_WR; i++)
+			pci_write_config_dword(pdev, VSEC_CVP_DATA, val32);
+	}
+
+	/*
+	 * STEP 15 - poll CVP_CONFIG_READY bit
+	 */
+	delay_us = 0;
+	while (1) {
+		pci_read_config_word(pdev, VSEC_CVP_STATUS, &val16);
+		if ((val16 & VSEC_CVP_STATUS_CFG_RDY) == 0)
+			break;
+
+		udelay(1);	/* wait 1us */
+
+		if (delay_us++ > TIMEOUT_IN_US) {
+			dev_warn(&mgr->dev, "CVP_CONFIG_READY == 0 timeout\n");
+			status = -ETIME;
+			break;
+		}
+	}
+
+	if (status < 0)
+		return status;
+
+	/*
+	 * STEP 16 - check CVP_CONFIG_ERROR_LATCHED bit
+	 */
+	pci_read_config_dword(pdev, VSEC_UNCOR_ERR_STATUS, &val32);
+	if (val32 & VSEC_UNCOR_ERR_CVP_CFG_ERR) {
+		dev_err(&mgr->dev, " detected CVP_CONFIG_ERROR_LATCHED!\n");
+		return -EFAULT;
+	}
+
+	/*
+	 * STEP 17 - reset CVP_MODE and HIP_CLK_SEL bit
+	 */
+	pci_read_config_dword(pdev, VSEC_CVP_MODE_CTRL, &val32);
+	val32 &= ~VSEC_CVP_MODE_CTRL_HIP_CLK_SEL;
+	val32 &= ~VSEC_CVP_MODE_CTRL_CVPMODE;
+	pci_write_config_dword(pdev, VSEC_CVP_MODE_CTRL, val32);
+
+	/*
+	 * STEP 18 - poll PLD_CLK_IN_USE and USER_MODE bits
+	 */
+	delay_us = 0;
+	status_msk = VSEC_CVP_STATUS_PLD_CLK_IN_USE | VSEC_CVP_STATUS_USERMODE;
+	while (1) {
+		pci_read_config_word(pdev, VSEC_CVP_STATUS, &val16);
+		if ((val16 & status_msk) == status_msk)
+			break;
+
+		udelay(1); /* wait 1us */
+
+		if (delay_us++ > TIMEOUT_IN_US) {
+			dev_warn(&mgr->dev, "PLD_CLK_IN_USE|USERMODE timeout\n");
+			status = -ETIME;
+			break;
+		}
+	}
+
+	return status;
+}
+
+static const struct fpga_manager_ops stratix_cvp_ops = {
+	.state		= stratix_cvp_state,
+	.write_init	= stratix_cvp_write_init,
+	.write		= stratix_cvp_write,
+	.write_complete	= stratix_cvp_write_complete,
+};
+
+static int stratix_cvp_probe(struct pci_dev *pdev,
+			     const struct pci_device_id *dev_id)
+{
+	struct stratix_cvp_conf *conf;
+	u16 cmd, val16;
+	int ret;
+
+	/* Enable memory BAR access */
+	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
+	if (!(cmd & PCI_COMMAND_MEMORY)) {
+		cmd |= PCI_COMMAND_MEMORY;
+		pci_write_config_word(pdev, PCI_COMMAND, cmd);
+	}
+
+	conf = devm_kzalloc(&pdev->dev, sizeof(*conf), GFP_KERNEL);
+	if (!conf) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	pci_bus_read_config_word(pdev->bus, FNC, VSEC_PCIE_EXT_CAP_ID, &val16);
+	if (val16 != VSEC_PCIE_EXT_CAP_ID_VAL) {
+		dev_err(&pdev->dev, "Wrong EXT_CAP_ID value 0x%x\n", val16);
+		ret = -ENODEV;
+		goto err;
+	}
+
+	conf->pci_dev = pdev;
+	conf->map_base = pci_resource_start(pdev, CVP_BAR);
+	conf->map_len = pci_resource_len(pdev, CVP_BAR);
+
+	if (pci_request_region(pdev, CVP_BAR, "CVP") < 0) {
+		dev_err(&pdev->dev, "Requesting CVP BAR region failed\n");
+		ret = -ENODEV;
+		goto err;
+	}
+
+	conf->map = ioremap_nocache(conf->map_base, conf->map_len);
+	if (!conf->map)
+		dev_warn(&pdev->dev, "Mapping CVP BAR failed\n");
+
+	ret = fpga_mgr_register(&pdev->dev, STRATIX_V_CVP_MGR_NAME,
+				&stratix_cvp_ops, conf);
+	if (ret)
+		goto err_unmap;
+
+	conf->mgr = fpga_mgr_get(&pdev->dev);
+	if (IS_ERR(conf->mgr)) {
+		dev_err(&pdev->dev, "Getting fpga mgr reference failed\n");
+		ret = -ENODEV;
+		goto err_unmap;
+	}
+
+	ret = fpga_mgr_firmware_load(conf->mgr, NULL, firmware);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to load fpga image\n");
+		goto err_mgr;
+	}
+
+	return 0;
+
+err_mgr:
+	fpga_mgr_put(conf->mgr);
+	fpga_mgr_unregister(&pdev->dev);
+err_unmap:
+	iounmap(conf->map);
+	pci_release_region(pdev, CVP_BAR);
+err:
+	cmd &= ~PCI_COMMAND_MEMORY;
+	pci_write_config_word(pdev, PCI_COMMAND, cmd);
+	return ret;
+}
+
+static void stratix_cvp_remove(struct pci_dev *pdev)
+{
+	struct fpga_manager *mgr = pci_get_drvdata(pdev);
+	struct stratix_cvp_conf *conf = mgr->priv;
+	u16 cmd;
+
+	fpga_mgr_put(conf->mgr);
+	fpga_mgr_unregister(&pdev->dev);
+	iounmap(conf->map);
+	pci_release_region(pdev, CVP_BAR);
+	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
+	cmd &= ~PCI_COMMAND_MEMORY;
+	pci_write_config_word(pdev, PCI_COMMAND, cmd);
+}
+
+#define PCI_VENDOR_ID_ALTERA	0x1172
+#define PCI_DEVICE_ID_FPGA	0xe001
+
+static struct pci_device_id stratix_cvp_id_tbl[] = {
+	{ PCI_VDEVICE(ALTERA, PCI_DEVICE_ID_FPGA) },
+	{ }
+};
+
+static struct pci_driver stratix_cvp_driver = {
+	.name   = DRV_NAME,
+	.id_table = stratix_cvp_id_tbl,
+	.probe  = stratix_cvp_probe,
+	.remove = stratix_cvp_remove,
+};
+
+module_pci_driver(stratix_cvp_driver)
+
+MODULE_DEVICE_TABLE(pci, stratix_cvp_id_tbl);
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Anatolij Gustschin <[e-mail]>");
+MODULE_DESCRIPTION("Module to load Altera FPGA over CvP");