On Fri, Jan 13, 2017 at 11:40:06AM +0000, Ferruh Yigit wrote: > On 1/13/2017 8:17 AM, Jerin Jacob wrote: > > On Thu, Jan 12, 2017 at 07:09:22PM +0000, Ferruh Yigit wrote: > >> 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? > > > > AFAIK, Compiler barrier is required for IA. I am not an IA expert, if > > someone thinks it needs to changed then I can fix it in following commit > > in this patch series by making rte_io_wmb() and rte_io_rmb() as empty. > > > > Let me know. > > I don't know, but what I know is this was working for IA without > compiler barrier before. > > Bruce or Konstantin can help here. > I think having a compiler barrier is safer. If all data being written before the actual register write to the device is volatile, none is needed, but also in that case, the compiler barrier should have no effect. If some of the data is not volatile, then a barrier is needed for correctness. IMHO, unless there is a performance regression by doing so, I think having the IO register writes have compiler barriers on IA is a good thing. It should save individual drivers from having to worry about the barriers themselves in most cases.
Regards, /Bruce
