On 11/3/2021 7:20 PM, Apeksha Gupta wrote:

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'