[dpdk-dev] [PATCH v2 06/13] baseband/fpga_5gnr_fec: add queue configuration

[Xu, Rosen]

Hi,

> -----Original Message-----
> From: Chautru, Nicolas <nicolas.chautru at intel.com>
> Sent: Tuesday, April 14, 2020 8:16
> To: Xu, Rosen <rosen.xu at intel.com>; dev at dpdk.org; akhil.goyal at nxp.com
> Cc: Richardson, Bruce <bruce.richardson at intel.com>
> Subject: RE: [dpdk-dev] [PATCH v2 06/13] baseband/fpga_5gnr_fec: add
> queue configuration
> 
> Thanks Rosen for your thorough code review. Some comments in-line below.
> 
> > From: Xu, Rosen <rosen.xu at intel.com>
> >
> > Hi,
> >
> > Could you prefix all functions name with the FPGA IP name? FPGA is a
> > very common device name.
> >
> 
> I don't see such a guideline being used across all other PMDs.
> Unsure it always help notably as this would create many long function names
> with fpga_5gnr_fec_ added each time, and these names are not exposed
> outside of this .c file.
> Also some of the fpga_ prefixes would apply for either fpga_lte_fec or
> fpga_5gnr_fec PMD.
> If this becomes of a new guide lines we can rename every single function in
> each existing baseband PMD in future release (not just this new PMD).

What I mentioned is that, let's take FVL for example, in our PMD, we name it's PMD functions with
I40e_xxx, i40e is Intel NIC name, but for your design it named with fpga_5gnr_xxx, fpga is a common device,
That means not Intel only provide FPGA, no sure if any other developer summit other FPGA based 5G acceleration IP, is it ok?

> > >  /* Read a register of FPGA 5GNR FEC device */  static uint32_t
> > > fpga_reg_read_32(void *mmio_base, uint32_t offset) @@ -31,9
> +106,115
> > > @@
> > >  	return rte_le_to_cpu_32(ret);
> > >  }
> >
> > Why you didn't use rte_writeXXX() API of DPDK rte library?
> >
> 
> rte_write32 includes some memory barriers which are not required here.
> Also we handle the byte endianness in these internal implementations.
> 
> Thanks,
> Nic