Re: [dpdk-dev] [PATCH v7 1/5] net/enetfec: introduce NXP ENETFEC driver

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 11/3/2021 7:20 PM, Apeksha Gupta wrote:
> ENETFEC (Fast Ethernet Controller) is a network poll mode driver
> for NXP SoC i.MX 8M Mini.
> 
> This patch adds skeleton for enetfec driver with probe function.
> 
> Signed-off-by: Sachin Saxena <sachin.saxena@nxp.com>
> Signed-off-by: Apeksha Gupta <apeksha.gupta@nxp.com>
> ---
> v7:
>   - Fix compilation
>   - code cleanup
> 
> v6:
>   - Fix document build errors
> ---
>   MAINTAINERS                            |   7 ++
>   doc/guides/nics/enetfec.rst            | 131 +++++++++++++++++++++++++
>   doc/guides/nics/features/enetfec.ini   |   9 ++
>   doc/guides/nics/index.rst              |   1 +
>   doc/guides/rel_notes/release_21_11.rst |   6 +-
>   drivers/net/enetfec/enet_ethdev.c      |  85 ++++++++++++++++
>   drivers/net/enetfec/enet_ethdev.h      |  58 +++++++++++
>   drivers/net/enetfec/enet_pmd_logs.h    |  31 ++++++
>   drivers/net/enetfec/meson.build        |   9 ++
>   drivers/net/enetfec/version.map        |   3 +
>   drivers/net/meson.build                |   2 +-
>   11 files changed, 340 insertions(+), 2 deletions(-)
>   create mode 100644 doc/guides/nics/enetfec.rst
>   create mode 100644 doc/guides/nics/features/enetfec.ini
>   create mode 100644 drivers/net/enetfec/enet_ethdev.c
>   create mode 100644 drivers/net/enetfec/enet_ethdev.h
>   create mode 100644 drivers/net/enetfec/enet_pmd_logs.h
>   create mode 100644 drivers/net/enetfec/meson.build
>   create mode 100644 drivers/net/enetfec/version.map
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0e5951f8f1..d000eb81af 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -882,6 +882,13 @@ F: drivers/net/enetc/
>   F: doc/guides/nics/enetc.rst
>   F: doc/guides/nics/features/enetc.ini
>   
> +NXP enetfec
> +M: Apeksha Gupta <apeksha.gupta@nxp.com>
> +M: Sachin Saxena <sachin.saxena@nxp.com>
> +F: drivers/net/enetfec/
> +F: doc/guides/nics/enetfec.rst
> +F: doc/guides/nics/features/enetfec.ini
> +
>   NXP pfe
>   M: Gagandeep Singh <g.singh@nxp.com>
>   F: doc/guides/nics/pfe.rst
> diff --git a/doc/guides/nics/enetfec.rst b/doc/guides/nics/enetfec.rst
> new file mode 100644
> index 0000000000..dfcd032098
> --- /dev/null
> +++ b/doc/guides/nics/enetfec.rst
> @@ -0,0 +1,131 @@
> +.. SPDX-License-Identifier: BSD-3-Clause
> +   Copyright 2021 NXP
> +
> +ENETFEC Poll Mode Driver
> +========================
> +
> +The ENETFEC NIC PMD (**librte_net_enetfec**) provides poll mode driver
> +support for the inbuilt NIC found in the ** NXP i.MX 8M Mini** SoC.
> +
> +More information can be found at NXP Official Website
> +<https://www.nxp.com/products/processors-and-microcontrollers/arm-processors/i-mx-applications-processors/i-mx-8-processors/i-mx-8m-mini-arm-cortex-a53-cortex-m4-audio-voice-video:i.MX8MMINI>
> +
> +ENETFEC
> +-------
> +
> +This section provides an overview of the NXP ENETFEC and how it is
> +integrated into the DPDK.
> +
> +Contents summary
> +
> +- ENETFEC overview
> +- ENETFEC features
> +- Supported ENETFEC SoCs
> +- Prerequisites
> +- Driver compilation and testing
> +- Limitations
> +
> +ENETFEC Overview
> +~~~~~~~~~~~~~~~~
> +The i.MX 8M Mini Media Applications Processor is built to achieve both
> +high performance and low power consumption. ENETFEC PMD is a hardware
> +programmable packet forwarding engine to provide high performance
> +Ethernet interface. It has only 1 GB Ethernet interface with RJ45
> +connector.
> +
> +The diagram below shows a system level overview of ENETFEC:
> +
> +  .. code-block:: console
> +
> +   =====================================================
> +   Userspace
> +        +-----------------------------------------+
> +        |             ENETFEC Driver              |
> +        |        +-------------------------+      |
> +        |        | virtual ethernet device |      |
> +        +-----------------------------------------+
> +                          ^   |
> +                          |   |
> +                          |   |
> +                     RXQ  |   |	TXQ
> +                          |   |
> +                          |   v
> +   =====================================================
> +   Kernel Space
> +                       +---------+
> +                       | fec-uio |
> +   ====================+=========+======================
> +   Hardware
> +        +-----------------------------------------+
> +        |           i.MX 8M MINI EVK              |
> +        |               +-----+                   |
> +        |               | MAC |                   |
> +        +---------------+-----+-------------------+
> +                        | PHY |
> +                        +-----+
> +
> +ENETFEC Ethernet driver is traditional DPDK PMD driver running in the
> +userspace.'fec-uio' is the kernel driver. The MAC and PHY are the hardware
> +blocks. ENETFEC PMD uses standard UIO interface to access kernel for PHY
> +initialisation and for mapping the allocated memory of register & buffer
> +descriptor with DPDK which gives access to non-cacheable memory for buffer
> +descriptor. net_enetfec is logical Ethernet interface, created by ENETFEC
> +driver.
> +
> +- ENETFEC driver registers the device in virtual device driver.
> +- RTE framework scans and will invoke the probe function of ENETFEC driver.
> +- The probe function will set the basic device registers and also setups BD rings.
> +- On packet Rx the respective BD Ring status bit is set which is then used for
> +  packet processing.
> +- Then Tx is done first followed by Rx via logical interfaces.
> +
> +ENETFEC Features
> +~~~~~~~~~~~~~~~~~
> +
> +- Linux
> +- ARMv8
> +
> +Supported ENETFEC SoCs
> +~~~~~~~~~~~~~~~~~~~~~~
> +
> +- i.MX 8M Mini
> +
> +Prerequisites
> +~~~~~~~~~~~~~
> +
> +There are three main pre-requisites for executing ENETFEC PMD on a i.MX 8M Mini
> +compatible board:
> +
> +1. **ARM 64 Tool Chain**
> +
> +   For example, the `*aarch64* Linaro Toolchain <https://releases.linaro.org/components/toolchain/binaries/7.4-2019.02/aarch64-linux-gnu/gcc-linaro-7.4.1-2019.02-x86_64_aarch64-linux-gnu.tar.xz>`_.
> +
> +2. **Linux Kernel**
> +
> +  It can be obtained from `NXP's Github hosting <https://source.codeaurora.org/external/qoriq/qoriq-components/linux>`_.
> +
> +  .. note::
> +
> +  Branch is 'lf-5.10.y'
> +
> +3. **Rootfile system**
> +
> +   Any *aarch64* supporting filesystem can be used. For example,
> +   Ubuntu 18.04 LTS (Bionic) or 20.04 LTS(Focal) userland which can be obtained
> +   from `here <http://cdimage.ubuntu.com/ubuntu-base/releases/18.04/release/ubuntu-base-18.04.1-base-arm64.tar.gz>`_.
> +
> +4. The Ethernet device will be registered as virtual device, so ENETFEC has dependency on
> +   **rte_bus_vdev** library and it is mandatory to use `--vdev` with value `net_enetfec` to
> +   run DPDK application.
> +
> +Driver compilation and testing
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +Follow instructions available in the document
> +:ref:`compiling and testing a PMD for a NIC <pmd_build_and_test>`
> +to launch **dpdk-testpmd**
> +
> +Limitations
> +~~~~~~~~~~~
> +
> +- Multi queue is not supported.

The question on the above limitation is went unanswered in previous version,
instead of not replying and sending a new version, it would be better if you
can answer the questions to have some common understanding.

Back to the question,

in 'enetfec_eth_info()', 'max_rx_queues'/'max_tx_queues' set to 'ENETFEC_MAX_Q'
and
#define ENETFEC_MAX_Q          3

Also file comment says:
* ENETFEC with AVB IP can support maximum 3 rx and tx queues.

The device reports and documents 3 queues are supported, but there is a
limitation documented saying multi queue is not supported, which one is
correct?

> diff --git a/doc/guides/nics/features/enetfec.ini b/doc/guides/nics/features/enetfec.ini
> new file mode 100644
> index 0000000000..bdfbdbd9d4
> --- /dev/null
> +++ b/doc/guides/nics/features/enetfec.ini
> @@ -0,0 +1,9 @@
> +;
> +; Supported features of the 'enetfec' network poll mode driver.
> +;
> +; Refer to default.ini for the full list of available PMD features.
> +;
> +[Features]
> +Linux		     = Y
> +ARMv8		     = Y
> +Usage doc	     = Y
> diff --git a/doc/guides/nics/index.rst b/doc/guides/nics/index.rst
> index 784d5d39f6..777fdab4a0 100644
> --- a/doc/guides/nics/index.rst
> +++ b/doc/guides/nics/index.rst
> @@ -26,6 +26,7 @@ Network Interface Controller Drivers
>       e1000em
>       ena
>       enetc
> +    enetfec
>       enic
>       fm10k
>       hinic
> diff --git a/doc/guides/rel_notes/release_21_11.rst b/doc/guides/rel_notes/release_21_11.rst
> index 502cc5ceb2..aed380c21f 100644
> --- a/doc/guides/rel_notes/release_21_11.rst
> +++ b/doc/guides/rel_notes/release_21_11.rst
> @@ -20,7 +20,6 @@ DPDK Release 21.11
>         ninja -C build doc
>         xdg-open build/doc/guides/html/rel_notes/release_21_11.html
>   
> -

unrelated change.

>   New Features
>   ------------
>   
> @@ -135,6 +134,11 @@ New Features
>   
>     Added an ethdev API which can help users get device configuration.
>   
> +* **Added NXP ENETFEC PMD.**
> +
> +  Added the new ENETFEC driver for the NXP IMX8MMEVK platform. See the
> +  :doc:`../nics/enetfec` NIC driver guide for more details on this new driver.
> +

PMDs are ordered by vendor name, can you pleae move this block below
'Mellanox' PMD update.

>   * **Updated AF_XDP PMD.**
>   
>     * Disabled secondary process support.
> diff --git a/drivers/net/enetfec/enet_ethdev.c b/drivers/net/enetfec/enet_ethdev.c
> new file mode 100644
> index 0000000000..a6c4bcbf2e
> --- /dev/null
> +++ b/drivers/net/enetfec/enet_ethdev.c
> @@ -0,0 +1,85 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright 2020-2021 NXP
> + */
> +
> +#include <stdio.h>
> +#include <fcntl.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <errno.h>
> +#include <sys/mman.h>
> +#include <rte_kvargs.h>
> +#include <ethdev_vdev.h>
> +#include <rte_bus_vdev.h>
> +#include <rte_dev.h>
> +#include <rte_ether.h>
> +#include "enet_pmd_logs.h"
> +#include "enet_ethdev.h"
> +
> +#define ENETFEC_NAME_PMD                net_enetfec
> +#define ENETFEC_CDEV_INVALID_FD         -1

Is this macro used at all?

> +
> +static int
> +enetfec_eth_init(struct rte_eth_dev *dev)
> +{
> +	rte_eth_dev_probing_finish(dev);
> +	return 0;
> +}
> +
> +static int
> +pmd_enetfec_probe(struct rte_vdev_device *vdev)
> +{
> +	struct rte_eth_dev *dev = NULL;
> +	struct enetfec_private *fep;
> +	const char *name;
> +	int rc;
> +
> +	name = rte_vdev_device_name(vdev);
> +	if (name == NULL)
> +		return -EINVAL;

At this function 'name' shouldn't be null, I think you can drop
the check.
But can you pleae double check to be sure?

> +	ENETFEC_PMD_LOG(INFO, "Initializing pmd_fec for %s", name);
> +
> +	dev = rte_eth_vdev_allocate(vdev, sizeof(*fep));
> +	if (dev == NULL)
> +		return -ENOMEM;
> +
> +	/* setup board info structure */
> +	fep = dev->data->dev_private;
> +	fep->dev = dev;
> +	rc = enetfec_eth_init(dev);
> +	if (rc)
> +		goto failed_init;
> +
> +	return 0;
> +
> +failed_init:
> +	ENETFEC_PMD_ERR("Failed to init");
> +	return rc;
> +}
> +
> +static int
> +pmd_enetfec_remove(struct rte_vdev_device *vdev)
> +{
> +	struct rte_eth_dev *eth_dev = NULL;
> +	int ret;
> +
> +	/* find the ethdev entry */
> +	eth_dev = rte_eth_dev_allocated(rte_vdev_device_name(vdev));
> +	if (eth_dev == NULL)
> +		return -ENODEV;
> +
> +	ret = rte_eth_dev_release_port(eth_dev);
> +	if (ret != 0)
> +		return -EINVAL;
> +
> +	ENETFEC_PMD_INFO("Release enetfec sw device");
> +	return 0;
> +}
> +
> +static struct rte_vdev_driver pmd_enetfec_drv = {
> +	.probe = pmd_enetfec_probe,
> +	.remove = pmd_enetfec_remove,
> +};
> +
> +RTE_PMD_REGISTER_VDEV(ENETFEC_NAME_PMD, pmd_enetfec_drv);
> +RTE_LOG_REGISTER_DEFAULT(enetfec_logtype_pmd, NOTICE);
> diff --git a/drivers/net/enetfec/enet_ethdev.h b/drivers/net/enetfec/enet_ethdev.h
> new file mode 100644
> index 0000000000..0e4558dd86
> --- /dev/null
> +++ b/drivers/net/enetfec/enet_ethdev.h
> @@ -0,0 +1,58 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright 2020-2021 NXP
> + */
> +
> +#ifndef __ENETFEC_ETHDEV_H__
> +#define __ENETFEC_ETHDEV_H__
> +
> +/*
> + * ENETFEC with AVB IP can support maximum 3 rx and tx queues.
> + */
> +
> +#define ENETFEC_MAX_Q		3
> +
> +/* Buffer descriptors of FEC are used to track the ring buffers. Buffer
> + * descriptor base is x_bd_base. Currently available buffer are x_cur
> + * and x_cur. where x is rx or tx. Current buffer is tracked by dirty_tx
> + * that is sent by the controller.
> + * The tx_cur and dirty_tx are same in completely full and empty
> + * conditions. Actual condition is determined by empty & ready bits.

is above comment correct?

Where are mentioned 'x_bd_base', 'rx_cur', 'tx_cur', 'dirty_tx', etc...

> + */
> +struct enetfec_private {
> +	struct rte_eth_dev	*dev;
> +	struct rte_eth_stats	stats;
> +	struct rte_mempool	*pool;
> +	uint16_t		max_rx_queues;
> +	uint16_t		max_tx_queues;
> +	unsigned int		total_tx_ring_size;
> +	unsigned int		total_rx_ring_size;
> +	bool			bufdesc_ex;
> +	unsigned int		tx_align;
> +	unsigned int		rx_align;
> +	int			full_duplex;
> +	unsigned int		phy_speed;
> +	uint32_t		quirks;
> +	int			flag_csum;
> +	int			flag_pause;
> +	int			flag_wol;
> +	bool			rgmii_txc_delay;
> +	bool			rgmii_rxc_delay;
> +	int			link;
> +	void			*hw_baseaddr_v;
> +	uint64_t		hw_baseaddr_p;
> +	void			*bd_addr_v;
> +	uint64_t		bd_addr_p;
> +	uint64_t		bd_addr_p_r[ENETFEC_MAX_Q];
> +	uint64_t		bd_addr_p_t[ENETFEC_MAX_Q];
> +	void			*dma_baseaddr_r[ENETFEC_MAX_Q];
> +	void			*dma_baseaddr_t[ENETFEC_MAX_Q];
> +	uint64_t		cbus_size;
> +	unsigned int		reg_size;
> +	unsigned int		bd_size;
> +	int			hw_ts_rx_en;
> +	int			hw_ts_tx_en;
> +	struct enetfec_priv_rx_q *rx_queues[ENETFEC_MAX_Q];
> +	struct enetfec_priv_tx_q *tx_queues[ENETFEC_MAX_Q];

most of these fields are not used at all, why not construct the struct
by adding the fields as you use them? This prevent clutter to remain,
like 'flag_wol' above, does it used at all in the driver?

> +};
> +
> +#endif /*__ENETFEC_ETHDEV_H__*/
> diff --git a/drivers/net/enetfec/enet_pmd_logs.h b/drivers/net/enetfec/enet_pmd_logs.h
> new file mode 100644
> index 0000000000..e7b3964a0e
> --- /dev/null
> +++ b/drivers/net/enetfec/enet_pmd_logs.h
> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright 2020-2021 NXP
> + */
> +
> +#ifndef _ENETFEC_LOGS_H_
> +#define _ENETFEC_LOGS_H_
> +
> +extern int enetfec_logtype_pmd;
> +
> +/* PMD related logs */
> +#define ENETFEC_PMD_LOG(level, fmt, args...) \
> +	rte_log(RTE_LOG_ ## level, enetfec_logtype_pmd, "\nfec_net: %s()" \
> +		fmt "\n", __func__, ##args)
> +
> +#define PMD_INIT_FUNC_TRACE() ENET_PMD_LOG(DEBUG, " >>")
> +
> +#define ENETFEC_PMD_DEBUG(fmt, args...) \
> +	ENETFEC_PMD_LOG(DEBUG, fmt, ## args)
> +#define ENETFEC_PMD_ERR(fmt, args...) \
> +	ENETFEC_PMD_LOG(ERR, fmt, ## args)
> +#define ENETFEC_PMD_INFO(fmt, args...) \
> +	ENETFEC_PMD_LOG(INFO, fmt, ## args)
> +
> +#define ENETFEC_PMD_WARN(fmt, args...) \
> +	ENETFEC_PMD_LOG(WARNING, fmt, ## args)
> +
> +/* DP Logs, toggled out at compile time if level lower than current level */
> +#define ENETFEC_DP_LOG(level, fmt, args...) \
> +	RTE_LOG_DP(level, PMD, fmt, ## args)
> +
> +#endif /* _ENETFEC_LOGS_H_ */
> diff --git a/drivers/net/enetfec/meson.build b/drivers/net/enetfec/meson.build
> new file mode 100644
> index 0000000000..6d6c64c94b
> --- /dev/null
> +++ b/drivers/net/enetfec/meson.build
> @@ -0,0 +1,9 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright 2021 NXP
> +
> +if not is_linux
> +	build = false
> +	reason = 'only supported on linux'
> +endif
> +
> +sources = files('enet_ethdev.c')
> diff --git a/drivers/net/enetfec/version.map b/drivers/net/enetfec/version.map
> new file mode 100644
> index 0000000000..b66517b171
> --- /dev/null
> +++ b/drivers/net/enetfec/version.map
> @@ -0,0 +1,3 @@
> +DPDK_22 {
> +        local: *;
> +};
> diff --git a/drivers/net/meson.build b/drivers/net/meson.build
> index bcf488f203..ac294d8507 100644
> --- a/drivers/net/meson.build
> +++ b/drivers/net/meson.build
> @@ -12,13 +12,13 @@ drivers = [
>           'bnx2x',
>           'bnxt',
>           'bonding',
> -        'cnxk',

Better to not remove competitor drivers ;)

>           'cxgbe',
>           'dpaa',
>           'dpaa2',
>           'e1000',
>           'ena',
>           'enetc',
> +        'enetfec',
>           'enic',
>           'failsafe',
>           'fm10k',
>