Re: [dpdk-dev] [PATCH v2] i40e: Fix eth_i40e_dev_init sequence on ThunderX

[Jerin Jacob <jerin.jacob@caviumnetworks.com>]

On Sun, Nov 20, 2016 at 11:21:43PM +0000, Ananyev, Konstantin wrote:
> Hi
> > 
> > i40e_asq_send_command: rd32 & wr32 under ThunderX gives unpredictable
> >                        results. To solve this include rte memory barriers
> > 
> > Signed-off-by: Satha Rao <skoteshwar@caviumnetworks.com>
> > ---
> >  drivers/net/i40e/base/i40e_osdep.h | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/drivers/net/i40e/base/i40e_osdep.h b/drivers/net/i40e/base/i40e_osdep.h
> > index 38e7ba5..ffa3160 100644
> > --- a/drivers/net/i40e/base/i40e_osdep.h
> > +++ b/drivers/net/i40e/base/i40e_osdep.h
> > @@ -158,7 +158,13 @@ do {                                                            \
> >  	((volatile uint32_t *)((char *)(a)->hw_addr + (reg)))
> >  static inline uint32_t i40e_read_addr(volatile void *addr)
> >  {
> > +#if defined(RTE_ARCH_ARM64)
> > +	uint32_t val = rte_le_to_cpu_32(I40E_PCI_REG(addr));
> > +	rte_rmb();
> > +	return val;
> 
> If you really need an rmb/wmb with MMIO read/writes on ARM,
> I think you can avoid #ifdefs here and use rte_smp_rmb/rte_smp_wmb.
> BTW, I suppose if you need it for i40e, you would need it for other devices too.

Yes. ARM would need for all devices(typically, the devices on external PCI bus).
I guess rte_smp_rmb may not be the correct abstraction. So we need more of
rte_rmb() as we need only non smp variant on IO side. I guess then it make sense to
create new abstraction in eal with following variants so that each arch
gets opportunity to make what it makes sense that specific platform

rte_readb_relaxed
rte_readw_relaxed
rte_readl_relaxed
rte_readq_relaxed
rte_writeb_relaxed
rte_writew_relaxed
rte_writel_relaxed
rte_writeq_relaxed
rte_readb
rte_readw
rte_readl
rte_readq
rte_writeb
rte_writew
rte_writel
rte_writeq

Thoughts ?

Jerin

> Konstantin
> 
> > +#else
> >  	return rte_le_to_cpu_32(I40E_PCI_REG(addr));
> > +#endif
> >  }
> >  #define I40E_PCI_REG_WRITE(reg, value) \
> >  	do { I40E_PCI_REG((reg)) = rte_cpu_to_le_32(value); } while (0)
> > @@ -171,8 +177,16 @@ static inline uint32_t i40e_read_addr(volatile void *addr)
> >  	I40E_PCI_REG_WRITE(I40E_PCI_REG_ADDR((hw), (reg)), (value))
> > 
> >  #define rd32(a, reg) i40e_read_addr(I40E_PCI_REG_ADDR((a), (reg)))
> > +#if defined(RTE_ARCH_ARM64)
> > +#define wr32(a, reg, value) \
> > +	do { \
> > +		I40E_PCI_REG_WRITE(I40E_PCI_REG_ADDR((a), (reg)), (value)); \
> > +		rte_wmb(); \
> > +	} while (0)
> > +#else
> >  #define wr32(a, reg, value) \
> >  	I40E_PCI_REG_WRITE(I40E_PCI_REG_ADDR((a), (reg)), (value))
> > +#endif
> >  #define flush(a) i40e_read_addr(I40E_PCI_REG_ADDR((a), (I40E_GLGEN_STAT)))
> > 
> >  #define ARRAY_SIZE(arr) (sizeof(arr)/sizeof(arr[0]))
> > --
> > 2.7.4
>