Hi,
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Friday, April 05, 2019 3:38
> To: Xu, Rosen <rosen...@intel.com>; dev@dpdk.org
> Cc: Zhang, Tianfei <tianfei.zh...@intel.com>; Wei, Dan
> <dan....@intel.com>; Pei, Andy <andy....@intel.com>; Yang, Qiming
> <qiming.y...@intel.com>; Wang, Haiyue <haiyue.w...@intel.com>; Chen,
> Santos <santos.c...@intel.com>; Zhang, Zhang <zhang.zh...@intel.com>;
> Lomartire, David <david.lomart...@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...@intel.com>
> > Signed-off-by: Andy Pei <andy....@intel.com>
> > Signed-off-by: Dan Wei <dan....@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.
