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

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

On 10/21/2021 5:46 AM, 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>

<...>

> +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.

in 'enetfec_eth_info()'
max_rx_queues/max_tx_queues returned as 3 (ENETFEC_MAX_Q).
If multi queue is not supported why it is not one?

<...>

> --- a/doc/guides/rel_notes/release_21_11.rst
> +++ b/doc/guides/rel_notes/release_21_11.rst
> @@ -20,6 +20,10 @@ DPDK Release 21.11
>         ninja -C build doc
>         xdg-open build/doc/guides/html/rel_notes/release_21_11.html
>   
> +* **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.
>   

Update is in the doc comment, can you please move it down, within the ethdev
driver group in alphabetically sorted manner.

<...>

> +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;

Can name be 'NULL'? Not sure if we need this check, can you please check?

<...>

> +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("Closing sw device");

Log can be misleading, there is another dev_ops to close the device.

<...>

> @@ -0,0 +1,179 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright 2020-2021 NXP
> + */
> +
> +#ifndef __ENETFEC_ETHDEV_H__
> +#define __ENETFEC_ETHDEV_H__
> +
> +#include <rte_ethdev.h>
> +
> +/*
> + * ENETFEC with AVB IP can support maximum 3 rx and tx queues.
> + */
> +#define ENETFEC_MAX_Q		3
> +
> +#define ETHER_ADDR_LEN		6
> +#define BD_LEN			49152
> +#define ENETFEC_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
> +

Some of the defines in this header is not used at all. What about
only adding structs/defines that are used? And add them as they are
needed?
This guarantees not having unused clutter in the code.

<...>

> +/* Required types */
> +typedef uint8_t         u8;
> +typedef uint16_t        u16;
> +typedef uint32_t        u32;
> +typedef uint64_t        u64;
> +

Do we need these type definitions, as far as I can see they are used only
in a few places, why not just use uint##_t types?

<...>

> +static inline struct
> +bufdesc *enet_get_nextdesc(struct bufdesc *bdp, struct bufdesc_prop *bd)
> +{
> +	return (bdp >= bd->last) ? bd->base
> +		: (struct bufdesc *)(((uintptr_t)bdp) + bd->d_size);
> +}
> +
> +static inline struct
> +bufdesc *enet_get_prevdesc(struct bufdesc *bdp, struct bufdesc_prop *bd)
> +{
> +	return (bdp <= bd->base) ? bd->last
> +		: (struct bufdesc *)(((uintptr_t)bdp) - bd->d_size);
> +}
> +
> +static inline int
> +enet_get_bd_index(struct bufdesc *bdp, struct bufdesc_prop *bd)
> +{
> +	return ((const char *)bdp - (const char *)bd->base) >> bd->d_size_log2;
> +}
> +
> +static inline int
> +fls64(unsigned long word)
> +{
> +	return (64 - __builtin_clzl(word)) - 1;
> +}
> +

Same for these static inline functions, can you please add they when that are
needed?

> +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);

These function definitions are not declared, at least not in this patch.

> +struct bufdesc *enet_get_nextdesc(struct bufdesc *bdp,
> +		struct bufdesc_prop *bd);

This is already static inline function, do we need separate definition for it?

> +int enet_new_rxbdp(struct enetfec_private *fep, struct bufdesc *bdp,
> +		struct rte_mbuf *mbuf);
> +

ditto, no function declaration.

<...>

> +
> +/* 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)
> +

Not used at all.

> +#endif /* _ENETFEC_LOGS_H_ */
> diff --git a/drivers/net/enetfec/meson.build b/drivers/net/enetfec/meson.build
> new file mode 100644
> index 0000000000..79dca58dea
> --- /dev/null
> +++ b/drivers/net/enetfec/meson.build
> @@ -0,0 +1,11 @@
> +# 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',
> +		'enet_uio.c',
> +		'enet_rxtx.c')

This should cause build error on this patch, isn't it? Since these files don't
exist in this patch.
Each patch should be built successfully.