Re: [PATCH v3 14/26] coresight: etm4x: Add sysreg access helpers

[Suzuki K Poulose]

Hi Mathieu,

On 11/5/20 8:52 PM, Mathieu Poirier wrote:
> On Wed, Oct 28, 2020 at 10:09:33PM +0000, Suzuki K Poulose wrote:
> > ETMv4.4 architecture defines the system instructions for accessing
> > ETM via register accesses. Add basic support for accessing a given
> > register via system instructions.
> >
> > Cc: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>
> > Cc: Mike Leach <mike.leach@xxxxxxxxxx>
> > Signed-off-by: Suzuki K Poulose <suzuki.poulose@xxxxxxx>
> > ---
> >   .../coresight/coresight-etm4x-core.c          |  39 ++
> >   drivers/hwtracing/coresight/coresight-etm4x.h | 348 ++++++++++++++++--
> >   2 files changed, 365 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > index 4af7d45dfe63..90b80982c615 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> > @@ -56,6 +56,45 @@ static u64 etm4_get_access_type(struct etmv4_config *config);
> >   
> >   static enum cpuhp_state hp_online;
> >   
> > +u64 etm4x_sysreg_read(struct csdev_access *csa,
> > +		      u32 offset,
> > +		      bool _relaxed,
> > +		      bool _64bit)
>
> Please fix the stacking.

Sure.


> > +
> > +void etm4x_sysreg_write(struct csdev_access *csa,
> > +			u64 val,
> > +			u32 offset,
> > +			bool _relaxed,
> > +			bool _64bit)
>
> Here too.

Sure.


> >   	/* Writing 0 to TRCOSLAR unlocks the trace registers */
> > diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
> > index 510828c73db6..5cf71b30a652 100644
> > --- a/drivers/hwtracing/coresight/coresight-etm4x.h
> > +++ b/drivers/hwtracing/coresight/coresight-etm4x.

> > +
> > +#define ETM4x_READ_CASES(res)	CASE_LIST(READ, (res))
> > +#define ETM4x_WRITE_CASES(val)	CASE_LIST(WRITE, (val))
> > +
> > +#define read_etm4x_sysreg_offset(csa, offset, _64bit)				\
> > +	({									\
> > +		u64 __val;							\
> > +										\
> > +		if (__builtin_constant_p((offset)))				\
>
> Neat trick - I wonder how you stumbled on that one.

:-). There are plenty of uses in the kernel and glibc.

> > +			__val = read_etm4x_sysreg_const_offset((offset));	\
> > +		else								\
> > +			__val = etm4x_sysreg_read((csa), (offset),		\
> > +						  true, _64bit);		\
> > +		__val;								\
> > +	 })
> > +
> > +#define write_etm4x_sysreg_offset(csa, val, offset, _64bit)		\
> > +	do {								\
> > +		if (__builtin_constant_p((offset)))			\
> > +			write_etm4x_sysreg_const_offset((val),		\
> > +							(offset));	\
> > +		else							\
> > +			etm4x_sysreg_write((csa), (val), (offset),	\
> > +						true, _64bit);		\
> > +	} while (0)
> > +
> > +
> > +#define etm4x_relaxed_read32(csa, offset)				\
> > +	((u32)((csa)->io_mem ?						\
> > +		 readl_relaxed((csa)->base + (offset)) :		\
> > +		 read_etm4x_sysreg_offset((csa), (offset), false)))
>
> Please add an extra new line - otherwise it is very hard to read.

Sure

> > +#define etm4x_relaxed_read64(csa, offset)				\
> > +	((u64)((csa)->io_mem ?						\
> > +		 readq_relaxed((csa)->base + (offset)) :		\
> > +		 read_etm4x_sysreg_offset((csa), (offset), true)))
>
> Here too.

sure

> > +#define etm4x_read32(csa, offset)					\
> > +	({								\
> > +		u32 __val = etm4x_relaxed_read32((csa), (offset));	\
> > +		__iormb(__val);						\
> > +		__val;							\
> > +	 })
> > +
> > +#define etm4x_read64(csa, offset)					\
> > +	({								\
> > +		u64 __val = etm4x_relaxed_read64((csa), (offset));	\
> > +		__iormb(__val);						\
> > +		__val;							\
> > +	 })
> > +
> > +#define etm4x_relaxed_write32(csa, val, offset)				\
> > +	do {								\
> > +		if ((csa)->io_mem)					\
> > +			writel_relaxed((val), (csa)->base + (offset));	\
> > +		else							\
> > +			write_etm4x_sysreg_offset((csa), (val),	\
> > +						    (offset), false);	\
>
> Why using an if/else statement and above the '?' condition marker?  I would
> really like a uniform approach.  Otherwise the reader keeps looking for
> something subtle when there isn't.

The write variants do not return a result, unlike the read.
So, we cant use the '?'

> > +	} while (0)
> > +
> > +#define etm4x_relaxed_write64(csa, val, offset)				\
> > +	do {								\
> > +		if ((csa)->io_mem)					\
> > +			writeq_relaxed((val), (csa)->base + (offset));	\
> > +		else							\
> > +			write_etm4x_sysreg_offset((csa), (val),	\
> > +						    (offset), true);	\
> > +	} while (0)
> > +
> > +#define etm4x_write32(csa, val, offset)					\
> > +	do {								\
> > +		__iowmb();						\
> > +		etm4x_relaxed_write32((csa), (val), (offset));		\
> > +	} while (0)
> > +
> > +#define etm4x_write64(csa, val, offset)					\
> > +	do {								\
> > +		__iowmb();						\
> > +		etm4x_relaxed_write64((csa), (val), (offset));		\
> > +	} while (0)
> >   
> > -#define etm4x_write64(csa, val, offset)			\
> > -	writeq((val), (csa)->base + (offset))
> >   
> >   /* ETMv4 resources */
> >   #define ETM_MAX_NR_PE			8
> > @@ -512,4 +806,14 @@ enum etm_addr_ctxtype {
> >   
> >   extern const struct attribute_group *coresight_etmv4_groups[];
> >   void etm4_config_trace_mode(struct etmv4_config *config);
> > +
> > +u64 etm4x_sysreg_read(struct csdev_access *csa,
> > +		      u32 offset,
> > +		      bool _relaxed,
> > +		      bool _64bit);
> > +void etm4x_sysreg_write(struct csdev_access *csa,
> > +			u64 val,
> > +			u32 offset,
> > +			bool _relaxed,
> > +			bool _64bit);
>
> With the above:
>
> Reviewed-by: Mathieu Poirier <mathieu.poirier@xxxxxxxxxx>

Thanks !

> This patch holds together well.  I commend you on rendering something that is
> quite complex into a manageable implementation.  That being said it will impact
> Mike's complex configuration patchset (or Mike's complex configuration patchset
> will have an impact on this).

I understand. Will see when we get to it.

Cheers
Suzuki