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

[Apeksha Gupta <apeksha.gupta@nxp.com>]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Thursday, November 4, 2021 11:56 PM
> To: Apeksha Gupta <apeksha.gupta@nxp.com>; david.marchand@redhat.com;
> andrew.rybchenko@oktetlabs.ru
> Cc: dev@dpdk.org; Sachin Saxena <sachin.saxena@nxp.com>; Hemant Agrawal
> <hemant.agrawal@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.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.
[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'