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.agra...@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(ð_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
>
