[dpdk-dev] [PATCH v5 03/14] drivers/net/ipn3ke: add IPN3KE ethdev PMD driver

[Xu, Rosen]

Hi,

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Friday, April 05, 2019 3:38
> To: Xu, Rosen <rosen.xu at intel.com>; dev at dpdk.org
> Cc: Zhang, Tianfei <tianfei.zhang at intel.com>; Wei, Dan
> <dan.wei at intel.com>; Pei, Andy <andy.pei at intel.com>; Yang, Qiming
> <qiming.yang at intel.com>; Wang, Haiyue <haiyue.wang at intel.com>; Chen,
> Santos <santos.chen at intel.com>; Zhang, Zhang <zhang.zhang at intel.com>;
> Lomartire, David <david.lomartire at intel.com>
> Subject: Re: [PATCH v5 03/14] drivers/net/ipn3ke: add IPN3KE ethdev PMD
> driver
> 
> On 4/3/2019 12:47 PM, Rosen Xu wrote:
> > Add Intel FPGA Acceleration NIC IPN3KE ethdev PMD driver.
> >
> > Signed-off-by: Rosen Xu <rosen.xu at intel.com>
> > Signed-off-by: Andy Pei <andy.pei at intel.com>
> > Signed-off-by: Dan Wei <dan.wei at intel.com>
> <...>
> 
> > +Config File Options
> > +~~~~~~~~~~~~~~~~~~~
> > +
> > +The following options can be modified in the ``config`` file.
> > +Please note that enabling debugging options may affect system
> performance.
> > +
> > +- ``CONFIG_RTE_LIBRTE_IPN3KE_PMD`` (default ``n``)
> 
> It looks like enabled by default in config file, which one is the intention.

Fixed in v6.

> > +
> > +  Toggle compilation of the ``librte_pmd_ipn3ke`` driver.
> > +
> > +Runtime Config Options
> > +~~~~~~~~~~~~~~~~~~~~~~
> > +
> > +- ``AFU name``
> > +
> > +  AFU name identifies which AFU is used by IPN3KE.
> > +
> > +- ``FPGA Acceleration list``
> > +
> > +  For IPN3KE FPGA can provide different bitstream, different
> > + bitstream includes different  Acceleration, so users need to identify
> which Acceleration is used.
> > +
> > +- ``I40e PF name list``
> > +
> > +  Users need to bind FPGA LineSidePort to FVL PF. So I40e PF name
> > + list should be involved in  startup command.
> 
> Can you please document the actual options themselves here?

Okay, added in v6.

> <...>
> 
> > +static int ipn3ke_indirect_read(struct ipn3ke_hw *hw,
> > +					 uint32_t *rd_data,
> > +					 uint32_t addr,
> > +					 uint32_t dev_sel,
> > +					 uint32_t eth_group_sel)
> 
> Can you please apply the coding convention:
> static int
> <function name> (parameters)
> {
> 
> Also how parameters aligned here?
> according coding convention it should be:
> 
> static int
> ipn3ke_indirect_read(struct ipn3ke_hw *hw, uint32_t *rd_data, uint32_t
> addr,
> 	uint32_t dev_sel, uint32_t eth_group_sel)
> 
> Can you please fix this for all functions?
> 
> <...>

Okay, checked all functions and fixed mismatch in v6.
 
> > +static int
> > +ipn3ke_cfg_parse_i40e_pf_ethdev(const char *afu_name, const char
> > +*pf_name)
> 
> Please fix indentation.

Okay, fixed in v6.

> <...>
> 
> > +RTE_PMD_REGISTER_VDEV(ipn3ke_cfg, ipn3ke_cfg_driver);
> > +RTE_PMD_REGISTER_ALIAS(ipn3ke_cfg, ipn3ke_cfg);
> 
> Creating alias with same name :) Please drop.

Okay, removed in v6 and checked in platform.

> <...>
> 
> > +RTE_INIT(ipn3ke_afu_init_log);
> > +static void
> > +ipn3ke_afu_init_log(void)
> 
> Can merge these lines into single:
> RTE_INIT(ipn3ke_afu_init_log)
> 
> It looks like I did same comment in previous versions as well, can you please
> check previous reviews for missed comments.

Sorry, I miss this comment.
Fixed in v6.

> > +{
> > +	ipn3ke_afu_logtype = rte_log_register("driver.afu.ipn3ke");
> 
> "pmd.afu.ipn3ke"

It seems better, so modified in v6.

> <...>
> 
> > +#include <rte_cycles.h>
> > +#include <rte_bus_ifpga.h>
> > +#include <rte_tm_driver.h>
> > +
> > +struct ipn3ke_rpst;
> 
> Is this forward decleration needed?

No need, so removed in v6.

> <...>
> 
> > @@ -0,0 +1,6 @@
> > +# SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2019 Intel
> > +Corporation
> > +
> > +allow_experimental_apis = true
> 
> Can you please add list of experimental APIs called in a comment as done in
> makefile

Okay, added in v6.

> <...>
> 
> > @@ -12,6 +12,8 @@
> >  # The PCI base class for all devices
> >  network_class = {'Class': '02', 'Vendor': None, 'Device': None,
> >                      'SVendor': None, 'SDevice': None}
> > +ifpga_class = {'Class': '12', 'Vendor': '8086', 'Device': 'bcc0,09c4,0b30',
> > +                    'SVendor': None, 'SDevice': None}
> 
> Only '0x0B30' added in this patch, right?

Yep, only keep '0x0B30' and remove others.

> If you can make this seperate patch, with older device ids, it becomes
> backportable, and in this patch you can add new ids.

Removed others, thanks.

> Should add VF ids, 0x0B31 etc... ?

VF supporting has not been released, but it will upstream in next release cycle.
So for this release, no VF ids.