[dpdk-dev] [PATCH v2] app/test-pmd: add IFPGA AFU register read/write access for testpmd

[Xu, Rosen]

Hi,

> -----Original Message-----
> From: Pattan, Reshma
> Sent: Saturday, December 15, 2018 1:38
> To: Xu, Rosen <rosen.xu at intel.com>; dev at dpdk.org
> Cc: Lu, Wenzhuo <wenzhuo.lu at intel.com>; Wu, Jingjing
> <jingjing.wu at intel.com>; Iremonger, Bernard
> <bernard.iremonger at intel.com>; Xu, Rosen <rosen.xu at intel.com>; Yigit,
> Ferruh <ferruh.yigit at intel.com>
> Subject: RE: [dpdk-dev] [PATCH v2] app/test-pmd: add IFPGA AFU register
> read/write access for testpmd
> 
> Hi,
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Rosen Xu
> > Sent: Friday, December 14, 2018 1:14 AM
> > To: dev at dpdk.org
> > Cc: Lu, Wenzhuo <wenzhuo.lu at intel.com>; Wu, Jingjing
> > <jingjing.wu at intel.com>; Iremonger, Bernard
> > <bernard.iremonger at intel.com>; Xu, Rosen <rosen.xu at intel.com>; Yigit,
> > Ferruh <ferruh.yigit at intel.com>
> > Subject: [dpdk-dev] [PATCH v2] app/test-pmd: add IFPGA AFU register
> > read/write access for testpmd
> >
> > Currently register read/write of testpmd is only for PCI device, but
> > more and more IFPGA based AFU devices need this feature to access
> > registers, this patch will add support for it.
> >
> > Signed-off-by: Rosen Xu <rosen.xu at intel.com>
> > -	pci_len = pci_dev->mem_resource[0].len;
> > -	if (reg_off >= pci_len) {
> > +	if (reg_off >= len) {
> >  		printf("Port %d: register offset %u (0x%X) out of port PCI "
> 
> Here log message mentions only PCI not ifpga device. Might need to edit the
> log.

I have fixed in revision V3.

> > port_reg_bit_display(portid_t port_id, uint32_t reg_off, uint8_t bit_x)  {
> >  	uint32_t reg_v;
> > -
> > +	const struct rte_bus *bus;
> >
> >  	if (port_id_is_invalid(port_id, ENABLED_WARN))
> >  		return;
> > @@ -935,7 +940,16 @@ void print_valid_ports(void)
> >  		return;
> >  	if (reg_bit_pos_is_invalid(bit_x))
> >  		return;
> > -	reg_v = port_id_pci_reg_read(port_id, reg_off);
> > +
> > +	bus = rte_bus_find_by_device(ports[port_id].dev_info.device);
> > +	if (bus && !strcmp(bus->name, "pci")) {
> > +		reg_v = port_id_pci_reg_read(port_id, reg_off);
> > +	} else if (bus && !strcmp(bus->name, "ifpga")) {
> > +		reg_v = port_id_afu_reg_read(port_id, reg_off);
> > +	} else {
> > +		printf("Not a PCI or AFU device\n");
> > +		return;
> > +	}
> 
> Here and in other places for reg_read , we have similar code  i.e. finding the
> device , checking its type, if ifpga call ifpga function else call pci functions.
> Can this common code be moved to new function say pci_read_reg like
> port_reg_set() which we have already.
> Also , again inside respective pci/ifpga reg read/write we are checking for pci
> type. So can all this be simplified, to remove redundant code.

PCI device and AFU device belongs to different bus, if we merge the register access
code to same function, it's not clarify.
 
> Thanks,
> Reshma
>