From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ferruh Yigit Subject: Re: [PATCHv5 13/33] net/dpaa2: introducing NXP dpaa2 pmd driver Date: Thu, 19 Jan 2017 19:15:37 +0000 Message-ID: References: <1484679174-4174-1-git-send-email-hemant.agrawal@nxp.com> <1484832240-2048-1-git-send-email-hemant.agrawal@nxp.com> <1484832240-2048-16-git-send-email-hemant.agrawal@nxp.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Cc: thomas.monjalon@6wind.com, bruce.richardson@intel.com, shreyansh.jain@nxp.com, john.mcnamara@intel.com, jerin.jacob@caviumnetworks.com To: Hemant Agrawal , dev@dpdk.org Return-path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id C5B155320 for ; Thu, 19 Jan 2017 20:15:40 +0100 (CET) In-Reply-To: <1484832240-2048-16-git-send-email-hemant.agrawal@nxp.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 <...> > 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 >