> On 6/6/2019 12:06 PM, Ziyang Xuan wrote:
> > Add build and doc files, and common code files.
> >
> > Signed-off-by: Ziyang Xuan <xuanziya...@huawei.com>
> > ---
> > MAINTAINERS | 9 +
> > config/common_base | 5 +
> > config/common_linux | 5 +
> > config/defconfig_arm-armv7a-linuxapp-gcc | 1 +
> > config/defconfig_i686-native-linuxapp-gcc | 5 +
> > config/defconfig_i686-native-linuxapp-icc | 5 +
> > config/defconfig_ppc_64-power8-linuxapp-gcc | 1 +
> > config/defconfig_x86_64-native-linuxapp-icc | 5 +
> > config/defconfig_x86_x32-native-linuxapp-gcc | 5 +
> > doc/guides/nics/features/hinic.ini | 37 +
> > doc/guides/nics/hinic.rst | 55 ++
>
> Can you also update the release notes documentation
> (doc/guides/rel_notes/release_19_08.rst) to announce new driver?
OK, I will update the release note documentation at next version patchset.
>
> <...>
>
> > @@ -0,0 +1,55 @@
> > +.. SPDX-License-Identifier: BSD-3-Clause
> > + Copyright(c) 2017 Huawei Technologies Co., Ltd
> > +
> > +
> > +HINIC Poll Mode Driver
> > +======================
> > +
> > +The hinic PMD (librte_pmd_hinic) provides poll mode driver support
> > +for
> > +25 Gbps Huawei Intelligent PCIE Network Adapters based on the Huawei
> > +Ethernet Controller Hi1822.
>
> Can you please provide a link to product documentation?
OK, I will post the link of our product documentation in the hinic.rst at next
patchset.
>
> > +
> > +
> > +Features
> > +--------
> > +
> > +- Multi arch support: x86_64, ARMv8.
> > +- Multiple queues for TX and RX
> > +- Receiver Side Scaling (RSS)
> > +- MAC/VLAN filtering
> > +- Checksum offload
> > +- TSO offload
> > +- Promiscuous mode
> > +- Port hardware statistics
> > +- Jumbo frames
>
> Jumbo frame marked as not supported in above .ini file?
I will remove it here.
>
> > +- Link state information
> > +- Link flow control
> > +- Scattered and gather for TX and RX
> > +
> > +Prerequisites
> > +-------------
> > +
> > +- Follow the DPDK :ref:`Getting Started Guide for Linux <linux_gsg>` to
> setup the basic DPDK environment.
> > +
> > +Requires firmware 1.6.2.5
>
> Please provide some link for this prerequisite.
>
> <...>
>
> > +#
> > +# Add extra flags for base driver files (also known as shared code) #
> > +to disable warnings # ifeq ($(CONFIG_RTE_TOOLCHAIN_ICC),y)
> > +CFLAGS_BASE_DRIVER = -diag-disable 593 else CFLAGS_BASE_DRIVER =
> > +-Wno-sign-compare CFLAGS_BASE_DRIVER += -Wno-unused-parameter
> > +CFLAGS_BASE_DRIVER += -Wno-strict-aliasing CFLAGS_BASE_DRIVER +=
> > +-Wno-missing-field-initializers CFLAGS_BASE_DRIVER +=
> > +-Wno-pointer-to-int-cast endif
>
> Since icc is not supported (yet), can drop this part.
The part of icc can be delete. And I will delete the rest of compilation shield
options by fix our codes.
>
> > +
> > +OBJS_BASE_DRIVER=$(sort $(patsubst %.c,%.o,$(notdir $(wildcard
> > +$(SRCDIR)/base/*.c)))) $(foreach obj, $(OBJS_BASE_DRIVER), $(eval
> > +CFLAGS_$(obj)+=$(CFLAGS_BASE_DRIVER)))
>
> And when icc part removed, I think nothing is left to set
> "CFLAGS_BASE_DRIVER", so this part can be removed too.
>
> > +
> > +VPATH += $(SRCDIR)/base
> > +
> > +#
> > +# all source are stored in SRCS-y
> > +#
> > +SRCS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += hinic_pmd_api_cmd.c
> > +SRCS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += hinic_pmd_cfg.c
> > +SRCS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += hinic_pmd_cmdq.c
> > +SRCS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += hinic_pmd_eqs.c
> > +SRCS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += hinic_pmd_hwdev.c
> > +SRCS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += hinic_pmd_hwif.c
> > +SRCS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += hinic_pmd_mgmt.c
> > +SRCS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += hinic_pmd_niccfg.c
> > +SRCS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += hinic_pmd_nicio.c
> > +SRCS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += hinic_pmd_qp.c
> > +SRCS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += hinic_pmd_wq.c
> > +
> > +SRCS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += hinic_pmd_dpdev.c
> > +SRCS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += hinic_pmd_ethdev.c
> > +#SRCS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += hinic_pmd_rx.c
> > +#SRCS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += hinic_pmd_tx.c
>
> Please remove commented lines.
OK.
>
> > +
> > +# this lib depends upon:
> > +DEPDIRS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += lib/librte_eal
> > +lib/librte_ether
> > +DEPDIRS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += lib/librte_mempool
> > +lib/librte_mbuf
> > +DEPDIRS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += lib/librte_net
> > +lib/librte_hash
> > +DEPDIRS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += lib/librte_kvargs
> > +DEPDIRS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += lib/librte_net
> > +DEPDIRS-$(CONFIG_RTE_LIBRTE_HINIC_PMD) += lib/librte_ring
>
> We stop using DEPDIRS, below LDLIBS is replacing it, can drop above lines.
OK.
>
> <...>
>
> > +headers = [
> > + 'hinic_compat.h',
> > + 'hinic_csr.h',
> > + 'hinic_ctx_def.h',
> > + 'hinic_pmd_api_cmd.h',
> > + 'hinic_pmd_cfg.h',
> > + 'hinic_pmd_cmdq.h',
> > + 'hinic_pmd_dpdev.h',
> > + 'hinic_pmd_eqs.h',
> > + 'hinic_pmd_hwdev.h',
> > + 'hinic_pmd_hw.h',
> > + 'hinic_pmd_hwif.h',
> > + 'hinic_pmd_hw_mgmt.h',
> > + 'hinic_pmd_mgmt.h',
> > + 'hinic_pmd_mgmt_interface.h',
> > + 'hinic_pmd_niccfg.h',
> > + 'hinic_pmd_nic.h',
> > + 'hinic_pmd_nicio.h',
> > + 'hinic_pmd_qp.h',
> > + 'hinic_pmd_wq.h',
> > + 'hinic_port_cmd.h',
> > + 'hinic_qe_def.h',
> > +]
>
> Does headers need to be defined for the PMD?
I will delete these.
>
> <...>
>
> > +
> > +void *hinic_dma_mem_zalloc(void *dev, size_t size, dma_addr_t
> *dma_handle,
> > + unsigned int flag, unsigned int align)
>
> Why not many of the functions are not 'static'? Please make them 'static' as
> much as possible.
OK.
>
> <...>
>
> > +int32_t hinic_nic_dev_create(struct rte_eth_dev *eth_dev) {
> > + struct hinic_nic_dev *nic_dev =
> > +
> HINIC_ETH_DEV_TO_PRIVATE_NIC_DEV(eth_dev);
> > + int rc;
> > +
> > + nic_dev->hwdev =
> > + (struct hinic_hwdev *)rte_zmalloc("hinic_hwdev",
> > + sizeof(*nic_dev->hwdev),
> > + RTE_CACHE_LINE_SIZE);
>
> There was a patch by Stephen [1] very recently to remove the casting from
> and to the "void *" which is not required by 'C'.
> To not need similar cleanups later, can you please check them at first place.
>
> [1]https://patches.dpdk.org/project/dpdk/list/?series=4816
OK.
>
> <...>
>
> > +sources = files(
> > + 'hinic_pmd_dpdev.c',
> > + 'hinic_pmd_ethdev.c',
> > +# 'hinic_pmd_rx.c',
> > +# 'hinic_pmd_tx.c'
>
> Please don't have commented code.
I will fix it.
>
> > + )
> > +
> > +deps += 'ethdev'
> > +deps += 'pci'
> > +deps += 'hash'
>
> Is hash a dependency?
Yes, we used hash table in the file hinic_pmd_dpdev.c. It is better: deps +=
['ethdev', 'pci', 'hash']
