[dpdk-dev] [PATCHv5 13/33] net/dpaa2: introducing NXP dpaa2 pmd driver

[Ferruh Yigit]

On 1/19/2017 1:23 PM, Hemant Agrawal wrote:
> add support for fsl-mc bus based dpaa2 pmd driver.
> 
> Signed-off-by: Hemant Agrawal <hemant.agrawal at nxp.com>

<...>

> diff --git a/drivers/common/Makefile b/drivers/common/Makefile
> index e5bfecb..76ec2d1 100644
> --- a/drivers/common/Makefile
> +++ b/drivers/common/Makefile
> @@ -31,6 +31,8 @@
>  
>  include $(RTE_SDK)/mk/rte.vars.mk
>  
> +CONFIG_RTE_LIBRTE_DPAA2_COMMON = $(CONFIG_RTE_LIBRTE_DPAA2_PMD)

This logic make sense, is there any reason DPAA2_COMMON to be a config
option, it doesn't look like something a user would like to change on
its own.

Instead, as done here, if there is a user of common folder it is enabled
in Makefile. For this DPAA2_COMMON doesn't need to be a config option
itself I think.

> +
>  DIRS-$(CONFIG_RTE_LIBRTE_DPAA2_COMMON) += dpaa2
>  
>  include $(RTE_SDK)/mk/rte.subdir.mk
> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
> index 40fc333..f716ca0 100644
> --- a/drivers/net/Makefile
> +++ b/drivers/net/Makefile
> @@ -57,7 +57,7 @@ DIRS-$(CONFIG_RTE_LIBRTE_THUNDERX_NICVF_PMD) += thunderx
>  DIRS-$(CONFIG_RTE_LIBRTE_VIRTIO_PMD) += virtio
>  DIRS-$(CONFIG_RTE_LIBRTE_VMXNET3_PMD) += vmxnet3
>  DIRS-$(CONFIG_RTE_LIBRTE_PMD_XENVIRT) += xenvirt
> -
> +DIRS-$(CONFIG_RTE_LIBRTE_DPAA2_PMD) += dpaa2

Add alphabetically please.

>  ifeq ($(CONFIG_RTE_LIBRTE_VHOST),y)
>  DIRS-$(CONFIG_RTE_LIBRTE_PMD_VHOST) += vhost
>  endif # $(CONFIG_RTE_LIBRTE_VHOST)

<...>

> diff --git a/drivers/net/dpaa2/dpaa2_ethdev.c b/drivers/net/dpaa2/dpaa2_ethdev.c
> new file mode 100644
> index 0000000..2295f82
> --- /dev/null
> +++ b/drivers/net/dpaa2/dpaa2_ethdev.c
<...>
> +/* Name of the DPAA2 Net PMD */
> +static const char *drivername = "DPAA2 PMD";

Custom names not preferred, please check any other PMD to be consistent.

<...>

> +static int
> +rte_dpaa2_probe(struct rte_dpaa2_driver *dpaa2_drv,
> +		struct rte_dpaa2_device *dpaa2_dev)
> +{
> +	struct eth_driver    *eth_drv;

whitespace error.

> +	struct rte_eth_dev *eth_dev;
> +	char ethdev_name[RTE_ETH_NAME_MAX_LEN];
> +
> +	int diag;
> +
> +	eth_drv = (struct eth_driver *)dpaa2_drv;

How this suppose to work?

struct eth_driver {

	struct rte_pci_driver pci_drv;
	eth_dev_init_t eth_dev_init;
	eth_dev_uninit_t eth_dev_uninit;
	unsigned int dev_private_size;
};

struct rte_dpaa2_driver {
	TAILQ_ENTRY(rte_dpaa2_driver) next;
	struct rte_driver driver;
	struct rte_fslmc_bus *fslmc_bus;
	uint32_t drv_flags;
	uint16_t drv_type;
	rte_dpaa2_probe_t probe
	rte_dpaa2_remove_t remove;
};

> +
> +	sprintf(ethdev_name, "dpni-%d", dpaa2_dev->object_id);
> +
> +	eth_dev = rte_eth_dev_allocate(ethdev_name);
> +	if (eth_dev == NULL)
> +		return -ENOMEM;
> +
> +	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> +		eth_dev->data->dev_private = rte_zmalloc(
> +						"ethdev private structure",
> +						sizeof(struct dpaa2_dev_priv),
> +						RTE_CACHE_LINE_SIZE);
> +		if (eth_dev->data->dev_private == NULL) {
> +			RTE_LOG(CRIT, PMD, "Cannot allocate memzone for"
> +				" private port data\n");
> +			return -ENOMEM;

release allocated port?

> +		}
> +	}
> +	eth_dev->device = &dpaa2_dev->device;
> +	dpaa2_dev->eth_dev = eth_dev;
> +	eth_dev->driver = eth_drv;
> +	eth_dev->data->rx_mbuf_alloc_failed = 0;
> +
> +	/* init user callbacks */
> +	TAILQ_INIT(&eth_dev->link_intr_cbs);

This is no more required, since done by rte_eth_dev_allocate()

> +
> +	/*
> +	 * Set the default MTU.
> +	 */
> +	eth_dev->data->mtu = ETHER_MTU;

Same, no more required to set in pmd.

> +
> +	/* Invoke PMD device initialization function */
> +	diag = dpaa2_dev_init(eth_dev);
> +	if (diag == 0)
> +		return 0;
> +
> +	if (rte_eal_process_type() == RTE_PROC_PRIMARY)
> +		rte_free(eth_dev->data->dev_private);
> +	rte_eth_dev_release_port(eth_dev);
> +	return diag;
> +}

<...>

> +static struct rte_dpaa2_driver rte_dpaa2_pmd = {
> +	.drv_type = DPAA2_MC_DPNI_DEVID,
> +	.driver = {
> +		.name = "DPAA2 PMD",

This is not required, RTE_PMD_REGISTER_DPAA2 will set it.

> +	},
> +	.probe = rte_dpaa2_probe,
> +	.remove = rte_dpaa2_remove,

These are PMD specific APIs, PCI equivalent of these are eth_dev_init()
and eth_dev_uninit() functions. Instead of rte_eth_dev_pci_probe() like
function.
Here it seems used as how it is used for virtual devices. But even for
virtual devices, it is desired to have more proper virtual bus structure.

I understand this part is a little problematic now, because of the
eth_driver <-> pci_driver relation, this is not generic.
What do you think first solve eth_driver <-> pci_driver problem, out of
this patchset, and later add this PMD?

<...>

> diff --git a/mk/rte.app.mk b/mk/rte.app.mk
> index a5daa84..c793dd2 100644
> --- a/mk/rte.app.mk
> +++ b/mk/rte.app.mk
> @@ -110,6 +110,11 @@ _LDLIBS-$(CONFIG_RTE_LIBRTE_PMD_AF_PACKET)  += -lrte_pmd_af_packet
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_BNX2X_PMD)      += -lrte_pmd_bnx2x -lz
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_BNXT_PMD)       += -lrte_pmd_bnxt
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_CXGBE_PMD)      += -lrte_pmd_cxgbe
> +ifeq ($(CONFIG_RTE_LIBRTE_DPAA2_COMMON),y)

If DPAA2_COMMON removed it can work here too, because if DPAA2_PMD
enabled, DPAA2_COMMON should be already enabled.

> +_LDLIBS-$(CONFIG_RTE_LIBRTE_DPAA2_PMD)      += -lrte_pmd_dpaa2
> +_LDLIBS-$(CONFIG_RTE_LIBRTE_DPAA2_PMD)      += -lrte_pmd_dpaa2_qbman
> +_LDLIBS-$(CONFIG_RTE_LIBRTE_DPAA2_PMD)      += -lrte_pmd_fslmcbus
> +endif
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_E1000_PMD)      += -lrte_pmd_e1000
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_ENA_PMD)        += -lrte_pmd_ena
>  _LDLIBS-$(CONFIG_RTE_LIBRTE_ENIC_PMD)       += -lrte_pmd_enic
>