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

[Steve Clevenger]

Hi Leo,

Comments below.

Steve

On 3/9/2023 4:11 AM, Leo Yan wrote:
> Hi Steve,
> 
> On Wed, Mar 08, 2023 at 11:08:34AM -0800, Steve Clevenger wrote:
> 
> [...]
> 
>> I'm familiar with this interface. The reason I chose not to use it is
>> this solution is configured at compile time. The writeq_relaxed (used by
>> etm4x_relaxed_write64) and readq_relaxed (used by etm4x_relaxed_read64)
>> otherwise default to 64-bit access. I was uncertain how a compile time
>> change would go over with the maintainers. Correct me, but compile time
>> configurations in the kernel seem to be related to ARM64 erratum, versus
>> manufacturer specific?
>>
>> My take (based on limited knowledge) of the changes necessary to support
>> a compile time decision seemed to exceed the changes compared with the
>> implementation I submitted upstream. If this becomes a sticking point,
>> let me know.
> 
> I am suggesting something like below:
> 
> #include <linux/io-64-nonatomic-hi-lo.h>
> 
> static inline u64 etm4x_relaxed_read64(struct csdev_access *csa, unsigned int offset)
> {
> 	if (csa->io_mem) {
> 		if (csa->no_quad_mmio)
>                         return hi_lo_readq_relaxed(csa->base + offset);
> 		else
> 			return readq_relaxed(csa->base + offset);
> 	} else {
> 		return read_etm4x_sysreg_offset(offset, true);
>         }
> }
> 
> I'm not saying to play magic on {writeq|readq}_relaxed(), would it be
> fine to directly call hi_lo_readq_relaxed()?

I can do this, and it would work fine. Without compile protection, one
concern is the silent inclusion of io-64-nonatomic-hi-lo.h definitions
for readq_relaxed/writeq_relaxed if the include hierarchy gets changed.
The only (minor) risk I see is to performance.

Second, it seems to me the hi_lo and lo_hi macros were intended to deal
with endianness. IMO, the implementation I offered is agnostic without a
hint to endianness or atomicity. The CoreSight maintainer audience is
limited and already aware of these issues, so I'll make the change for you.

> 
> Thanks,
> Leo