Re: [PATCH v2 2/8] coresight: Convert tag clear function to take a struct cs_access

[James Clark <james.clark@linaro.org>]

On 19/03/2025 12:00 pm, Suzuki K Poulose wrote:
> On 18/03/2025 16:21, James Clark wrote:
>> The self hosted claim tag will be reset on device probe in a later
>> commit. We'll want to do this before coresight_register() is called so
>> won't have a coresight_device and have to use cs_access instead.
>>
>> Also make them public and create locked and unlocked versions for
>> later use.
>>
>> Signed-off-by: James Clark <james.clark@linaro.org>
> 
> minor nit:
> 
> The restructuring looks a bit confusing. Could we start with :
> 
> coresight_{set,clear}_self_claim_tag_unlocked in Patch 1. Rather
> than rename what we renamed in patch 1 again here.
> 
> 

I considered collapsing these two actually, would you be ok with that?

>> ---
>>   drivers/hwtracing/coresight/coresight-core.c | 17 +++++++++++++----
>>   include/linux/coresight.h                    |  3 ++-
>>   2 files changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/ 
>> hwtracing/coresight/coresight-core.c
>> index 3f1c996d668a..9ff601e2415a 100644
>> --- a/drivers/hwtracing/coresight/coresight-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-core.c
>> @@ -151,12 +151,21 @@ static inline void 
>> coresight_set_self_claim_tag(struct coresight_device *csdev)
>>       isb();
>>   }
>> -static inline void coresight_clear_self_claim_tag(struct 
>> coresight_device *csdev)
>> +void coresight_clear_self_claim_tag(struct csdev_access *csa)
>>   {
>> -    csdev_access_relaxed_write32(&csdev->access, 
>> CORESIGHT_CLAIM_SELF_HOSTED,
>> +    CS_UNLOCK(csa->base);
> 
> This looks to be hard coding MMIO based access ? Should we abstract it 
> based on the csdev_access_** ?
> 
> 
> Suzuki
> 
> 

I did something similar earlier but it ended up being huge to do it 
properly and I wasn't sure of the benefits so I deleted it. By 
'properly' I mean change all of the CS_UNLOCK() calls to be abstracted 
and delete etm4_cs_unlock() because CS_UNLOCK() would handle both. It 
ends adding in an if and indirection for every lock/unlock for devices 
that are never sysreg access anyway.

Because there's already this:

static void etm4_cs_unlock(struct etmv4_drvdata *drvdata,
			   struct csdev_access *csa)
{
	if (csa->io_mem)
		CS_UNLOCK(csa->base);

But yeah I can add the if into coresight_clear_self_claim_tag(), 
although it technically doesn't do anything now because it's never 
called by any devices that aren't MMIO. And then it makes 
etm4_cs_unlock() stand out a bit because its not a core function when it 
could be. But that's probably fine.