Hi Jerin, On 1/12/2017 9:17 AM, Jerin Jacob wrote: <...>
> +#include <rte_io.h> > + > /* CSR write macro */ > -#define ADF_CSR_WR(csrAddr, csrOffset, val) \ > - (void)((*((volatile uint32_t *)(((uint8_t *)csrAddr) + csrOffset)) \ > - = (val))) > +#define ADF_CSR_WR(csrAddr, csrOffset, val) \ > + rte_write32(val, (((uint8_t *)csrAddr) + csrOffset)) For IA, this update introduces an extra compiler barrier (rte_io_wmb()), which is indeed not a must, is this correct? If so, does it make sense to override these functions for x86, and make rte_writeX = rte_writeX_relaxed rte_readX = rte_readX_relaxed > > /* CSR read macro */ > -#define ADF_CSR_RD(csrAddr, csrOffset) \ > - (*((volatile uint32_t *)(((uint8_t *)csrAddr) + csrOffset))) > +#define ADF_CSR_RD(csrAddr, csrOffset) \ > + rte_read32((((uint8_t *)csrAddr) + csrOffset)) This patchset both introduces new rte_readX/rte_writeX functions, also applies them into drivers. While applying them, it changes the behavior. Like above code was doing a read, but after update it does read and read_memory_barrier. What do you think this patchset updates usage in a manner that keeps behavior exact same. Like using rte_read32_relaxed for this case. And doing architecture related updates in a different patchset? This both makes easy to see architecture specific updates, and makes easy to trace any possible performance issues by this patchset. > > #define ADF_BANK_INT_SRC_SEL_MASK_0 0x4444444CUL > #define ADF_BANK_INT_SRC_SEL_MASK_X 0x44444444UL >
