[PATCH 3/3] coresight etm4x: Add 32-bit read/write option to split 64-bit words

[Leo Yan]

Hi Steve,

On Sun, Mar 05, 2023 at 10:54:08PM -0700, Steve Clevenger wrote:
> An Ampere Computing design decision to not support 64-bit read/write
> access in the ETMv4.6 implementation.
> 
> The Ampere work around is to split ETMv4.6 64-bit register access
> into 2 ea. 32-bit read/write accesses.
> AC03_DEBUG_10 is described in the AmpereOne Developer Errata document:
> 
> https://solutions.amperecomputing.com/customer-connect/products/AmpereOne-device-documentation
> 
> Signed-off-by: Steve Clevenger <scclevenger at os.amperecomputing.com>
> ---
>  drivers/hwtracing/coresight/coresight-etm4x.h | 58 ++++++++++++++-----
>  1 file changed, 44 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
> index 434f4e95ee17..17457ec71876 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x.h
> +++ b/drivers/hwtracing/coresight/coresight-etm4x.h
> @@ -539,11 +539,6 @@
>  		 readl_relaxed((csa)->base + (offset)) :		\
>  		 read_etm4x_sysreg_offset((offset), false)))
>  
> -#define etm4x_relaxed_read64(csa, offset)				\
> -	((u64)((csa)->io_mem ?						\
> -		 readq_relaxed((csa)->base + (offset)) :		\
> -		 read_etm4x_sysreg_offset((offset), true)))
> -
>  #define etm4x_read32(csa, offset)					\
>  	({								\
>  		u32 __val = etm4x_relaxed_read32((csa), (offset));	\
> @@ -567,15 +562,6 @@
>  						  false);		\
>  	} 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((val), (offset),	\
> -						  true);		\
> -	} while (0)
> -
>  #define etm4x_write32(csa, val, offset)					\
>  	do {								\
>  		__io_bw();						\
> @@ -1091,6 +1077,50 @@ void etm4_config_trace_mode(struct etmv4_config *config);
>  u64 etm4x_sysreg_read(u32 offset, bool _relaxed, bool _64bit);
>  void etm4x_sysreg_write(u64 val, u32 offset, bool _relaxed, bool _64bit);
>  
> +/* 64-bit aligned to convert 64-bit access to 2 ea. 32-bit access */
> +#pragma pack(push, 8)
> +
> +struct etm_quad_split {
> +	u32 lsw;
> +	u32 msw;
> +};
> +
> +#pragma pack(pop)
> +
> +static inline u64 etm4x_relaxed_read64(struct csdev_access *csa, unsigned int offset)
> +{
> +	if (csa->io_mem) {
> +		if (csa->no_quad_mmio) {
> +			/* split 64-bit reads into 2 consecutive 32-bit reads */
> +			struct etm_quad_split container;
> +
> +			container.lsw = etm4x_read32(csa, offset);
> +			container.msw = etm4x_read32(csa, offset + sizeof(u32));
> +
> +			return *(u64 *) &container;

To be honest, I am not familiar with this part, just want to remind two
things.

Firstly, here you could use hi_lo_readq_relaxed(), seems to me,
hi_lo_readq_relaxed() does the same thing with above section and
you don't need to define the structure etm_quad_split().

Secondly, IIUC, a main problem with splitting 64-bit access into two
32-bit accesses is breaking atomicity.  If here have race condition
between reading and writing 64-bit registers, we need to consider to
use spinlock for register accessing.

I'd like to leave the second issue to Suzuki/Mike/James for
comfirmation in case I introduced complexity.

> +		} else
> +			return readq_relaxed(csa->base + offset);
> +	} else
> +		return read_etm4x_sysreg_offset(offset, true);

Here need to add brackets:

        } else {
                return read_etm4x_sysreg_offset(offset, true);
        }

> +}
> +
> +static inline void etm4x_relaxed_write64(struct csdev_access *csa, u64 quad, unsigned int offset)
> +{
> +	if (csa->io_mem) {
> +		/* split 64-bit writes into 2 consecutive 32-bit writes */
> +		if (csa->no_quad_mmio) {
> +			struct etm_quad_split container;
> +
> +			*(u64 *) &container = quad;
> +
> +			etm4x_relaxed_write32(csa, container.lsw, offset);
> +			etm4x_relaxed_write32(csa, container.msw, offset + sizeof(u32));
> +		} else
> +			writeq_relaxed(quad, csa->base + offset);
> +	} else
> +		write_etm4x_sysreg_offset(quad, offset, true);		\

Ditto.  Please drop the character '\' at the end of the line.

Thanks,
Leo

P.s. I have a ADLink AVA platform (Ampere Altra SoC with 32 CPUs), I
am glad to give a test if you can confirm this patch set can apply on
it (and please clarify if there have any prerequisite for firmwares).

> +}
> +
>  static inline bool etm4x_is_ete(struct etmv4_drvdata *drvdata)
>  {
>  	return drvdata->arch >= ETM_ARCH_ETE;
> -- 
> 2.25.1
>