> -----Original Message-----
> From: Ferruh Yigit <ferruh.yi...@intel.com>
> Sent: Thursday, November 4, 2021 11:56 PM
> To: Apeksha Gupta <apeksha.gu...@nxp.com>; david.march...@redhat.com;
> andrew.rybche...@oktetlabs.ru
> Cc: dev@dpdk.org; Sachin Saxena <sachin.sax...@nxp.com>; Hemant Agrawal
> <hemant.agra...@nxp.com>
> Subject: [EXT] Re: [PATCH v7 2/5] net/enetfec: add UIO support
>
> Caution: EXT Email
>
> 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.sax...@nxp.com>
> > Signed-off-by: Apeksha Gupta <apeksha.gu...@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.
[Apeksha] sure, we will use RTE_BIT32 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?
[Apeksha] This driver is implemented for IMX8MM board which has only one port.
So implemented accordingly. We will check when multiple ports supported.
>
> > +/*
> > + * 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.
[Apeksha] MAC set is not required on each restart. So we will remove this
unwanted code.
MAC address will set from probe function only.
>
> > + 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'
