[v9,03/10] baseband/acc100: add info get function

Message ID 1601339385-117424-4-git-send-email-nicolas.chautru@intel.com (mailing list archive)
State Superseded, archived
Delegated to: akhil goyal
Headers
Series bbdev PMD ACC100 |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Chautru, Nicolas Sept. 29, 2020, 12:29 a.m. UTC
  Add in the "info_get" function to the driver, to allow us to query the
device.
No processing capability are available yet.
Linking bbdev-test to support the PMD with null capability.

Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
Acked-by: Liu Tianjiao <Tianjiao.liu@intel.com>
---
 app/test-bbdev/meson.build               |   3 +
 drivers/baseband/acc100/rte_acc100_cfg.h |  96 +++++++++++++
 drivers/baseband/acc100/rte_acc100_pmd.c | 225 +++++++++++++++++++++++++++++++
 drivers/baseband/acc100/rte_acc100_pmd.h |   3 +
 4 files changed, 327 insertions(+)
 create mode 100644 drivers/baseband/acc100/rte_acc100_cfg.h
  

Comments

Tom Rix Sept. 29, 2020, 9:13 p.m. UTC | #1
On 9/28/20 5:29 PM, Nicolas Chautru wrote:
> Add in the "info_get" function to the driver, to allow us to query the
> device.
> No processing capability are available yet.
> Linking bbdev-test to support the PMD with null capability.
>
> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> Acked-by: Liu Tianjiao <Tianjiao.liu@intel.com>
> ---
>  app/test-bbdev/meson.build               |   3 +
>  drivers/baseband/acc100/rte_acc100_cfg.h |  96 +++++++++++++
>  drivers/baseband/acc100/rte_acc100_pmd.c | 225 +++++++++++++++++++++++++++++++
>  drivers/baseband/acc100/rte_acc100_pmd.h |   3 +
>  4 files changed, 327 insertions(+)
>  create mode 100644 drivers/baseband/acc100/rte_acc100_cfg.h
>
> diff --git a/app/test-bbdev/meson.build b/app/test-bbdev/meson.build
> index 18ab6a8..fbd8ae3 100644
> --- a/app/test-bbdev/meson.build
> +++ b/app/test-bbdev/meson.build
> @@ -12,3 +12,6 @@ endif
>  if dpdk_conf.has('RTE_LIBRTE_PMD_BBDEV_FPGA_5GNR_FEC')
>  	deps += ['pmd_bbdev_fpga_5gnr_fec']
>  endif
> +if dpdk_conf.has('RTE_LIBRTE_PMD_BBDEV_ACC100')
> +	deps += ['pmd_bbdev_acc100']
> +endif
> \ No newline at end of file
> diff --git a/drivers/baseband/acc100/rte_acc100_cfg.h b/drivers/baseband/acc100/rte_acc100_cfg.h
> new file mode 100644
> index 0000000..73bbe36
> --- /dev/null
> +++ b/drivers/baseband/acc100/rte_acc100_cfg.h
> @@ -0,0 +1,96 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2020 Intel Corporation
> + */
> +
> +#ifndef _RTE_ACC100_CFG_H_
> +#define _RTE_ACC100_CFG_H_
> +
> +/**
> + * @file rte_acc100_cfg.h
> + *
> + * Functions for configuring ACC100 HW, exposed directly to applications.
> + * Configuration related to encoding/decoding is done through the
> + * librte_bbdev library.
> + *
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice
When will this experimental tag be removed ?
> + */
> +
> +#include <stdint.h>
> +#include <stdbool.h>
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +/**< Number of Virtual Functions ACC100 supports */
> +#define RTE_ACC100_NUM_VFS 16
This is already defined with ACC100_NUM_VFS
> +
> +/**
> + * Definition of Queue Topology for ACC100 Configuration
> + * Some level of details is abstracted out to expose a clean interface
> + * given that comprehensive flexibility is not required
> + */
> +struct rte_q_topology_t {
> +	/** Number of QGroups in incremental order of priority */
> +	uint16_t num_qgroups;
> +	/**
> +	 * All QGroups have the same number of AQs here.
> +	 * Note : Could be made a 16-array if more flexibility is really
> +	 * required
> +	 */
> +	uint16_t num_aqs_per_groups;
> +	/**
> +	 * Depth of the AQs is the same of all QGroups here. Log2 Enum : 2^N
> +	 * Note : Could be made a 16-array if more flexibility is really
> +	 * required
> +	 */
> +	uint16_t aq_depth_log2;
> +	/**
> +	 * Index of the first Queue Group Index - assuming contiguity
> +	 * Initialized as -1
> +	 */
> +	int8_t first_qgroup_index;
> +};
> +
> +/**
> + * Definition of Arbitration related parameters for ACC100 Configuration
> + */
> +struct rte_arbitration_t {
> +	/** Default Weight for VF Fairness Arbitration */
> +	uint16_t round_robin_weight;
> +	uint32_t gbr_threshold1; /**< Guaranteed Bitrate Threshold 1 */
> +	uint32_t gbr_threshold2; /**< Guaranteed Bitrate Threshold 2 */
> +};
> +
> +/**
> + * Structure to pass ACC100 configuration.
> + * Note: all VF Bundles will have the same configuration.
> + */
> +struct acc100_conf {
> +	bool pf_mode_en; /**< 1 if PF is used for dataplane, 0 for VFs */
> +	/** 1 if input '1' bit is represented by a positive LLR value, 0 if '1'
> +	 * bit is represented by a negative value.
> +	 */
> +	bool input_pos_llr_1_bit;
> +	/** 1 if output '1' bit is represented by a positive value, 0 if '1'
> +	 * bit is represented by a negative value.
> +	 */
> +	bool output_pos_llr_1_bit;
> +	uint16_t num_vf_bundles; /**< Number of VF bundles to setup */
> +	/** Queue topology for each operation type */
> +	struct rte_q_topology_t q_ul_4g;
> +	struct rte_q_topology_t q_dl_4g;
> +	struct rte_q_topology_t q_ul_5g;
> +	struct rte_q_topology_t q_dl_5g;
> +	/** Arbitration configuration for each operation type */
> +	struct rte_arbitration_t arb_ul_4g[RTE_ACC100_NUM_VFS];
> +	struct rte_arbitration_t arb_dl_4g[RTE_ACC100_NUM_VFS];
> +	struct rte_arbitration_t arb_ul_5g[RTE_ACC100_NUM_VFS];
> +	struct rte_arbitration_t arb_dl_5g[RTE_ACC100_NUM_VFS];
> +};
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* _RTE_ACC100_CFG_H_ */
> diff --git a/drivers/baseband/acc100/rte_acc100_pmd.c b/drivers/baseband/acc100/rte_acc100_pmd.c
> index 1b4cd13..7807a30 100644
> --- a/drivers/baseband/acc100/rte_acc100_pmd.c
> +++ b/drivers/baseband/acc100/rte_acc100_pmd.c
> @@ -26,6 +26,184 @@
>  RTE_LOG_REGISTER(acc100_logtype, pmd.bb.acc100, NOTICE);
>  #endif
>  
> +/* Read a register of a ACC100 device */
> +static inline uint32_t
> +acc100_reg_read(struct acc100_device *d, uint32_t offset)
> +{
> +
> +	void *reg_addr = RTE_PTR_ADD(d->mmio_base, offset);
> +	uint32_t ret = *((volatile uint32_t *)(reg_addr));
> +	return rte_le_to_cpu_32(ret);
> +}
> +
> +/* Calculate the offset of the enqueue register */
> +static inline uint32_t
> +queue_offset(bool pf_device, uint8_t vf_id, uint8_t qgrp_id, uint16_t aq_id)
> +{
> +	if (pf_device)
> +		return ((vf_id << 12) + (qgrp_id << 7) + (aq_id << 3) +
> +				HWPfQmgrIngressAq);
> +	else
> +		return ((qgrp_id << 7) + (aq_id << 3) +
> +				HWVfQmgrIngressAq);
Could you add *QmrIngressAq to the acc100_registry_addr and skip the if (pf_device) check ?
> +}
> +
> +enum {UL_4G = 0, UL_5G, DL_4G, DL_5G, NUM_ACC};
> +
> +/* Return the queue topology for a Queue Group Index */
> +static inline void
> +qtopFromAcc(struct rte_q_topology_t **qtop, int acc_enum,
> +		struct acc100_conf *acc100_conf)
> +{
> +	struct rte_q_topology_t *p_qtop;
> +	p_qtop = NULL;
> +	switch (acc_enum) {
> +	case UL_4G:
> +		p_qtop = &(acc100_conf->q_ul_4g);
> +		break;
> +	case UL_5G:
> +		p_qtop = &(acc100_conf->q_ul_5g);
> +		break;
> +	case DL_4G:
> +		p_qtop = &(acc100_conf->q_dl_4g);
> +		break;
> +	case DL_5G:
> +		p_qtop = &(acc100_conf->q_dl_5g);
> +		break;
> +	default:
> +		/* NOTREACHED */
> +		rte_bbdev_log(ERR, "Unexpected error evaluating qtopFromAcc");
Use in fetch_acc100_config does not check for NULL.
> +		break;
> +	}
> +	*qtop = p_qtop;
> +}
> +
> +static void
> +initQTop(struct acc100_conf *acc100_conf)
> +{
> +	acc100_conf->q_ul_4g.num_aqs_per_groups = 0;
> +	acc100_conf->q_ul_4g.num_qgroups = 0;
> +	acc100_conf->q_ul_4g.first_qgroup_index = -1;
> +	acc100_conf->q_ul_5g.num_aqs_per_groups = 0;
> +	acc100_conf->q_ul_5g.num_qgroups = 0;
> +	acc100_conf->q_ul_5g.first_qgroup_index = -1;
> +	acc100_conf->q_dl_4g.num_aqs_per_groups = 0;
> +	acc100_conf->q_dl_4g.num_qgroups = 0;
> +	acc100_conf->q_dl_4g.first_qgroup_index = -1;
> +	acc100_conf->q_dl_5g.num_aqs_per_groups = 0;
> +	acc100_conf->q_dl_5g.num_qgroups = 0;
> +	acc100_conf->q_dl_5g.first_qgroup_index = -1;
> +}
> +
> +static inline void
> +updateQtop(uint8_t acc, uint8_t qg, struct acc100_conf *acc100_conf,
> +		struct acc100_device *d) {
> +	uint32_t reg;
> +	struct rte_q_topology_t *q_top = NULL;
> +	qtopFromAcc(&q_top, acc, acc100_conf);
> +	if (unlikely(q_top == NULL))
> +		return;
as above, this error is not handled by caller fetch_acc100_config
> +	uint16_t aq;
> +	q_top->num_qgroups++;
> +	if (q_top->first_qgroup_index == -1) {
> +		q_top->first_qgroup_index = qg;
> +		/* Can be optimized to assume all are enabled by default */
> +		reg = acc100_reg_read(d, queue_offset(d->pf_device,
> +				0, qg, ACC100_NUM_AQS - 1));
> +		if (reg & QUEUE_ENABLE) {
> +			q_top->num_aqs_per_groups = ACC100_NUM_AQS;
> +			return;
> +		}
> +		q_top->num_aqs_per_groups = 0;
> +		for (aq = 0; aq < ACC100_NUM_AQS; aq++) {
> +			reg = acc100_reg_read(d, queue_offset(d->pf_device,
> +					0, qg, aq));
> +			if (reg & QUEUE_ENABLE)
> +				q_top->num_aqs_per_groups++;
> +		}
> +	}
> +}
> +
> +/* Fetch configuration enabled for the PF/VF using MMIO Read (slow) */
> +static inline void
> +fetch_acc100_config(struct rte_bbdev *dev)
> +{
> +	struct acc100_device *d = dev->data->dev_private;
> +	struct acc100_conf *acc100_conf = &d->acc100_conf;
> +	const struct acc100_registry_addr *reg_addr;
> +	uint8_t acc, qg;
> +	uint32_t reg, reg_aq, reg_len0, reg_len1;
> +	uint32_t reg_mode;
> +
> +	/* No need to retrieve the configuration is already done */
> +	if (d->configured)
> +		return;
Warn ?
> +
> +	/* Choose correct registry addresses for the device type */
> +	if (d->pf_device)
> +		reg_addr = &pf_reg_addr;
> +	else
> +		reg_addr = &vf_reg_addr;
> +
> +	d->ddr_size = (1 + acc100_reg_read(d, reg_addr->ddr_range)) << 10;
> +
> +	/* Single VF Bundle by VF */
> +	acc100_conf->num_vf_bundles = 1;
> +	initQTop(acc100_conf);
> +
> +	struct rte_q_topology_t *q_top = NULL;
> +	int qman_func_id[5] = {0, 2, 1, 3, 4};
Do these magic numbers need #defines ?
> +	reg = acc100_reg_read(d, reg_addr->qman_group_func);
> +	for (qg = 0; qg < ACC100_NUM_QGRPS_PER_WORD; qg++) {
> +		reg_aq = acc100_reg_read(d,
> +				queue_offset(d->pf_device, 0, qg, 0));
> +		if (reg_aq & QUEUE_ENABLE) {
> +			acc = qman_func_id[(reg >> (qg * 4)) & 0x7];
0x7 and [5], this could overflow.
> +			updateQtop(acc, qg, acc100_conf, d);
> +		}
> +	}
> +
> +	/* Check the depth of the AQs*/
> +	reg_len0 = acc100_reg_read(d, reg_addr->depth_log0_offset);
> +	reg_len1 = acc100_reg_read(d, reg_addr->depth_log1_offset);
> +	for (acc = 0; acc < NUM_ACC; acc++) {
> +		qtopFromAcc(&q_top, acc, acc100_conf);
> +		if (q_top->first_qgroup_index < ACC100_NUM_QGRPS_PER_WORD)
> +			q_top->aq_depth_log2 = (reg_len0 >>
> +					(q_top->first_qgroup_index * 4))
> +					& 0xF;
> +		else
> +			q_top->aq_depth_log2 = (reg_len1 >>
> +					((q_top->first_qgroup_index -
> +					ACC100_NUM_QGRPS_PER_WORD) * 4))
> +					& 0xF;
> +	}
> +
> +	/* Read PF mode */
> +	if (d->pf_device) {
> +		reg_mode = acc100_reg_read(d, HWPfHiPfMode);
> +		acc100_conf->pf_mode_en = (reg_mode == 2) ? 1 : 0;

2 is a magic number, consider a #define

Tom

> +	}
> +
> +	rte_bbdev_log_debug(
> +			"%s Config LLR SIGN IN/OUT %s %s QG %u %u %u %u AQ %u %u %u %u Len %u %u %u %u\n",
> +			(d->pf_device) ? "PF" : "VF",
> +			(acc100_conf->input_pos_llr_1_bit) ? "POS" : "NEG",
> +			(acc100_conf->output_pos_llr_1_bit) ? "POS" : "NEG",
> +			acc100_conf->q_ul_4g.num_qgroups,
> +			acc100_conf->q_dl_4g.num_qgroups,
> +			acc100_conf->q_ul_5g.num_qgroups,
> +			acc100_conf->q_dl_5g.num_qgroups,
> +			acc100_conf->q_ul_4g.num_aqs_per_groups,
> +			acc100_conf->q_dl_4g.num_aqs_per_groups,
> +			acc100_conf->q_ul_5g.num_aqs_per_groups,
> +			acc100_conf->q_dl_5g.num_aqs_per_groups,
> +			acc100_conf->q_ul_4g.aq_depth_log2,
> +			acc100_conf->q_dl_4g.aq_depth_log2,
> +			acc100_conf->q_ul_5g.aq_depth_log2,
> +			acc100_conf->q_dl_5g.aq_depth_log2);
> +}
> +
>  /* Free 64MB memory used for software rings */
>  static int
>  acc100_dev_close(struct rte_bbdev *dev  __rte_unused)
> @@ -33,8 +211,55 @@
>  	return 0;
>  }
>  
> +/* Get ACC100 device info */
> +static void
> +acc100_dev_info_get(struct rte_bbdev *dev,
> +		struct rte_bbdev_driver_info *dev_info)
> +{
> +	struct acc100_device *d = dev->data->dev_private;
> +
> +	static const struct rte_bbdev_op_cap bbdev_capabilities[] = {
> +		RTE_BBDEV_END_OF_CAPABILITIES_LIST()
> +	};
> +
> +	static struct rte_bbdev_queue_conf default_queue_conf;
> +	default_queue_conf.socket = dev->data->socket_id;
> +	default_queue_conf.queue_size = MAX_QUEUE_DEPTH;
> +
> +	dev_info->driver_name = dev->device->driver->name;
> +
> +	/* Read and save the populated config from ACC100 registers */
> +	fetch_acc100_config(dev);
> +
> +	/* This isn't ideal because it reports the maximum number of queues but
> +	 * does not provide info on how many can be uplink/downlink or different
> +	 * priorities
> +	 */
> +	dev_info->max_num_queues =
> +			d->acc100_conf.q_dl_5g.num_aqs_per_groups *
> +			d->acc100_conf.q_dl_5g.num_qgroups +
> +			d->acc100_conf.q_ul_5g.num_aqs_per_groups *
> +			d->acc100_conf.q_ul_5g.num_qgroups +
> +			d->acc100_conf.q_dl_4g.num_aqs_per_groups *
> +			d->acc100_conf.q_dl_4g.num_qgroups +
> +			d->acc100_conf.q_ul_4g.num_aqs_per_groups *
> +			d->acc100_conf.q_ul_4g.num_qgroups;
> +	dev_info->queue_size_lim = MAX_QUEUE_DEPTH;
> +	dev_info->hardware_accelerated = true;
> +	dev_info->max_dl_queue_priority =
> +			d->acc100_conf.q_dl_4g.num_qgroups - 1;
> +	dev_info->max_ul_queue_priority =
> +			d->acc100_conf.q_ul_4g.num_qgroups - 1;
> +	dev_info->default_queue_conf = default_queue_conf;
> +	dev_info->cpu_flag_reqs = NULL;
> +	dev_info->min_alignment = 64;
> +	dev_info->capabilities = bbdev_capabilities;
> +	dev_info->harq_buffer_size = d->ddr_size;
> +}
> +
>  static const struct rte_bbdev_ops acc100_bbdev_ops = {
>  	.close = acc100_dev_close,
> +	.info_get = acc100_dev_info_get,
>  };
>  
>  /* ACC100 PCI PF address map */
> diff --git a/drivers/baseband/acc100/rte_acc100_pmd.h b/drivers/baseband/acc100/rte_acc100_pmd.h
> index cd77570..662e2c8 100644
> --- a/drivers/baseband/acc100/rte_acc100_pmd.h
> +++ b/drivers/baseband/acc100/rte_acc100_pmd.h
> @@ -7,6 +7,7 @@
>  
>  #include "acc100_pf_enum.h"
>  #include "acc100_vf_enum.h"
> +#include "rte_acc100_cfg.h"
>  
>  /* Helper macro for logging */
>  #define rte_bbdev_log(level, fmt, ...) \
> @@ -520,6 +521,8 @@ struct acc100_registry_addr {
>  /* Private data structure for each ACC100 device */
>  struct acc100_device {
>  	void *mmio_base;  /**< Base address of MMIO registers (BAR0) */
> +	uint32_t ddr_size; /* Size in kB */
> +	struct acc100_conf acc100_conf; /* ACC100 Initial configuration */
>  	bool pf_device; /**< True if this is a PF ACC100 device */
>  	bool configured; /**< True if this ACC100 device is configured */
>  };
  
Chautru, Nicolas Sept. 30, 2020, 12:25 a.m. UTC | #2
Hi Tom, 

> From: Tom Rix <trix@redhat.com>
> On 9/28/20 5:29 PM, Nicolas Chautru wrote:
> > Add in the "info_get" function to the driver, to allow us to query the
> > device.
> > No processing capability are available yet.
> > Linking bbdev-test to support the PMD with null capability.
> >
> > Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> > Acked-by: Liu Tianjiao <Tianjiao.liu@intel.com>
> > ---
> >  app/test-bbdev/meson.build               |   3 +
> >  drivers/baseband/acc100/rte_acc100_cfg.h |  96 +++++++++++++
> > drivers/baseband/acc100/rte_acc100_pmd.c | 225
> +++++++++++++++++++++++++++++++
> >  drivers/baseband/acc100/rte_acc100_pmd.h |   3 +
> >  4 files changed, 327 insertions(+)
> >  create mode 100644 drivers/baseband/acc100/rte_acc100_cfg.h
> >
> > diff --git a/app/test-bbdev/meson.build b/app/test-bbdev/meson.build
> > index 18ab6a8..fbd8ae3 100644
> > --- a/app/test-bbdev/meson.build
> > +++ b/app/test-bbdev/meson.build
> > @@ -12,3 +12,6 @@ endif
> >  if dpdk_conf.has('RTE_LIBRTE_PMD_BBDEV_FPGA_5GNR_FEC')
> >  	deps += ['pmd_bbdev_fpga_5gnr_fec']
> >  endif
> > +if dpdk_conf.has('RTE_LIBRTE_PMD_BBDEV_ACC100')
> > +	deps += ['pmd_bbdev_acc100']
> > +endif
> > \ No newline at end of file
> > diff --git a/drivers/baseband/acc100/rte_acc100_cfg.h
> > b/drivers/baseband/acc100/rte_acc100_cfg.h
> > new file mode 100644
> > index 0000000..73bbe36
> > --- /dev/null
> > +++ b/drivers/baseband/acc100/rte_acc100_cfg.h
> > @@ -0,0 +1,96 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2020 Intel Corporation  */
> > +
> > +#ifndef _RTE_ACC100_CFG_H_
> > +#define _RTE_ACC100_CFG_H_
> > +
> > +/**
> > + * @file rte_acc100_cfg.h
> > + *
> > + * Functions for configuring ACC100 HW, exposed directly to applications.
> > + * Configuration related to encoding/decoding is done through the
> > + * librte_bbdev library.
> > + *
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> When will this experimental tag be removed ?

I have pushed a patch to remove it. But the feedback from some of the community was to wait a bit more until next year.

> > + */
> > +
> > +#include <stdint.h>
> > +#include <stdbool.h>
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +/**< Number of Virtual Functions ACC100 supports */ #define
> > +RTE_ACC100_NUM_VFS 16
> This is already defined with ACC100_NUM_VFS

Thanks. 

> > +
> > +/**
> > + * Definition of Queue Topology for ACC100 Configuration
> > + * Some level of details is abstracted out to expose a clean
> > +interface
> > + * given that comprehensive flexibility is not required  */ struct
> > +rte_q_topology_t {
> > +	/** Number of QGroups in incremental order of priority */
> > +	uint16_t num_qgroups;
> > +	/**
> > +	 * All QGroups have the same number of AQs here.
> > +	 * Note : Could be made a 16-array if more flexibility is really
> > +	 * required
> > +	 */
> > +	uint16_t num_aqs_per_groups;
> > +	/**
> > +	 * Depth of the AQs is the same of all QGroups here. Log2 Enum : 2^N
> > +	 * Note : Could be made a 16-array if more flexibility is really
> > +	 * required
> > +	 */
> > +	uint16_t aq_depth_log2;
> > +	/**
> > +	 * Index of the first Queue Group Index - assuming contiguity
> > +	 * Initialized as -1
> > +	 */
> > +	int8_t first_qgroup_index;
> > +};
> > +
> > +/**
> > + * Definition of Arbitration related parameters for ACC100
> > +Configuration  */ struct rte_arbitration_t {
> > +	/** Default Weight for VF Fairness Arbitration */
> > +	uint16_t round_robin_weight;
> > +	uint32_t gbr_threshold1; /**< Guaranteed Bitrate Threshold 1 */
> > +	uint32_t gbr_threshold2; /**< Guaranteed Bitrate Threshold 2 */ };
> > +
> > +/**
> > + * Structure to pass ACC100 configuration.
> > + * Note: all VF Bundles will have the same configuration.
> > + */
> > +struct acc100_conf {
> > +	bool pf_mode_en; /**< 1 if PF is used for dataplane, 0 for VFs */
> > +	/** 1 if input '1' bit is represented by a positive LLR value, 0 if '1'
> > +	 * bit is represented by a negative value.
> > +	 */
> > +	bool input_pos_llr_1_bit;
> > +	/** 1 if output '1' bit is represented by a positive value, 0 if '1'
> > +	 * bit is represented by a negative value.
> > +	 */
> > +	bool output_pos_llr_1_bit;
> > +	uint16_t num_vf_bundles; /**< Number of VF bundles to setup */
> > +	/** Queue topology for each operation type */
> > +	struct rte_q_topology_t q_ul_4g;
> > +	struct rte_q_topology_t q_dl_4g;
> > +	struct rte_q_topology_t q_ul_5g;
> > +	struct rte_q_topology_t q_dl_5g;
> > +	/** Arbitration configuration for each operation type */
> > +	struct rte_arbitration_t arb_ul_4g[RTE_ACC100_NUM_VFS];
> > +	struct rte_arbitration_t arb_dl_4g[RTE_ACC100_NUM_VFS];
> > +	struct rte_arbitration_t arb_ul_5g[RTE_ACC100_NUM_VFS];
> > +	struct rte_arbitration_t arb_dl_5g[RTE_ACC100_NUM_VFS]; };
> > +
> > +#ifdef __cplusplus
> > +}
> > +#endif
> > +
> > +#endif /* _RTE_ACC100_CFG_H_ */
> > diff --git a/drivers/baseband/acc100/rte_acc100_pmd.c
> > b/drivers/baseband/acc100/rte_acc100_pmd.c
> > index 1b4cd13..7807a30 100644
> > --- a/drivers/baseband/acc100/rte_acc100_pmd.c
> > +++ b/drivers/baseband/acc100/rte_acc100_pmd.c
> > @@ -26,6 +26,184 @@
> >  RTE_LOG_REGISTER(acc100_logtype, pmd.bb.acc100, NOTICE);  #endif
> >
> > +/* Read a register of a ACC100 device */ static inline uint32_t
> > +acc100_reg_read(struct acc100_device *d, uint32_t offset) {
> > +
> > +	void *reg_addr = RTE_PTR_ADD(d->mmio_base, offset);
> > +	uint32_t ret = *((volatile uint32_t *)(reg_addr));
> > +	return rte_le_to_cpu_32(ret);
> > +}
> > +
> > +/* Calculate the offset of the enqueue register */ static inline
> > +uint32_t queue_offset(bool pf_device, uint8_t vf_id, uint8_t qgrp_id,
> > +uint16_t aq_id) {
> > +	if (pf_device)
> > +		return ((vf_id << 12) + (qgrp_id << 7) + (aq_id << 3) +
> > +				HWPfQmgrIngressAq);
> > +	else
> > +		return ((qgrp_id << 7) + (aq_id << 3) +
> > +				HWVfQmgrIngressAq);
> Could you add *QmrIngressAq to the acc100_registry_addr and skip the if
> (pf_device) check ?

I am not convinced. That acc100_registry_addr is not kept with the driver, you
still need to check pf_device flag to now what registers are to be used.


> > +}
> > +
> > +enum {UL_4G = 0, UL_5G, DL_4G, DL_5G, NUM_ACC};
> > +
> > +/* Return the queue topology for a Queue Group Index */ static inline
> > +void qtopFromAcc(struct rte_q_topology_t **qtop, int acc_enum,
> > +		struct acc100_conf *acc100_conf)
> > +{
> > +	struct rte_q_topology_t *p_qtop;
> > +	p_qtop = NULL;
> > +	switch (acc_enum) {
> > +	case UL_4G:
> > +		p_qtop = &(acc100_conf->q_ul_4g);
> > +		break;
> > +	case UL_5G:
> > +		p_qtop = &(acc100_conf->q_ul_5g);
> > +		break;
> > +	case DL_4G:
> > +		p_qtop = &(acc100_conf->q_dl_4g);
> > +		break;
> > +	case DL_5G:
> > +		p_qtop = &(acc100_conf->q_dl_5g);
> > +		break;
> > +	default:
> > +		/* NOTREACHED */
> > +		rte_bbdev_log(ERR, "Unexpected error evaluating
> qtopFromAcc");
> Use in fetch_acc100_config does not check for NULL.

Yes because it can't be null. This function is called explictly for supported values. 

> > +		break;
> > +	}
> > +	*qtop = p_qtop;
> > +}
> > +
> > +static void
> > +initQTop(struct acc100_conf *acc100_conf) {
> > +	acc100_conf->q_ul_4g.num_aqs_per_groups = 0;
> > +	acc100_conf->q_ul_4g.num_qgroups = 0;
> > +	acc100_conf->q_ul_4g.first_qgroup_index = -1;
> > +	acc100_conf->q_ul_5g.num_aqs_per_groups = 0;
> > +	acc100_conf->q_ul_5g.num_qgroups = 0;
> > +	acc100_conf->q_ul_5g.first_qgroup_index = -1;
> > +	acc100_conf->q_dl_4g.num_aqs_per_groups = 0;
> > +	acc100_conf->q_dl_4g.num_qgroups = 0;
> > +	acc100_conf->q_dl_4g.first_qgroup_index = -1;
> > +	acc100_conf->q_dl_5g.num_aqs_per_groups = 0;
> > +	acc100_conf->q_dl_5g.num_qgroups = 0;
> > +	acc100_conf->q_dl_5g.first_qgroup_index = -1; }
> > +
> > +static inline void
> > +updateQtop(uint8_t acc, uint8_t qg, struct acc100_conf *acc100_conf,
> > +		struct acc100_device *d) {
> > +	uint32_t reg;
> > +	struct rte_q_topology_t *q_top = NULL;
> > +	qtopFromAcc(&q_top, acc, acc100_conf);
> > +	if (unlikely(q_top == NULL))
> > +		return;
> as above, this error is not handled by caller fetch_acc100_config

It cannot really fail for fetch_acc100_config. If you insist I can add. 

> > +	uint16_t aq;
> > +	q_top->num_qgroups++;
> > +	if (q_top->first_qgroup_index == -1) {
> > +		q_top->first_qgroup_index = qg;
> > +		/* Can be optimized to assume all are enabled by default */
> > +		reg = acc100_reg_read(d, queue_offset(d->pf_device,
> > +				0, qg, ACC100_NUM_AQS - 1));
> > +		if (reg & QUEUE_ENABLE) {
> > +			q_top->num_aqs_per_groups = ACC100_NUM_AQS;
> > +			return;
> > +		}
> > +		q_top->num_aqs_per_groups = 0;
> > +		for (aq = 0; aq < ACC100_NUM_AQS; aq++) {
> > +			reg = acc100_reg_read(d, queue_offset(d-
> >pf_device,
> > +					0, qg, aq));
> > +			if (reg & QUEUE_ENABLE)
> > +				q_top->num_aqs_per_groups++;
> > +		}
> > +	}
> > +}
> > +
> > +/* Fetch configuration enabled for the PF/VF using MMIO Read (slow)
> > +*/ static inline void fetch_acc100_config(struct rte_bbdev *dev) {
> > +	struct acc100_device *d = dev->data->dev_private;
> > +	struct acc100_conf *acc100_conf = &d->acc100_conf;
> > +	const struct acc100_registry_addr *reg_addr;
> > +	uint8_t acc, qg;
> > +	uint32_t reg, reg_aq, reg_len0, reg_len1;
> > +	uint32_t reg_mode;
> > +
> > +	/* No need to retrieve the configuration is already done */
> > +	if (d->configured)
> > +		return;
> Warn ?

No this can genuinely happen on a regular basis, just no need to fetch it all again. 

> > +
> > +	/* Choose correct registry addresses for the device type */
> > +	if (d->pf_device)
> > +		reg_addr = &pf_reg_addr;
> > +	else
> > +		reg_addr = &vf_reg_addr;
> > +
> > +	d->ddr_size = (1 + acc100_reg_read(d, reg_addr->ddr_range)) << 10;
> > +
> > +	/* Single VF Bundle by VF */
> > +	acc100_conf->num_vf_bundles = 1;
> > +	initQTop(acc100_conf);
> > +
> > +	struct rte_q_topology_t *q_top = NULL;
> > +	int qman_func_id[5] = {0, 2, 1, 3, 4};
> Do these magic numbers need #defines ?

ok. 

> > +	reg = acc100_reg_read(d, reg_addr->qman_group_func);
> > +	for (qg = 0; qg < ACC100_NUM_QGRPS_PER_WORD; qg++) {
> > +		reg_aq = acc100_reg_read(d,
> > +				queue_offset(d->pf_device, 0, qg, 0));
> > +		if (reg_aq & QUEUE_ENABLE) {
> > +			acc = qman_func_id[(reg >> (qg * 4)) & 0x7];
> 0x7 and [5], this could overflow.

ok thanks, I can add exception handling. Not clear to me right now why it did not trigger tool warning. 

> > +			updateQtop(acc, qg, acc100_conf, d);
> > +		}
> > +	}
> > +
> > +	/* Check the depth of the AQs*/
> > +	reg_len0 = acc100_reg_read(d, reg_addr->depth_log0_offset);
> > +	reg_len1 = acc100_reg_read(d, reg_addr->depth_log1_offset);
> > +	for (acc = 0; acc < NUM_ACC; acc++) {
> > +		qtopFromAcc(&q_top, acc, acc100_conf);
> > +		if (q_top->first_qgroup_index <
> ACC100_NUM_QGRPS_PER_WORD)
> > +			q_top->aq_depth_log2 = (reg_len0 >>
> > +					(q_top->first_qgroup_index * 4))
> > +					& 0xF;
> > +		else
> > +			q_top->aq_depth_log2 = (reg_len1 >>
> > +					((q_top->first_qgroup_index -
> > +					ACC100_NUM_QGRPS_PER_WORD) *
> 4))
> > +					& 0xF;
> > +	}
> > +
> > +	/* Read PF mode */
> > +	if (d->pf_device) {
> > +		reg_mode = acc100_reg_read(d, HWPfHiPfMode);
> > +		acc100_conf->pf_mode_en = (reg_mode == 2) ? 1 : 0;
> 
> 2 is a magic number, consider a #define
> 

ok

> Tom
> 

Thanks
Nic

> > +	}
> > +
> > +	rte_bbdev_log_debug(
> > +			"%s Config LLR SIGN IN/OUT %s %s QG %u %u %u %u
> AQ %u %u %u %u Len %u %u %u %u\n",
> > +			(d->pf_device) ? "PF" : "VF",
> > +			(acc100_conf->input_pos_llr_1_bit) ? "POS" : "NEG",
> > +			(acc100_conf->output_pos_llr_1_bit) ? "POS" :
> "NEG",
> > +			acc100_conf->q_ul_4g.num_qgroups,
> > +			acc100_conf->q_dl_4g.num_qgroups,
> > +			acc100_conf->q_ul_5g.num_qgroups,
> > +			acc100_conf->q_dl_5g.num_qgroups,
> > +			acc100_conf->q_ul_4g.num_aqs_per_groups,
> > +			acc100_conf->q_dl_4g.num_aqs_per_groups,
> > +			acc100_conf->q_ul_5g.num_aqs_per_groups,
> > +			acc100_conf->q_dl_5g.num_aqs_per_groups,
> > +			acc100_conf->q_ul_4g.aq_depth_log2,
> > +			acc100_conf->q_dl_4g.aq_depth_log2,
> > +			acc100_conf->q_ul_5g.aq_depth_log2,
> > +			acc100_conf->q_dl_5g.aq_depth_log2);
> > +}
> > +
> >  /* Free 64MB memory used for software rings */  static int
> > acc100_dev_close(struct rte_bbdev *dev  __rte_unused) @@ -33,8
> +211,55
> > @@
> >  	return 0;
> >  }
> >
> > +/* Get ACC100 device info */
> > +static void
> > +acc100_dev_info_get(struct rte_bbdev *dev,
> > +		struct rte_bbdev_driver_info *dev_info) {
> > +	struct acc100_device *d = dev->data->dev_private;
> > +
> > +	static const struct rte_bbdev_op_cap bbdev_capabilities[] = {
> > +		RTE_BBDEV_END_OF_CAPABILITIES_LIST()
> > +	};
> > +
> > +	static struct rte_bbdev_queue_conf default_queue_conf;
> > +	default_queue_conf.socket = dev->data->socket_id;
> > +	default_queue_conf.queue_size = MAX_QUEUE_DEPTH;
> > +
> > +	dev_info->driver_name = dev->device->driver->name;
> > +
> > +	/* Read and save the populated config from ACC100 registers */
> > +	fetch_acc100_config(dev);
> > +
> > +	/* This isn't ideal because it reports the maximum number of queues
> but
> > +	 * does not provide info on how many can be uplink/downlink or
> different
> > +	 * priorities
> > +	 */
> > +	dev_info->max_num_queues =
> > +			d->acc100_conf.q_dl_5g.num_aqs_per_groups *
> > +			d->acc100_conf.q_dl_5g.num_qgroups +
> > +			d->acc100_conf.q_ul_5g.num_aqs_per_groups *
> > +			d->acc100_conf.q_ul_5g.num_qgroups +
> > +			d->acc100_conf.q_dl_4g.num_aqs_per_groups *
> > +			d->acc100_conf.q_dl_4g.num_qgroups +
> > +			d->acc100_conf.q_ul_4g.num_aqs_per_groups *
> > +			d->acc100_conf.q_ul_4g.num_qgroups;
> > +	dev_info->queue_size_lim = MAX_QUEUE_DEPTH;
> > +	dev_info->hardware_accelerated = true;
> > +	dev_info->max_dl_queue_priority =
> > +			d->acc100_conf.q_dl_4g.num_qgroups - 1;
> > +	dev_info->max_ul_queue_priority =
> > +			d->acc100_conf.q_ul_4g.num_qgroups - 1;
> > +	dev_info->default_queue_conf = default_queue_conf;
> > +	dev_info->cpu_flag_reqs = NULL;
> > +	dev_info->min_alignment = 64;
> > +	dev_info->capabilities = bbdev_capabilities;
> > +	dev_info->harq_buffer_size = d->ddr_size; }
> > +
> >  static const struct rte_bbdev_ops acc100_bbdev_ops = {
> >  	.close = acc100_dev_close,
> > +	.info_get = acc100_dev_info_get,
> >  };
> >
> >  /* ACC100 PCI PF address map */
> > diff --git a/drivers/baseband/acc100/rte_acc100_pmd.h
> > b/drivers/baseband/acc100/rte_acc100_pmd.h
> > index cd77570..662e2c8 100644
> > --- a/drivers/baseband/acc100/rte_acc100_pmd.h
> > +++ b/drivers/baseband/acc100/rte_acc100_pmd.h
> > @@ -7,6 +7,7 @@
> >
> >  #include "acc100_pf_enum.h"
> >  #include "acc100_vf_enum.h"
> > +#include "rte_acc100_cfg.h"
> >
> >  /* Helper macro for logging */
> >  #define rte_bbdev_log(level, fmt, ...) \ @@ -520,6 +521,8 @@ struct
> > acc100_registry_addr {
> >  /* Private data structure for each ACC100 device */  struct
> > acc100_device {
> >  	void *mmio_base;  /**< Base address of MMIO registers (BAR0) */
> > +	uint32_t ddr_size; /* Size in kB */
> > +	struct acc100_conf acc100_conf; /* ACC100 Initial configuration */
> >  	bool pf_device; /**< True if this is a PF ACC100 device */
> >  	bool configured; /**< True if this ACC100 device is configured */
> > };
  
Tom Rix Sept. 30, 2020, 11:20 p.m. UTC | #3
On 9/29/20 5:25 PM, Chautru, Nicolas wrote:
> Hi Tom, 
>
>> From: Tom Rix <trix@redhat.com>
>> On 9/28/20 5:29 PM, Nicolas Chautru wrote:
>>> Add in the "info_get" function to the driver, to allow us to query the
>>> device.
>>> No processing capability are available yet.
>>> Linking bbdev-test to support the PMD with null capability.
>>>
>>> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
>>> Acked-by: Liu Tianjiao <Tianjiao.liu@intel.com>
>>> ---
>>>  app/test-bbdev/meson.build               |   3 +
>>>  drivers/baseband/acc100/rte_acc100_cfg.h |  96 +++++++++++++
>>> drivers/baseband/acc100/rte_acc100_pmd.c | 225
>> +++++++++++++++++++++++++++++++
>>>  drivers/baseband/acc100/rte_acc100_pmd.h |   3 +
>>>  4 files changed, 327 insertions(+)
>>>  create mode 100644 drivers/baseband/acc100/rte_acc100_cfg.h
>>>
>>> diff --git a/app/test-bbdev/meson.build b/app/test-bbdev/meson.build
>>> index 18ab6a8..fbd8ae3 100644
>>> --- a/app/test-bbdev/meson.build
>>> +++ b/app/test-bbdev/meson.build
>>> @@ -12,3 +12,6 @@ endif
>>>  if dpdk_conf.has('RTE_LIBRTE_PMD_BBDEV_FPGA_5GNR_FEC')
>>>  	deps += ['pmd_bbdev_fpga_5gnr_fec']
>>>  endif
>>> +if dpdk_conf.has('RTE_LIBRTE_PMD_BBDEV_ACC100')
>>> +	deps += ['pmd_bbdev_acc100']
>>> +endif
>>> \ No newline at end of file
>>> diff --git a/drivers/baseband/acc100/rte_acc100_cfg.h
>>> b/drivers/baseband/acc100/rte_acc100_cfg.h
>>> new file mode 100644
>>> index 0000000..73bbe36
>>> --- /dev/null
>>> +++ b/drivers/baseband/acc100/rte_acc100_cfg.h
>>> @@ -0,0 +1,96 @@
>>> +/* SPDX-License-Identifier: BSD-3-Clause
>>> + * Copyright(c) 2020 Intel Corporation  */
>>> +
>>> +#ifndef _RTE_ACC100_CFG_H_
>>> +#define _RTE_ACC100_CFG_H_
>>> +
>>> +/**
>>> + * @file rte_acc100_cfg.h
>>> + *
>>> + * Functions for configuring ACC100 HW, exposed directly to applications.
>>> + * Configuration related to encoding/decoding is done through the
>>> + * librte_bbdev library.
>>> + *
>>> + * @warning
>>> + * @b EXPERIMENTAL: this API may change without prior notice
>> When will this experimental tag be removed ?
> I have pushed a patch to remove it. But the feedback from some of the community was to wait a bit more until next year.
ok, i am late to the party. i'll chip in when this issue comes around again.
>
>>> + */
>>> +
>>> +#include <stdint.h>
>>> +#include <stdbool.h>
>>> +
>>> +#ifdef __cplusplus
>>> +extern "C" {
>>> +#endif
>>> +/**< Number of Virtual Functions ACC100 supports */ #define
>>> +RTE_ACC100_NUM_VFS 16
>> This is already defined with ACC100_NUM_VFS
> Thanks. 
>
>>> +
>>> +/**
>>> + * Definition of Queue Topology for ACC100 Configuration
>>> + * Some level of details is abstracted out to expose a clean
>>> +interface
>>> + * given that comprehensive flexibility is not required  */ struct
>>> +rte_q_topology_t {
>>> +	/** Number of QGroups in incremental order of priority */
>>> +	uint16_t num_qgroups;
>>> +	/**
>>> +	 * All QGroups have the same number of AQs here.
>>> +	 * Note : Could be made a 16-array if more flexibility is really
>>> +	 * required
>>> +	 */
>>> +	uint16_t num_aqs_per_groups;
>>> +	/**
>>> +	 * Depth of the AQs is the same of all QGroups here. Log2 Enum : 2^N
>>> +	 * Note : Could be made a 16-array if more flexibility is really
>>> +	 * required
>>> +	 */
>>> +	uint16_t aq_depth_log2;
>>> +	/**
>>> +	 * Index of the first Queue Group Index - assuming contiguity
>>> +	 * Initialized as -1
>>> +	 */
>>> +	int8_t first_qgroup_index;
>>> +};
>>> +
>>> +/**
>>> + * Definition of Arbitration related parameters for ACC100
>>> +Configuration  */ struct rte_arbitration_t {
>>> +	/** Default Weight for VF Fairness Arbitration */
>>> +	uint16_t round_robin_weight;
>>> +	uint32_t gbr_threshold1; /**< Guaranteed Bitrate Threshold 1 */
>>> +	uint32_t gbr_threshold2; /**< Guaranteed Bitrate Threshold 2 */ };
>>> +
>>> +/**
>>> + * Structure to pass ACC100 configuration.
>>> + * Note: all VF Bundles will have the same configuration.
>>> + */
>>> +struct acc100_conf {
>>> +	bool pf_mode_en; /**< 1 if PF is used for dataplane, 0 for VFs */
>>> +	/** 1 if input '1' bit is represented by a positive LLR value, 0 if '1'
>>> +	 * bit is represented by a negative value.
>>> +	 */
>>> +	bool input_pos_llr_1_bit;
>>> +	/** 1 if output '1' bit is represented by a positive value, 0 if '1'
>>> +	 * bit is represented by a negative value.
>>> +	 */
>>> +	bool output_pos_llr_1_bit;
>>> +	uint16_t num_vf_bundles; /**< Number of VF bundles to setup */
>>> +	/** Queue topology for each operation type */
>>> +	struct rte_q_topology_t q_ul_4g;
>>> +	struct rte_q_topology_t q_dl_4g;
>>> +	struct rte_q_topology_t q_ul_5g;
>>> +	struct rte_q_topology_t q_dl_5g;
>>> +	/** Arbitration configuration for each operation type */
>>> +	struct rte_arbitration_t arb_ul_4g[RTE_ACC100_NUM_VFS];
>>> +	struct rte_arbitration_t arb_dl_4g[RTE_ACC100_NUM_VFS];
>>> +	struct rte_arbitration_t arb_ul_5g[RTE_ACC100_NUM_VFS];
>>> +	struct rte_arbitration_t arb_dl_5g[RTE_ACC100_NUM_VFS]; };
>>> +
>>> +#ifdef __cplusplus
>>> +}
>>> +#endif
>>> +
>>> +#endif /* _RTE_ACC100_CFG_H_ */
>>> diff --git a/drivers/baseband/acc100/rte_acc100_pmd.c
>>> b/drivers/baseband/acc100/rte_acc100_pmd.c
>>> index 1b4cd13..7807a30 100644
>>> --- a/drivers/baseband/acc100/rte_acc100_pmd.c
>>> +++ b/drivers/baseband/acc100/rte_acc100_pmd.c
>>> @@ -26,6 +26,184 @@
>>>  RTE_LOG_REGISTER(acc100_logtype, pmd.bb.acc100, NOTICE);  #endif
>>>
>>> +/* Read a register of a ACC100 device */ static inline uint32_t
>>> +acc100_reg_read(struct acc100_device *d, uint32_t offset) {
>>> +
>>> +	void *reg_addr = RTE_PTR_ADD(d->mmio_base, offset);
>>> +	uint32_t ret = *((volatile uint32_t *)(reg_addr));
>>> +	return rte_le_to_cpu_32(ret);
>>> +}
>>> +
>>> +/* Calculate the offset of the enqueue register */ static inline
>>> +uint32_t queue_offset(bool pf_device, uint8_t vf_id, uint8_t qgrp_id,
>>> +uint16_t aq_id) {
>>> +	if (pf_device)
>>> +		return ((vf_id << 12) + (qgrp_id << 7) + (aq_id << 3) +
>>> +				HWPfQmgrIngressAq);
>>> +	else
>>> +		return ((qgrp_id << 7) + (aq_id << 3) +
>>> +				HWVfQmgrIngressAq);
>> Could you add *QmrIngressAq to the acc100_registry_addr and skip the if
>> (pf_device) check ?
> I am not convinced. That acc100_registry_addr is not kept with the driver, you
> still need to check pf_device flag to now what registers are to be used.

ok

The values of pf and vf regs are tantalizingly close

my first inclination is to abstract all of them.

>
>
>>> +}
>>> +
>>> +enum {UL_4G = 0, UL_5G, DL_4G, DL_5G, NUM_ACC};
>>> +
>>> +/* Return the queue topology for a Queue Group Index */ static inline
>>> +void qtopFromAcc(struct rte_q_topology_t **qtop, int acc_enum,
>>> +		struct acc100_conf *acc100_conf)
>>> +{
>>> +	struct rte_q_topology_t *p_qtop;
>>> +	p_qtop = NULL;
>>> +	switch (acc_enum) {
>>> +	case UL_4G:
>>> +		p_qtop = &(acc100_conf->q_ul_4g);
>>> +		break;
>>> +	case UL_5G:
>>> +		p_qtop = &(acc100_conf->q_ul_5g);
>>> +		break;
>>> +	case DL_4G:
>>> +		p_qtop = &(acc100_conf->q_dl_4g);
>>> +		break;
>>> +	case DL_5G:
>>> +		p_qtop = &(acc100_conf->q_dl_5g);
>>> +		break;
>>> +	default:
>>> +		/* NOTREACHED */
>>> +		rte_bbdev_log(ERR, "Unexpected error evaluating
>> qtopFromAcc");
>> Use in fetch_acc100_config does not check for NULL.
> Yes because it can't be null. This function is called explictly for supported values. 
ok
>>> +		break;
>>> +	}
>>> +	*qtop = p_qtop;
>>> +}
>>> +
>>> +static void
>>> +initQTop(struct acc100_conf *acc100_conf) {
>>> +	acc100_conf->q_ul_4g.num_aqs_per_groups = 0;
>>> +	acc100_conf->q_ul_4g.num_qgroups = 0;
>>> +	acc100_conf->q_ul_4g.first_qgroup_index = -1;
>>> +	acc100_conf->q_ul_5g.num_aqs_per_groups = 0;
>>> +	acc100_conf->q_ul_5g.num_qgroups = 0;
>>> +	acc100_conf->q_ul_5g.first_qgroup_index = -1;
>>> +	acc100_conf->q_dl_4g.num_aqs_per_groups = 0;
>>> +	acc100_conf->q_dl_4g.num_qgroups = 0;
>>> +	acc100_conf->q_dl_4g.first_qgroup_index = -1;
>>> +	acc100_conf->q_dl_5g.num_aqs_per_groups = 0;
>>> +	acc100_conf->q_dl_5g.num_qgroups = 0;
>>> +	acc100_conf->q_dl_5g.first_qgroup_index = -1; }
>>> +
>>> +static inline void
>>> +updateQtop(uint8_t acc, uint8_t qg, struct acc100_conf *acc100_conf,
>>> +		struct acc100_device *d) {
>>> +	uint32_t reg;
>>> +	struct rte_q_topology_t *q_top = NULL;
>>> +	qtopFromAcc(&q_top, acc, acc100_conf);
>>> +	if (unlikely(q_top == NULL))
>>> +		return;
>> as above, this error is not handled by caller fetch_acc100_config
> It cannot really fail for fetch_acc100_config. If you insist I can add. 

An alternative to handing is to print a debug message.

could do this generally for the other checks i commented on.

>>> +	uint16_t aq;
>>> +	q_top->num_qgroups++;
>>> +	if (q_top->first_qgroup_index == -1) {
>>> +		q_top->first_qgroup_index = qg;
>>> +		/* Can be optimized to assume all are enabled by default */
>>> +		reg = acc100_reg_read(d, queue_offset(d->pf_device,
>>> +				0, qg, ACC100_NUM_AQS - 1));
>>> +		if (reg & QUEUE_ENABLE) {
>>> +			q_top->num_aqs_per_groups = ACC100_NUM_AQS;
>>> +			return;
>>> +		}
>>> +		q_top->num_aqs_per_groups = 0;
>>> +		for (aq = 0; aq < ACC100_NUM_AQS; aq++) {
>>> +			reg = acc100_reg_read(d, queue_offset(d-
>>> pf_device,
>>> +					0, qg, aq));
>>> +			if (reg & QUEUE_ENABLE)
>>> +				q_top->num_aqs_per_groups++;
>>> +		}
>>> +	}
>>> +}
>>> +
>>> +/* Fetch configuration enabled for the PF/VF using MMIO Read (slow)
>>> +*/ static inline void fetch_acc100_config(struct rte_bbdev *dev) {
>>> +	struct acc100_device *d = dev->data->dev_private;
>>> +	struct acc100_conf *acc100_conf = &d->acc100_conf;
>>> +	const struct acc100_registry_addr *reg_addr;
>>> +	uint8_t acc, qg;
>>> +	uint32_t reg, reg_aq, reg_len0, reg_len1;
>>> +	uint32_t reg_mode;
>>> +
>>> +	/* No need to retrieve the configuration is already done */
>>> +	if (d->configured)
>>> +		return;
>> Warn ?
> No this can genuinely happen on a regular basis, just no need to fetch it all again. 

ok

Tom

>
>>> +
>>> +	/* Choose correct registry addresses for the device type */
>>> +	if (d->pf_device)
>>> +		reg_addr = &pf_reg_addr;
>>> +	else
>>> +		reg_addr = &vf_reg_addr;
>>> +
>>> +	d->ddr_size = (1 + acc100_reg_read(d, reg_addr->ddr_range)) << 10;
>>> +
>>> +	/* Single VF Bundle by VF */
>>> +	acc100_conf->num_vf_bundles = 1;
>>> +	initQTop(acc100_conf);
>>> +
>>> +	struct rte_q_topology_t *q_top = NULL;
>>> +	int qman_func_id[5] = {0, 2, 1, 3, 4};
>> Do these magic numbers need #defines ?
> ok. 
>
>>> +	reg = acc100_reg_read(d, reg_addr->qman_group_func);
>>> +	for (qg = 0; qg < ACC100_NUM_QGRPS_PER_WORD; qg++) {
>>> +		reg_aq = acc100_reg_read(d,
>>> +				queue_offset(d->pf_device, 0, qg, 0));
>>> +		if (reg_aq & QUEUE_ENABLE) {
>>> +			acc = qman_func_id[(reg >> (qg * 4)) & 0x7];
>> 0x7 and [5], this could overflow.
> ok thanks, I can add exception handling. Not clear to me right now why it did not trigger tool warning. 
>
>>> +			updateQtop(acc, qg, acc100_conf, d);
>>> +		}
>>> +	}
>>> +
>>> +	/* Check the depth of the AQs*/
>>> +	reg_len0 = acc100_reg_read(d, reg_addr->depth_log0_offset);
>>> +	reg_len1 = acc100_reg_read(d, reg_addr->depth_log1_offset);
>>> +	for (acc = 0; acc < NUM_ACC; acc++) {
>>> +		qtopFromAcc(&q_top, acc, acc100_conf);
>>> +		if (q_top->first_qgroup_index <
>> ACC100_NUM_QGRPS_PER_WORD)
>>> +			q_top->aq_depth_log2 = (reg_len0 >>
>>> +					(q_top->first_qgroup_index * 4))
>>> +					& 0xF;
>>> +		else
>>> +			q_top->aq_depth_log2 = (reg_len1 >>
>>> +					((q_top->first_qgroup_index -
>>> +					ACC100_NUM_QGRPS_PER_WORD) *
>> 4))
>>> +					& 0xF;
>>> +	}
>>> +
>>> +	/* Read PF mode */
>>> +	if (d->pf_device) {
>>> +		reg_mode = acc100_reg_read(d, HWPfHiPfMode);
>>> +		acc100_conf->pf_mode_en = (reg_mode == 2) ? 1 : 0;
>> 2 is a magic number, consider a #define
>>
> ok
>
>> Tom
>>
> Thanks
> Nic
>
>>> +	}
>>> +
>>> +	rte_bbdev_log_debug(
>>> +			"%s Config LLR SIGN IN/OUT %s %s QG %u %u %u %u
>> AQ %u %u %u %u Len %u %u %u %u\n",
>>> +			(d->pf_device) ? "PF" : "VF",
>>> +			(acc100_conf->input_pos_llr_1_bit) ? "POS" : "NEG",
>>> +			(acc100_conf->output_pos_llr_1_bit) ? "POS" :
>> "NEG",
>>> +			acc100_conf->q_ul_4g.num_qgroups,
>>> +			acc100_conf->q_dl_4g.num_qgroups,
>>> +			acc100_conf->q_ul_5g.num_qgroups,
>>> +			acc100_conf->q_dl_5g.num_qgroups,
>>> +			acc100_conf->q_ul_4g.num_aqs_per_groups,
>>> +			acc100_conf->q_dl_4g.num_aqs_per_groups,
>>> +			acc100_conf->q_ul_5g.num_aqs_per_groups,
>>> +			acc100_conf->q_dl_5g.num_aqs_per_groups,
>>> +			acc100_conf->q_ul_4g.aq_depth_log2,
>>> +			acc100_conf->q_dl_4g.aq_depth_log2,
>>> +			acc100_conf->q_ul_5g.aq_depth_log2,
>>> +			acc100_conf->q_dl_5g.aq_depth_log2);
>>> +}
>>> +
>>>  /* Free 64MB memory used for software rings */  static int
>>> acc100_dev_close(struct rte_bbdev *dev  __rte_unused) @@ -33,8
>> +211,55
>>> @@
>>>  	return 0;
>>>  }
>>>
>>> +/* Get ACC100 device info */
>>> +static void
>>> +acc100_dev_info_get(struct rte_bbdev *dev,
>>> +		struct rte_bbdev_driver_info *dev_info) {
>>> +	struct acc100_device *d = dev->data->dev_private;
>>> +
>>> +	static const struct rte_bbdev_op_cap bbdev_capabilities[] = {
>>> +		RTE_BBDEV_END_OF_CAPABILITIES_LIST()
>>> +	};
>>> +
>>> +	static struct rte_bbdev_queue_conf default_queue_conf;
>>> +	default_queue_conf.socket = dev->data->socket_id;
>>> +	default_queue_conf.queue_size = MAX_QUEUE_DEPTH;
>>> +
>>> +	dev_info->driver_name = dev->device->driver->name;
>>> +
>>> +	/* Read and save the populated config from ACC100 registers */
>>> +	fetch_acc100_config(dev);
>>> +
>>> +	/* This isn't ideal because it reports the maximum number of queues
>> but
>>> +	 * does not provide info on how many can be uplink/downlink or
>> different
>>> +	 * priorities
>>> +	 */
>>> +	dev_info->max_num_queues =
>>> +			d->acc100_conf.q_dl_5g.num_aqs_per_groups *
>>> +			d->acc100_conf.q_dl_5g.num_qgroups +
>>> +			d->acc100_conf.q_ul_5g.num_aqs_per_groups *
>>> +			d->acc100_conf.q_ul_5g.num_qgroups +
>>> +			d->acc100_conf.q_dl_4g.num_aqs_per_groups *
>>> +			d->acc100_conf.q_dl_4g.num_qgroups +
>>> +			d->acc100_conf.q_ul_4g.num_aqs_per_groups *
>>> +			d->acc100_conf.q_ul_4g.num_qgroups;
>>> +	dev_info->queue_size_lim = MAX_QUEUE_DEPTH;
>>> +	dev_info->hardware_accelerated = true;
>>> +	dev_info->max_dl_queue_priority =
>>> +			d->acc100_conf.q_dl_4g.num_qgroups - 1;
>>> +	dev_info->max_ul_queue_priority =
>>> +			d->acc100_conf.q_ul_4g.num_qgroups - 1;
>>> +	dev_info->default_queue_conf = default_queue_conf;
>>> +	dev_info->cpu_flag_reqs = NULL;
>>> +	dev_info->min_alignment = 64;
>>> +	dev_info->capabilities = bbdev_capabilities;
>>> +	dev_info->harq_buffer_size = d->ddr_size; }
>>> +
>>>  static const struct rte_bbdev_ops acc100_bbdev_ops = {
>>>  	.close = acc100_dev_close,
>>> +	.info_get = acc100_dev_info_get,
>>>  };
>>>
>>>  /* ACC100 PCI PF address map */
>>> diff --git a/drivers/baseband/acc100/rte_acc100_pmd.h
>>> b/drivers/baseband/acc100/rte_acc100_pmd.h
>>> index cd77570..662e2c8 100644
>>> --- a/drivers/baseband/acc100/rte_acc100_pmd.h
>>> +++ b/drivers/baseband/acc100/rte_acc100_pmd.h
>>> @@ -7,6 +7,7 @@
>>>
>>>  #include "acc100_pf_enum.h"
>>>  #include "acc100_vf_enum.h"
>>> +#include "rte_acc100_cfg.h"
>>>
>>>  /* Helper macro for logging */
>>>  #define rte_bbdev_log(level, fmt, ...) \ @@ -520,6 +521,8 @@ struct
>>> acc100_registry_addr {
>>>  /* Private data structure for each ACC100 device */  struct
>>> acc100_device {
>>>  	void *mmio_base;  /**< Base address of MMIO registers (BAR0) */
>>> +	uint32_t ddr_size; /* Size in kB */
>>> +	struct acc100_conf acc100_conf; /* ACC100 Initial configuration */
>>>  	bool pf_device; /**< True if this is a PF ACC100 device */
>>>  	bool configured; /**< True if this ACC100 device is configured */
>>> };
  

Patch

diff --git a/app/test-bbdev/meson.build b/app/test-bbdev/meson.build
index 18ab6a8..fbd8ae3 100644
--- a/app/test-bbdev/meson.build
+++ b/app/test-bbdev/meson.build
@@ -12,3 +12,6 @@  endif
 if dpdk_conf.has('RTE_LIBRTE_PMD_BBDEV_FPGA_5GNR_FEC')
 	deps += ['pmd_bbdev_fpga_5gnr_fec']
 endif
+if dpdk_conf.has('RTE_LIBRTE_PMD_BBDEV_ACC100')
+	deps += ['pmd_bbdev_acc100']
+endif
\ No newline at end of file
diff --git a/drivers/baseband/acc100/rte_acc100_cfg.h b/drivers/baseband/acc100/rte_acc100_cfg.h
new file mode 100644
index 0000000..73bbe36
--- /dev/null
+++ b/drivers/baseband/acc100/rte_acc100_cfg.h
@@ -0,0 +1,96 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright(c) 2020 Intel Corporation
+ */
+
+#ifndef _RTE_ACC100_CFG_H_
+#define _RTE_ACC100_CFG_H_
+
+/**
+ * @file rte_acc100_cfg.h
+ *
+ * Functions for configuring ACC100 HW, exposed directly to applications.
+ * Configuration related to encoding/decoding is done through the
+ * librte_bbdev library.
+ *
+ * @warning
+ * @b EXPERIMENTAL: this API may change without prior notice
+ */
+
+#include <stdint.h>
+#include <stdbool.h>
+
+#ifdef __cplusplus
+extern "C" {
+#endif
+/**< Number of Virtual Functions ACC100 supports */
+#define RTE_ACC100_NUM_VFS 16
+
+/**
+ * Definition of Queue Topology for ACC100 Configuration
+ * Some level of details is abstracted out to expose a clean interface
+ * given that comprehensive flexibility is not required
+ */
+struct rte_q_topology_t {
+	/** Number of QGroups in incremental order of priority */
+	uint16_t num_qgroups;
+	/**
+	 * All QGroups have the same number of AQs here.
+	 * Note : Could be made a 16-array if more flexibility is really
+	 * required
+	 */
+	uint16_t num_aqs_per_groups;
+	/**
+	 * Depth of the AQs is the same of all QGroups here. Log2 Enum : 2^N
+	 * Note : Could be made a 16-array if more flexibility is really
+	 * required
+	 */
+	uint16_t aq_depth_log2;
+	/**
+	 * Index of the first Queue Group Index - assuming contiguity
+	 * Initialized as -1
+	 */
+	int8_t first_qgroup_index;
+};
+
+/**
+ * Definition of Arbitration related parameters for ACC100 Configuration
+ */
+struct rte_arbitration_t {
+	/** Default Weight for VF Fairness Arbitration */
+	uint16_t round_robin_weight;
+	uint32_t gbr_threshold1; /**< Guaranteed Bitrate Threshold 1 */
+	uint32_t gbr_threshold2; /**< Guaranteed Bitrate Threshold 2 */
+};
+
+/**
+ * Structure to pass ACC100 configuration.
+ * Note: all VF Bundles will have the same configuration.
+ */
+struct acc100_conf {
+	bool pf_mode_en; /**< 1 if PF is used for dataplane, 0 for VFs */
+	/** 1 if input '1' bit is represented by a positive LLR value, 0 if '1'
+	 * bit is represented by a negative value.
+	 */
+	bool input_pos_llr_1_bit;
+	/** 1 if output '1' bit is represented by a positive value, 0 if '1'
+	 * bit is represented by a negative value.
+	 */
+	bool output_pos_llr_1_bit;
+	uint16_t num_vf_bundles; /**< Number of VF bundles to setup */
+	/** Queue topology for each operation type */
+	struct rte_q_topology_t q_ul_4g;
+	struct rte_q_topology_t q_dl_4g;
+	struct rte_q_topology_t q_ul_5g;
+	struct rte_q_topology_t q_dl_5g;
+	/** Arbitration configuration for each operation type */
+	struct rte_arbitration_t arb_ul_4g[RTE_ACC100_NUM_VFS];
+	struct rte_arbitration_t arb_dl_4g[RTE_ACC100_NUM_VFS];
+	struct rte_arbitration_t arb_ul_5g[RTE_ACC100_NUM_VFS];
+	struct rte_arbitration_t arb_dl_5g[RTE_ACC100_NUM_VFS];
+};
+
+#ifdef __cplusplus
+}
+#endif
+
+#endif /* _RTE_ACC100_CFG_H_ */
diff --git a/drivers/baseband/acc100/rte_acc100_pmd.c b/drivers/baseband/acc100/rte_acc100_pmd.c
index 1b4cd13..7807a30 100644
--- a/drivers/baseband/acc100/rte_acc100_pmd.c
+++ b/drivers/baseband/acc100/rte_acc100_pmd.c
@@ -26,6 +26,184 @@ 
 RTE_LOG_REGISTER(acc100_logtype, pmd.bb.acc100, NOTICE);
 #endif
 
+/* Read a register of a ACC100 device */
+static inline uint32_t
+acc100_reg_read(struct acc100_device *d, uint32_t offset)
+{
+
+	void *reg_addr = RTE_PTR_ADD(d->mmio_base, offset);
+	uint32_t ret = *((volatile uint32_t *)(reg_addr));
+	return rte_le_to_cpu_32(ret);
+}
+
+/* Calculate the offset of the enqueue register */
+static inline uint32_t
+queue_offset(bool pf_device, uint8_t vf_id, uint8_t qgrp_id, uint16_t aq_id)
+{
+	if (pf_device)
+		return ((vf_id << 12) + (qgrp_id << 7) + (aq_id << 3) +
+				HWPfQmgrIngressAq);
+	else
+		return ((qgrp_id << 7) + (aq_id << 3) +
+				HWVfQmgrIngressAq);
+}
+
+enum {UL_4G = 0, UL_5G, DL_4G, DL_5G, NUM_ACC};
+
+/* Return the queue topology for a Queue Group Index */
+static inline void
+qtopFromAcc(struct rte_q_topology_t **qtop, int acc_enum,
+		struct acc100_conf *acc100_conf)
+{
+	struct rte_q_topology_t *p_qtop;
+	p_qtop = NULL;
+	switch (acc_enum) {
+	case UL_4G:
+		p_qtop = &(acc100_conf->q_ul_4g);
+		break;
+	case UL_5G:
+		p_qtop = &(acc100_conf->q_ul_5g);
+		break;
+	case DL_4G:
+		p_qtop = &(acc100_conf->q_dl_4g);
+		break;
+	case DL_5G:
+		p_qtop = &(acc100_conf->q_dl_5g);
+		break;
+	default:
+		/* NOTREACHED */
+		rte_bbdev_log(ERR, "Unexpected error evaluating qtopFromAcc");
+		break;
+	}
+	*qtop = p_qtop;
+}
+
+static void
+initQTop(struct acc100_conf *acc100_conf)
+{
+	acc100_conf->q_ul_4g.num_aqs_per_groups = 0;
+	acc100_conf->q_ul_4g.num_qgroups = 0;
+	acc100_conf->q_ul_4g.first_qgroup_index = -1;
+	acc100_conf->q_ul_5g.num_aqs_per_groups = 0;
+	acc100_conf->q_ul_5g.num_qgroups = 0;
+	acc100_conf->q_ul_5g.first_qgroup_index = -1;
+	acc100_conf->q_dl_4g.num_aqs_per_groups = 0;
+	acc100_conf->q_dl_4g.num_qgroups = 0;
+	acc100_conf->q_dl_4g.first_qgroup_index = -1;
+	acc100_conf->q_dl_5g.num_aqs_per_groups = 0;
+	acc100_conf->q_dl_5g.num_qgroups = 0;
+	acc100_conf->q_dl_5g.first_qgroup_index = -1;
+}
+
+static inline void
+updateQtop(uint8_t acc, uint8_t qg, struct acc100_conf *acc100_conf,
+		struct acc100_device *d) {
+	uint32_t reg;
+	struct rte_q_topology_t *q_top = NULL;
+	qtopFromAcc(&q_top, acc, acc100_conf);
+	if (unlikely(q_top == NULL))
+		return;
+	uint16_t aq;
+	q_top->num_qgroups++;
+	if (q_top->first_qgroup_index == -1) {
+		q_top->first_qgroup_index = qg;
+		/* Can be optimized to assume all are enabled by default */
+		reg = acc100_reg_read(d, queue_offset(d->pf_device,
+				0, qg, ACC100_NUM_AQS - 1));
+		if (reg & QUEUE_ENABLE) {
+			q_top->num_aqs_per_groups = ACC100_NUM_AQS;
+			return;
+		}
+		q_top->num_aqs_per_groups = 0;
+		for (aq = 0; aq < ACC100_NUM_AQS; aq++) {
+			reg = acc100_reg_read(d, queue_offset(d->pf_device,
+					0, qg, aq));
+			if (reg & QUEUE_ENABLE)
+				q_top->num_aqs_per_groups++;
+		}
+	}
+}
+
+/* Fetch configuration enabled for the PF/VF using MMIO Read (slow) */
+static inline void
+fetch_acc100_config(struct rte_bbdev *dev)
+{
+	struct acc100_device *d = dev->data->dev_private;
+	struct acc100_conf *acc100_conf = &d->acc100_conf;
+	const struct acc100_registry_addr *reg_addr;
+	uint8_t acc, qg;
+	uint32_t reg, reg_aq, reg_len0, reg_len1;
+	uint32_t reg_mode;
+
+	/* No need to retrieve the configuration is already done */
+	if (d->configured)
+		return;
+
+	/* Choose correct registry addresses for the device type */
+	if (d->pf_device)
+		reg_addr = &pf_reg_addr;
+	else
+		reg_addr = &vf_reg_addr;
+
+	d->ddr_size = (1 + acc100_reg_read(d, reg_addr->ddr_range)) << 10;
+
+	/* Single VF Bundle by VF */
+	acc100_conf->num_vf_bundles = 1;
+	initQTop(acc100_conf);
+
+	struct rte_q_topology_t *q_top = NULL;
+	int qman_func_id[5] = {0, 2, 1, 3, 4};
+	reg = acc100_reg_read(d, reg_addr->qman_group_func);
+	for (qg = 0; qg < ACC100_NUM_QGRPS_PER_WORD; qg++) {
+		reg_aq = acc100_reg_read(d,
+				queue_offset(d->pf_device, 0, qg, 0));
+		if (reg_aq & QUEUE_ENABLE) {
+			acc = qman_func_id[(reg >> (qg * 4)) & 0x7];
+			updateQtop(acc, qg, acc100_conf, d);
+		}
+	}
+
+	/* Check the depth of the AQs*/
+	reg_len0 = acc100_reg_read(d, reg_addr->depth_log0_offset);
+	reg_len1 = acc100_reg_read(d, reg_addr->depth_log1_offset);
+	for (acc = 0; acc < NUM_ACC; acc++) {
+		qtopFromAcc(&q_top, acc, acc100_conf);
+		if (q_top->first_qgroup_index < ACC100_NUM_QGRPS_PER_WORD)
+			q_top->aq_depth_log2 = (reg_len0 >>
+					(q_top->first_qgroup_index * 4))
+					& 0xF;
+		else
+			q_top->aq_depth_log2 = (reg_len1 >>
+					((q_top->first_qgroup_index -
+					ACC100_NUM_QGRPS_PER_WORD) * 4))
+					& 0xF;
+	}
+
+	/* Read PF mode */
+	if (d->pf_device) {
+		reg_mode = acc100_reg_read(d, HWPfHiPfMode);
+		acc100_conf->pf_mode_en = (reg_mode == 2) ? 1 : 0;
+	}
+
+	rte_bbdev_log_debug(
+			"%s Config LLR SIGN IN/OUT %s %s QG %u %u %u %u AQ %u %u %u %u Len %u %u %u %u\n",
+			(d->pf_device) ? "PF" : "VF",
+			(acc100_conf->input_pos_llr_1_bit) ? "POS" : "NEG",
+			(acc100_conf->output_pos_llr_1_bit) ? "POS" : "NEG",
+			acc100_conf->q_ul_4g.num_qgroups,
+			acc100_conf->q_dl_4g.num_qgroups,
+			acc100_conf->q_ul_5g.num_qgroups,
+			acc100_conf->q_dl_5g.num_qgroups,
+			acc100_conf->q_ul_4g.num_aqs_per_groups,
+			acc100_conf->q_dl_4g.num_aqs_per_groups,
+			acc100_conf->q_ul_5g.num_aqs_per_groups,
+			acc100_conf->q_dl_5g.num_aqs_per_groups,
+			acc100_conf->q_ul_4g.aq_depth_log2,
+			acc100_conf->q_dl_4g.aq_depth_log2,
+			acc100_conf->q_ul_5g.aq_depth_log2,
+			acc100_conf->q_dl_5g.aq_depth_log2);
+}
+
 /* Free 64MB memory used for software rings */
 static int
 acc100_dev_close(struct rte_bbdev *dev  __rte_unused)
@@ -33,8 +211,55 @@ 
 	return 0;
 }
 
+/* Get ACC100 device info */
+static void
+acc100_dev_info_get(struct rte_bbdev *dev,
+		struct rte_bbdev_driver_info *dev_info)
+{
+	struct acc100_device *d = dev->data->dev_private;
+
+	static const struct rte_bbdev_op_cap bbdev_capabilities[] = {
+		RTE_BBDEV_END_OF_CAPABILITIES_LIST()
+	};
+
+	static struct rte_bbdev_queue_conf default_queue_conf;
+	default_queue_conf.socket = dev->data->socket_id;
+	default_queue_conf.queue_size = MAX_QUEUE_DEPTH;
+
+	dev_info->driver_name = dev->device->driver->name;
+
+	/* Read and save the populated config from ACC100 registers */
+	fetch_acc100_config(dev);
+
+	/* This isn't ideal because it reports the maximum number of queues but
+	 * does not provide info on how many can be uplink/downlink or different
+	 * priorities
+	 */
+	dev_info->max_num_queues =
+			d->acc100_conf.q_dl_5g.num_aqs_per_groups *
+			d->acc100_conf.q_dl_5g.num_qgroups +
+			d->acc100_conf.q_ul_5g.num_aqs_per_groups *
+			d->acc100_conf.q_ul_5g.num_qgroups +
+			d->acc100_conf.q_dl_4g.num_aqs_per_groups *
+			d->acc100_conf.q_dl_4g.num_qgroups +
+			d->acc100_conf.q_ul_4g.num_aqs_per_groups *
+			d->acc100_conf.q_ul_4g.num_qgroups;
+	dev_info->queue_size_lim = MAX_QUEUE_DEPTH;
+	dev_info->hardware_accelerated = true;
+	dev_info->max_dl_queue_priority =
+			d->acc100_conf.q_dl_4g.num_qgroups - 1;
+	dev_info->max_ul_queue_priority =
+			d->acc100_conf.q_ul_4g.num_qgroups - 1;
+	dev_info->default_queue_conf = default_queue_conf;
+	dev_info->cpu_flag_reqs = NULL;
+	dev_info->min_alignment = 64;
+	dev_info->capabilities = bbdev_capabilities;
+	dev_info->harq_buffer_size = d->ddr_size;
+}
+
 static const struct rte_bbdev_ops acc100_bbdev_ops = {
 	.close = acc100_dev_close,
+	.info_get = acc100_dev_info_get,
 };
 
 /* ACC100 PCI PF address map */
diff --git a/drivers/baseband/acc100/rte_acc100_pmd.h b/drivers/baseband/acc100/rte_acc100_pmd.h
index cd77570..662e2c8 100644
--- a/drivers/baseband/acc100/rte_acc100_pmd.h
+++ b/drivers/baseband/acc100/rte_acc100_pmd.h
@@ -7,6 +7,7 @@ 
 
 #include "acc100_pf_enum.h"
 #include "acc100_vf_enum.h"
+#include "rte_acc100_cfg.h"
 
 /* Helper macro for logging */
 #define rte_bbdev_log(level, fmt, ...) \
@@ -520,6 +521,8 @@  struct acc100_registry_addr {
 /* Private data structure for each ACC100 device */
 struct acc100_device {
 	void *mmio_base;  /**< Base address of MMIO registers (BAR0) */
+	uint32_t ddr_size; /* Size in kB */
+	struct acc100_conf acc100_conf; /* ACC100 Initial configuration */
 	bool pf_device; /**< True if this is a PF ACC100 device */
 	bool configured; /**< True if this ACC100 device is configured */
 };