Re: [dpdk-dev] [PATCH 1/4] drivers/net/enetfec: Introduce NXP ENETFEC driver

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 4/30/21 7:34 AM, Apeksha Gupta wrote:
> ENET fec (Fast Ethernet Controller) is a network poll mode driver
> for NXP SoC imx8mmevk.
> 
> This patch add skeleton for enetfec driver with probe and
> uintialisation functions
> 
> Signed-off-by: Sachin Saxena <sachin.saxena@nxp.com>
> Signed-off-by: Apeksha Gupta <apeksha.gupta@nxp.com>
> ---
>  doc/guides/nics/enetfec.rst          | 121 ++++++++++++++++
>  doc/guides/nics/features/enetfec.ini |   8 ++
>  doc/guides/nics/index.rst            |   1 +
>  drivers/net/enetfec/enet_ethdev.c    |  89 ++++++++++++
>  drivers/net/enetfec/enet_ethdev.h    | 203 +++++++++++++++++++++++++++
>  drivers/net/enetfec/enet_pmd_logs.h  |  31 ++++
>  drivers/net/enetfec/meson.build      |  15 ++
>  drivers/net/enetfec/version.map      |   3 +
>  drivers/net/meson.build              |   1 +
>  9 files changed, 472 insertions(+)
>  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/doc/guides/nics/enetfec.rst b/doc/guides/nics/enetfec.rst
> new file mode 100644
> index 000000000..10f495fb9
> --- /dev/null
> +++ b/doc/guides/nics/enetfec.rst
> @@ -0,0 +1,121 @@
> +.. 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 is a hardware programmable
> +packet forwarding engine to provide high performance Ethernet interface.
> +The diagram below shows a system level overview of ENETFEC:
> +
> +   ====================================================+===============
> +   US   +-----------------------------------------+    | Kernel Space
> +        |                                         |    |
> +        |       ENETFEC Ethernet Driver           |    |
> +        +-----------------------------------------+    |
> +                          ^   |                        |
> +   ENETFEC            RXQ |   | TXQ		       |
> +   PMD                    |   |         	       |
> +                          |   v      		       |   +----------+
> +                     +-------------+                   |   | fec-uio  |
> +                     | net_enetfec |                   |   +----------+
> +                     +-------------+                   |
> +                          ^   |                        |
> +                      TXQ |   | RXQ                    |
> +                          |   |                        |
> +                          |   v                        |
> +    ===================================================+===============
> +         +----------------------------------------+
> +         |                                        |        HW
> +         |  i.MX 8M MINI EVK                      |
> +         |               +-----+                  |
> +         |               | MAC |                  |
> +         +---------------+-----+------------------+
> +			 | PHY |
> +			 +-----+
> +
> +ENETFEC ethernet driver is traditional DPDK PMD driver running in the userspace.

ethernet -> Ethernet

> +The MAC and PHY are the hardware blocks. 'fec-uio' is the uio driver, enetfec PMD
> +uses uio interface to interact with kernel for PHY initialisation and for mapping

uio -> UIO ?

> +the allocated memory of register & BD in kernel with DPDK which gives access to
> +non-cacheble memory for BD. net_enetfec is logical ethernet interface, created by

cacheble -> cacheable
ethernet -> Ethernet

> +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
> +~~~~~~~~~~~~~~~~~
> +
> +- ARMv8
> +
> +Supported ENETFEC SoCs
> +~~~~~~~~~~~~~~~~~~~~~~
> +
> +- i.MX 8M Mini
> +
> +Prerequisites
> +~~~~~~~~~~~~~
> +
> +There are three main pre-requisites for executing ENETfec PMD on a i.MX

I see "enetfec", "ENETfec" and "ENETFEC" referencing driver or
PMD. Please, be consistent.

> +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 bitbucket: <https://bitbucket.sw.nxp.com/projects/LFAC/repos/linux-nxp/commits?until=refs%2Fheads%2Fnet%2Ffec-uio&merges=include>`_.
> +
> +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

ethernet -> Ethernet

> +   **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 **testpmd**
> +
> +Limitations
> +~~~~~~~~~~~
> +
> +- Multi queue is not supported.
> +- Link status is down always.
> +- Single ethernet interface.
> diff --git a/doc/guides/nics/features/enetfec.ini b/doc/guides/nics/features/enetfec.ini
> new file mode 100644
> index 000000000..570069798
> --- /dev/null
> +++ b/doc/guides/nics/features/enetfec.ini
> @@ -0,0 +1,8 @@
> +;
> +; Supported features of the 'enetfec' network poll mode driver.
> +;
> +; Refer to default.ini for the full list of available PMD features.
> +;
> +[Features]
> +ARMv8                = Y
> +Usage doc            = Y
> diff --git a/doc/guides/nics/index.rst b/doc/guides/nics/index.rst
> index 799697caf..93b68e701 100644
> --- a/doc/guides/nics/index.rst
> +++ b/doc/guides/nics/index.rst
> @@ -25,6 +25,7 @@ Network Interface Controller Drivers
>      e1000em
>      ena
>      enetc
> +    enetfec
>      enic
>      fm10k
>      hinic
> diff --git a/drivers/net/enetfec/enet_ethdev.c b/drivers/net/enetfec/enet_ethdev.c
> new file mode 100644
> index 000000000..5fd2dbc2d
> --- /dev/null
> +++ b/drivers/net/enetfec/enet_ethdev.c
> @@ -0,0 +1,89 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright 2021 NXP
> + */
> +#include <stdio.h>
> +#include <fcntl.h>
> +#include <stdlib.h>
> +#include <unistd.h>
> +#include <errno.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 ENET_VDEV_GEM_ID_ARG    "intf"
> +#define ENET_CDEV_INVALID_FD    -1
> +
> +int enetfec_logtype_pmd;
> +
> +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 = -1;
> +
> +	name = rte_vdev_device_name(vdev);
> +	if (name == NULL)
> +		return -EINVAL;
> +	ENET_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)

DPDK coding style requires excplicit check vs 0 and NULL [1].

[1] https://doc.dpdk.org/guides/contributing/coding_style.html#null-pointers

> +		goto failed_init;
> +	return 0;
> +failed_init:
> +	ENET_PMD_ERR("Failed to init");
> +	return rc;
> +}
> +
> +static int
> +pmd_enetfec_remove(struct rte_vdev_device *vdev)
> +{
> +	struct rte_eth_dev *eth_dev = NULL;
> +
> +	/* find the ethdev entry */
> +	eth_dev = rte_eth_dev_allocated(rte_vdev_device_name(vdev));
> +	if (!eth_dev)

eth_dev == NULL

> +		return -ENODEV;
> +
> +	rte_eth_dev_release_port(eth_dev);
> +
> +	ENET_PMD_INFO("Closing sw device\n");

Unnecessary \n since it is added by the macro.

> +	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_PMD_REGISTER_PARAM_STRING(ENETFEC_NAME_PMD, ENET_VDEV_GEM_ID_ARG "=<int>");
> +
> +RTE_INIT(enetfec_pmd_init_log)
> +{
> +	enetfec_logtype_pmd = rte_log_register("pmd.net.enetfec");
> +	if (enetfec_logtype_pmd >= 0)
> +		rte_log_set_level(enetfec_logtype_pmd, RTE_LOG_NOTICE);

rte_log_register_type_and_pick_level() should be used.

> +}
> diff --git a/drivers/net/enetfec/enet_ethdev.h b/drivers/net/enetfec/enet_ethdev.h
> new file mode 100644
> index 000000000..3833a70fc
> --- /dev/null
> +++ b/drivers/net/enetfec/enet_ethdev.h
> @@ -0,0 +1,203 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright 2021 NXP
> + */
> +
> +#ifndef __ENET_ETHDEV_H__
> +#define __ENET_ETHDEV_H__
> +
> +#include <compat.h>
> +#include <rte_ethdev.h>
> +
> +/* ENET with AVB IP can support maximum 3 rx and tx queues.
> + */
> +#define ENET_MAX_Q	3
> +
> +#define BD_LEN			49152
> +#define ENET_TX_FR_SIZE		2048
> +#define MAX_TX_BD_RING_SIZE		512	/* It should be power of 2 */
> +#define MAX_RX_BD_RING_SIZE		512
> +
> +/* full duplex or half duplex */
> +#define HALF_DUPLEX             0x00
> +#define FULL_DUPLEX             0x01
> +#define UNKNOWN_DUPLEX          0xff
> +
> +#define PKT_MAX_BUF_SIZE        1984
> +#define OPT_FRAME_SIZE		(PKT_MAX_BUF_SIZE << 16)
> +#define ETH_ALEN		RTE_ETHER_ADDR_LEN
> +#define ETH_HLEN		RTE_ETHER_HDR_LEN
> +#define VLAN_HLEN		4
> +
> +
> +struct bufdesc {
> +	uint16_t	bd_datlen;  /* buffer data length */
> +	uint16_t	bd_sc;	    /* buffer control & status */
> +	uint32_t	bd_bufaddr; /* buffer address */
> +};
> +
> +struct bufdesc_ex {
> +	struct		bufdesc desc;
> +	uint32_t	bd_esc;
> +	uint32_t	bd_prot;
> +	uint32_t	bd_bdu;
> +	uint32_t	ts;
> +	uint16_t	res0[4];
> +};
> +
> +struct bufdesc_prop {
> +	int que_id;
> +	/* Addresses of Tx and Rx buffers */
> +	struct bufdesc	*base;
> +	struct bufdesc	*last;
> +	struct bufdesc	*cur;
> +	void __iomem	*active_reg_desc;
> +	uint64_t	descr_baseaddr_p;
> +	unsigned short	ring_size;
> +	unsigned char	d_size;
> +	unsigned char	d_size_log2;
> +};
> +
> +struct enetfec_priv_tx_q {
> +	struct bufdesc_prop	bd;
> +	struct rte_mbuf		*tx_mbuf[MAX_TX_BD_RING_SIZE];
> +	struct bufdesc		*dirty_tx;
> +	struct rte_mempool	*pool;
> +	struct enetfec_private	*fep;
> +};
> +
> +struct enetfec_priv_rx_q {
> +	struct bufdesc_prop	bd;
> +	struct rte_mbuf		*rx_mbuf[MAX_RX_BD_RING_SIZE];
> +	struct rte_mempool	*pool;
> +	struct enetfec_private	*fep;
> +};
> +
> +/* Buffer descriptors of FEC are used to track the ring buffers. Buffer

Standalone FEC is used in DPDK ask Forward Error correction.
I think it is always better to use in the driver together
with ENET prefix.

> + * 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 determine by empty & ready bits.
> + */
> +struct enetfec_private {
> +	struct rte_eth_dev	*dev;
> +	struct rte_eth_stats	stats;
> +	struct rte_mempool	*pool;
> +
> +	struct enetfec_priv_rx_q *rx_queues[ENET_MAX_Q];
> +	struct enetfec_priv_tx_q *tx_queues[ENET_MAX_Q];
> +	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;
> +	u_int32_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[ENET_MAX_Q];
> +	uint64_t	bd_addr_p_t[ENET_MAX_Q];
> +	void		*dma_baseaddr_r[ENET_MAX_Q];
> +	void		*dma_baseaddr_t[ENET_MAX_Q];
> +	uint64_t	cbus_size;
> +	unsigned int	reg_size;
> +	unsigned int	bd_size;
> +	int		hw_ts_rx_en;
> +	int		hw_ts_tx_en;
> +};
> +
> +#define writel(v, p) ({*(volatile unsigned int *)(p) = (v); })
> +#define readl(p) rte_read32(p)
> +
> +static __always_inline
> +void __read_once_size(volatile void *p, void *res, int size)

Please, move void to the previous line to be consistent
with return type placement when function is implemented.

> +{
> +	switch (size) {
> +	case 1:
> +		*(__u8 *)res = *(volatile __u8 *)p;
> +		break;
> +	case 2:
> +		*(__u16 *)res = *(volatile __u16 *)p;
> +		break;
> +	case 4:
> +		*(__u32 *)res = *(volatile __u32 *)p;
> +		break;
> +	case 8:
> +		*(__u64 *)res = *(volatile __u64 *)p;
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
> +#define __READ_ONCE(x)\
> +({\
> +	union { typeof(x) __val; char __c[1]; } __u;\
> +	 __read_once_size(&(x), __u.__c, sizeof(x));\
> +	 __u.__val;\
> +})
> +#ifndef READ_ONCE
> +#define READ_ONCE(x) __READ_ONCE(x)
> +#endif
> +
> +static inline struct
> +bufdesc *enet_get_nextdesc(struct bufdesc *bdp,

Really strange line split. May be:
static inline struct bufdesc *
enet_get_nextdesc(struct bufdesc *bdp, struct bufdesc_prop *bd)

> +

Extra empty line?

> +						struct bufdesc_prop *bd)
> +{
> +	return (bdp >= bd->last) ? bd->base
> +			: (struct bufdesc *)(((void *)bdp) + bd->d_size);
> +}
> +
> +static inline struct
> +bufdesc *enet_get_prevdesc(struct bufdesc *bdp,

Really strange line split (same as above)

> +						struct bufdesc_prop *bd)
> +{
> +	return (bdp <= bd->base) ? bd->last
> +			: (struct bufdesc *)(((void *)bdp) - bd->d_size);
> +}
> +
> +static inline int
> +enet_get_bd_index(struct bufdesc *bdp,
> +					struct bufdesc_prop *bd)

This one LGTM, but I guess bd could fit into previous line.

> +{
> +	return ((const char *)bdp - (const char *)bd->base) >> bd->d_size_log2;
> +}
> +
> +static inline phys_addr_t enetfec_mem_vtop(uint64_t vaddr)

when function is implemented, function name is at postion 0
in above function. Please, be consistnet and follow it here
and in the next function.

> +{
> +	const struct rte_memseg *memseg;
> +	memseg = rte_mem_virt2memseg((void *)(uintptr_t)vaddr, NULL);
> +	if (memseg)
> +		return memseg->iova + RTE_PTR_DIFF(vaddr, memseg->addr);
> +	return (size_t)NULL;
> +}
> +
> +static inline int fls64(unsigned long word)
> +{
> +	return (64 - __builtin_clzl(word)) - 1;
> +}
> +
> +uint16_t enetfec_recv_pkts(void *rxq1, __rte_unused struct rte_mbuf **rx_pkts,
> +		uint16_t nb_pkts);
> +uint16_t
> +enetfec_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts);

function name is on the same line as return time above,
I think it would be good to be consistent here as well.

> +struct bufdesc *enet_get_nextdesc(struct bufdesc *bdp,
> +		struct bufdesc_prop *bd);
> +int enet_new_rxbdp(struct enetfec_private *fep, struct bufdesc *bdp,
> +		struct rte_mbuf *mbuf);
> +
> +#endif /*__FEC_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 000000000..ff8daa359
> --- /dev/null
> +++ b/drivers/net/enetfec/enet_pmd_logs.h
> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright 2021 NXP
> + */
> +
> +#ifndef _ENET_LOGS_H_
> +#define _ENET_LOGS_H_
> +
> +extern int enetfec_logtype_pmd;
> +
> +/* PMD related logs */
> +#define ENET_PMD_LOG(level, fmt, args...) \
> +	rte_log(RTE_LOG_ ## level, enetfec_logtype_pmd, "fec_net: %s()" \

It looks like prefix "fec_net" is inconsistent with PMD name

> +		fmt "\n", __func__, ##args)

There is no space betwen ## and args below. Please, be
consistent.

> +
> +#define PMD_INIT_FUNC_TRACE() ENET_PMD_LOG(DEBUG, " >>")
> +
> +#define ENET_PMD_DEBUG(fmt, args...) \
> +	ENET_PMD_LOG(DEBUG, fmt, ## args)
> +#define ENET_PMD_ERR(fmt, args...) \
> +	ENET_PMD_LOG(ERR, fmt, ## args)
> +#define ENET_PMD_INFO(fmt, args...) \
> +	ENET_PMD_LOG(INFO, fmt, ## args)
> +
> +#define ENET_PMD_WARN(fmt, args...) \
> +	ENET_PMD_LOG(WARNING, fmt, ## args)
> +
> +/* DP Logs, toggled out at compile time if level lower than current level */
> +#define ENET_DP_LOG(level, fmt, args...) \
> +	RTE_LOG_DP(level, PMD, fmt, ## args)
> +
> +#endif /* _ENET_LOGS_H_ */
> diff --git a/drivers/net/enetfec/meson.build b/drivers/net/enetfec/meson.build
> new file mode 100644
> index 000000000..252bf8330
> --- /dev/null
> +++ b/drivers/net/enetfec/meson.build
> @@ -0,0 +1,15 @@
> +# SPDX-License-Identifier: BSD-3-Clause
> +# Copyright 2021 NXP
> +
> +if not is_linux
> +	build = false
> +	reason = 'only supported on linux'
> +endif
> +
> +deps += ['common_dpaax']
> +
> +sources = files('enet_ethdev.c')
> +
> +if cc.has_argument('-Wno-pointer-arith')
> +	cflags += '-Wno-pointer-arith'
> +endif
> diff --git a/drivers/net/enetfec/version.map b/drivers/net/enetfec/version.map
> new file mode 100644
> index 000000000..6e4fb220a
> --- /dev/null
> +++ b/drivers/net/enetfec/version.map
> @@ -0,0 +1,3 @@
> +DPDK_21 {
> +        local: *;
> +};
> diff --git a/drivers/net/meson.build b/drivers/net/meson.build
> index c8b5ce298..c1307a3a6 100644
> --- a/drivers/net/meson.build
> +++ b/drivers/net/meson.build
> @@ -18,6 +18,7 @@ drivers = [
>          'e1000',
>          'ena',
>          'enetc',
> +	'enetfec',
>          'enic',
>          'failsafe',
>          'fm10k',
>