On 3/19/2018 12:33 PM, Kumar, Ravi1 wrote: >> >> >> -----Original Message----- >> From: Ferruh Yigit [mailto:ferruh.yi...@intel.com] >> Sent: Friday, March 16, 2018 11:12 PM >> To: Kumar, Ravi1 <ravi1.ku...@amd.com>; dev@dpdk.org >> Subject: Re: [PATCH v3 01/18] net/axgbe: add minimal dev init and uninit >> support >> >> On 3/9/2018 8:42 AM, Ravi Kumar wrote: >>> Signed-off-by: Ravi Kumar <ravi1.ku...@amd.com> >> >> <...> >> >>> @@ -412,6 +412,12 @@ CONFIG_RTE_PMD_RING_MAX_TX_RINGS=16 >>> CONFIG_RTE_LIBRTE_PMD_SOFTNIC=y >>> >>> # >>> +# Compile AMD PMD >>> +# >>> +CONFIG_RTE_LIBRTE_AXGBE_PMD=y >>> +CONFIG_RTE_LIBRTE_AXGBE_DEBUG_INIT=n >> >> Config file recently updated to have PMD options sorted alphabetically, can >> you please move config option according. >> >> <...> >> >>> +/* The set of PCI devices this driver supports */ >>> +#define AMD_PCI_VENDOR_ID 0x1022 >>> +#define AMD_PCI_AXGBE_DEVICE_ID1 0x1458 #define >>> +AMD_PCI_AXGBE_DEVICE_ID2 0x1459 >> >> More descriptive name can help others to find which devices are supported, >> instead of id1 and id2. >> >> <...> >> >>> +static int >>> +eth_axgbe_dev_init(struct rte_eth_dev *eth_dev) { >>> + PMD_INIT_FUNC_TRACE(); >>> + struct axgbe_port *pdata; >>> + struct rte_pci_device *pci_dev; >>> + >>> + pdata = (struct axgbe_port *)eth_dev->data->dev_private; >>> + pdata->eth_dev = eth_dev; >> >> Since there is already a RTE_PROC_PRIMARY check below, these should be below >> that check because secondary doesn't need to update pdata->eth_dev >> >> <...> >> >>> +RTE_PMD_REGISTER_PCI(net_axgbe, rte_axgbe_pmd); >>> +RTE_PMD_REGISTER_PCI_TABLE(net_axgbe, pci_id_axgbe_map); >>> +RTE_PMD_REGISTER_KMOD_DEP(net_axgbe, "* igb_uio | uio_pci_generic"); >> >> Is vfio-pci intentionally not included as dependency? >> >>> + >>> +RTE_INIT(axgbe_init_log); >>> +static void >>> +axgbe_init_log(void) >>> +{ >>> + axgbe_logtype_init = rte_log_register("pmd.axgbe.init"); >> >> The log string syntax updated, now it should be "pmd.net.axgbe.*" >> >> <...> >> >>> +#ifdef RTE_LIBRTE_AXGBE_DEBUG_INIT >>> +#define PMD_INIT_FUNC_TRACE() PMD_INIT_LOG(DEBUG, " >>") #else >>> +#define PMD_INIT_FUNC_TRACE() do { } while (0) #endif >> >> Do we need this config option? The idea of dynamic logging is get rid of >> them. >> If you want to control trace logs seperately, you can create a new logtype >> for it can control dynamically. As long as it is not in datapath we can >> remove log config options. >> >>> +extern int axgbe_logtype_driver; >>> +#define PMD_DRV_LOG_RAW(level, fmt, args...) \ >>> + rte_log(RTE_LOG_ ## level, axgbe_logtype_driver, "%s(): " fmt, \ >>> + __func__, ## args) >>> + >>> +#define PMD_DRV_LOG(level, fmt, args...) \ >>> + PMD_DRV_LOG_RAW(level, fmt "\n", ## args) >> >> Do you need interim PMD_DRV_LOG_RAW? Why not directly define PMD_DRV_LOG? >> >> <...> > > Hi Ferruh, > > Thanks a lot for the review comments. We are working on these and will upload > a patch-set that will address these and the previous SPDX licensing comments.
Hi Ravi, Is there any update on PMD, will it able to make before integration deadline? Thanks, ferruh > > Regards, > Ravi >
