Re: [PATCH v3 3/4] baseband/acc100: configuration of ACC101 from PF

[Maxime Coquelin <maxime.coquelin@redhat.com>]

Hi Nicolas,

On 5/16/22 22:48, Nicolas Chautru wrote:
 > Adding companion function specific to ACC100 and it
 > can be called from bbdev-test when running from PF.
 >
 > Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
 > ---
 >   app/test-bbdev/test_bbdev_perf.c         |  22 ++-
 >   drivers/baseband/acc100/rte_acc100_cfg.h |  17 ++
 >   drivers/baseband/acc100/rte_acc100_pmd.c | 302 
+++++++++++++++++++++++++++++++
 >   drivers/baseband/acc100/version.map      |   2 +-
 >   4 files changed, 336 insertions(+), 7 deletions(-)
 >
 > diff --git a/app/test-bbdev/test_bbdev_perf.c 
b/app/test-bbdev/test_bbdev_perf.c
 > index 0fa119a..b10b93d 100644
 > --- a/app/test-bbdev/test_bbdev_perf.c
 > +++ b/app/test-bbdev/test_bbdev_perf.c
 > @@ -63,6 +63,8 @@
 >   #define ACC100_QMGR_INVALID_IDX -1
 >   #define ACC100_QMGR_RR 1
 >   #define ACC100_QOS_GBR 0
 > +#define ACC101PF_DRIVER_NAME   ("intel_acc101_pf")
 > +#define ACC101VF_DRIVER_NAME   ("intel_acc101_vf")
 >   #endif
 >
 >   #define OPS_CACHE_SIZE 256U
 > @@ -711,11 +713,12 @@ typedef int (test_case_function)(struct 
active_device *ad,
 >   #endif
 >   #ifdef RTE_BASEBAND_ACC100
 >   	if ((get_init_device() == true) &&
 > -		(!strcmp(info->drv.driver_name, ACC100PF_DRIVER_NAME))) {
 > +			((!strcmp(info->drv.driver_name, ACC100PF_DRIVER_NAME)) ||
 > +			(!strcmp(info->drv.driver_name, ACC101PF_DRIVER_NAME)))) {
 >   		struct rte_acc100_conf conf;
 >   		unsigned int i;
 >
 > -		printf("Configure ACC100 FEC Driver %s with default values\n",
 > +		printf("Configure ACC10X FEC Driver %s with default values\n",
 >   				info->drv.driver_name);
 >
 >   		/* clear default configuration before initialization */
 > @@ -760,10 +763,17 @@ typedef int (test_case_function)(struct 
active_device *ad,
 >   		conf.q_dl_5g.aq_depth_log2 = ACC100_QMGR_AQ_DEPTH;
 >
 >   		/* setup PF with configuration information */
 > -		ret = rte_acc100_configure(info->dev_name, &conf);
 > -		TEST_ASSERT_SUCCESS(ret,
 > -				"Failed to configure ACC100 PF for bbdev %s",
 > -				info->dev_name);
 > +		if (!strcmp(info->drv.driver_name, ACC100PF_DRIVER_NAME)) {
 > +			ret = rte_acc100_configure(info->dev_name, &conf);
 > +			TEST_ASSERT_SUCCESS(ret,
 > +					"Failed to configure ACC100 PF for bbdev %s",
 > +					info->dev_name);
 > +		} else {
 > +			ret = rte_acc101_configure(info->dev_name, &conf);
 > +			TEST_ASSERT_SUCCESS(ret,
 > +					"Failed to configure ACC101 PF for bbdev %s",
 > +					info->dev_name);
 > +		}
 >   	}
 >   #endif
 >   	/* Let's refresh this now this is configured */
 > diff --git a/drivers/baseband/acc100/rte_acc100_cfg.h 
b/drivers/baseband/acc100/rte_acc100_cfg.h
 > index d233e42..2e3c43f 100644
 > --- a/drivers/baseband/acc100/rte_acc100_cfg.h
 > +++ b/drivers/baseband/acc100/rte_acc100_cfg.h
 > @@ -106,6 +106,23 @@ struct rte_acc100_conf {
 >   int
 >   rte_acc100_configure(const char *dev_name, struct rte_acc100_conf 
*conf);
 >
 > +/**
 > + * Configure a ACC101 device
 > + *
 > + * @param dev_name
 > + *   The name of the device. This is the short form of PCI BDF, e.g. 
00:01.0.
 > + *   It can also be retrieved for a bbdev device from the dev_name 
field in the
 > + *   rte_bbdev_info structure returned by rte_bbdev_info_get().
 > + * @param conf
 > + *   Configuration to apply to ACC101 HW.
 > + *
 > + * @return
 > + *   Zero on success, negative value on failure.
 > + */
 > +__rte_experimental
 > +int
 > +rte_acc101_configure(const char *dev_name, struct rte_acc100_conf 
*conf);
 > +
 >   #ifdef __cplusplus
 >   }
 >   #endif
 > diff --git a/drivers/baseband/acc100/rte_acc100_pmd.c 
b/drivers/baseband/acc100/rte_acc100_pmd.c
 > index daf2ce0..b03cedc 100644
 > --- a/drivers/baseband/acc100/rte_acc100_pmd.c
 > +++ b/drivers/baseband/acc100/rte_acc100_pmd.c
 > @@ -4921,3 +4921,305 @@ static int acc100_pci_remove(struct 
rte_pci_device *pci_dev)
 >   	rte_bbdev_log_debug("PF Tip configuration complete for %s", dev_name);
 >   	return 0;
 >   }
 > +
 > +
 > +/* Initial configuration of a ACC101 device prior to running 
configure() */
 > +int
 > +rte_acc101_configure(const char *dev_name, struct rte_acc100_conf *conf)
 > +{
Please don't introduce new API for every new variant.
You should have a single API, and within it call specific configuration
function based on the device ID.

It will be easier for the application which calls it not to have to know
which variant it is for.

 > +	rte_bbdev_log(INFO, "rte_acc101_configure");
 > +	uint32_t value, address, status;
 > +	int qg_idx, template_idx, vf_idx, acc, i;
 > +	struct rte_bbdev *bbdev = rte_bbdev_get_named_dev(dev_name);
 > +
 > +	/* Compile time checks */
 > +	RTE_BUILD_BUG_ON(sizeof(struct acc100_dma_req_desc) != 256);
 > +	RTE_BUILD_BUG_ON(sizeof(union acc100_dma_desc) != 256);
 > +	RTE_BUILD_BUG_ON(sizeof(struct acc100_fcw_td) != 24);
 > +	RTE_BUILD_BUG_ON(sizeof(struct acc100_fcw_te) != 32);
 > +
 > +	if (bbdev == NULL) {
 > +		rte_bbdev_log(ERR,
 > +		"Invalid dev_name (%s), or device is not yet initialised",
 > +		dev_name);
 > +		return -ENODEV;
 > +	}
 > +	struct acc100_device *d = bbdev->data->dev_private;
 > +
 > +	/* Store configuration */
 > +	rte_memcpy(&d->acc100_conf, conf, sizeof(d->acc100_conf));
 > +
 > +	/* PCIe Bridge configuration */
 > +	acc100_reg_write(d, HwPfPcieGpexBridgeControl, ACC101_CFG_PCI_BRIDGE);
 > +	for (i = 1; i < ACC101_GPEX_AXIMAP_NUM; i++)
 > +		acc100_reg_write(d, HwPfPcieGpexAxiAddrMappingWindowPexBaseHigh + 
i * 16, 0);
 > +
 > +	/* Prevent blocking AXI read on BRESP for AXI Write */
 > +	address = HwPfPcieGpexAxiPioControl;
 > +	value = ACC101_CFG_PCI_AXI;
 > +	acc100_reg_write(d, address, value);
 > +
 > +	/* Explicitly releasing AXI as this may be stopped after PF FLR/BME */
 > +	usleep(2000);
This sleep looks weird, especially when ACC100 does not require it.
Isn't there a way to know it is ready to release the AXI? Or at least
document why it is necessary.

 > +	acc100_reg_write(d, HWPfDmaAxiControl, 1);
 > +
 > +	/* Set the default 5GDL DMA configuration */
 > +	acc100_reg_write(d, HWPfDmaInboundDrainDataSize, ACC101_DMA_INBOUND);
 > +
 > +	/* Enable granular dynamic clock gating */
 > +	address = HWPfHiClkGateHystReg;
 > +	value = ACC101_CLOCK_GATING_EN;
 > +	acc100_reg_write(d, address, value);
 > +
 > +	/* Set default descriptor signature */
 > +	address = HWPfDmaDescriptorSignatuture;
 > +	value = 0;
 > +	acc100_reg_write(d, address, value);
 > +
 > +	/* Enable the Error Detection in DMA */
 > +	value = ACC101_CFG_DMA_ERROR;
 > +	address = HWPfDmaErrorDetectionEn;
 > +	acc100_reg_write(d, address, value);
 > +
 > +	/* AXI Cache configuration */
 > +	value = ACC101_CFG_AXI_CACHE;
 > +	address = HWPfDmaAxcacheReg;
 > +	acc100_reg_write(d, address, value);
 > +
 > +	/* Default DMA Configuration (Qmgr Enabled) */
 > +	address = HWPfDmaConfig0Reg;
 > +	value = 0;
 > +	acc100_reg_write(d, address, value);
 > +	address = HWPfDmaQmanen;
 > +	value = 0;
 > +	acc100_reg_write(d, address, value);
 > +
 > +	/* Default RLIM/ALEN configuration */
 > +	address = HWPfDmaConfig1Reg;
 > +	int alen_r = 0xF;
 > +	int alen_w = 0x7;
 > +	value = (1 << 31) + (alen_w << 20)  + (1 << 6) + alen_r;
 > +	acc100_reg_write(d, address, value);
 > +
 > +	/* Configure DMA Qmanager addresses */
 > +	address = HWPfDmaQmgrAddrReg;
 > +	value = HWPfQmgrEgressQueuesTemplate;
 > +	acc100_reg_write(d, address, value);
 > +
 > +	/* ===== Qmgr Configuration ===== */
 > +	/* Configuration of the AQueue Depth QMGR_GRP_0_DEPTH_LOG2 for UL */
 > +	int totalQgs = conf->q_ul_4g.num_qgroups +
 > +			conf->q_ul_5g.num_qgroups +
 > +			conf->q_dl_4g.num_qgroups +
 > +			conf->q_dl_5g.num_qgroups;
 > +	for (qg_idx = 0; qg_idx < totalQgs; qg_idx++) {
 > +		address = HWPfQmgrDepthLog2Grp +
 > +		ACC101_BYTES_IN_WORD * qg_idx;
 > +		value = aqDepth(qg_idx, conf);
 > +		acc100_reg_write(d, address, value);
 > +		address = HWPfQmgrTholdGrp +
 > +		ACC101_BYTES_IN_WORD * qg_idx;
 > +		value = (1 << 16) + (1 << (aqDepth(qg_idx, conf) - 1));
 > +		acc100_reg_write(d, address, value);
 > +	}
 > +
 > +	/* Template Priority in incremental order */
 > +	for (template_idx = 0; template_idx < ACC101_NUM_TMPL;
 > +			template_idx++) {
 > +		address = HWPfQmgrGrpTmplateReg0Indx + ACC101_BYTES_IN_WORD * 
template_idx;
 > +		value = ACC101_TMPL_PRI_0;
 > +		acc100_reg_write(d, address, value);
 > +		address = HWPfQmgrGrpTmplateReg1Indx + ACC101_BYTES_IN_WORD * 
template_idx;
 > +		value = ACC101_TMPL_PRI_1;
 > +		acc100_reg_write(d, address, value);
 > +		address = HWPfQmgrGrpTmplateReg2indx + ACC101_BYTES_IN_WORD * 
template_idx;
 > +		value = ACC101_TMPL_PRI_2;
 > +		acc100_reg_write(d, address, value);
 > +		address = HWPfQmgrGrpTmplateReg3Indx + ACC101_BYTES_IN_WORD * 
template_idx;
 > +		value = ACC101_TMPL_PRI_3;
 > +		acc100_reg_write(d, address, value);
For both ACC100 and ACC101, ACC10x_NUM_TMPL is 32.
Isn't there a bug here or in ACC100?

ACC100 performs a modulo 8 on template_idx, but ACC101 does not. But the
registers mapping is the same, so I guess this is the same HW block.

I think ACC100 is the buggy one, because it seems weird to write the
same register with the same value multiple times.

In case it is ACC100 that is buggy, it needs to be fixed in a dedicated
patch, with proper fixes tag so that it is backported.

 > +	}
 > +
 > +	address = HWPfQmgrGrpPriority;
 > +	value = ACC101_CFG_QMGR_HI_P;
 > +	acc100_reg_write(d, address, value);
 > +
 > +	/* Template Configuration */
 > +	for (template_idx = 0; template_idx < ACC101_NUM_TMPL;
 > +			template_idx++) {
 > +		value = 0;
 > +		address = HWPfQmgrGrpTmplateReg4Indx
 > +				+ ACC101_BYTES_IN_WORD * template_idx;
 > +		acc100_reg_write(d, address, value);
 > +	}
 > +	/* 4GUL */
 > +	int numQgs = conf->q_ul_4g.num_qgroups;
 > +	int numQqsAcc = 0;
 > +	value = 0;
 > +	for (qg_idx = numQqsAcc; qg_idx < (numQgs + numQqsAcc); qg_idx++)
 > +		value |= (1 << qg_idx);
 > +	for (template_idx = ACC101_SIG_UL_4G;
 > +			template_idx <= ACC101_SIG_UL_4G_LAST;
 > +			template_idx++) {
 > +		address = HWPfQmgrGrpTmplateReg4Indx
 > +				+ ACC101_BYTES_IN_WORD * template_idx;
 > +		acc100_reg_write(d, address, value);
 > +	}
 > +	/* 5GUL */
 > +	numQqsAcc += numQgs;
 > +	numQgs	= conf->q_ul_5g.num_qgroups;
 > +	value = 0;
 > +	int numEngines = 0;
 > +	for (qg_idx = numQqsAcc; qg_idx < (numQgs + numQqsAcc); qg_idx++)
 > +		value |= (1 << qg_idx);
 > +	for (template_idx = ACC101_SIG_UL_5G;
 > +			template_idx <= ACC101_SIG_UL_5G_LAST;
 > +			template_idx++) {
 > +		/* Check engine power-on status */
 > +		address = HwPfFecUl5gIbDebugReg +
 > +				ACC101_ENGINE_OFFSET * template_idx;
 > +		status = (acc100_reg_read(d, address) >> 4) & 0xF;
 > +		address = HWPfQmgrGrpTmplateReg4Indx
 > +				+ ACC101_BYTES_IN_WORD * template_idx;
 > +		if (status == 1) {
 > +			acc100_reg_write(d, address, value);
 > +			numEngines++;
 > +		} else
 > +			acc100_reg_write(d, address, 0);
 > +#if RTE_ACC101_SINGLE_FEC == 1
 > +		value = 0;
 > +#endif
 > +	}
 > +	printf("Number of 5GUL engines %d\n", numEngines);
 > +	/* 4GDL */
 > +	numQqsAcc += numQgs;
 > +	numQgs	= conf->q_dl_4g.num_qgroups;
 > +	value = 0;
 > +	for (qg_idx = numQqsAcc; qg_idx < (numQgs + numQqsAcc); qg_idx++)
 > +		value |= (1 << qg_idx);
 > +	for (template_idx = ACC101_SIG_DL_4G;
 > +			template_idx <= ACC101_SIG_DL_4G_LAST;
 > +			template_idx++) {
 > +		address = HWPfQmgrGrpTmplateReg4Indx
 > +				+ ACC101_BYTES_IN_WORD * template_idx;
 > +		acc100_reg_write(d, address, value);
 > +#if RTE_ACC101_SINGLE_FEC == 1
 > +			value = 0;
 > +#endif
 > +	}
 > +	/* 5GDL */
 > +	numQqsAcc += numQgs;
 > +	numQgs	= conf->q_dl_5g.num_qgroups;
 > +	value = 0;
 > +	for (qg_idx = numQqsAcc; qg_idx < (numQgs + numQqsAcc); qg_idx++)
 > +		value |= (1 << qg_idx);
 > +	for (template_idx = ACC101_SIG_DL_5G;
 > +			template_idx <= ACC101_SIG_DL_5G_LAST;
 > +			template_idx++) {
 > +		address = HWPfQmgrGrpTmplateReg4Indx
 > +				+ ACC101_BYTES_IN_WORD * template_idx;
 > +		acc100_reg_write(d, address, value);
 > +#if RTE_ACC101_SINGLE_FEC == 1
 > +		value = 0;
 > +#endif
 > +	}
 > +
 > +	/* Queue Group Function mapping */
 > +	int qman_func_id[8] = {0, 2, 1, 3, 4, 0, 0, 0};
 > +	address = HWPfQmgrGrpFunction0;
 > +	value = 0;
 > +	for (qg_idx = 0; qg_idx < 8; qg_idx++) {
 > +		acc = accFromQgid(qg_idx, conf);
 > +		value |= qman_func_id[acc]<<(qg_idx * 4);
 > +	}
 > +	acc100_reg_write(d, address, value);
 > +
 > +	/* Configuration of the Arbitration QGroup depth to 1 */
 > +	for (qg_idx = 0; qg_idx < totalQgs; qg_idx++) {
 > +		address = HWPfQmgrArbQDepthGrp +
 > +		ACC101_BYTES_IN_WORD * qg_idx;
 > +		value = 0;
 > +		acc100_reg_write(d, address, value);
 > +	}
 > +
 > +	/* Enabling AQueues through the Queue hierarchy*/
 > +	for (vf_idx = 0; vf_idx < ACC101_NUM_VFS; vf_idx++) {
 > +		for (qg_idx = 0; qg_idx < ACC101_NUM_QGRPS; qg_idx++) {
 > +			value = 0;
 > +			if (vf_idx < conf->num_vf_bundles &&
 > +					qg_idx < totalQgs)
 > +				value = (1 << aqNum(qg_idx, conf)) - 1;
 > +			address = HWPfQmgrAqEnableVf
 > +					+ vf_idx * ACC101_BYTES_IN_WORD;
 > +			value += (qg_idx << 16);
 > +			acc100_reg_write(d, address, value);
 > +		}
 > +	}
 > +
 > +	/* This pointer to ARAM (128kB) is shifted by 2 (4B per register) */
 > +	uint32_t aram_address = 0;
 > +	for (qg_idx = 0; qg_idx < totalQgs; qg_idx++) {
 > +		for (vf_idx = 0; vf_idx < conf->num_vf_bundles; vf_idx++) {
 > +			address = HWPfQmgrVfBaseAddr + vf_idx
 > +					* ACC101_BYTES_IN_WORD + qg_idx
 > +					* ACC101_BYTES_IN_WORD * 64;
 > +			value = aram_address;
 > +			acc100_reg_write(d, address, value);
 > +			/* Offset ARAM Address for next memory bank
 > +			 * - increment of 4B
 > +			 */
 > +			aram_address += aqNum(qg_idx, conf) *
 > +					(1 << aqDepth(qg_idx, conf));
 > +		}
 > +	}
 > +
 > +	if (aram_address > ACC101_WORDS_IN_ARAM_SIZE) {
 > +		rte_bbdev_log(ERR, "ARAM Configuration not fitting %d %d\n",
 > +				aram_address, ACC101_WORDS_IN_ARAM_SIZE);
 > +		return -EINVAL;
 > +	}
 > +
 > +	/* ==== HI Configuration ==== */
 > +
 > +	/* No Info Ring/MSI by default */
 > +	acc100_reg_write(d, HWPfHiInfoRingIntWrEnRegPf, 0);
 > +	acc100_reg_write(d, HWPfHiInfoRingVf2pfLoWrEnReg, 0);
 > +	acc100_reg_write(d, HWPfHiCfgMsiIntWrEnRegPf, 0xFFFFFFFF);
 > +	acc100_reg_write(d, HWPfHiCfgMsiVf2pfLoWrEnReg, 0xFFFFFFFF);
 > +	/* Prevent Block on Transmit Error */
 > +	address = HWPfHiBlockTransmitOnErrorEn;
 > +	value = 0;
 > +	acc100_reg_write(d, address, value);
 > +	/* Prevents to drop MSI */
 > +	address = HWPfHiMsiDropEnableReg;
 > +	value = 0;
 > +	acc100_reg_write(d, address, value);
 > +	/* Set the PF Mode register */
 > +	address = HWPfHiPfMode;
 > +	value = (conf->pf_mode_en) ? ACC101_PF_VAL : 0;
 > +	acc100_reg_write(d, address, value);
 > +	/* Explicitly releasing AXI after PF Mode and 2 ms */
 > +	usleep(2000);
 > +	acc100_reg_write(d, HWPfDmaAxiControl, 1);
 > +
 > +	/* QoS overflow init */
 > +	value = 1;
 > +	address = HWPfQosmonAEvalOverflow0;
 > +	acc100_reg_write(d, address, value);
 > +	address = HWPfQosmonBEvalOverflow0;
 > +	acc100_reg_write(d, address, value);
 > +
 > +	/* HARQ DDR Configuration */
 > +	unsigned int ddrSizeInMb = ACC101_HARQ_DDR;
 > +	for (vf_idx = 0; vf_idx < conf->num_vf_bundles; vf_idx++) {
 > +		address = HWPfDmaVfDdrBaseRw + vf_idx
 > +				* 0x10;
 > +		value = ((vf_idx * (ddrSizeInMb / 64)) << 16) +
 > +				(ddrSizeInMb - 1);
 > +		acc100_reg_write(d, address, value);
 > +	}
 > +	usleep(ACC101_LONG_WAIT);
 > +
 > +	rte_bbdev_log_debug("PF TIP configuration complete for %s", dev_name);
 > +	return 0;
 > +}
 > diff --git a/drivers/baseband/acc100/version.map 
b/drivers/baseband/acc100/version.map
 > index 40604c7..37b850f 100644
 > --- a/drivers/baseband/acc100/version.map
 > +++ b/drivers/baseband/acc100/version.map
 > @@ -6,5 +6,5 @@ EXPERIMENTAL {
 >   	global:
 >
 >   	rte_acc100_configure;
 > -
 > +	rte_acc101_configure;
 >   };