Re: [dpdk-dev] [PATCH v7 2/5] net/enetfec: add UIO support

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

On 11/3/2021 7:20 PM, Apeksha Gupta wrote:
> Implemented the fec-uio driver in kernel. enetfec PMD uses
> UIO interface to interact with "fec-uio" driver implemented in
> kernel for PHY initialisation and for mapping the allocated memory
> of register & BD from kernel to DPDK which gives access to
> non-cacheable memory for BD.
> 
> Signed-off-by: Sachin Saxena <sachin.saxena@nxp.com>
> Signed-off-by: Apeksha Gupta <apeksha.gupta@nxp.com>
> ---
>   drivers/net/enetfec/enet_ethdev.c | 227 ++++++++++++++++++++++++
>   drivers/net/enetfec/enet_ethdev.h |  14 ++
>   drivers/net/enetfec/enet_regs.h   | 106 ++++++++++++
>   drivers/net/enetfec/enet_uio.c    | 278 ++++++++++++++++++++++++++++++
>   drivers/net/enetfec/enet_uio.h    |  64 +++++++
>   drivers/net/enetfec/meson.build   |   3 +-
>   6 files changed, 691 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/net/enetfec/enet_regs.h
>   create mode 100644 drivers/net/enetfec/enet_uio.c
>   create mode 100644 drivers/net/enetfec/enet_uio.h
> 
> diff --git a/drivers/net/enetfec/enet_ethdev.c b/drivers/net/enetfec/enet_ethdev.c
> index a6c4bcbf2e..410c395039 100644
> --- a/drivers/net/enetfec/enet_ethdev.c
> +++ b/drivers/net/enetfec/enet_ethdev.c
> @@ -13,16 +13,212 @@
>   #include <rte_bus_vdev.h>
>   #include <rte_dev.h>
>   #include <rte_ether.h>
> +#include <rte_io.h>
>   #include "enet_pmd_logs.h"
>   #include "enet_ethdev.h"
> +#include "enet_regs.h"
> +#include "enet_uio.h"
>   
>   #define ENETFEC_NAME_PMD                net_enetfec
>   #define ENETFEC_CDEV_INVALID_FD         -1
> +#define BIT(nr)				(1u << (nr))

We already have 'RTE_BIT32' macro, it can be used instead of defining
a new macro.

> +
> +/* FEC receive acceleration */
> +#define ENETFEC_RACC_IPDIS		BIT(1)
> +#define ENETFEC_RACC_PRODIS		BIT(2)
> +#define ENETFEC_RACC_SHIFT16		BIT(7)
> +#define ENETFEC_RACC_OPTIONS		(ENETFEC_RACC_IPDIS | \
> +						ENETFEC_RACC_PRODIS)
> +
> +#define ENETFEC_PAUSE_FLAG_AUTONEG	0x1
> +#define ENETFEC_PAUSE_FLAG_ENABLE	0x2
> +
> +/* Pause frame field and FIFO threshold */
> +#define ENETFEC_FCE			BIT(5)
> +#define ENETFEC_RSEM_V			0x84
> +#define ENETFEC_RSFL_V			16
> +#define ENETFEC_RAEM_V			0x8
> +#define ENETFEC_RAFL_V			0x8
> +#define ENETFEC_OPD_V			0xFFF0
> +
> +#define NUM_OF_BD_QUEUES		6
> +
> +static uint32_t enetfec_e_cntl;
> +

Again, question on the usage of this global variable in previous version
is not answered, let me copy/paste here:


Is this global variable really needed, most of the times what you need is
per port varible.
For example I can see this variable is updated based on port start/stop,
what if you have multiple ports and they are different start/stop state,
will the value of variable still be correct?

> +/*
> + * This function is called to start or restart the ENETFEC during a link
> + * change, transmit timeout, or to reconfigure the ENETFEC. The network
> + * packet processing for this device must be stopped before this call.
> + */
> +static void
> +enetfec_restart(struct rte_eth_dev *dev)
> +{
> +	struct enetfec_private *fep = dev->data->dev_private;
> +	uint32_t temp_mac[2];
> +	uint32_t rcntl = OPT_FRAME_SIZE | 0x04;
> +	uint32_t ecntl = ENETFEC_ETHEREN;
> +
> +	/* default mac address */
> +	struct rte_ether_addr addr = {
> +		.addr_bytes = {0x1, 0x2, 0x3, 0x4, 0x5, 0x6} };

This mac address is set on device, but device data, 'dev->data->mac_addrs',
doesn't have it.
- Is MAC set required on each restart?
- What about save the MAC address to 'dev->data->mac_addrs', and use that
value in restart? In that case the MAC value device data and acual device
config matches.

> +	uint32_t val;
> +
> +	/*
> +	 * enet-mac reset will reset mac address registers too,
> +	 * so need to reconfigure it.
> +	 */
> +	memcpy(&temp_mac, addr.addr_bytes, ETH_ALEN);
> +	rte_write32(rte_cpu_to_be_32(temp_mac[0]),
> +		(uint8_t *)fep->hw_baseaddr_v + ENETFEC_PALR);
> +	rte_write32(rte_cpu_to_be_32(temp_mac[1]),
> +		(uint8_t *)fep->hw_baseaddr_v + ENETFEC_PAUR);
> +
> +	/* Clear any outstanding interrupt. */
> +	writel(0xffffffff, (uint8_t *)fep->hw_baseaddr_v + ENETFEC_EIR);
> +
> +	/* Enable MII mode */
> +	if (fep->full_duplex == FULL_DUPLEX) {
> +		/* FD enable */
> +		rte_write32(rte_cpu_to_le_32(0x04),
> +			(uint8_t *)fep->hw_baseaddr_v + ENETFEC_TCR);
> +	} else {
> +	/* No Rcv on Xmit */
> +		rcntl |= 0x02;
> +		rte_write32(0, (uint8_t *)fep->hw_baseaddr_v + ENETFEC_TCR);
> +	}
> +
> +	if (fep->quirks & QUIRK_RACC) {
> +		val = rte_read32((uint8_t *)fep->hw_baseaddr_v + ENETFEC_RACC);
> +		/* align IP header */
> +		val |= ENETFEC_RACC_SHIFT16;
> +		val &= ~ENETFEC_RACC_OPTIONS;
> +		rte_write32(rte_cpu_to_le_32(val),
> +			(uint8_t *)fep->hw_baseaddr_v + ENETFEC_RACC);
> +		rte_write32(rte_cpu_to_le_32(PKT_MAX_BUF_SIZE),
> +			(uint8_t *)fep->hw_baseaddr_v + ENETFEC_FRAME_TRL);
> +	}
> +
> +	/*
> +	 * The phy interface and speed need to get configured
> +	 * differently on enet-mac.
> +	 */
> +	if (fep->quirks & QUIRK_HAS_ENETFEC_MAC) {
> +		/* Enable flow control and length check */
> +		rcntl |= 0x40000000 | 0x00000020;
> +
> +		/* RGMII, RMII or MII */
> +		rcntl |= BIT(6);
> +		ecntl |= BIT(5);
> +	}
> +
> +	/* enable pause frame*/
> +	if ((fep->flag_pause & ENETFEC_PAUSE_FLAG_ENABLE) ||
> +		((fep->flag_pause & ENETFEC_PAUSE_FLAG_AUTONEG)
> +		/*&& ndev->phydev && ndev->phydev->pause*/)) {
> +		rcntl |= ENETFEC_FCE;
> +
> +		/* set FIFO threshold parameter to reduce overrun */
> +		rte_write32(rte_cpu_to_le_32(ENETFEC_RSEM_V),
> +			(uint8_t *)fep->hw_baseaddr_v + ENETFEC_R_FIFO_SEM);
> +		rte_write32(rte_cpu_to_le_32(ENETFEC_RSFL_V),
> +			(uint8_t *)fep->hw_baseaddr_v + ENETFEC_R_FIFO_SFL);
> +		rte_write32(rte_cpu_to_le_32(ENETFEC_RAEM_V),
> +			(uint8_t *)fep->hw_baseaddr_v + ENETFEC_R_FIFO_AEM);
> +		rte_write32(rte_cpu_to_le_32(ENETFEC_RAFL_V),
> +			(uint8_t *)fep->hw_baseaddr_v + ENETFEC_R_FIFO_AFL);
> +
> +		/* OPD */
> +		rte_write32(rte_cpu_to_le_32(ENETFEC_OPD_V),
> +			(uint8_t *)fep->hw_baseaddr_v + ENETFEC_OPD);
> +	} else {
> +		rcntl &= ~ENETFEC_FCE;
> +	}
> +
> +	rte_write32(rte_cpu_to_le_32(rcntl),
> +		(uint8_t *)fep->hw_baseaddr_v + ENETFEC_RCR);
> +
> +	rte_write32(0, (uint8_t *)fep->hw_baseaddr_v + ENETFEC_IAUR);
> +	rte_write32(0, (uint8_t *)fep->hw_baseaddr_v + ENETFEC_IALR);
> +
> +	if (fep->quirks & QUIRK_HAS_ENETFEC_MAC) {
> +		/* enable ENETFEC endian swap */
> +		ecntl |= (1 << 8);
> +		/* enable ENETFEC store and forward mode */
> +		rte_write32(rte_cpu_to_le_32(1 << 8),
> +			(uint8_t *)fep->hw_baseaddr_v + ENETFEC_TFWR);
> +	}
> +	if (fep->bufdesc_ex)
> +		ecntl |= (1 << 4);
> +	if (fep->quirks & QUIRK_SUPPORT_DELAYED_CLKS &&
> +		fep->rgmii_txc_delay)
> +		ecntl |= ENETFEC_TXC_DLY;
> +	if (fep->quirks & QUIRK_SUPPORT_DELAYED_CLKS &&
> +		fep->rgmii_rxc_delay)
> +		ecntl |= ENETFEC_RXC_DLY;
> +	/* Enable the MIB statistic event counters */
> +	rte_write32(0, (uint8_t *)fep->hw_baseaddr_v + ENETFEC_MIBC);
> +
> +	ecntl |= 0x70000000;
> +	enetfec_e_cntl = ecntl;
> +	/* And last, enable the transmit and receive processing */
> +	rte_write32(rte_cpu_to_le_32(ecntl),
> +		(uint8_t *)fep->hw_baseaddr_v + ENETFEC_ECR);
> +	rte_delay_us(10);
> +}
> +
> +static int
> +enetfec_eth_configure(__rte_unused struct rte_eth_dev *dev)

unnecessary '__rte_unused'