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

From: Leo Yan <leo.yan@linaro.org>
To: Andrew Murray <andrew.murray@arm.com>
Cc: Mathieu Poirier <mathieu.poirier@linaro.org>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Alexander Shishkin <alexander.shishkin@linux.intel.com>,
	coresight@lists.linaro.org, stable@vger.kernel.org,
	Sudeep Holla <sudeep.holla@arm.com>,
	linux-arm-kernel@lists.infradead.org,
	Mike Leach <mike.leach@linaro.org>
Subject: Re: [PATCH v2 2/5] coresight: etm4x: use explicit barriers on enable/disable
Date: Fri, 28 Jun 2019 17:41:16 +0800	[thread overview]
Message-ID: <20190628094116.GF32370@leoy-ThinkPad-X240s> (raw)
In-Reply-To: <20190628090013.GI34530@e119886-lin.cambridge.arm.com>

Hi Andrew,

On Fri, Jun 28, 2019 at 10:00:14AM +0100, Andrew Murray wrote:
> 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.

Understood your point.

I am a bit suspect it's right thing to always set dependency on ARM64
for ETMv4 driver.  The reason is Armv8 CPU can also run with aarch32
mode in EL1.

If we let ETMv4 driver to support both aarch32 and aarch64, then we
will see dsb(sy) might break building for some old Arm arches.

Thanks,
Leo Yan

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel