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

[James Clark]

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@xxxxxxxxxx>
>
> 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.