Re: [PATCH v2 2/5] coresight: etm4x: use explicit barriers on enable/disable

[Andrew Murray <andrew.murray@arm.com>]

On Fri, Jun 28, 2019 at 04:51:54PM +0800, Leo Yan wrote:
> Hi Andrew,
> 
> On Fri, Jun 28, 2019 at 09:35:24AM +0100, Andrew Murray wrote:
> 
> [...]
> 
> > > > @@ -454,7 +458,8 @@ static void etm4_disable_hw(void *info)
> > > >  	control &= ~0x1;
> > > >  
> > > >  	/* make sure everything completes before disabling */
> > > > -	mb();
> > > > +	/* As recommended by 7.3.77 of ARM IHI 0064D */
> > > > +	dsb(sy);
> > > 
> > > Here the old code should be right, mb() is the same thing with
> > > dsb(sy).
> > > 
> > > So we don't need to change at here?
> > 
> > Correct - on arm64 there is no difference between mb and dsb(sy) so no
> > functional change on this hunk.
> > 
> > In repsonse to Suzuki's feedback on this patch, I've updated the commit
> > message to describe why I've made this change, as follows:
> >      
> > "On armv8 the mb macro is defined as dsb(sy) - Given that the etm4x is
> > only used on armv8 let's directly use dsb(sy) instead of mb(). This
> > removes some ambiguity and makes it easier to correlate the code with
> > the TRM."
> > 
> > Does that make sense?
> 
> On reason for preferring to use mb() rather than dsb(sy) is for
> compatibility cross different architectures (armv7, armv8, and
> so on ...).  Seems to me mb() is a general API and transparent for
> architecture's difference.
> 
> dsb(sy) is quite dependent on specific Arm architecture, e.g. some old
> Arm architecures might don't support dsb(sy); and we are not sure later
> it will change for new architectures.

Yes but please note that the KConfig for this driver depends on ARM64.

Thanks,

Andrew Murray

> 
> Thanks,
> Leo Yan