[v2] fpga manager: Add Altera V series CvP driver

Message ID	1487595903-13876-1-git-send-email-agust@denx.de (mailing list archive)
State	Superseded, archived
Headers	show Return-Path: <linux-fpga-owner@kernel.org> From: Anatolij Gustschin <agust@denx.de> To: linux-fpga@vger.kernel.org Cc: Alan Tull <atull@opensource.altera.com>, matthew.gerlach@linux.intel.com, Yi Li <yi1.li@linux.intel.com>, Moritz Fischer <moritz.fischer@ettus.com> Subject: [PATCH v2] fpga manager: Add Altera V series CvP driver Date: Mon, 20 Feb 2017 14:05:03 +0100 Message-Id: <1487595903-13876-1-git-send-email-agust@denx.de> Sender: linux-fpga-owner@vger.kernel.org Precedence: bulk

Anatolij Gustschin Feb. 20, 2017, 1:05 p.m. UTC

Add FPGA manager driver for loading V-series FPGAs using CvP.

Signed-off-by: Anatolij Gustschin <agust@denx.de>
---
Changes in v2:

  - rebase for v4.10, change subject ("Stratix V" to "V series")
    and add GPL header

  - use BIT() in register bit macro definitions

  - change .state() to return status or FPGA_MGR_STATE_UNKNOWN

  - use unique name for registered FPGA manager (append "@B:D.F")

  - use PCI_ANY_ID for device ID, users can set custom ID using
    new_id sysfs interface

  - remove map_base/map_len init and use pci_iomap() instead

  - simplify to use pci_read_config_word() instead of
    pci_bus_read_config_word()

  - run fpga_mgr_put() after usage, so the module can be removed
    without unbinding the driver

  - access CVP_STATUS as a 32-bit register, update CVP_STATUS
    bits macros

  - check CONFIG_READY bit in write_init and perform a teardown
    if this bit is set. Move teardown code to separate function
    as suggested

  - change function names to altera_v_*() as we can use
    the driver with other V-series FPGAs. Also change
    the driver file and Kconfig option names accordingly

  - pad written firmware data if the file size is not a multiple of 4

  - when config error checking is enabled, run checks after a 4KiB
    chunk is written

  - remove single built-in firmware string, add module parameter
    to allow setting default firmware for multiple cards as a
    Bus:Device.Function specific strings (optional)

  - remove polling of non-existing data bits DATA_ENC, DATA_COMP.
    Instead, add module parameter for bitstream specific clock to
    data ratio setting

 drivers/fpga/Kconfig        |   7 +
 drivers/fpga/Makefile       |   1 +
 drivers/fpga/altera-v-cvp.c | 657 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 665 insertions(+)
 create mode 100644 drivers/fpga/altera-v-cvp.c

Li, Yi Feb. 21, 2017, 8:58 p.m. UTC | #1

hi Anatolij


On 2/20/2017 7:05 AM, Anatolij Gustschin wrote:
> Add FPGA manager driver for loading V-series FPGAs using CvP.
>
> Signed-off-by: Anatolij Gustschin <agust@denx.de>
> ---
> Changes in v2:
>
>    - rebase for v4.10, change subject ("Stratix V" to "V series")
>      and add GPL header
>
>    - use BIT() in register bit macro definitions
>
>    - change .state() to return status or FPGA_MGR_STATE_UNKNOWN
>
>    - use unique name for registered FPGA manager (append "@B:D.F")
>
>    - use PCI_ANY_ID for device ID, users can set custom ID using
>      new_id sysfs interface
>
>    - remove map_base/map_len init and use pci_iomap() instead
>
>    - simplify to use pci_read_config_word() instead of
>      pci_bus_read_config_word()
>
>    - run fpga_mgr_put() after usage, so the module can be removed
>      without unbinding the driver
>
>    - access CVP_STATUS as a 32-bit register, update CVP_STATUS
>      bits macros
>
>    - check CONFIG_READY bit in write_init and perform a teardown
>      if this bit is set. Move teardown code to separate function
>      as suggested
>
>    - change function names to altera_v_*() as we can use
>      the driver with other V-series FPGAs. Also change
>      the driver file and Kconfig option names accordingly
>
>    - pad written firmware data if the file size is not a multiple of 4
>
>    - when config error checking is enabled, run checks after a 4KiB
>      chunk is written
>
>    - remove single built-in firmware string, add module parameter
>      to allow setting default firmware for multiple cards as a
>      Bus:Device.Function specific strings (optional)
>
>    - remove polling of non-existing data bits DATA_ENC, DATA_COMP.
>      Instead, add module parameter for bitstream specific clock to
>      data ratio setting
>
>   drivers/fpga/Kconfig        |   7 +
>   drivers/fpga/Makefile       |   1 +
>   drivers/fpga/altera-v-cvp.c | 657 ++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 665 insertions(+)
>   create mode 100644 drivers/fpga/altera-v-cvp.c
>
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index ce861a2..be93f04 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -20,6 +20,13 @@ config FPGA_REGION
>   	  FPGA Regions allow loading FPGA images under control of
>   	  the Device Tree.
>   
> +config FPGA_MGR_ALTERA_V_CVP
> +	tristate "Altera V-series CvP FPGA Manager"
> +	depends on PCI
> +	help
> +	  FPGA manager driver support for Altera V-series FPGAs
> +	  using the CvP interface over PCIe
> +
>   config FPGA_MGR_SOCFPGA
>   	tristate "Altera SOCFPGA FPGA Manager"
>   	depends on ARCH_SOCFPGA || COMPILE_TEST
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index 8df07bc..b70f8ff 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -6,6 +6,7 @@
>   obj-$(CONFIG_FPGA)			+= fpga-mgr.o
>   
>   # FPGA Manager Drivers
> +obj-$(CONFIG_FPGA_MGR_ALTERA_V_CVP)	+= altera-v-cvp.o
>   obj-$(CONFIG_FPGA_MGR_SOCFPGA)		+= socfpga.o
>   obj-$(CONFIG_FPGA_MGR_SOCFPGA_A10)	+= socfpga-a10.o
>   obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA)	+= zynq-fpga.o
> diff --git a/drivers/fpga/altera-v-cvp.c b/drivers/fpga/altera-v-cvp.c
> new file mode 100644
> index 0000000..3e53051
> --- /dev/null
> +++ b/drivers/fpga/altera-v-cvp.c
> @@ -0,0 +1,657 @@
> +/*
> + * FPGA Manager Driver for Altera V-Series CvP
> + *
> + * Copyright (C) 2017 DENX Software Engineering
> + *
> + * Anatolij Gustschin <agust@denx.de>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; version 2 of the License.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * Manage V-Series 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/pci.h>
> +#include <linux/sizes.h>
> +
> +#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	2000
> +
> +/* Vendor Specific Extended Capability Offset */
> +#define VSEC_OFFSET			0x200
> +#define VSEC_PCIE_EXT_CAP_ID_VAL	0x000b
> +
> +/* 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 + 0x1c)	/* 32bit */
> +#define VSEC_CVP_STATUS_DATA_ENC	BIT(16)	/* is treated as encrypted */
> +#define VSEC_CVP_STATUS_DATA_COMP	BIT(17)	/* is treated as compressed */
> +#define VSEC_CVP_STATUS_CFG_RDY		BIT(18)	/* CVP_CONFIG_READY */
> +#define VSEC_CVP_STATUS_CFG_ERR		BIT(19)	/* CVP_CONFIG_ERROR */
> +#define VSEC_CVP_STATUS_CVP_EN		BIT(20)	/* ctrl block is enabling CVP */
> +#define VSEC_CVP_STATUS_USERMODE	BIT(21)	/* USERMODE */
> +#define VSEC_CVP_STATUS_CFG_DONE	BIT(23)	/* CVP_CONFIG_DONE */
> +#define VSEC_CVP_STATUS_PLD_CLK_IN_USE	BIT(24)	/* PLD_CLK_IN_USE */
> +
> +#define VSEC_CVP_MODE_CTRL		(VSEC_OFFSET + 0x20)	/* 32bit */
> +#define VSEC_CVP_MODE_CTRL_CVP_MODE	BIT(0) /* CVP (1) or normal mode (0) */
> +#define VSEC_CVP_MODE_CTRL_HIP_CLK_SEL	BIT(1) /* PMA (1) or fabric clock (0) */
> +#define VSEC_CVP_MODE_CTRL_FULL_CFG	BIT(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	BIT(0)
> +#define VSEC_CVP_PROG_CTRL_START_XFER	BIT(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	BIT(5)	/* CVP_CONFIG_ERROR_LATCHED */
> +
> +#define	DRV_NAME		"altera-v-cvp"
> +#define ALTERA_V_CVP_MGR_NAME	"V-Series CvP FPGA Manager"
> +
> +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)");
> +
> +static int numclks = 1; /* default 1 for uncompressed and unencrypted */
> +module_param(numclks, int, 0664);
> +MODULE_PARM_DESC(numclks, "Clock to data ratio 1, 4 or 8 (default 1)");
> +
> +#define MAX_FPGAS	4
> +static char *fw_list[MAX_FPGAS];
> +static int fw_list_n;
> +module_param_array_named(fw, fw_list, charp, &fw_list_n, 0444);
> +MODULE_PARM_DESC(fw, "Mapping of BDF to firmware name (fw=B:D.F-card_cvp.rbf)");

I like it. :)

> +
> +struct altera_v_cvp_fw_param {
> +	int bus;
> +	int dev;
> +	int func;
> +	const char *fw_file;
> +};
> +
> +struct altera_v_cvp_fw_param fw_par[MAX_FPGAS] = {
> +	{ 6, 0, 0, "altera_v_cvp.rbf" }, /* default firmware */
> +};
> +
> +struct altera_v_cvp_conf {
> +	struct fpga_manager	*mgr;
> +	struct pci_dev		*pci_dev;
> +	void __iomem		*map;
> +	const char		*firmware;
> +	char			mgr_name[64];
> +};
> +
> +static void fw_param_format_err(int idx)
> +{
> +	pr_warn("Wrong format in param %d: %s\n", idx, fw_list[idx]);
> +}
> +
> +static void altera_v_cvp_parse_fw_param(void)
> +{
> +	size_t par_size = ARRAY_SIZE(fw_list);
> +	struct altera_v_cvp_fw_param p;
> +	size_t par_num;
> +	const char *fw;
> +	int i, ret;
> +
> +	if (!fw_list_n)
> +		return;
> +
> +	par_num = fw_list_n;
> +	if (par_num > par_size)
> +		par_num = par_size;
> +
> +	for (i = 0; i < par_num; i++) {
> +		fw = strchr(fw_list[i], '-');
> +		if (fw) {
> +			ret = sscanf(fw_list[i], "%d:%d.%d",
> +				     &p.bus, &p.dev, &p.func);
> +			if (ret == 3) {
> +				if (strnlen(fw + 1, NAME_MAX)) {
> +					p.fw_file = fw + 1;
> +					fw_par[i] = p;
> +				} else
> +					fw_param_format_err(i);
> +			} else
> +				fw_param_format_err(i);
> +		} else
> +			fw_param_format_err(i);
> +	}
> +}
> +
> +static void altera_v_cvp_set_dev_fw_param(struct altera_v_cvp_conf *conf)
> +{
> +	struct pci_dev *pdev = conf->pci_dev;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(fw_par); i++) {
> +		if (pdev->bus->number == fw_par[i].bus &&
> +		    PCI_SLOT(pdev->devfn) == fw_par[i].dev &&
> +		    PCI_FUNC(pdev->devfn) == fw_par[i].func) {
> +			conf->firmware = fw_par[i].fw_file;
> +			break;
> +		}
> +	}
> +}
> +
> +static inline void altera_v_cvp_chk_numclks(void)
> +{
> +	switch (numclks) {
> +	case 1:
> +	case 4:
> +	case 8:
> +		break;
> +	default:
> +		numclks = 1;
> +		break;
> +	}
> +}
> +
> +static enum fpga_mgr_states altera_v_cvp_state(struct fpga_manager *mgr)
> +{
> +	struct altera_v_cvp_conf *conf = mgr->priv;
> +	u32 status;
> +
> +	pci_read_config_dword(conf->pci_dev, VSEC_CVP_STATUS, &status);
> +
> +	if (status & VSEC_CVP_STATUS_CFG_DONE)
> +		return FPGA_MGR_STATE_OPERATING;
> +
> +	if (status & VSEC_CVP_STATUS_CVP_EN)
> +		return FPGA_MGR_STATE_POWER_UP;
> +
> +	return FPGA_MGR_STATE_UNKNOWN;
> +}
> +
> +static int altera_v_cvp_teardown(struct fpga_manager *mgr,
> +				 struct fpga_image_info *info)
> +{
> +	struct altera_v_cvp_conf *conf = mgr->priv;
> +	struct pci_dev *pdev = conf->pci_dev;
> +	int status = 0;
> +	int delay_us, i;
> +	u32 val32;
> +	u32 status_msk;
> +
> +	/*
> +	 * 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_dword(pdev, VSEC_CVP_STATUS, &val32);
> +		if ((val32 & 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 = -ETIMEDOUT;
> +			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_CVP_MODE;
> +	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_dword(pdev, VSEC_CVP_STATUS, &val32);
> +		if ((val32 & 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 = -ETIMEDOUT;
> +			break;
> +		}
> +	}
> +
> +	return status;

I might not explain clearly before.  At least Step 18 should not belong 
to the teardown function. If the CvP is not programmed, the FPGA will 
not get into the user mode.
Suggest to move the Step 16 to Step 18 out of the teardown function to 
write_complete function.

> +}
> +
> +static int altera_v_cvp_write_init(struct fpga_manager *mgr,
> +				   struct fpga_image_info *info,
> +				   const char *buf, size_t count)
> +{
> +	struct altera_v_cvp_conf *conf = mgr->priv;
> +	struct pci_dev *pdev = conf->pci_dev;
> +	int delay_us, i;
> +	u32 val32;
> +	int ret;
> +
> +	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_read_config_dword(pdev, VSEC_CVP_STATUS, &val32);
> +	if (!(val32 & VSEC_CVP_STATUS_CVP_EN)) {
> +		dev_err(&mgr->dev, "CVP mode off: 0x%04x\n", val32);
> +		return -ENODEV;
> +	}
> +
> +	if (val32 & VSEC_CVP_STATUS_CFG_RDY) {
> +		dev_warn(&mgr->dev, "CvP already started, teardown first\n");
> +		ret = altera_v_cvp_teardown(mgr, info);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	/*
> +	 * 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_CVP_MODE;
> +	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_dword(pdev, VSEC_CVP_STATUS, &val32);
> +		if ((val32 & 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 (set CVP_NUMCLKS)
> +	 */
> +	altera_v_cvp_chk_numclks();
> +	pci_read_config_dword(pdev, VSEC_CVP_MODE_CTRL, &val32);
> +	val32 &= ~VSEC_CVP_MODE_CTRL_NUMCLKS;
> +	val32 |= numclks << 8; /* bitstream specific clock count */
> +	pci_write_config_dword(pdev, VSEC_CVP_MODE_CTRL, val32);
> +
> +	return 0;
> +}
> +
> +static inline int altera_v_cvp_chk_error(struct fpga_manager *mgr,
> +					 size_t bytes)
> +{
> +	struct altera_v_cvp_conf *conf = mgr->priv;
> +	u32 val32;
> +
> +	/*
> +	 * STEP 10 (optional) - check CVP_CONFIG_ERROR flag
> +	 */
> +	pci_read_config_dword(conf->pci_dev, VSEC_CVP_STATUS, &val32);
> +	if (val32 & VSEC_CVP_STATUS_CFG_ERR) {
> +		dev_err(&mgr->dev, "CVP_CONFIG_ERROR after %d bytes!\n",
> +			bytes);

One minor compilation warning:
drivers/fpga/altera-v-cvp.c:396:22: warning: format '%d' expects 
argument of type 'int', but argument 3 has type 'size_t {aka long 
unsigned int}' [-Wformat=]
    dev_err(&mgr->dev, "CVP_CONFIG_ERROR after %d bytes!\n",

> +		return -EPROTO;
> +	}
> +	return 0;
> +}
> +
> +static int altera_v_cvp_write(struct fpga_manager *mgr, const char *buf,
> +			      size_t count)
> +{
> +	struct altera_v_cvp_conf *conf = mgr->priv;
> +	struct pci_dev *pdev = conf->pci_dev;
> +	const u32 *data;
> +	size_t bytes = 0;
> +	int status = 0;
> +	u32 mask;
> +	u32 *map;
> +
> +	/*
> +	 * STEP 9
> +	 * - write 32-bit configuration data from RBF file to CVP data register
> +	 */
> +	data = (u32 *)buf;
> +	map = conf->map;
> +	bytes = count;
> +
> +	if (map) {
> +		while (bytes >= 4) {
> +			*map = *data++;
> +			bytes -= 4;
> +
> +			/*
> +			 * STEP 10 (optional) and STEP 11
> +			 * - check error flag
> +			 * - loop until data transfer completed
> +			 */
> +			if (chkcfg && !(bytes % SZ_4K)) {
> +				size_t done = count - bytes;
> +
> +				status = altera_v_cvp_chk_error(mgr, done);
> +				if (status < 0)
> +					return status;
> +			}
> +		}
> +	} else {
> +		while (bytes >= 4) {
> +			pci_write_config_dword(pdev, VSEC_CVP_DATA, *data++);
> +			bytes -= 4;
> +
> +			if (chkcfg && !(bytes % SZ_4K)) {
> +				size_t done = count - bytes;
> +
> +				status = altera_v_cvp_chk_error(mgr, done);
> +				if (status < 0)
> +					return status;
> +			}
> +		}
> +	}

Should we put the "if (map) else" check inside the while loop to avoid 
duplicating code?

> +
> +	switch (bytes) {
> +	case 3:
> +		mask = 0x00ffffff;
> +		break;
> +	case 2:
> +		mask = 0x0000ffff;
> +		break;
> +	case 1:
> +		mask = 0x000000ff;
> +		break;
> +	case 0:
> +		mask = 0x00000000;
> +		break;
> +	}
> +
> +	if (mask) {
> +		u32 tail = *data & mask;
> +
> +		if (map)
> +			*map = tail;
> +		else
> +			pci_write_config_dword(pdev, VSEC_CVP_DATA, tail);
> +
> +		if (chkcfg)
> +			status = altera_v_cvp_chk_error(mgr, count);
> +	}
> +
> +	return status;
> +}
> +
> +static int altera_v_cvp_write_complete(struct fpga_manager *mgr,
> +				       struct fpga_image_info *info)
> +{
> +	return altera_v_cvp_teardown(mgr, info);
> +}
> +

Maybe adding a print to show the CvP programming status will be useful?
     if (!status)
         dev_info(&mgr->dev, "CvP programming successful\n");

> +static const struct fpga_manager_ops altera_v_cvp_ops = {
> +	.state		= altera_v_cvp_state,
> +	.write_init	= altera_v_cvp_write_init,
> +	.write		= altera_v_cvp_write,
> +	.write_complete	= altera_v_cvp_write_complete,
> +};
> +
> +static int altera_v_cvp_probe(struct pci_dev *pdev,
> +			      const struct pci_device_id *dev_id)
> +{
> +	struct altera_v_cvp_conf *conf;
> +	u16 cmd, val16;
> +	int ret;
> +
> +	pci_read_config_word(pdev, 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;
> +	}
> +
> +	/* 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;
> +	}
> +
> +	conf->pci_dev = pdev;
> +
> +	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 = pci_iomap(pdev, CVP_BAR, 0);
> +	if (!conf->map)
> +		dev_warn(&pdev->dev, "Mapping CVP BAR failed\n");
> +
> +	snprintf(conf->mgr_name, sizeof(conf->mgr_name), "%s @%02d:%02d.%d",
> +		 ALTERA_V_CVP_MGR_NAME, pdev->bus->number,
> +		 PCI_SLOT(pdev->devfn), PCI_FUNC(pdev->devfn));
> +
> +	ret = fpga_mgr_register(&pdev->dev, conf->mgr_name,
> +				&altera_v_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_unreg;
> +	}
> +
> +	altera_v_cvp_set_dev_fw_param(conf);
> +	if (conf->firmware)
> +		fpga_mgr_firmware_load(conf->mgr, NULL, conf->firmware);
> +	fpga_mgr_put(conf->mgr);
> +
> +	return 0;
> +
> +err_unreg:
> +	fpga_mgr_unregister(&pdev->dev);
> +err_unmap:
> +	pci_iounmap(pdev, 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 altera_v_cvp_remove(struct pci_dev *pdev)
> +{
> +	struct fpga_manager *mgr = pci_get_drvdata(pdev);
> +	struct altera_v_cvp_conf *conf = mgr->priv;
> +	u16 cmd;
> +
> +	fpga_mgr_unregister(&pdev->dev);
> +	pci_iounmap(pdev, 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
> +
> +static struct pci_device_id altera_v_cvp_id_tbl[] = {
> +	{ PCI_VDEVICE(ALTERA, PCI_ANY_ID) },
> +	{ }
> +};
> +
> +MODULE_DEVICE_TABLE(pci, altera_v_cvp_id_tbl);
> +
> +static struct pci_driver altera_v_cvp_driver = {
> +	.name   = DRV_NAME,
> +	.id_table = altera_v_cvp_id_tbl,
> +	.probe  = altera_v_cvp_probe,
> +	.remove = altera_v_cvp_remove,
> +};
> +
> +static int __init altera_v_cvp_init(void)
> +{
> +	altera_v_cvp_parse_fw_param();
> +	return pci_register_driver(&altera_v_cvp_driver);
> +}
> +
> +static void __exit altera_v_cvp_exit(void)
> +{
> +	pci_unregister_driver(&altera_v_cvp_driver);
> +}
> +
> +module_init(altera_v_cvp_init);
> +module_exit(altera_v_cvp_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Anatolij Gustschin <agust@denx.de>");
> +MODULE_DESCRIPTION("Module to load Altera V-Series FPGA over CvP");

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

Anatolij Gustschin Feb. 21, 2017, 9:46 p.m. UTC | #2

Hi Yi,

On Tue, 21 Feb 2017 14:58:50 -0600
Li, Yi yi1.li@linux.intel.com wrote:
...
>> +#define MAX_FPGAS	4
>> +static char *fw_list[MAX_FPGAS];
>> +static int fw_list_n;
>> +module_param_array_named(fw, fw_list, charp, &fw_list_n, 0444);
>> +MODULE_PARM_DESC(fw, "Mapping of BDF to firmware name (fw=B:D.F-card_cvp.rbf)");  
>
>I like it. :)

Do you have a setup with multiple FPGAs by chance? If so, could you
please test if using e.g. fw=B:D.F-card1_cvp.rbf,B:D.F-card2_cvp.rbf works
as expected? I only tested with one FPGA.

...
>> +		if (delay_us++ > TIMEOUT_IN_US) {
>> +			dev_warn(&mgr->dev, "PLD_CLK_IN_USE|USERMODE timeout\n");
>> +			status = -ETIMEDOUT;
>> +			break;
>> +		}
>> +	}
>> +
>> +	return status;  
>
>I might not explain clearly before.  At least Step 18 should not belong 
>to the teardown function. If the CvP is not programmed, the FPGA will 
>not get into the user mode.
>Suggest to move the Step 16 to Step 18 out of the teardown function to 
>write_complete function.

Okay, will do. Thanks!

...
>> +		dev_err(&mgr->dev, "CVP_CONFIG_ERROR after %d bytes!\n",
>> +			bytes);  
>
>One minor compilation warning:
>drivers/fpga/altera-v-cvp.c:396:22: warning: format '%d' expects 
>argument of type 'int', but argument 3 has type 'size_t {aka long 
>unsigned int}' [-Wformat=]
>    dev_err(&mgr->dev, "CVP_CONFIG_ERROR after %d bytes!\n",

Which toolchain (gcc version) do you use? I build with gcc v4.9.2
and do not see this warning. But I'll try with a newer toolchain.

...
>> +	if (map) {
>> +		while (bytes >= 4) {
>> +			*map = *data++;
>> +			bytes -= 4;
>> +
>> +			/*
>> +			 * STEP 10 (optional) and STEP 11
>> +			 * - check error flag
>> +			 * - loop until data transfer completed
>> +			 */
>> +			if (chkcfg && !(bytes % SZ_4K)) {
>> +				size_t done = count - bytes;
>> +
>> +				status = altera_v_cvp_chk_error(mgr, done);
>> +				if (status < 0)
>> +					return status;
>> +			}
>> +		}
>> +	} else {
>> +		while (bytes >= 4) {
>> +			pci_write_config_dword(pdev, VSEC_CVP_DATA, *data++);
>> +			bytes -= 4;
>> +
>> +			if (chkcfg && !(bytes % SZ_4K)) {
>> +				size_t done = count - bytes;
>> +
>> +				status = altera_v_cvp_chk_error(mgr, done);
>> +				if (status < 0)
>> +					return status;
>> +			}
>> +		}
>> +	}  
>
>Should we put the "if (map) else" check inside the while loop to avoid 
>duplicating code?

I prefer to avoid additional if statement in this loop. Frankly,
I don't even like these if chkcfg statements in the loop. The
code duplication is not so huge here, I think.

...
>> +static int altera_v_cvp_write_complete(struct fpga_manager *mgr,
>> +				       struct fpga_image_info *info)
>> +{
>> +	return altera_v_cvp_teardown(mgr, info);
>> +}
>> +  
>
>Maybe adding a print to show the CvP programming status will be useful?
>     if (!status)
>         dev_info(&mgr->dev, "CvP programming successful\n");

I think "no news is good news" is better here. If something went
wrong, the error will be reported by FPGA Manager code. When there
is no error report, we assume programming successfully completed.

Thank you for review and comments!

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

Moritz Fischer Feb. 21, 2017, 10:47 p.m. UTC | #3

All,

On Tue, Feb 21, 2017 at 1:46 PM, Anatolij Gustschin <agust@denx.de> wrote:
> Hi Yi,
>
> On Tue, 21 Feb 2017 14:58:50 -0600
> Li, Yi yi1.li@linux.intel.com wrote:
> ...
>>> +#define MAX_FPGAS   4
>>> +static char *fw_list[MAX_FPGAS];
>>> +static int fw_list_n;
>>> +module_param_array_named(fw, fw_list, charp, &fw_list_n, 0444);
>>> +MODULE_PARM_DESC(fw, "Mapping of BDF to firmware name (fw=B:D.F-card_cvp.rbf)");
>>
>>I like it. :)
>
> Do you have a setup with multiple FPGAs by chance? If so, could you
> please test if using e.g. fw=B:D.F-card1_cvp.rbf,B:D.F-card2_cvp.rbf works
> as expected? I only tested with one FPGA.

No, no, no, no. No! This is again bypassing the framework and device
model to load the bit-stream,
how is this different from having a custom sysfs node that bypasses
the framework?

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

Anatolij Gustschin Feb. 21, 2017, 11:19 p.m. UTC | #4

Hi Moritz,

On Tue, 21 Feb 2017 14:47:22 -0800
Moritz Fischer moritz.fischer@ettus.com wrote:

>All,
>
>On Tue, Feb 21, 2017 at 1:46 PM, Anatolij Gustschin <agust@denx.de> wrote:
>> Hi Yi,
>>
>> On Tue, 21 Feb 2017 14:58:50 -0600
>> Li, Yi yi1.li@linux.intel.com wrote:
>> ...  
>>>> +#define MAX_FPGAS   4
>>>> +static char *fw_list[MAX_FPGAS];
>>>> +static int fw_list_n;
>>>> +module_param_array_named(fw, fw_list, charp, &fw_list_n, 0444);
>>>> +MODULE_PARM_DESC(fw, "Mapping of BDF to firmware name (fw=B:D.F-card_cvp.rbf)");  
>>>
>>>I like it. :)  
>>
>> Do you have a setup with multiple FPGAs by chance? If so, could you
>> please test if using e.g. fw=B:D.F-card1_cvp.rbf,B:D.F-card2_cvp.rbf works
>> as expected? I only tested with one FPGA.  
>
>No, no, no, no. No! This is again bypassing the framework and device
>model to load the bit-stream,
>how is this different from having a custom sysfs node that bypasses
>the framework?

Okay. How should I use the framework with hot-pluggable and
discoverable Devices (e.g. FPGAs on PCIe-Bus, USB-Interface Adapters
for FPGA configuration) without bypassing it? I have a requirement of
similar userspace configuration interface for all these configuration
devices on DT and non DT based platforms.

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

Li, Yi Feb. 21, 2017, 11:26 p.m. UTC | #5

hi Anatolij


On 2/21/2017 3:46 PM, Anatolij Gustschin wrote:
> Hi Yi,
>
> On Tue, 21 Feb 2017 14:58:50 -0600
> Li, Yi yi1.li@linux.intel.com wrote:
> ...
>>> +#define MAX_FPGAS	4
>>> +static char *fw_list[MAX_FPGAS];
>>> +static int fw_list_n;
>>> +module_param_array_named(fw, fw_list, charp, &fw_list_n, 0444);
>>> +MODULE_PARM_DESC(fw, "Mapping of BDF to firmware name (fw=B:D.F-card_cvp.rbf)");
>> I like it. :)
> Do you have a setup with multiple FPGAs by chance? If so, could you
> please test if using e.g. fw=B:D.F-card1_cvp.rbf,B:D.F-card2_cvp.rbf works
> as expected? I only tested with one FPGA.

No, I do not have a system with multiple FPGAs yet.  Will need more time 
to think about how to embedded device info into the stream.
...

>> One minor compilation warning:
>> drivers/fpga/altera-v-cvp.c:396:22: warning: format '%d' expects
>> argument of type 'int', but argument 3 has type 'size_t {aka long
>> unsigned int}' [-Wformat=]
>>     dev_err(&mgr->dev, "CVP_CONFIG_ERROR after %d bytes!\n",
> Which toolchain (gcc version) do you use? I build with gcc v4.9.2
> and do not see this warning. But I'll try with a newer toolchain.

I was using gcc 5.4.0 from ubuntu 16.04
...

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

Anatolij Gustschin Feb. 22, 2017, 12:02 a.m. UTC | #6

On Wed, 22 Feb 2017 00:19:02 +0100
Anatolij Gustschin agust@denx.de wrote:
...
>>No, no, no, no. No! This is again bypassing the framework and device
>>model to load the bit-stream,
>>how is this different from having a custom sysfs node that bypasses
>>the framework?  
>
>Okay. How should I use the framework with hot-pluggable and
>discoverable Devices (e.g. FPGAs on PCIe-Bus...

here, I can think of creating an fpga bridge driver for PCIe interface.
It should be able to unbind the drivers attached to the devices
implemented in the FPGA on PCIe bus. Then the bridge driver would
bind the low level CvP manager driver to the PCIe device and create
fpga-region device associated with CvP FPGA manager and then register
this region with the framework. Patches for FPGA Region enhancements
from Alan should be used as a base for fpga-region creating and
registering.

When configuration is completed, the bridge driver would unbind
the low level CvP Manager from PCIe device and trigger binding
the drivers for devices in FPGA. Is this the right direction?

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

Moritz Fischer Feb. 22, 2017, 12:03 a.m. UTC | #7

Anatolij,

On Tue, Feb 21, 2017 at 3:19 PM, Anatolij Gustschin <agust@denx.de> wrote:

> Okay. How should I use the framework with hot-pluggable and
> discoverable Devices (e.g. FPGAs on PCIe-Bus, USB-Interface Adapters
> for FPGA configuration) without bypassing it? I have a requirement of
> similar userspace configuration interface for all these configuration
> devices on DT and non DT based platforms.

Short answer: Fix/Create the framework.

I think we can agree that module parameters are not the unified
userland interface,
that would work for DT and non-DT platforms alike in an extensible and
maintainable
way. Whatever we merge becomes ABI. A module parameter as well as a sysfs entry
is pretty much the same as a /dev/xdevcfg or /dev/fpga0 into which you
cat your firmware,
which is fine if you don't have any kernel drivers loaded. But only then.

I do realize we don't have anything that works for the use case of
either PCIe or USB
at the moment, which is why we need to start writing actual code that
deals with that
instead of getting all these 'works for me' patches that work for one
corner-case.

We need to come up with a solution that solves the enumeration problem - which -
is not trivial seen that almost anything could hang off of the FPGA on
the other side
of your bus.

Thanks,

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

Moritz Fischer Feb. 22, 2017, 12:07 a.m. UTC | #8

Anatolij,

On Tue, Feb 21, 2017 at 4:02 PM, Anatolij Gustschin <agust@denx.de> wrote:

> here, I can think of creating an fpga bridge driver for PCIe interface.
> It should be able to unbind the drivers attached to the devices
> implemented in the FPGA on PCIe bus. Then the bridge driver would
> bind the low level CvP manager driver to the PCIe device and create
> fpga-region device associated with CvP FPGA manager and then register
> this region with the framework. Patches for FPGA Region enhancements
> from Alan should be used as a base for fpga-region creating and
> registering.

Sounds like an idea. Let me think about this :)

> When configuration is completed, the bridge driver would unbind
> the low level CvP Manager from PCIe device and trigger binding
> the drivers for devices in FPGA. Is this the right direction?

It's certainly much more the right direction than module parameters,
thanks for keeping up the discussion.

Thanks,

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

Anatolij Gustschin Feb. 22, 2017, 12:26 a.m. UTC | #9

On Tue, 21 Feb 2017 16:03:14 -0800
Moritz Fischer moritz.fischer@ettus.com wrote:

>Anatolij,
>
>On Tue, Feb 21, 2017 at 3:19 PM, Anatolij Gustschin <agust@denx.de> wrote:
>
>> Okay. How should I use the framework with hot-pluggable and
>> discoverable Devices (e.g. FPGAs on PCIe-Bus, USB-Interface Adapters
>> for FPGA configuration) without bypassing it? I have a requirement of
>> similar userspace configuration interface for all these configuration
>> devices on DT and non DT based platforms.  
>
>Short answer: Fix/Create the framework.
>
>I think we can agree that module parameters are not the unified
>userland interface,
>that would work for DT and non-DT platforms alike in an extensible and
>maintainable
>way. Whatever we merge becomes ABI. A module parameter as well as a sysfs entry
>is pretty much the same as a /dev/xdevcfg or /dev/fpga0 into which you
>cat your firmware,
>which is fine if you don't have any kernel drivers loaded. But only then.
>
>I do realize we don't have anything that works for the use case of
>either PCIe or USB
>at the moment, which is why we need to start writing actual code that
>deals with that
>instead of getting all these 'works for me' patches that work for one
>corner-case.
>
>We need to come up with a solution that solves the enumeration problem - which -
>is not trivial seen that almost anything could hang off of the FPGA on
>the other side
>of your bus.

my understanding was that an FPGA on PCIe interface is a special case.
I cannot attach/bind the low level CvP manager driver to the PCIe device
if there is some driver for devices in FPGA already running. I have first
to either unload the driver module or unbind it (so that the remove()
callback in this driver is called. It would unregister and free the
resources and thus prevent accessing the devices in FPGA, if implemented
properly). Only then binding the CvP manager driver to the PCIe device
will succeed. Then it triggers the configuration when probing.

Thanks,

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

Matthew Gerlach Feb. 23, 2017, 4:59 p.m. UTC | #10

On Tue, 21 Feb 2017, Moritz Fischer wrote:

> Anatolij,
>
> On Tue, Feb 21, 2017 at 3:19 PM, Anatolij Gustschin <agust@denx.de> wrote:
>
>> Okay. How should I use the framework with hot-pluggable and
>> discoverable Devices (e.g. FPGAs on PCIe-Bus, USB-Interface Adapters
>> for FPGA configuration) without bypassing it? I have a requirement of
>> similar userspace configuration interface for all these configuration
>> devices on DT and non DT based platforms.
>
> Short answer: Fix/Create the framework.
>
I think the short answer is on the right track, but while the answer is 
short, the solution is clearly non-trivial.

> I think we can agree that module parameters are not the unified
> userland interface,
> that would work for DT and non-DT platforms alike in an extensible and
> maintainable
> way. Whatever we merge becomes ABI. A module parameter as well as a sysfs entry
> is pretty much the same as a /dev/xdevcfg or /dev/fpga0 into which you
> cat your firmware,
> which is fine if you don't have any kernel drivers loaded. But only then.
>
> I do realize we don't have anything that works for the use case of
> either PCIe or USB
> at the moment, which is why we need to start writing actual code that
> deals with that
> instead of getting all these 'works for me' patches that work for one
> corner-case.
>
> We need to come up with a solution that solves the enumeration problem - which -
> is not trivial seen that almost anything could hang off of the FPGA on
> the other side
> of your bus.

From my point of view, the enumeration problem is "the" problem.  I 
believe the most elegent solution to the problem I've seen is using device tree 
overlays.  Removing the overlay performs the function of safely shutting 
down drivers accessing the fpga contents.  Applying the overlay, performs 
the fpga configuration and subsequent enumeration.

Anyway you look at it, the FPGA is blank piece of paper where anything can 
be written on it.  Once you put in place some framework that requires the 
FPGA developer to fit into the framework's box you've removed some of the 
flexibility that a FPGA provides.  So for the most flexibility, once the 
FPGA has been configured, the kernel must be provided some form of meta 
data describing the contents of FPGA so the enumeration can occur.  The 
meta data can be in the fpga image itself, or the meta data can handed to 
the kernel "out of band", or the meta data can be part of some wrapper to 
the bitstream.  The question becomes "what is the best format of the data 
describing the fpga contents?"  As far as I can tell, any new meta data 
format will be a bad copy of the device tree format.

Matthew

>
> Thanks,
>
> Moritz
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fpga" in
> the body of a message to majordomo@vger.kernel.org
> 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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Moritz Fischer Feb. 24, 2017, 6:45 p.m. UTC | #11

On Thu, Feb 23, 2017 at 8:59 AM,  <matthew.gerlach@linux.intel.com> wrote:
>
>
> On Tue, 21 Feb 2017, Moritz Fischer wrote:
>
>> Anatolij,
>>
>> On Tue, Feb 21, 2017 at 3:19 PM, Anatolij Gustschin <agust@denx.de> wrote:
>>
>>> Okay. How should I use the framework with hot-pluggable and
>>> discoverable Devices (e.g. FPGAs on PCIe-Bus, USB-Interface Adapters
>>> for FPGA configuration) without bypassing it? I have a requirement of
>>> similar userspace configuration interface for all these configuration
>>> devices on DT and non DT based platforms.
>>
>>
>> Short answer: Fix/Create the framework.
>>
> I think the short answer is on the right track, but while the answer is
> short, the solution is clearly non-trivial.

Yeah, but we can't just merge random one-offs in the meantime, just so
we have something. I do get that people have products, and need
software to support them, but one-off hacks go into vendor trees.

> From my point of view, the enumeration problem is "the" problem.  I believe
> the most elegent solution to the problem I've seen is using device tree
> overlays.  Removing the overlay performs the function of safely shutting
> down drivers accessing the fpga contents.  Applying the overlay, performs
> the fpga configuration and subsequent enumeration.

I totally agree with everything you said here.

> Anyway you look at it, the FPGA is blank piece of paper where anything can
> be written on it.  Once you put in place some framework that requires the
> FPGA developer to fit into the framework's box you've removed some of the
> flexibility that a FPGA provides.  So for the most flexibility, once the
> FPGA has been configured, the kernel must be provided some form of meta data
> describing the contents of FPGA so the enumeration can occur.

By describing the hardware you already limit what you can implement
already, though,
right? Hardware you cannot describe in the format cannot be detected, right?
Essentially what we do with DT with hardware we cannot describe, is we
update the
bindings and core code to be able to describe it.

 Maybe going for a format that goes for a subset first, but staying extensible
is the right direction rather than trying to tackle all possible
things one could instantiate
in programmable logic at once. Imho most of the hardware in an FPGA
somewhat looks
like traditional hardware (at least in the designs I'm working with).

> The meta data
> can be in the fpga image itself, or the meta data can handed to the kernel
> "out of band", or the meta data can be part of some wrapper to the
> bitstream.  The question becomes "what is the best format of the data
> describing the fpga contents?"  As far as I can tell, any new meta data
> format will be a bad copy of the device tree format.

That.

Cheers

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

[v2] fpga manager: Add Altera V series CvP driver

Commit Message

Comments

Patch