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

[Leo Yan <leo.yan@linaro.org>]

Hi Suzuki,

On Mon, Jul 01, 2019 at 09:58:29AM +0100, Suzuki K Poulose wrote:
> Hi Leo,
> 
> On 28/06/2019 10:41, Leo Yan wrote:
> > 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.
> 
> If we add support for ETMv4 on aarch32, I would recommend adding a "dsb"
> explicitly for aarch32 to make sure, it doesn't default to something else
> that the mb() may cover up as.

For aarch32, mb() should work well with below definition:

#ifdef CONFIG_ARM_HEAVY_MB
#define __arm_heavy_mb(x...) do { dsb(x); arm_heavy_mb(); } while (0)
#else
#define __arm_heavy_mb(x...) dsb(x)
#endif

#if defined(CONFIG_ARM_DMA_MEM_BUFFERABLE) || defined(CONFIG_SMP)
#define mb()		__arm_heavy_mb()
#else
#define mb()		barrier()
#endif

> There is no point in creating another level
> of indirection when the architecture is clear about it and the ETMv4 supporting
> architectures must implement "dsb". Had this been in a generic code, I would
> be happy to retain mb(). But this is specific to the ETMv4 driver and we know
> that dsb must be there.

Okay, I understand the purpose for more explict barrier in the code;
this would be fine for me.

Thanks,
Leo Yan

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