Re: [PATCH v3 2/7] coresight: Only check bottom two claim bits

[James Clark]

On 21/03/2025 12:10 pm, Suzuki K Poulose wrote:
> On 20/03/2025 14:34, James Clark wrote:
> > The use of the whole register and == could break the claim mechanism if
> > any of the other bits are used in the future. The referenced doc "PSCI -
> > ARM DEN 0022D" also says to only read and clear the bottom two bits.
> >
> > Use FIELD_GET() to extract only the relevant part.
> >
> > Reviewed-by: Leo Yan <leo.yan@xxxxxxx>
> > Signed-off-by: James Clark <james.clark@xxxxxxxxxx>
> > ---
> >   drivers/hwtracing/coresight/coresight-core.c | 3 ++-
> >   drivers/hwtracing/coresight/coresight-priv.h | 1 +
> >   2 files changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/hwtracing/coresight/coresight-core.c b/drivers/ 
> > hwtracing/coresight/coresight-core.c
> > index 8471aefeac76..26d149a4c579 100644
> > --- a/drivers/hwtracing/coresight/coresight-core.c
> > +++ b/drivers/hwtracing/coresight/coresight-core.c
> > @@ -131,7 +131,8 @@ coresight_find_out_connection(struct 
> > coresight_device *csdev,
> >   static inline u32 coresight_read_claim_tags(struct coresight_device 
> > *csdev)
> >   {
> > -    return csdev_access_relaxed_read32(&csdev->access, 
> > CORESIGHT_CLAIMCLR);
> > +    return FIELD_GET(CORESIGHT_CLAIM_MASK,
> > +             csdev_access_relaxed_read32(&csdev->access, 
> > CORESIGHT_CLAIMCLR));
> >   }
> >   static inline bool coresight_is_claimed_self_hosted(struct 
> > coresight_device *csdev)
> > diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/ 
> > hwtracing/coresight/coresight-priv.h
> > index 82644aff8d2b..38bb4e8b50ef 100644
> > --- a/drivers/hwtracing/coresight/coresight-priv.h
> > +++ b/drivers/hwtracing/coresight/coresight-priv.h
> > @@ -35,6 +35,7 @@ extern const struct device_type coresight_dev_type[];
> >    * Coresight device CLAIM protocol.
> >    * See PSCI - ARM DEN 0022D, Section: 6.8.1 Debug and Trace save and 
> > restore.
> >    */
> > +#define CORESIGHT_CLAIM_MASK        GENMASK(1, 0)
> >   #define CORESIGHT_CLAIM_SELF_HOSTED    BIT(1)
>
> I am checking with the Arm CoreSight architects on this. This is
> problematic, if another agent is assigned, say BIT(2) and we wouldn't
> forward compatible.
>
> Suzuki

For the benefit of the list, I think the conclusion is that in order to 
be forwards compatible with adding new bits, the spec would already have 
to have been released indicating that the extra bits may be used. As it 
specifically only mentions the two bits, any other implementation 
following it would also hit the same problem.

Seems like any further updates would have to be either be breaking or 
done in some other way.

James