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

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

On 11/8/2021 8:24 PM, Apeksha Gupta wrote:
> 
> 
>> -----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.
> 

If only a single port is supported, isn't it even easier to have this variable
as device specific data, instead of global variable. You can have it as part of
'struct enetfec_private'.