DPDK patches and discussions
 help / color / Atom feed
* [dpdk-dev] [20.11, PATCH v2] BBDEV FPGA PF Config app
@ 2020-07-16 20:20 Nicolas Chautru
  2020-07-16 20:20 ` [dpdk-dev] [20.11, PATCH v2] baseband/fpga_5gnr_fec: add companion PF config App Nicolas Chautru
  0 siblings, 1 reply; 8+ messages in thread
From: Nicolas Chautru @ 2020-07-16 20:20 UTC (permalink / raw)
  To: dev, akhil.goyal, thomas; +Cc: Nicolas Chautru

v2: Couple of typos in documentation and removed
dependency on igb_uio and related PCIe VF creation feature option. 

Addition of an stand alone application in PMD driver to be able to configure the HW from  PF. Then the VF can be used from container/VM running BBDEV/DPDK. 

The history of the previous discussions can be tracked on this previous patch https://patches.dpdk.org/patch/67379/

PMD documentation is updated to provides steps to build with related dependency 

Nicolas Chautru (1):
  baseband/fpga_5gnr_fec: add companion PF config App

 doc/guides/bbdevs/fpga_5gnr_fec.rst                |  80 +++--
 .../baseband/fpga_5gnr_fec/pf_config_app/Makefile  |  36 +++
 .../fpga_5gnr_fec/pf_config_app/config_app.c       | 333 +++++++++++++++++++
 .../pf_config_app/fpga_5gnr_cfg_app.c              | 351 +++++++++++++++++++++
 .../pf_config_app/fpga_5gnr_cfg_app.h              | 102 ++++++
 .../pf_config_app/fpga_5gnr_cfg_parser.c           | 187 +++++++++++
 .../pf_config_app/fpga_5gnr_config.cfg             |  18 ++
 7 files changed, 1087 insertions(+), 20 deletions(-)
 create mode 100644 drivers/baseband/fpga_5gnr_fec/pf_config_app/Makefile
 create mode 100644 drivers/baseband/fpga_5gnr_fec/pf_config_app/config_app.c
 create mode 100644 drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_cfg_app.c
 create mode 100644 drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_cfg_app.h
 create mode 100644 drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_cfg_parser.c
 create mode 100644 drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_config.cfg

-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [dpdk-dev] [20.11, PATCH v2] baseband/fpga_5gnr_fec: add companion PF config App
  2020-07-16 20:20 [dpdk-dev] [20.11, PATCH v2] BBDEV FPGA PF Config app Nicolas Chautru
@ 2020-07-16 20:20 ` Nicolas Chautru
  2020-07-31 10:35   ` Maxime Coquelin
  0 siblings, 1 reply; 8+ messages in thread
From: Nicolas Chautru @ 2020-07-16 20:20 UTC (permalink / raw)
  To: dev, akhil.goyal, thomas; +Cc: Nicolas Chautru

Adding companion application to configure HW Device from the PF.
Then the device can be accessed through BBDEV from VF (or PF).

Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
---
 doc/guides/bbdevs/fpga_5gnr_fec.rst                |  80 +++--
 .../baseband/fpga_5gnr_fec/pf_config_app/Makefile  |  36 +++
 .../fpga_5gnr_fec/pf_config_app/config_app.c       | 333 +++++++++++++++++++
 .../pf_config_app/fpga_5gnr_cfg_app.c              | 351 +++++++++++++++++++++
 .../pf_config_app/fpga_5gnr_cfg_app.h              | 102 ++++++
 .../pf_config_app/fpga_5gnr_cfg_parser.c           | 187 +++++++++++
 .../pf_config_app/fpga_5gnr_config.cfg             |  18 ++
 7 files changed, 1087 insertions(+), 20 deletions(-)
 create mode 100644 drivers/baseband/fpga_5gnr_fec/pf_config_app/Makefile
 create mode 100644 drivers/baseband/fpga_5gnr_fec/pf_config_app/config_app.c
 create mode 100644 drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_cfg_app.c
 create mode 100644 drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_cfg_app.h
 create mode 100644 drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_cfg_parser.c
 create mode 100644 drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_config.cfg

diff --git a/doc/guides/bbdevs/fpga_5gnr_fec.rst b/doc/guides/bbdevs/fpga_5gnr_fec.rst
index 19bba36..9674e4c 100644
--- a/doc/guides/bbdevs/fpga_5gnr_fec.rst
+++ b/doc/guides/bbdevs/fpga_5gnr_fec.rst
@@ -218,33 +218,73 @@ parameters defined in ``fpga_5gnr_fec_conf`` structure:
   time_out = flr_time_out x 16.384us. For instance, if you want to set 10ms for
   the FLR time out then set this setting to 0x262=610.
 
+A companion application pf_config_app is provided as a standalone application
+described in next section.
 
-An example configuration code calling the function ``fpga_5gnr_fec_configure()`` is shown
-below:
+PF Config App
+-------------
 
-.. code-block:: c
+The PF Configuration Application ``pf_config_app`` provides a means to
+configure the baseband device at the host-level. The program sets the various
+parameters through memory-mapped IO read/writes. Then the BBDEV driver can be
+used to run the workload.
+
+The parameters are parsed from a given configuration file (with .cfg extensions)
+that is specific to particular BBDEV device, although they follow same format.
+
+External Dependencies
+~~~~~~~~~~~~~~~~~~~~~
+
+The third party .INI parser is a pre-requisite for building the application.
+It can be downloaded from [github]:
+
+.. code-block:: console
 
-  struct fpga_5gnr_fec_conf conf;
-  unsigned int i;
+    git clone https://github.com/benhoyt/inih
 
-  memset(&conf, 0, sizeof(struct fpga_5gnr_fec_conf));
-  conf.pf_mode_en = 1;
+Use the version release 44 tracked by tag 'r44':
+
+.. code-block:: console
+
+    git checkout r44
+
+The application features a makefile in the `extra/` directory which generates
+the library, `libinih.a`. To compile the inih library, run make as:
+
+.. code-block:: console
+
+    make -f Makefile.static
+
+Building PF Config App
+~~~~~~~~~~~~~~~~~~~~~~
+
+Before building the application, set the following environment variables with
+the location where the INI library is located:
+
+.. code-block:: console
+
+    export INIH_PATH=<path-to-inih-lib>
+
+If not set, makefile will look into current folder.
+
+Next, build the program by typing ``make`` from the pf_Config_app subdirectory
+to produce the binary ``pf_config_app_fpga_5gnr``.
+
+Usage
+~~~~~
+
+The application executes as the following:
+
+.. code-block:: console
 
-  for (i = 0; i < FPGA_5GNR_FEC_NUM_VFS; ++i) {
-      conf.vf_ul_queues_number[i] = 4;
-      conf.vf_dl_queues_number[i] = 4;
-  }
-  conf.ul_bandwidth = 12;
-  conf.dl_bandwidth = 5;
-  conf.dl_load_balance = 64;
-  conf.ul_load_balance = 64;
+    ./pf_config_app_fpga_5gnr [-h] [-a] [-c CFG_FILE] [-p PCI_ID]
 
-  /* setup FPGA PF */
-  ret = fpga_5gnr_fec_configure(info->dev_name, &conf);
-  TEST_ASSERT_SUCCESS(ret,
-      "Failed to configure 4G FPGA PF for bbdev %s",
-      info->dev_name);
+* ``-c CFG_FILE``: Specifies configuration file to use
+* ``-p PCI_ID``: Specifies PCI ID of device to configure
+* ``-a``: Configures all PCI devices
+* ``-h``: Prints help
 
+Default configure file is provided: ``fpga_5gnr_config.cfg``.
 
 Test Application
 ----------------
diff --git a/drivers/baseband/fpga_5gnr_fec/pf_config_app/Makefile b/drivers/baseband/fpga_5gnr_fec/pf_config_app/Makefile
new file mode 100644
index 0000000..4f7517d
--- /dev/null
+++ b/drivers/baseband/fpga_5gnr_fec/pf_config_app/Makefile
@@ -0,0 +1,36 @@
+
+CC=gcc
+CFLAGS=-O0 -g -Wall
+ODIR=build
+DEPS=
+
+ifeq ($(INIH_PATH),)
+INCLUDE=-I.
+LDFLAGS=-L.
+else
+INCLUDE=-I. -I$(INIH_PATH)
+LDFLAGS=-L. -L$(INIH_PATH)
+endif
+
+LDLIBS=-linih
+
+SRC = config_app.c fpga_5gnr_cfg_app.c fpga_5gnr_cfg_parser.c
+OBJ = $(patsubst %.c,$(ODIR)/%.o,$(SRC))
+
+.PHONY: clean
+
+all: pf_config_app_fpga_5gnr
+
+$(ODIR):
+	mkdir -p $(ODIR)
+
+$(OBJ): $(ODIR)/%.o: ./%.c | $(DEPS) $(ODIR)
+	@mkdir -p $(@D)
+	$(CC) -c -o $@ $< $(CFLAGS) $(INCLUDE)
+
+pf_config_app_fpga_5gnr: $(OBJ)
+	$(CC) -o $@ $^ $(CFLAGS) $(LDFLAGS) $(LDLIBS)
+
+clean:
+	rm -rf $(ODIR)
+	rm -rf pf_config_app_fpga_5gnr
diff --git a/drivers/baseband/fpga_5gnr_fec/pf_config_app/config_app.c b/drivers/baseband/fpga_5gnr_fec/pf_config_app/config_app.c
new file mode 100644
index 0000000..a993ebd
--- /dev/null
+++ b/drivers/baseband/fpga_5gnr_fec/pf_config_app/config_app.c
@@ -0,0 +1,333 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2020 Intel Corporation
+ */
+
+#include <stdlib.h>
+#include <stdint.h>
+#include <dirent.h>
+#include <stdio.h>
+#include <string.h>
+#include <strings.h>
+#include <unistd.h>
+#include <stdbool.h>
+#include <limits.h>
+#include <linux/vfio.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <sys/mman.h>
+#include <sys/ioctl.h>
+#include <fcntl.h>
+#include <errno.h>
+
+#include "fpga_5gnr_cfg_app.h"
+
+#define SYS_DIR "/sys/bus/pci/devices"
+#define CUR_DIR "."
+#define PREV_DIR ".."
+
+#define DRIVER_LINK  "driver"
+#define DEVICE_FILE  "device"
+#define VENDOR_FILE  "vendor"
+#define BAR0_FILE    "resource0"
+
+#define PCI_STR_SIZE 15
+#define DEV_STR_SIZE 10
+#define NULL_PAD     2
+
+/* Function Pointer for device specific configuration file */
+typedef int (*configuration)(void *bar0addr, const char *arg_cfg_filename);
+
+typedef struct hw_device {
+	const char *device_name;
+	char *config_file;
+	int vendor_id;
+	int device_id;
+	char pci_address[PCI_STR_SIZE];
+	bool driver_found;
+	configuration conf;
+	int config_all;
+} hw_device;
+
+static void *
+get_bar0_mapping(const char *pci_addr, unsigned int bar_size)
+{
+	char bar0path[PATH_MAX];
+	int bar0addrfd;
+	void *map;
+
+	snprintf(bar0path, sizeof(bar0path),
+			"%s/%s/%s", SYS_DIR, pci_addr, BAR0_FILE);
+	bar0addrfd = open(bar0path, O_RDWR | O_SYNC);
+	if (bar0addrfd < 0) {
+		printf("\nFailed to open BAR %s\n", bar0path);
+		exit(1);
+	}
+	map = mmap(0, bar_size, PROT_READ | PROT_WRITE, MAP_SHARED,
+			bar0addrfd, 0);
+	close(bar0addrfd);
+	return map;
+}
+
+static unsigned long
+get_file_val(const char *file_path)
+{
+	char content[BUFSIZ];
+	FILE *f;
+
+	f = fopen(file_path, "r");
+	if (f == NULL) {
+		printf("\nFailed to open %s\n", file_path);
+		exit(1);
+	}
+	if (fgets(content, sizeof(content), f) == NULL) {
+		fclose(f);
+		return false;
+	}
+	fclose(f);
+	const unsigned long content_val = strtoul(content, NULL, 16);
+
+	return content_val;
+}
+
+static bool
+get_device_id(hw_device *device, const char *location)
+{
+	unsigned long vendor_id = -1, device_id = -1;
+	struct dirent *dirent;
+	DIR *dir;
+	char pci_path[PATH_MAX];
+
+	snprintf(pci_path, sizeof(pci_path), "%s/%s", SYS_DIR, location);
+	dir = opendir(pci_path);
+	if (dir == NULL) {
+		printf("Failed to open %s (%s)\n", pci_path, strerror(errno));
+		return false;
+	}
+
+	while ((dirent = readdir(dir)) != NULL) {
+		char file_path[PATH_MAX];
+
+		/* Omit Current Directory & Previous Directory Lookup */
+		if (strncmp(dirent->d_name, CUR_DIR,
+				strlen(dirent->d_name)) == 0 ||
+				strncmp(dirent->d_name, PREV_DIR,
+				strlen(dirent->d_name)) == 0)
+			continue;
+
+		/* Set filepath as next PCI folder (xxxx:xx:xx.x) */
+		snprintf(file_path, sizeof(file_path), "%s/%s",
+				pci_path, dirent->d_name);
+
+		/* Get Device ID */
+		if (strncmp(dirent->d_name, DEVICE_FILE,
+				strlen(dirent->d_name)) == 0 &&
+				dirent->d_type == DT_REG)
+			device_id = get_file_val(file_path);
+
+		/* Get Vendor ID */
+		if (strncmp(dirent->d_name, VENDOR_FILE,
+				strlen(dirent->d_name)) == 0 &&
+				dirent->d_type == DT_REG)
+			vendor_id = get_file_val(file_path);
+	}
+
+	closedir(dir);
+	/* Check if device is found */
+	return (vendor_id == device->vendor_id &&
+			device_id == device->device_id);
+}
+
+static int
+probe_pci_bus(hw_device *device, char **found_devices)
+{
+	struct dirent *dirent;
+	DIR *dir;
+	int num_devices = 0;
+
+	/* Locate PCI Devices */
+	dir = opendir(SYS_DIR);
+	if (dir == NULL)
+		return -1;
+
+	/* Iterate Through Directories */
+	while ((dirent = readdir(dir)) != NULL) {
+
+		/* Omit Current Directory and Previous Directory Lookup */
+		if (strncmp(dirent->d_name, CUR_DIR,
+				strlen(dirent->d_name)) == 0 ||
+				strncmp(dirent->d_name, PREV_DIR,
+						strlen(dirent->d_name)) == 0)
+			continue;
+		/* Check if current device matches requested device */
+		if (get_device_id(device, dirent->d_name)) {
+			found_devices[num_devices] =
+					(char *) malloc(PCI_STR_SIZE);
+			/* Copy PCI slot of device */
+			strncpy(found_devices[num_devices], dirent->d_name,
+					sizeof(device->pci_address) - NULL_PAD);
+			num_devices++;
+		}
+	}
+
+	return num_devices;
+}
+
+static int
+match_device(char *pci_address, char **found_devices, int num_devices)
+{
+	int i;
+	for (i = 0; i < num_devices; i++) {
+		if (!strncmp(pci_address, found_devices[i],
+				sizeof(pci_address) - NULL_PAD))
+			return 0;
+	}
+
+	printf("Given PCI ID is not available\n");
+	return -1;
+}
+
+static int
+select_device(hw_device *device, char **found_devices, int num_devices)
+{
+	int i, selected;
+	/* If more than one device found, get user input on which to use */
+	if (num_devices >= 2) {
+		printf("More than one device found. Please select which device you would like to use from the list:\n");
+
+		/*Print PCI Slots */
+		for (i = 0; i < num_devices; i++)
+			printf("[%i]: %s\n", i+1, found_devices[i]);
+
+		printf("> ");
+		scanf("%d", &selected);
+		if (selected >= 1 && selected <= num_devices) {
+			strncpy(device->pci_address, found_devices[selected-1],
+					sizeof(device->pci_address) - NULL_PAD);
+			return 0;
+		}
+
+		printf("Invalid Option, please try again..\n");
+		return -1;
+	}
+
+	strncpy(device->pci_address, found_devices[0],
+			sizeof(device->pci_address) - NULL_PAD);
+	return 0;
+}
+
+void set_device(hw_device *device)
+{
+	device->vendor_id = FPGA_5GNR_FEC_VENDOR_ID;
+	device->device_id = FPGA_5GNR_FEC_DEVICE_ID;
+	device->conf = fpga_5gnr_configure;
+	if (device->config_file == NULL) {
+		device->config_file = getenv("FPGA_5GNR_CONFIG_FILE");
+		if (device->config_file == NULL)
+			device->config_file = "fpga_5gnr_config.cfg";
+	}
+}
+
+static void
+print_helper(const char *prgname)
+{
+	printf("Usage: %s [-h] [-a] [-c CFG_FILE] [-p PCI_ID]\n\n"
+			" -c CFG_FILE \t specifies configuration file to use\n"
+			" -p PCI_ID \t specifies PCI ID of device to configure\n"
+			" -a \t\t configures all PCI devices matching the given DEVICE_NAME\n"
+			" -h \t\t prints this helper\n\n", prgname);
+}
+
+static int
+parse_args(int argc, char **argv,
+		struct hw_device *device)
+{
+	int opt;
+	char *prgname = argv[0];
+	device->device_name = FPGA_5GNR_FEC_DEV_NAME;
+
+	while ((opt = getopt(argc, argv, "c:p:ah")) != -1) {
+		switch (opt) {
+		case 'c':
+			device->config_file = optarg;
+			break;
+		case 'p':
+			strncpy(device->pci_address, optarg,
+					sizeof(device->pci_address)
+					- NULL_PAD);
+			break;
+		case 'a':
+			device->config_all = 1;
+			break;
+		case 'h':
+		default:
+			print_helper(prgname);
+			return 1;
+		}
+	}
+	return 0;
+}
+
+static int
+configure_device(hw_device *device)
+{
+	/* Get BAR0 Mapping for device */
+	void *bar0addr = get_bar0_mapping(device->pci_address,
+			FPGA_5GNR_FEC_BAR_SIZE);
+
+	/* Call device specific configuration function */
+	if (device->conf(bar0addr, device->config_file) == 0) {
+		printf("%s PF [%s] configuration complete!\n\n",
+				device->device_name, device->pci_address
+				- NULL_PAD);
+		return 0;
+	}
+	printf("Configuration error!!\n");
+	return -1;
+}
+
+int
+main(int argc, char *argv[])
+{
+	int i, num_devices;
+	hw_device device;
+	char *found_devices[DEV_STR_SIZE];
+
+	memset(&device, 0, sizeof(device));
+
+	if (parse_args(argc, argv, &device) > 0)
+		return 0;
+
+	/* Set Device Info */
+	set_device(&device);
+	/* Check if device is installed */
+	num_devices = probe_pci_bus(&device, found_devices);
+	if (num_devices == 0) {
+		printf("No devices found!!\n");
+		return -1;
+	} else if (num_devices < 0) {
+		return num_devices;
+	}
+
+	if (device.pci_address[0] != 0) {
+		if (match_device(device.pci_address, found_devices,
+				num_devices) < 0)
+			return -1;
+	}
+
+	if (device.config_all) {
+		for (i = 0; i < num_devices; i++) {
+			strncpy(device.pci_address, found_devices[i],
+					sizeof(device.pci_address) - NULL_PAD);
+			configure_device(&device);
+		}
+	} else {
+		select_device(&device, found_devices, num_devices);
+		configure_device(&device);
+	}
+
+	/* Free memory for stored PCI slots */
+	for (i = 0; i < num_devices; i++)
+		free(found_devices[i]);
+
+	return 0;
+}
diff --git a/drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_cfg_app.c b/drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_cfg_app.c
new file mode 100644
index 0000000..ac01722
--- /dev/null
+++ b/drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_cfg_app.c
@@ -0,0 +1,351 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2020 Intel Corporation
+ */
+
+#include <stdbool.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <ctype.h>
+#include <inttypes.h>
+#include <errno.h>
+
+#include "fpga_5gnr_cfg_app.h"
+
+extern int
+fpga_5gnr_parse_conf_file(const char *file_name,
+		struct fpga_5gnr_fec_conf *fpga_conf);
+
+/* Read 8-bit register of FPGA 5GNR FEC device */
+static uint8_t
+fpga_reg_read_8(void *mmio_base, uint32_t offset)
+{
+	void *reg_addr = mmio_base + offset;
+	return *((volatile uint8_t *)(reg_addr));
+}
+
+/* Read 16-bit register of FPGA 5GNR FEC device */
+static uint16_t
+fpga_reg_read_16(void *mmio_base, uint32_t offset)
+{
+	void *reg_addr = mmio_base + offset;
+	uint16_t ret = *((volatile uint16_t *)(reg_addr));
+#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
+	return __bswap_16(ret);
+#else
+	return ret;
+#endif
+}
+
+/* Read 32-bit register of FPGA 5GNR FEC device */
+static uint32_t
+fpga_reg_read_32(void *mmio_base, uint32_t offset)
+{
+	void *reg_addr = mmio_base + offset;
+	uint32_t ret = *((volatile uint32_t *)(reg_addr));
+#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
+	return __bswap_32(ret);
+#else
+	return ret;
+#endif
+}
+
+static inline void
+fpga_reg_write_16(void *mmio_base, uint32_t offset,
+		uint16_t payload) {
+	void *reg_addr = mmio_base + offset;
+#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
+	payload = __bswap_16(payload);
+#endif
+	*((volatile uint16_t *) (reg_addr)) = payload;
+}
+
+static inline void
+fpga_reg_write_32(void *mmio_base, uint32_t offset,
+		uint32_t payload)
+{
+	void *reg_addr = mmio_base + offset;
+#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
+	payload = __bswap_32(payload);
+#endif
+	*((volatile uint32_t *) (reg_addr)) = payload;
+}
+
+static inline void
+set_default_fpga_conf(struct fpga_5gnr_fec_conf *def_conf)
+{
+	/* Set pf mode to true */
+	def_conf->pf_mode_en = true;
+
+	/* Set ratio between UL and DL to 1:1 (unit of weight is 3 CBs) */
+	def_conf->ul_bandwidth = 3;
+	def_conf->dl_bandwidth = 3;
+
+	/* Set Load Balance Factor to 64 */
+	def_conf->dl_load_balance = 64;
+	def_conf->ul_load_balance = 64;
+}
+
+static int
+fpga_read_config_file(const char *arg_cfg_filename,
+		struct fpga_5gnr_fec_conf *fpga_5gnr_fec_conf)
+{
+	const char *cfg_filename;
+
+	if (arg_cfg_filename == NULL) {
+		cfg_filename = getenv(FPGA_5GNR_FEC_CONFIG_FILE_ENV);
+		if (cfg_filename == NULL) {
+			cfg_filename = FPGA_5GNR_FEC_CONFIG_FILE_NAME;
+			printf("'%s' was not set. %s will be used\n",
+					FPGA_5GNR_FEC_CONFIG_FILE_NAME,
+					cfg_filename);
+		} else
+			printf("'%s=%s' config file will be used\n",
+					FPGA_5GNR_FEC_CONFIG_FILE_ENV,
+					cfg_filename);
+	} else
+		cfg_filename = arg_cfg_filename;
+
+	return fpga_5gnr_parse_conf_file(cfg_filename, fpga_5gnr_fec_conf);
+}
+
+/* Read Static Register of FPGA 5GNR FEC device */
+static inline void
+print_static_reg_debug_info(void *mmio_base)
+{
+	uint8_t i, q_id;
+	uint32_t fid;
+	uint32_t version_id = fpga_reg_read_32(mmio_base,
+			FPGA_5GNR_FEC_VERSION_ID);
+	uint16_t config = fpga_reg_read_16(mmio_base,
+			FPGA_5GNR_FEC_CONFIGURATION);
+	uint8_t qmap_done = fpga_reg_read_8(mmio_base,
+			FPGA_5GNR_FEC_QUEUE_PF_VF_MAP_DONE);
+	uint16_t lb_factor = fpga_reg_read_16(mmio_base,
+			FPGA_5GNR_FEC_LOAD_BALANCE_FACTOR);
+	uint16_t ring_desc_len = fpga_reg_read_16(mmio_base,
+			FPGA_5GNR_FEC_RING_DESC_LEN);
+	uint16_t flr_time_out = fpga_reg_read_16(mmio_base,
+			FPGA_5GNR_FEC_FLR_TIME_OUT);
+
+	printf("FEC FPGA RTL v%u.%u\n",
+		((uint16_t)(version_id >> 16)), ((uint16_t)version_id));
+	printf("UL.DL Weights = %u.%u\n",
+			((uint8_t)config), ((uint8_t)(config >> 8)));
+	printf("UL.DL Load Balance = %u.%u\n",
+			((uint8_t)lb_factor), ((uint8_t)(lb_factor >> 8)));
+	printf("Queue-PF/VF Mapping Table = %s\n",
+			(qmap_done > 0) ? "READY" : "NOT-READY");
+	printf("Ring Descriptor Size = %u bytes\n",
+			ring_desc_len*FPGA_RING_DESC_LEN_UNIT_BYTES);
+	printf("FLR Timeout = %f usec\n",
+			(float)flr_time_out*FPGA_FLR_TIMEOUT_UNIT);
+
+	printf("\n--------+-----+-----+-----+-----+-----+-----+-----+-----+-----+\n");
+	printf("        |  PF | VF0 | VF1 | VF2 | VF3 | VF4 | VF5 | VF6 | VF7 |\n");
+	printf("--------+-----+-----+-----+-----+-----+-----+-----+-----+-----+\n");
+
+	for (q_id = 0; q_id < FPGA_TOTAL_NUM_QUEUES; q_id++) {
+
+		printf("%s-Q'%02u |",
+			(q_id < FPGA_NUM_UL_QUEUES) ? "UL" : "DL", q_id);
+
+		fid = fpga_reg_read_32(mmio_base,
+				FPGA_5GNR_FEC_QUEUE_MAP + (q_id << 2));
+
+		for (i = 0; i < 9; ++i) {
+
+			if (!((fid >> 16) & (0x80)) && i == 0) {
+				printf("  X  |");
+				continue;
+			}
+
+			if (((((fid >> 16) & (0x7f)) + 1) == i) &&
+					((fid >> 16) & (0x80)))
+				printf("  X  |");
+			else
+				printf("     |");
+		}
+		printf("\n");
+	}
+	printf("--------+-----+-----+-----+-----+-----+-----+-----+-----+-----+\n\n");
+}
+
+static int
+fpga_write_config(void *mapaddr, struct fpga_5gnr_fec_conf *conf)
+{
+
+	uint32_t payload_32, address;
+	uint16_t payload_16;
+	uint16_t q_id, vf_id, total_q_id, total_ul_q_id, total_dl_q_id;
+
+	uint32_t *bar0addr = mapaddr;
+
+	/*
+	 * Configure UL:DL ratio.
+	 * [7:0]: UL weight
+	 * [15:8]: DL weight
+	 */
+	payload_16 = (conf->dl_bandwidth << 8) | conf->ul_bandwidth;
+	address = FPGA_5GNR_FEC_CONFIGURATION;
+	fpga_reg_write_16(bar0addr, address, payload_16);
+
+	/* Clear all queues registers */
+	payload_32 = FPGA_INVALID_HW_QUEUE_ID;
+	for (q_id = 0; q_id < FPGA_TOTAL_NUM_QUEUES; ++q_id) {
+		address = (q_id << 2) + FPGA_5GNR_FEC_QUEUE_MAP;
+		fpga_reg_write_32(bar0addr, address, payload_32);
+	}
+
+	/*
+	 * If PF mode is enabled allocate all queues for PF only.
+	 *
+	 * For VF mode each VF can have different number of UL and DL queues.
+	 * Total number of queues to configure cannot exceed FPGA
+	 * capabilities - 64 queues - 32 queues for UL and 32 queues for DL.
+	 * Queues mapping is done according to configuration:
+	 *
+	 * UL queues:
+	 * |                Q_ID              | VF_ID |
+	 * |                 0                |   0   |
+	 * |                ...               |   0   |
+	 * | conf->vf_dl_queues_number[0] - 1 |   0   |
+	 * | conf->vf_dl_queues_number[0]     |   1   |
+	 * |                ...               |   1   |
+	 * | conf->vf_dl_queues_number[1] - 1 |   1   |
+	 * |                ...               |  ...  |
+	 * | conf->vf_dl_queues_number[7] - 1 |   7   |
+	 *
+	 * DL queues:
+	 * |                Q_ID              | VF_ID |
+	 * |                 32               |   0   |
+	 * |                ...               |   0   |
+	 * | conf->vf_ul_queues_number[0] - 1 |   0   |
+	 * | conf->vf_ul_queues_number[0]     |   1   |
+	 * |                ...               |   1   |
+	 * | conf->vf_ul_queues_number[1] - 1 |   1   |
+	 * |                ...               |  ...  |
+	 * | conf->vf_ul_queues_number[7] - 1 |   7   |
+	 *
+	 * Example of configuration:
+	 * conf->vf_ul_queues_number[0] = 4;  -> 4 UL queues for VF0
+	 * conf->vf_dl_queues_number[0] = 4;  -> 4 DL queues for VF0
+	 * conf->vf_ul_queues_number[1] = 2;  -> 2 UL queues for VF1
+	 * conf->vf_dl_queues_number[1] = 2;  -> 2 DL queues for VF1
+	 *
+	 * UL:
+	 * | Q_ID | VF_ID |
+	 * |   0  |   0   |
+	 * |   1  |   0   |
+	 * |   2  |   0   |
+	 * |   3  |   0   |
+	 * |   4  |   1   |
+	 * |   5  |   1   |
+	 *
+	 * DL:
+	 * | Q_ID | VF_ID |
+	 * |  32  |   0   |
+	 * |  33  |   0   |
+	 * |  34  |   0   |
+	 * |  35  |   0   |
+	 * |  36  |   1   |
+	 * |  37  |   1   |
+	 */
+	if (conf->pf_mode_en) {
+		payload_32 = 0x1;
+		for (q_id = 0; q_id < FPGA_TOTAL_NUM_QUEUES; ++q_id) {
+			address = (q_id << 2) + FPGA_5GNR_FEC_QUEUE_MAP;
+			fpga_reg_write_32(bar0addr, address, payload_32);
+		}
+	} else {
+		/* Calculate total number of UL and DL queues to configure */
+		total_ul_q_id = total_dl_q_id = 0;
+		for (vf_id = 0; vf_id < FPGA_5GNR_FEC_NUM_VFS; ++vf_id) {
+			total_ul_q_id += conf->vf_ul_queues_number[vf_id];
+			total_dl_q_id += conf->vf_dl_queues_number[vf_id];
+		}
+		total_q_id = total_dl_q_id + total_ul_q_id;
+		/*
+		 * Check if total number of queues to configure does not exceed
+		 * FPGA capabilities (64 queues - 32 UL and 32 DL queues)
+		 */
+		if ((total_ul_q_id > FPGA_NUM_UL_QUEUES) ||
+			(total_dl_q_id > FPGA_NUM_DL_QUEUES) ||
+			(total_q_id > FPGA_TOTAL_NUM_QUEUES)) {
+			printf(
+					"FPGA Configuration failed. Too many queues to configure: UL_Q %u, DL_Q %u, FPGA_Q %u",
+					total_ul_q_id, total_dl_q_id,
+					FPGA_TOTAL_NUM_QUEUES);
+			return -EINVAL;
+		}
+		total_ul_q_id = 0;
+		for (vf_id = 0; vf_id < FPGA_5GNR_FEC_NUM_VFS; ++vf_id) {
+			for (q_id = 0; q_id < conf->vf_ul_queues_number[vf_id];
+					++q_id, ++total_ul_q_id) {
+				address = (total_ul_q_id << 2) +
+						FPGA_5GNR_FEC_QUEUE_MAP;
+				payload_32 = ((0x80 + vf_id) << 16) | 0x1;
+				fpga_reg_write_32(bar0addr, address,
+						payload_32);
+			}
+		}
+		total_dl_q_id = 0;
+		for (vf_id = 0; vf_id < FPGA_5GNR_FEC_NUM_VFS; ++vf_id) {
+			for (q_id = 0; q_id < conf->vf_dl_queues_number[vf_id];
+					++q_id, ++total_dl_q_id) {
+				address = ((total_dl_q_id + FPGA_NUM_UL_QUEUES)
+						<< 2) + FPGA_5GNR_FEC_QUEUE_MAP;
+				payload_32 = ((0x80 + vf_id) << 16) | 0x1;
+				fpga_reg_write_32(bar0addr, address,
+						payload_32);
+			}
+		}
+	}
+
+	/* Setting Load Balance Factor */
+	payload_16 = (conf->dl_load_balance << 8) | (conf->ul_load_balance);
+	address = FPGA_5GNR_FEC_LOAD_BALANCE_FACTOR;
+	fpga_reg_write_16(bar0addr, address, payload_16);
+
+	/* Setting length of ring descriptor entry */
+	payload_16 = FPGA_RING_DESC_ENTRY_LENGTH;
+	address = FPGA_5GNR_FEC_RING_DESC_LEN;
+	fpga_reg_write_16(bar0addr, address, payload_16);
+
+	/* Setting FLR timeout value */
+	payload_16 = conf->flr_time_out;
+	address = FPGA_5GNR_FEC_FLR_TIME_OUT;
+	fpga_reg_write_16(bar0addr, address, payload_16);
+
+	/* Queue PF/VF mapping table is ready */
+	payload_16 = 0x1;
+	address = FPGA_5GNR_FEC_QUEUE_PF_VF_MAP_DONE;
+	fpga_reg_write_16(bar0addr, address, payload_16);
+
+	print_static_reg_debug_info(bar0addr);
+	printf("Mode of operation = %s-mode\n",
+			conf->pf_mode_en ? "PF" : "VF");
+
+	return 0;
+}
+
+int
+fpga_5gnr_configure(void *bar0addr, const char *cfg_filename)
+{
+	struct fpga_5gnr_fec_conf fpga_conf;
+	int ret;
+
+	ret = fpga_read_config_file(cfg_filename, &fpga_conf);
+	if (ret != 0) {
+		printf("Error reading config file.\n");
+		return -1;
+	}
+
+	ret = fpga_write_config(bar0addr, &fpga_conf);
+	if (ret != 0) {
+		printf("Error writing configuration for FPGA.\n");
+		return -1;
+	}
+
+	return 0;
+}
diff --git a/drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_cfg_app.h b/drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_cfg_app.h
new file mode 100644
index 0000000..f7ab8e1
--- /dev/null
+++ b/drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_cfg_app.h
@@ -0,0 +1,102 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2020 Intel Corporation
+ */
+
+#ifndef _FPGA_5GNR_CFG_APP_H_
+#define _FPGA_5GNR_CFG_APP_H_
+
+#include <stdbool.h>
+#include <stdlib.h>
+#include <stdio.h>
+#include <string.h>
+#include <ctype.h>
+#include <inttypes.h>
+#include <errno.h>
+
+/**< Number of Virtual Functions FGPA 5GNR FEC supports */
+#define FPGA_5GNR_FEC_NUM_VFS 8
+#define FPGA_5GNR_FEC_VENDOR_ID 0x8086
+#define FPGA_5GNR_FEC_DEVICE_ID 0x0D8F
+#define FPGA_5GNR_FEC_BAR_SIZE  0x1000
+#define FPGA_5GNR_FEC_DEV_NAME  "FPGA_5GNR_FEC"
+#define FPGA_5GNR_FEC_CONFIG_FILE_ENV "FPGA_5GNR_FEC_CONFIG_FILE"
+#define FPGA_5GNR_FEC_CONFIG_FILE_NAME "fpga_5gnr_fec_config.cfg"
+
+/* Multiplier of 256 bits (32 bytes) */
+#define FPGA_RING_DESC_ENTRY_LENGTH (8)
+#define FPGA_RING_DESC_LEN_UNIT_BYTES (32)
+/* Maximum size of queue */
+#define FPGA_RING_MAX_SIZE (1024)
+#define FPGA_FLR_TIMEOUT_UNIT (16.384)
+
+#define FPGA_NUM_UL_QUEUES (32)
+#define FPGA_NUM_DL_QUEUES (32)
+#define FPGA_TOTAL_NUM_QUEUES (FPGA_NUM_UL_QUEUES + FPGA_NUM_DL_QUEUES)
+
+#define FPGA_INVALID_HW_QUEUE_ID (0xFFFFFFFF)
+
+/* FPGA 5GNR FEC Register mapping on BAR0 */
+enum {
+	FPGA_5GNR_FEC_VERSION_ID = 0x00000000, /* len: 4B */
+	FPGA_5GNR_FEC_CONFIGURATION = 0x00000004, /* len: 2B */
+	FPGA_5GNR_FEC_QUEUE_PF_VF_MAP_DONE = 0x00000008, /* len: 1B */
+	FPGA_5GNR_FEC_LOAD_BALANCE_FACTOR = 0x0000000a, /* len: 2B */
+	FPGA_5GNR_FEC_RING_DESC_LEN = 0x0000000c, /* len: 2B */
+	FPGA_5GNR_FEC_FLR_TIME_OUT = 0x0000000e, /* len: 2B */
+	FPGA_5GNR_FEC_VFQ_FLUSH_STATUS_LW = 0x00000018, /* len: 4B */
+	FPGA_5GNR_FEC_VFQ_FLUSH_STATUS_HI = 0x0000001c, /* len: 4B */
+	FPGA_5GNR_FEC_QUEUE_MAP = 0x00000040, /* len: 256B */
+	FPGA_5GNR_FEC_RING_CTRL_REGS = 0x00000200, /* len: 2048B */
+	FPGA_5GNR_FEC_DDR4_WR_ADDR_REGS = 0x00000A00, /* len: 4B */
+	FPGA_5GNR_FEC_DDR4_WR_DATA_REGS = 0x00000A08, /* len: 8B */
+	FPGA_5GNR_FEC_DDR4_WR_DONE_REGS = 0x00000A10, /* len: 1B */
+	FPGA_5GNR_FEC_DDR4_RD_ADDR_REGS = 0x00000A18, /* len: 4B */
+	FPGA_5GNR_FEC_DDR4_RD_DONE_REGS = 0x00000A20, /* len: 1B */
+	FPGA_5GNR_FEC_DDR4_RD_RDY_REGS = 0x00000A28, /* len: 1B */
+	FPGA_5GNR_FEC_DDR4_RD_DATA_REGS = 0x00000A30, /* len: 8B */
+	FPGA_5GNR_FEC_DDR4_ADDR_RDY_REGS = 0x00000A38, /* len: 1B */
+	FPGA_5GNR_FEC_HARQ_BUF_SIZE_RDY_REGS = 0x00000A40, /* len: 1B */
+	FPGA_5GNR_FEC_HARQ_BUF_SIZE_REGS = 0x00000A48, /* len: 4B */
+	FPGA_5GNR_FEC_MUTEX = 0x00000A60, /* len: 4B */
+	FPGA_5GNR_FEC_MUTEX_RESET = 0x00000A68  /* len: 4B */
+};
+
+/* FIXME Add HARQ/DDR Registers */
+
+/* FPGA 5GNR FEC Ring Control Registers */
+enum {
+	FPGA_5GNR_FEC_RING_HEAD_ADDR = 0x00000008,
+	FPGA_5GNR_FEC_RING_SIZE = 0x00000010,
+	FPGA_5GNR_FEC_RING_MISC = 0x00000014,
+	FPGA_5GNR_FEC_RING_ENABLE = 0x00000015,
+	FPGA_5GNR_FEC_RING_FLUSH_QUEUE_EN = 0x00000016,
+	FPGA_5GNR_FEC_RING_SHADOW_TAIL = 0x00000018,
+	FPGA_5GNR_FEC_RING_HEAD_POINT = 0x0000001C
+};
+
+struct
+fpga_5gnr_fec_conf {
+	/**< 1 if PF is used for dataplane, 0 for VFs */
+	bool pf_mode_en;
+	/**< Number of UL queues per VF */
+	uint8_t vf_ul_queues_number[FPGA_5GNR_FEC_NUM_VFS];
+	/**< Number of DL queues per VF */
+	uint8_t vf_dl_queues_number[FPGA_5GNR_FEC_NUM_VFS];
+	/**< UL bandwidth. Needed for schedule algorithm */
+	uint8_t ul_bandwidth;
+	/**< DL bandwidth. Needed for schedule algorithm */
+	uint8_t dl_bandwidth;
+	/**< UL Load Balance */
+	uint8_t ul_load_balance;
+	/**< DL Load Balance */
+	uint8_t dl_load_balance;
+	/**< FLR timeout value */
+	uint16_t flr_time_out;
+};
+
+/**
+ * Configure FPGA
+ */
+int fpga_5gnr_configure(void *bar0addr, const char *arg_cfg_filename);
+
+#endif /* _FPGA_5GNR_CFG_APP_H_ */
diff --git a/drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_cfg_parser.c b/drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_cfg_parser.c
new file mode 100644
index 0000000..271b213
--- /dev/null
+++ b/drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_cfg_parser.c
@@ -0,0 +1,187 @@
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2020 Intel Corporation
+ */
+
+#include <stdbool.h>
+#include <stdlib.h>
+#include <string.h>
+#include <ctype.h>
+#include <inttypes.h>
+#include <errno.h>
+
+#include <ini.h>
+#include "fpga_5gnr_cfg_app.h"
+
+/* Names of sections used in the configuration file */
+#define MODE "MODE"
+#define UL "UL"
+#define DL "DL"
+#define FLR "FLR"
+
+/* Names of entries in sections used in the configuration file */
+#define PFMODE "pf_mode_en"
+#define BANDWIDTH "bandwidth"
+#define LOAD_BALANCE "load_balance"
+#define QUEUE_MAP "vfqmap"
+#define FLR_TIME_OUT "flr_time_out"
+
+/* Default values for FPGA device configuration variables */
+#define DEFAULT_PF_MODE_EN 1
+#define DEFAULT_UL_BANDWIDTH 3
+#define DEFAULT_DL_BANDWIDTH 3
+#define DEFAULT_UL_LOAD_BALANCE 64
+#define DEFAULT_DL_LOAD_BALANCE 64
+#define DEFAULT_NUM_VF_QUEUE 8
+
+/* Possible values for MODE and LLR_SIGN */
+#define ZERO "0"
+#define ONE "1"
+
+static int
+parse_number8(const char *str, uint8_t *value)
+{
+	uint64_t val = 0;
+	char *end;
+
+	if (str == NULL)
+		return -EINVAL;
+
+	val = strtoul(str, &end, 0);
+	if (val > UINT8_MAX || str == end) {
+		printf("ERROR: Invalid value %" PRIu64 "\n", val);
+		return -ERANGE;
+	}
+
+	*value = (uint8_t) val;
+	return 1;
+}
+
+static int
+parse_number16(const char *str, uint16_t *value)
+{
+	uint64_t val = 0;
+	char *end;
+
+	if (str == NULL)
+		return -EINVAL;
+
+	val = strtoul(str, &end, 0);
+	if (val > UINT16_MAX || str == end) {
+		printf("ERROR: Invalid value %" PRIu64 "\n", val);
+		return -ERANGE;
+	}
+
+	*value = (uint16_t) val;
+	return 1;
+}
+
+static int
+parse_array8(const char *str, uint8_t *array)
+{
+	int i;
+	uint64_t val = 0;
+	char *end;
+
+	if (str == NULL)
+		return -EINVAL;
+
+	char *val_ch = strtok((char *)str, ",");
+	for (i = 0; i < 8 && NULL != val_ch; i++) {
+
+		val = strtoul(val_ch, &end, 0);
+		if (val > UINT8_MAX || val_ch == end) {
+			printf("ERROR: Invalid value %" PRIu64 "\n", val);
+			return -ERANGE;
+		}
+		array[i] = (uint8_t) val;
+
+		val_ch = strtok(NULL, ",");
+	}
+	return 1;
+}
+
+static void
+set_default_config(struct fpga_5gnr_fec_conf *fpga_conf)
+{
+	int i;
+
+	/* Set pf mode to true */
+	fpga_conf->pf_mode_en = DEFAULT_PF_MODE_EN;
+
+	/* Set ratio between UL and DL to 1:1 (unit of weight is 3 CBs) */
+	fpga_conf->ul_bandwidth = DEFAULT_UL_BANDWIDTH;
+	fpga_conf->dl_bandwidth = DEFAULT_DL_BANDWIDTH;
+
+	/* Set Load Balance Factor to 64 */
+	fpga_conf->ul_load_balance = DEFAULT_UL_LOAD_BALANCE;
+	fpga_conf->dl_load_balance = DEFAULT_DL_LOAD_BALANCE;
+
+	for (i = 0; i < FPGA_5GNR_FEC_NUM_VFS; i++) {
+		fpga_conf->vf_ul_queues_number[i] = DEFAULT_NUM_VF_QUEUE / 2;
+		fpga_conf->vf_dl_queues_number[i] = DEFAULT_NUM_VF_QUEUE / 2;
+	}
+}
+
+static int
+fpga_handler(void *user, const char *section,
+	     const char *name, const char *value)
+{
+	struct fpga_5gnr_fec_conf *fpga_conf =
+			(struct fpga_5gnr_fec_conf *) user;
+	int ret = 1;
+
+	if (!strcmp(section, MODE) && !strcmp(name, PFMODE)) {
+		if (!strcmp(value, ZERO))
+			fpga_conf->pf_mode_en = false;
+		else if (!strcmp(value, ONE))
+			fpga_conf->pf_mode_en = true;
+		else
+			ret = 0;
+	} else if (!strcmp(section, UL) && !strcmp(name, BANDWIDTH)) {
+		ret = parse_number8(value, &fpga_conf->ul_bandwidth);
+	} else if (!strcmp(section, DL) && !strcmp(name, BANDWIDTH)) {
+		ret = parse_number8(value, &fpga_conf->dl_bandwidth);
+	} else if (!strcmp(section, UL) && !strcmp(name, LOAD_BALANCE)) {
+		ret = parse_number8(value, &fpga_conf->ul_load_balance);
+	} else if (!strcmp(section, DL) && !strcmp(name, LOAD_BALANCE)) {
+		ret = parse_number8(value, &fpga_conf->dl_load_balance);
+	} else if (!strcmp(section, UL) && !strcmp(name, QUEUE_MAP)) {
+		ret = parse_array8(value, fpga_conf->vf_ul_queues_number);
+	} else if (!strcmp(section, DL) && !strcmp(name, QUEUE_MAP)) {
+		ret = parse_array8(value, fpga_conf->vf_dl_queues_number);
+	} else if (!strcmp(section, FLR) && !strcmp(name, FLR_TIME_OUT)) {
+		ret = parse_number16(value, &fpga_conf->flr_time_out);
+	} else {
+		printf("ERROR: Section (%s) or name (%s) is not valid.\n",
+		       section, name);
+		return 0;
+	}
+	if (ret != 1)
+		printf("Error: Conversion of value (%s) failed.\n", value);
+
+	return ret;
+}
+
+int
+fpga_5gnr_parse_conf_file(const char *file_name,
+		struct fpga_5gnr_fec_conf *fpga_conf)
+{
+	int ret;
+
+	set_default_config(fpga_conf);
+
+	ret = ini_parse(file_name, fpga_handler, fpga_conf);
+
+	if (ret == -1) {
+		printf("ERROR: Error loading configuration file %s\n",
+			file_name);
+		set_default_config(fpga_conf);
+		return -ENOENT;
+	} else if (ret == -2) {
+		printf("ERROR: Memory allocation error\n");
+		set_default_config(fpga_conf);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
diff --git a/drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_config.cfg b/drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_config.cfg
new file mode 100644
index 0000000..0854672
--- /dev/null
+++ b/drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_config.cfg
@@ -0,0 +1,18 @@
+; SPDX-License-Identifier: BSD-3-Clause
+; Copyright(c) 2020 Intel Corporation
+
+[MODE]
+pf_mode_en = 1
+
+[UL]
+bandwidth = 3
+load_balance = 128
+vfqmap = 4,4,4,4,4,4,4,4
+
+[DL]
+bandwidth = 3
+load_balance = 128
+vfqmap = 4,4,4,4,4,4,4,4
+
+[FLR]
+flr_time_out = 610
-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [20.11, PATCH v2] baseband/fpga_5gnr_fec: add companion PF config App
  2020-07-16 20:20 ` [dpdk-dev] [20.11, PATCH v2] baseband/fpga_5gnr_fec: add companion PF config App Nicolas Chautru
@ 2020-07-31 10:35   ` Maxime Coquelin
  2020-07-31 15:17     ` Chautru, Nicolas
  0 siblings, 1 reply; 8+ messages in thread
From: Maxime Coquelin @ 2020-07-31 10:35 UTC (permalink / raw)
  To: Nicolas Chautru, dev, akhil.goyal, thomas, David Marchand

Hi Nicolas,

On 7/16/20 10:20 PM, Nicolas Chautru wrote:
> Adding companion application to configure HW Device from the PF.
> Then the device can be accessed through BBDEV from VF (or PF).
> 
> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> ---
>  doc/guides/bbdevs/fpga_5gnr_fec.rst                |  80 +++--
>  .../baseband/fpga_5gnr_fec/pf_config_app/Makefile  |  36 +++
>  .../fpga_5gnr_fec/pf_config_app/config_app.c       | 333 +++++++++++++++++++
>  .../pf_config_app/fpga_5gnr_cfg_app.c              | 351 +++++++++++++++++++++
>  .../pf_config_app/fpga_5gnr_cfg_app.h              | 102 ++++++
>  .../pf_config_app/fpga_5gnr_cfg_parser.c           | 187 +++++++++++
>  .../pf_config_app/fpga_5gnr_config.cfg             |  18 ++
>  7 files changed, 1087 insertions(+), 20 deletions(-)
>  create mode 100644 drivers/baseband/fpga_5gnr_fec/pf_config_app/Makefile
>  create mode 100644 drivers/baseband/fpga_5gnr_fec/pf_config_app/config_app.c
>  create mode 100644 drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_cfg_app.c
>  create mode 100644 drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_cfg_app.h
>  create mode 100644 drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_cfg_parser.c
>  create mode 100644 drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_config.cfg

I think having the pf_config_app in the driver directory is not a good
idea, this is not the place for applications.

Also, it is not integrated in the DPDK build system, so it cannot
benefit from the CI. Having an external dependency that is not packaged
in distributions will not help to have it integrated in the build
system.

I see some alternatives:
1. Move it in another directory in the main DPDK repo, but it is not a
DPDK example, not a dev tool and not a build tool, so it would need a
new directory.
2. Create a BBDEV tools repository on dpdk.org (It would require
techboard approval).
3. Host it in a dedicated repository on Intel's github
4. Move it into some Intel FPGA tools repository

I think option 3 would be the best to get it packaged into
distributions as it has no build dependency with any DPDK library.
You could maybe add inih library as a git sub-repository within this
repo. Other advantage is you wouldn't depend on DPDK release cycles to
get fixes merged.

Regarding the tool itself, I understand from the commit message that the
tool has a dependency on the BBDEV PMD version, but the tool run on the
host while the PMD driver is used in the guest/container. So having it
in the driver directory will not really help, as host DPDK (if any) and
guest DPDK may come from different parties.

One question I have is whether this is the tool itself that has a
dependency on the PMD, or just the config file?

Regards,
Maxime


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [20.11, PATCH v2] baseband/fpga_5gnr_fec: add companion PF config App
  2020-07-31 10:35   ` Maxime Coquelin
@ 2020-07-31 15:17     ` Chautru, Nicolas
  2020-08-03  8:25       ` Maxime Coquelin
  0 siblings, 1 reply; 8+ messages in thread
From: Chautru, Nicolas @ 2020-07-31 15:17 UTC (permalink / raw)
  To: Maxime Coquelin, dev, akhil.goyal, thomas, David Marchand,
	Richardson, Bruce

Hi Maxime, 

> 
> Hi Nicolas,
> 
> On 7/16/20 10:20 PM, Nicolas Chautru wrote:
> > Adding companion application to configure HW Device from the PF.
> > Then the device can be accessed through BBDEV from VF (or PF).
> >
> > Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> > ---
> >  doc/guides/bbdevs/fpga_5gnr_fec.rst                |  80 +++--
> >  .../baseband/fpga_5gnr_fec/pf_config_app/Makefile  |  36 +++
> >  .../fpga_5gnr_fec/pf_config_app/config_app.c       | 333
> +++++++++++++++++++
> >  .../pf_config_app/fpga_5gnr_cfg_app.c              | 351
> +++++++++++++++++++++
> >  .../pf_config_app/fpga_5gnr_cfg_app.h              | 102 ++++++
> >  .../pf_config_app/fpga_5gnr_cfg_parser.c           | 187 +++++++++++
> >  .../pf_config_app/fpga_5gnr_config.cfg             |  18 ++
> >  7 files changed, 1087 insertions(+), 20 deletions(-)  create mode
> > 100644 drivers/baseband/fpga_5gnr_fec/pf_config_app/Makefile
> >  create mode 100644
> > drivers/baseband/fpga_5gnr_fec/pf_config_app/config_app.c
> >  create mode 100644
> > drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_cfg_app.c
> >  create mode 100644
> > drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_cfg_app.h
> >  create mode 100644
> > drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_cfg_parser.c
> >  create mode 100644
> > drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_config.cfg
> 
> I think having the pf_config_app in the driver directory is not a good idea,
> this is not the place for applications.
> 
> Also, it is not integrated in the DPDK build system, so it cannot benefit from
> the CI. Having an external dependency that is not packaged in distributions
> will not help to have it integrated in the build system.
> 

Thanks for sharing.
Note that all these points were raised openly explicitly earlier as you know, ie part of both pros and cons.  
Still happy to get feedback from others notably Thomas. It appears you had side conversations with him on this very topic. 

> I see some alternatives:
> 1. Move it in another directory in the main DPDK repo, but it is not a DPDK
> example, not a dev tool and not a build tool, so it would need a new
> directory.
> 2. Create a BBDEV tools repository on dpdk.org (It would require techboard
> approval).
> 3. Host it in a dedicated repository on Intel's github 4. Move it into some
> Intel FPGA tools repository

There are several others options which were indeed considered in case this option was not viable. 
Still DPDK was considered best option so far to keep everything in one recognized place for BBDEV devices but happy to get further input from others. 

> I think option 3 would be the best to get it packaged into distributions as it
> has no build dependency with any DPDK library.
> You could maybe add inih library as a git sub-repository within this repo.
> Other advantage is you wouldn't depend on DPDK release cycles to get fixes
> merged.
> 
> Regarding the tool itself, I understand from the commit message that the
> tool has a dependency on the BBDEV PMD version, but the tool run on the
> host while the PMD driver is used in the guest/container. So having it in the
> driver directory will not really help, as host DPDK (if any) and guest DPDK may
> come from different parties.

Yes this was captured earlier, purely stored there as a companion application for a given
version of the PMD (ie. different subdirectories for each PMD directory).
They do no run in the same container for deployment and are not built at the same time indeed, their interface is the HW really and one being needed to be run prior to the other one to be functional.  

> One question I have is whether this is the tool itself that has a dependency on
> the PMD, or just the config file?

Each PMD directory would have its own version of the companion PF config application.
Ie. the patch above is only for baseband/fpga_5gnr_fec ie. Intel Vista Creek with 5G LDPC user image. 
There will be different companion applications upstreamed for each other PMD directories (current and future) as they rely on different HW devices with independent MMIO access. 
Said otherwise both the config file (features exposed) and implementation (registers required for these features) are defined per HW device (+ user image for FPGA)  hence per PMD version. 

There 2 entities have no API between themselves, only indirectly through HW (no shared memory, VF2PF comms, etc..). 
New features may have to be added concurrently though, hence splitting repos create room for version mismatch and complicate the ingredients line up. 
That was part of the pros and cons described earlier and I can totally see arguments both ways, and that's what I have been trying to share openly in this ticket history. 


Basically I see nothing fundamentally new here in the discussion, but it is great to receive input and I am happy to hear further input from tech board or others towards a decision. 
This started as an open discussion on this DPDK mailing list capturing explicitly both pros and cons of this approach which are arguable, and in case this is not deemed practical eventually then we can still totally come back internally to the drawing board with other options outside of DPDK. 

Thanks, 
Nic


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [20.11, PATCH v2] baseband/fpga_5gnr_fec: add companion PF config App
  2020-07-31 15:17     ` Chautru, Nicolas
@ 2020-08-03  8:25       ` Maxime Coquelin
  2020-08-03 16:18         ` Chautru, Nicolas
  0 siblings, 1 reply; 8+ messages in thread
From: Maxime Coquelin @ 2020-08-03  8:25 UTC (permalink / raw)
  To: Chautru, Nicolas, dev, akhil.goyal, thomas, David Marchand,
	Richardson, Bruce

Hi Nicolas,

On 7/31/20 5:17 PM, Chautru, Nicolas wrote:
> Hi Maxime, 
> 
>>
>> Hi Nicolas,
>>
>> On 7/16/20 10:20 PM, Nicolas Chautru wrote:
>>> Adding companion application to configure HW Device from the PF.
>>> Then the device can be accessed through BBDEV from VF (or PF).
>>>
>>> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
>>> ---
>>>  doc/guides/bbdevs/fpga_5gnr_fec.rst                |  80 +++--
>>>  .../baseband/fpga_5gnr_fec/pf_config_app/Makefile  |  36 +++
>>>  .../fpga_5gnr_fec/pf_config_app/config_app.c       | 333
>> +++++++++++++++++++
>>>  .../pf_config_app/fpga_5gnr_cfg_app.c              | 351
>> +++++++++++++++++++++
>>>  .../pf_config_app/fpga_5gnr_cfg_app.h              | 102 ++++++
>>>  .../pf_config_app/fpga_5gnr_cfg_parser.c           | 187 +++++++++++
>>>  .../pf_config_app/fpga_5gnr_config.cfg             |  18 ++
>>>  7 files changed, 1087 insertions(+), 20 deletions(-)  create mode
>>> 100644 drivers/baseband/fpga_5gnr_fec/pf_config_app/Makefile
>>>  create mode 100644
>>> drivers/baseband/fpga_5gnr_fec/pf_config_app/config_app.c
>>>  create mode 100644
>>> drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_cfg_app.c
>>>  create mode 100644
>>> drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_cfg_app.h
>>>  create mode 100644
>>> drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_cfg_parser.c
>>>  create mode 100644
>>> drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_config.cfg
>>
>> I think having the pf_config_app in the driver directory is not a good idea,
>> this is not the place for applications.
>>
>> Also, it is not integrated in the DPDK build system, so it cannot benefit from
>> the CI. Having an external dependency that is not packaged in distributions
>> will not help to have it integrated in the build system.
>>
> 
> Thanks for sharing.
> Note that all these points were raised openly explicitly earlier as you know, ie part of both pros and cons.  
> Still happy to get feedback from others notably Thomas. It appears you had side conversations with him on this very topic. 
> 
>> I see some alternatives:
>> 1. Move it in another directory in the main DPDK repo, but it is not a DPDK
>> example, not a dev tool and not a build tool, so it would need a new
>> directory.
>> 2. Create a BBDEV tools repository on dpdk.org (It would require techboard
>> approval).
>> 3. Host it in a dedicated repository on Intel's github 4. Move it into some
>> Intel FPGA tools repository
> 
> There are several others options which were indeed considered in case this option was not viable. 
> Still DPDK was considered best option so far to keep everything in one recognized place for BBDEV devices but happy to get further input from others. 
> 
>> I think option 3 would be the best to get it packaged into distributions as it
>> has no build dependency with any DPDK library.
>> You could maybe add inih library as a git sub-repository within this repo.
>> Other advantage is you wouldn't depend on DPDK release cycles to get fixes
>> merged.
>>
>> Regarding the tool itself, I understand from the commit message that the
>> tool has a dependency on the BBDEV PMD version, but the tool run on the
>> host while the PMD driver is used in the guest/container. So having it in the
>> driver directory will not really help, as host DPDK (if any) and guest DPDK may
>> come from different parties.
> 
> Yes this was captured earlier, purely stored there as a companion application for a given
> version of the PMD (ie. different subdirectories for each PMD directory).
> They do no run in the same container for deployment and are not built at the same time indeed, their interface is the HW really and one being needed to be run prior to the other one to be functional.  
> 
>> One question I have is whether this is the tool itself that has a dependency on
>> the PMD, or just the config file?
> 
> Each PMD directory would have its own version of the companion PF config application.
> Ie. the patch above is only for baseband/fpga_5gnr_fec ie. Intel Vista Creek with 5G LDPC user image.

OK. Does it mean the same application and configuration will work for
baseband/fpga_5gnr_fec PMD versions v20.11, v21.02, v21.05, etc, ...?

If not, is there a way for the PMD driver to detect whether a wrong
configuration was applied? Something like checking the FW version of a
NIC is supported by the PMD driver.

> There will be different companion applications upstreamed for each other PMD directories (current and future) as they rely on different HW devices with independent MMIO access. 
> Said otherwise both the config file (features exposed) and implementation (registers required for these features) are defined per HW device (+ user image for FPGA)  hence per PMD version.


Would it make sense to have a single application, with having the
registers map and their values to apply in a configuration file?
It would avoid code duplication between devices and so ease the
maintenance.

> 
> There 2 entities have no API between themselves, only indirectly through HW (no shared memory, VF2PF comms, etc..). 
> New features may have to be added concurrently though, hence splitting repos create room for version mismatch and complicate the ingredients line up.

I am not sure that this argument holds, because one could argue in this
case that we could place the FPGA bitstream in the PMD directory too to
ensure there is no mismatch.

> That was part of the pros and cons described earlier and I can totally see arguments both ways, and that's what I have been trying to share openly in this ticket history. 

Thanks, I appreciate that.

> 
> Basically I see nothing fundamentally new here in the discussion, but it is great to receive input and I am happy to hear further input from tech board or others towards a decision. 
> This started as an open discussion on this DPDK mailing list capturing explicitly both pros and cons of this approach which are arguable, and in case this is not deemed practical eventually then we can still totally come back internally to the drawing board with other options outside of DPDK. 
> 
> Thanks, 
> Nic
> 

Regards,
Maxime


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [20.11, PATCH v2] baseband/fpga_5gnr_fec: add companion PF config App
  2020-08-03  8:25       ` Maxime Coquelin
@ 2020-08-03 16:18         ` Chautru, Nicolas
  2020-08-04  8:49           ` Maxime Coquelin
  0 siblings, 1 reply; 8+ messages in thread
From: Chautru, Nicolas @ 2020-08-03 16:18 UTC (permalink / raw)
  To: Maxime Coquelin, dev, akhil.goyal, thomas, David Marchand,
	Richardson, Bruce

Hi Maxime, Thomas, 

> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Hi Nicolas,
> 
> On 7/31/20 5:17 PM, Chautru, Nicolas wrote:
> > Hi Maxime,
> >
> >>
> >> Hi Nicolas,
> >>
> >> On 7/16/20 10:20 PM, Nicolas Chautru wrote:
> >>> Adding companion application to configure HW Device from the PF.
> >>> Then the device can be accessed through BBDEV from VF (or PF).
> >>>
> >>> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> >>> ---
> >>>  doc/guides/bbdevs/fpga_5gnr_fec.rst                |  80 +++--
> >>>  .../baseband/fpga_5gnr_fec/pf_config_app/Makefile  |  36 +++
> >>>  .../fpga_5gnr_fec/pf_config_app/config_app.c       | 333
> >> +++++++++++++++++++
> >>>  .../pf_config_app/fpga_5gnr_cfg_app.c              | 351
> >> +++++++++++++++++++++
> >>>  .../pf_config_app/fpga_5gnr_cfg_app.h              | 102 ++++++
> >>>  .../pf_config_app/fpga_5gnr_cfg_parser.c           | 187 +++++++++++
> >>>  .../pf_config_app/fpga_5gnr_config.cfg             |  18 ++
> >>>  7 files changed, 1087 insertions(+), 20 deletions(-)  create mode
> >>> 100644 drivers/baseband/fpga_5gnr_fec/pf_config_app/Makefile
> >>>  create mode 100644
> >>> drivers/baseband/fpga_5gnr_fec/pf_config_app/config_app.c
> >>>  create mode 100644
> >>> drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_cfg_app.c
> >>>  create mode 100644
> >>> drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_cfg_app.h
> >>>  create mode 100644
> >>>
> drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_cfg_parser.c
> >>>  create mode 100644
> >>> drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_config.cfg
> >>
> >> I think having the pf_config_app in the driver directory is not a
> >> good idea, this is not the place for applications.
> >>
> >> Also, it is not integrated in the DPDK build system, so it cannot
> >> benefit from the CI. Having an external dependency that is not
> >> packaged in distributions will not help to have it integrated in the build
> system.
> >>
> >
> > Thanks for sharing.
> > Note that all these points were raised openly explicitly earlier as you know,
> ie part of both pros and cons.
> > Still happy to get feedback from others notably Thomas. It appears you had
> side conversations with him on this very topic.
> >
> >> I see some alternatives:
> >> 1. Move it in another directory in the main DPDK repo, but it is not
> >> a DPDK example, not a dev tool and not a build tool, so it would need
> >> a new directory.
> >> 2. Create a BBDEV tools repository on dpdk.org (It would require
> >> techboard approval).
> >> 3. Host it in a dedicated repository on Intel's github 4. Move it
> >> into some Intel FPGA tools repository
> >
> > There are several others options which were indeed considered in case this
> option was not viable.
> > Still DPDK was considered best option so far to keep everything in one
> recognized place for BBDEV devices but happy to get further input from
> others.
> >
> >> I think option 3 would be the best to get it packaged into
> >> distributions as it has no build dependency with any DPDK library.
> >> You could maybe add inih library as a git sub-repository within this repo.
> >> Other advantage is you wouldn't depend on DPDK release cycles to get
> >> fixes merged.
> >>
> >> Regarding the tool itself, I understand from the commit message that
> >> the tool has a dependency on the BBDEV PMD version, but the tool run
> >> on the host while the PMD driver is used in the guest/container. So
> >> having it in the driver directory will not really help, as host DPDK
> >> (if any) and guest DPDK may come from different parties.
> >
> > Yes this was captured earlier, purely stored there as a companion
> > application for a given version of the PMD (ie. different subdirectories for
> each PMD directory).
> > They do no run in the same container for deployment and are not built at
> the same time indeed, their interface is the HW really and one being needed
> to be run prior to the other one to be functional.
> >
> >> One question I have is whether this is the tool itself that has a
> >> dependency on the PMD, or just the config file?
> >
> > Each PMD directory would have its own version of the companion PF
> config application.
> > Ie. the patch above is only for baseband/fpga_5gnr_fec ie. Intel Vista Creek
> with 5G LDPC user image.
> 
> OK. Does it mean the same application and configuration will work for
> baseband/fpga_5gnr_fec PMD versions v20.11, v21.02, v21.05, etc, ...?
> 
> If not, is there a way for the PMD driver to detect whether a wrong
> configuration was applied? Something like checking the FW version of a NIC
> is supported by the PMD driver.
> 

As mentioned above there is no API between the 2 config-app companion and BBDEV/DPDK, as they belong logically to 2 different entities (possibly containers) without shared interface except indirectly through HW. 
There is no strict API which could be broken between the 2, but purely 2 SW ingredients that ideally should be aligned to avoid discrepancy during deployment or validation. 
To answer your question I don't foresee a specific case right now causing a strict loss of functional compatibility. 

Note that the configure function being used within config-app already exists right now within the DPDK PMD : ie. that function is the one being exposed by PMD and used when running bbdev-test in DPDK for PF. The limitation being that this is only used when running *within DPDK and from PF*, which is not the deployment use case ie. when DPDK is on the VF only, and PF only performs HW configuration without DPDK dependency (apologize for repeating). 
You can see 'fpga_write_config()' in the current patchset which does match line-for-line with 'fpga_5gnr_fec_configure()' already in the PMD on DPDK main branch. Virtually the same code really with same config input structure 'fpga_5gnr_fec_conf' and same registers being written to.
Difference being that the former implementation is formally integrated in DPDK while the other (config app companion in same directory) has no DPDK dependency. That's it. 
There is concern in having these 2 different implementations of the same initialization sequence diverging (a fix to a register setting would need to be applied on both repositories with different versioning) and splitting the HW support into multiple repos, hence my personal preference to keep everything in one place. 

As stated from the beginning I personally see pros and cons on both sides, all in all DPDK arguably being cleaner to me for the reasons above. 
Still the most important is that it needs to make sense and be valuable to DPDK. 
Based on latest feedback from Maxime and Thomas, I believe this is now trending towards being preferred outside of DPDK. If you can confirm by email then that is fine and I will just abandon this ticket so that we can consider alternate options and split this into different repos. 

> > There will be different companion applications upstreamed for each other
> PMD directories (current and future) as they rely on different HW devices
> with independent MMIO access.
> > Said otherwise both the config file (features exposed) and implementation
> (registers required for these features) are defined per HW device (+ user
> image for FPGA)  hence per PMD version.
> 
> 
> Would it make sense to have a single application, with having the registers
> map and their values to apply in a configuration file?
> It would avoid code duplication between devices and so ease the
> maintenance.
> 

Some pros and cons as well, but if outside DPDK we may just do this. 

> >
> > There 2 entities have no API between themselves, only indirectly through
> HW (no shared memory, VF2PF comms, etc..).
> > New features may have to be added concurrently though, hence splitting
> repos create room for version mismatch and complicate the ingredients line
> up.
> 
> I am not sure that this argument holds, because one could argue in this case
> that we could place the FPGA bitstream in the PMD directory too to ensure
> there is no mismatch.
> 

There are obviously licenses differences between the DPDK code and FPGA user images preventing to do this. 
But still for the sake of the argument logically a different user image may indeed not
work with a PMD version not matching the feature set (as happened prior to PRQ). 
From now on any future user image on the FPGA would need to be strictly compatible with current PMD. 
The main point being that splitting these SW ingredients means increasing the number of different versions to keep track of and different repos to use to create a working environment. 
Both options are viable with different pros and cons.  

> > That was part of the pros and cons described earlier and I can totally see
> arguments both ways, and that's what I have been trying to share openly in
> this ticket history.
> 
> Thanks, I appreciate that.
> 
> >
> > Basically I see nothing fundamentally new here in the discussion, but it is
> great to receive input and I am happy to hear further input from tech board
> or others towards a decision.
> > This started as an open discussion on this DPDK mailing list capturing
> explicitly both pros and cons of this approach which are arguable, and in case
> this is not deemed practical eventually then we can still totally come back
> internally to the drawing board with other options outside of DPDK.
> >
> > Thanks,
> > Nic
> >
> 
> Regards,
> Maxime

Thanks
Nic


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [20.11, PATCH v2] baseband/fpga_5gnr_fec: add companion PF config App
  2020-08-03 16:18         ` Chautru, Nicolas
@ 2020-08-04  8:49           ` Maxime Coquelin
  2020-08-05  8:51             ` David Marchand
  0 siblings, 1 reply; 8+ messages in thread
From: Maxime Coquelin @ 2020-08-04  8:49 UTC (permalink / raw)
  To: Chautru, Nicolas, dev, akhil.goyal, thomas, David Marchand,
	Richardson, Bruce

Hi Nicolas,

On 8/3/20 6:18 PM, Chautru, Nicolas wrote:
> Hi Maxime, Thomas, 
> 
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Hi Nicolas,
>>
>> On 7/31/20 5:17 PM, Chautru, Nicolas wrote:
>>> Hi Maxime,
>>>
>>>>
>>>> Hi Nicolas,
>>>>
>>>> On 7/16/20 10:20 PM, Nicolas Chautru wrote:
>>>>> Adding companion application to configure HW Device from the PF.
>>>>> Then the device can be accessed through BBDEV from VF (or PF).
>>>>>
>>>>> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
>>>>> ---
>>>>>  doc/guides/bbdevs/fpga_5gnr_fec.rst                |  80 +++--
>>>>>  .../baseband/fpga_5gnr_fec/pf_config_app/Makefile  |  36 +++
>>>>>  .../fpga_5gnr_fec/pf_config_app/config_app.c       | 333
>>>> +++++++++++++++++++
>>>>>  .../pf_config_app/fpga_5gnr_cfg_app.c              | 351
>>>> +++++++++++++++++++++
>>>>>  .../pf_config_app/fpga_5gnr_cfg_app.h              | 102 ++++++
>>>>>  .../pf_config_app/fpga_5gnr_cfg_parser.c           | 187 +++++++++++
>>>>>  .../pf_config_app/fpga_5gnr_config.cfg             |  18 ++
>>>>>  7 files changed, 1087 insertions(+), 20 deletions(-)  create mode
>>>>> 100644 drivers/baseband/fpga_5gnr_fec/pf_config_app/Makefile
>>>>>  create mode 100644
>>>>> drivers/baseband/fpga_5gnr_fec/pf_config_app/config_app.c
>>>>>  create mode 100644
>>>>> drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_cfg_app.c
>>>>>  create mode 100644
>>>>> drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_cfg_app.h
>>>>>  create mode 100644
>>>>>
>> drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_cfg_parser.c
>>>>>  create mode 100644
>>>>> drivers/baseband/fpga_5gnr_fec/pf_config_app/fpga_5gnr_config.cfg
>>>>
>>>> I think having the pf_config_app in the driver directory is not a
>>>> good idea, this is not the place for applications.
>>>>
>>>> Also, it is not integrated in the DPDK build system, so it cannot
>>>> benefit from the CI. Having an external dependency that is not
>>>> packaged in distributions will not help to have it integrated in the build
>> system.
>>>>
>>>
>>> Thanks for sharing.
>>> Note that all these points were raised openly explicitly earlier as you know,
>> ie part of both pros and cons.
>>> Still happy to get feedback from others notably Thomas. It appears you had
>> side conversations with him on this very topic.
>>>
>>>> I see some alternatives:
>>>> 1. Move it in another directory in the main DPDK repo, but it is not
>>>> a DPDK example, not a dev tool and not a build tool, so it would need
>>>> a new directory.
>>>> 2. Create a BBDEV tools repository on dpdk.org (It would require
>>>> techboard approval).
>>>> 3. Host it in a dedicated repository on Intel's github 4. Move it
>>>> into some Intel FPGA tools repository
>>>
>>> There are several others options which were indeed considered in case this
>> option was not viable.
>>> Still DPDK was considered best option so far to keep everything in one
>> recognized place for BBDEV devices but happy to get further input from
>> others.
>>>
>>>> I think option 3 would be the best to get it packaged into
>>>> distributions as it has no build dependency with any DPDK library.
>>>> You could maybe add inih library as a git sub-repository within this repo.
>>>> Other advantage is you wouldn't depend on DPDK release cycles to get
>>>> fixes merged.
>>>>
>>>> Regarding the tool itself, I understand from the commit message that
>>>> the tool has a dependency on the BBDEV PMD version, but the tool run
>>>> on the host while the PMD driver is used in the guest/container. So
>>>> having it in the driver directory will not really help, as host DPDK
>>>> (if any) and guest DPDK may come from different parties.
>>>
>>> Yes this was captured earlier, purely stored there as a companion
>>> application for a given version of the PMD (ie. different subdirectories for
>> each PMD directory).
>>> They do no run in the same container for deployment and are not built at
>> the same time indeed, their interface is the HW really and one being needed
>> to be run prior to the other one to be functional.
>>>
>>>> One question I have is whether this is the tool itself that has a
>>>> dependency on the PMD, or just the config file?
>>>
>>> Each PMD directory would have its own version of the companion PF
>> config application.
>>> Ie. the patch above is only for baseband/fpga_5gnr_fec ie. Intel Vista Creek
>> with 5G LDPC user image.
>>
>> OK. Does it mean the same application and configuration will work for
>> baseband/fpga_5gnr_fec PMD versions v20.11, v21.02, v21.05, etc, ...?
>>
>> If not, is there a way for the PMD driver to detect whether a wrong
>> configuration was applied? Something like checking the FW version of a NIC
>> is supported by the PMD driver.
>>
> 
> As mentioned above there is no API between the 2 config-app companion and BBDEV/DPDK, as they belong logically to 2 different entities (possibly containers) without shared interface except indirectly through HW. 
> There is no strict API which could be broken between the 2, but purely 2 SW ingredients that ideally should be aligned to avoid discrepancy during deployment or validation. 
> To answer your question I don't foresee a specific case right now causing a strict loss of functional compatibility. 
> 
> Note that the configure function being used within config-app already exists right now within the DPDK PMD : ie. that function is the one being exposed by PMD and used when running bbdev-test in DPDK for PF. The limitation being that this is only used when running *within DPDK and from PF*, which is not the deployment use case ie. when DPDK is on the VF only, and PF only performs HW configuration without DPDK dependency (apologize for repeating).

OK, got it.

> You can see 'fpga_write_config()' in the current patchset which does match line-for-line with 'fpga_5gnr_fec_configure()' already in the PMD on DPDK main branch. Virtually the same code really with same config input structure 'fpga_5gnr_fec_conf' and same registers being written to.
> Difference being that the former implementation is formally integrated in DPDK while the other (config app companion in same directory) has no DPDK dependency. That's it. 
> There is concern in having these 2 different implementations of the same initialization sequence diverging (a fix to a register setting would need to be applied on both repositories with different versioning) and splitting the HW support into multiple repos, hence my personal preference to keep everything in one place. 

I understand it would be easier for you.
Maybe an alternative would be to have a simple DPDK application that
uses the PMD driver on the PF just for the initialization. Doing that,
you would not even have to duplicate the config part between the PMD
driver and the application, and you would have a single config app for
every cards and bitstreams. It would also mean you get rid off the
external dependency on inih library.

> As stated from the beginning I personally see pros and cons on both sides, all in all DPDK arguably being cleaner to me for the reasons above. 
> Still the most important is that it needs to make sense and be valuable to DPDK. 
> Based on latest feedback from Maxime and Thomas, I believe this is now trending towards being preferred outside of DPDK. If you can confirm by email then that is fine and I will just abandon this ticket so that we can consider alternate options and split this into different repos. 

In its current form, this is a nack for me. I would prefer either having
it outside of DPDK, or having a DPDK application calling the PMD
driver init.

>>> There will be different companion applications upstreamed for each other
>> PMD directories (current and future) as they rely on different HW devices
>> with independent MMIO access.
>>> Said otherwise both the config file (features exposed) and implementation
>> (registers required for these features) are defined per HW device (+ user
>> image for FPGA)  hence per PMD version.
>>
>>
>> Would it make sense to have a single application, with having the registers
>> map and their values to apply in a configuration file?
>> It would avoid code duplication between devices and so ease the
>> maintenance.
>>
> 
> Some pros and cons as well, but if outside DPDK we may just do this. 
> 
>>>
>>> There 2 entities have no API between themselves, only indirectly through
>> HW (no shared memory, VF2PF comms, etc..).
>>> New features may have to be added concurrently though, hence splitting
>> repos create room for version mismatch and complicate the ingredients line
>> up.
>>
>> I am not sure that this argument holds, because one could argue in this case
>> that we could place the FPGA bitstream in the PMD directory too to ensure
>> there is no mismatch.
>>
> 
> There are obviously licenses differences between the DPDK code and FPGA user images preventing to do this. 
> But still for the sake of the argument logically a different user image may indeed not
> work with a PMD version not matching the feature set (as happened prior to PRQ). 
> From now on any future user image on the FPGA would need to be strictly compatible with current PMD. 
> The main point being that splitting these SW ingredients means increasing the number of different versions to keep track of and different repos to use to create a working environment. 

Ok for prototyping, but for production host and guest SW are likely to
come from different streams.

Thanks for the detailed explanations,
Maxime

> Both options are viable with different pros and cons.  
> 
>>> That was part of the pros and cons described earlier and I can totally see
>> arguments both ways, and that's what I have been trying to share openly in
>> this ticket history.
>>
>> Thanks, I appreciate that.
>>
>>>
>>> Basically I see nothing fundamentally new here in the discussion, but it is
>> great to receive input and I am happy to hear further input from tech board
>> or others towards a decision.
>>> This started as an open discussion on this DPDK mailing list capturing
>> explicitly both pros and cons of this approach which are arguable, and in case
>> this is not deemed practical eventually then we can still totally come back
>> internally to the drawing board with other options outside of DPDK.
>>>
>>> Thanks,
>>> Nic
>>>
>>
>> Regards,
>> Maxime
> 
> Thanks
> Nic
> 


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [dpdk-dev] [20.11, PATCH v2] baseband/fpga_5gnr_fec: add companion PF config App
  2020-08-04  8:49           ` Maxime Coquelin
@ 2020-08-05  8:51             ` David Marchand
  0 siblings, 0 replies; 8+ messages in thread
From: David Marchand @ 2020-08-05  8:51 UTC (permalink / raw)
  To: Chautru, Nicolas, Maxime Coquelin
  Cc: dev, akhil.goyal, thomas, Richardson, Bruce

Hello Nicolas, Maxime,

On Tue, Aug 4, 2020 at 10:49 AM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
> >>>> One question I have is whether this is the tool itself that has a
> >>>> dependency on the PMD, or just the config file?
> >>>
> >>> Each PMD directory would have its own version of the companion PF
> >> config application.
> >>> Ie. the patch above is only for baseband/fpga_5gnr_fec ie. Intel Vista Creek
> >> with 5G LDPC user image.
> >>
> >> OK. Does it mean the same application and configuration will work for
> >> baseband/fpga_5gnr_fec PMD versions v20.11, v21.02, v21.05, etc, ...?
> >>
> >> If not, is there a way for the PMD driver to detect whether a wrong
> >> configuration was applied? Something like checking the FW version of a NIC
> >> is supported by the PMD driver.
> >>
> >
> > As mentioned above there is no API between the 2 config-app companion and BBDEV/DPDK, as they belong logically to 2 different entities (possibly containers) without shared interface except indirectly through HW.
> > There is no strict API which could be broken between the 2, but purely 2 SW ingredients that ideally should be aligned to avoid discrepancy during deployment or validation.
> > To answer your question I don't foresee a specific case right now causing a strict loss of functional compatibility.
> >
> > Note that the configure function being used within config-app already exists right now within the DPDK PMD : ie. that function is the one being exposed by PMD and used when running bbdev-test in DPDK for PF. The limitation being that this is only used when running *within DPDK and from PF*, which is not the deployment use case ie. when DPDK is on the VF only, and PF only performs HW configuration without DPDK dependency (apologize for repeating).

I am naively thinking that the bitstream + this configuration are a
whole for a given usecase and should be loaded as a single step.
A bit like how vendors provide ready-to-use firmwares on their nics.

Leaving the users with the possibility to load whatever config they
write is scary.
When hitting an init issue, application crash etc... in a customer
env, how will we know the configuration is actually what it should be?


>
> OK, got it.
>
> > You can see 'fpga_write_config()' in the current patchset which does match line-for-line with 'fpga_5gnr_fec_configure()' already in the PMD on DPDK main branch. Virtually the same code really with same config input structure 'fpga_5gnr_fec_conf' and same registers being written to.
> > Difference being that the former implementation is formally integrated in DPDK while the other (config app companion in same directory) has no DPDK dependency. That's it.
> > There is concern in having these 2 different implementations of the same initialization sequence diverging (a fix to a register setting would need to be applied on both repositories with different versioning) and splitting the HW support into multiple repos, hence my personal preference to keep everything in one place.
>
> I understand it would be easier for you.
> Maybe an alternative would be to have a simple DPDK application that
> uses the PMD driver on the PF just for the initialization. Doing that,
> you would not even have to duplicate the config part between the PMD
> driver and the application, and you would have a single config app for
> every cards and bitstreams. It would also mean you get rid off the
> external dependency on inih library.
>
> > As stated from the beginning I personally see pros and cons on both sides, all in all DPDK arguably being cleaner to me for the reasons above.
> > Still the most important is that it needs to make sense and be valuable to DPDK.
> > Based on latest feedback from Maxime and Thomas, I believe this is now trending towards being preferred outside of DPDK. If you can confirm by email then that is fine and I will just abandon this ticket so that we can consider alternate options and split this into different repos.
>
> In its current form, this is a nack for me. I would prefer either having
> it outside of DPDK, or having a DPDK application calling the PMD
> driver init.

This is a nack from me too.

About having a DPDK application calling the PMD driver init.
This requires to have a DPDK application from version X touching the
hw in the host, while a guest could be running DPDK version Y.
This will result in ping/pong when the support on the host and guest
sides is done by two different teams.


Thanks.

-- 
David Marchand


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-16 20:20 [dpdk-dev] [20.11, PATCH v2] BBDEV FPGA PF Config app Nicolas Chautru
2020-07-16 20:20 ` [dpdk-dev] [20.11, PATCH v2] baseband/fpga_5gnr_fec: add companion PF config App Nicolas Chautru
2020-07-31 10:35   ` Maxime Coquelin
2020-07-31 15:17     ` Chautru, Nicolas
2020-08-03  8:25       ` Maxime Coquelin
2020-08-03 16:18         ` Chautru, Nicolas
2020-08-04  8:49           ` Maxime Coquelin
2020-08-05  8:51             ` David Marchand

DPDK patches and discussions

Archives are clonable:
	git clone --mirror http://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ http://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev


Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox