Message-ID: <CAJ9a7Vi+Oq3Zma0Cs+w8m0kRE0pG6ax3=26EQK=u7d=vQfNFQw@mail.gmail.com>
Date: Wed, 22 Oct 2025 13:35:46 +0100
From: Mike Leach <mike.leach@...aro.org>
To: Leo Yan <leo.yan@....com>
Cc: coresight@...ts.linaro.org, linux-arm-kernel@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, suzuki.poulose@....com, james.clark@...aro.org
Subject: Re: [PATCH 1/1] coresight: fix issue where coresight component has no claimtags

Hi Leo

On Wed, 22 Oct 2025 at 10:30, Leo Yan <leo.yan@....com> wrote:
>
> On Wed, Oct 22, 2025 at 12:45:20AM +0100, Mike Leach wrote:
>
> [...]
>
> > +/*
> > + * Reading CLIAMSET returns  a bitfield representing the number of claim tags
> > + * implemented from bit 0 to bit nTag-1, valid bits set to 1.
> > + *
> > + * Claim protocol requires 2 bits so test for highest bit required,
> > + * bit 1 -  CORESIGHT_CLAIM_SELF_HOSTED_BIT
> > + *
> > + * return true if sufficient claim tags implemented for protocol
> > + */
> > +static bool coresight_claim_tags_implemented_unlocked(struct csdev_access *csa)
> > +{
> > +     u32 claim_bits_impl = FIELD_GET(CORESIGHT_CLAIM_BITS_MAX_MASK,
> > +                      csdev_access_relaxed_read32(csa, CORESIGHT_CLAIMSET));
> > +     return ((claim_bits_impl & CORESIGHT_CLAIM_SELF_HOSTED_BIT) != 0);
> > +}
> > +
>
> The logic looks good to me.
>
> Can we improve a bit with a cached flag to avoid every time checking the
> CLAIMSET, we can read it only once at init time instead.
>

When I considered how often we use the claim tags - compared to all
the other programming of devices, a single extra read did not seem
excessive.

> - We can add a new flag ("bool claim_impl" in the struct csdev_access),
>   by default the field will be zero.
>

I considered a bool - but the correct place for this would be
coresight_device - where we keep all the information about the
hardware features.

> - The drivers support claim tags call coresight_clear_self_claim_tag()
>   in probe (see __catu_probe() as an example), we can call a new
>   function coresight_init_claim_tag() to replace it, this function sets
>   "claim_impl" properly and clear the tag if supported.
>

I considered moving this initialisation to the common coresight code
where we create the coresight_device. Certainly with the new check for
number of claim tags it is safe for all devices, and there would
appear to be no reason to do it as early as it appears in some of the
drivers. However that would be a larger change.

> - Afterwards, simply check the "claim_impl" flag.


>
> [...]
>
> > +/*
> > + * Coresight specification defines a maximum of 8 claim tag bits.
> > + * The precise number is implementation defined, and may be obtained by
> > + * reading the CLAIMSET register.
> > + */
> > +#define CORESIGHT_CLAIM_BITS_MAX_MASK        GENMASK(7, 0)
> > +#define CORESIGHT_CLAIM_SELF_HOSTED_BIT      BIT(1)
>
> Now we only care about the self-host bit. Can reuse existed macros ?
>

I feel that drivers should be written to match the specification - the
macros above are a correct mask value per specification and the
correct bitfield comparison for the MSBit of the protocol.

The ones below are a protocol masks field, and a specific protocol
value that is used in value rather than bit comparisons in the
claimtag code. I felt it clearer to differentiate between the uses
when reading the code.

>   CORESIGHT_CLAIM_MASK
>   CORESIGHT_CLAIM_SELF_HOSTED
>
> I don't mind if extend CORESIGHT_CLAIM_MASK to 8 bits and even remove
> CORESIGHT_CLAIM_INVALID (as it cannot cover other invalid values). Or we
> can defer the extension as needed later.
>

The invalid value is used to as a value comparison to check for a race
condition with both protocol bits set.

Thanks

Mike


> Thanks,
> Leo



-- 
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK

Powered by blists - more mailing lists