Re: [PATCH 01/15] coresight: Make ETM4x TRCIDR0 register accesses consistent with sysreg.h

[James Clark]

On 02/02/2022 17:05, Suzuki K Poulose wrote:
> Hi James
> 
> Thanks for taking this tedious task of cleaning the code and making
> this robust and readable.
> 
> One minor comment below.
> 
> On 02/02/2022 16:02, James Clark wrote:
>> This is a no-op change for style and consistency and has no effect on the
>> binary produced by gcc-11.
>>
>> Signed-off-by: James Clark <james.clark@xxxxxxx>
>> ---
>>   .../coresight/coresight-etm4x-core.c          | 37 +++++--------------
>>   drivers/hwtracing/coresight/coresight-etm4x.h | 17 +++++++++
>>   drivers/hwtracing/coresight/coresight-priv.h  |  1 +
>>   3 files changed, 27 insertions(+), 28 deletions(-)
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> index bf18128cf5de..8aefee4e72fd 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> @@ -1091,41 +1091,22 @@ static void etm4_init_arch_data(void *info)
>>       etmidr0 = etm4x_relaxed_read32(csa, TRCIDR0);
>>         /* INSTP0, bits[2:1] P0 tracing support field */
>> -    if (BMVAL(etmidr0, 1, 1) && BMVAL(etmidr0, 2, 2))
>> -        drvdata->instrp0 = true;
>> -    else
>> -        drvdata->instrp0 = false;
>> -
>> +    drvdata->instrp0 = !!((REG_VAL(etmidr0, TRCIDR0_INSTP0) & 0b01) &&
>> +                  (REG_VAL(etmidr0, TRCIDR0_INSTP0) & 0b10));
> 
> I don't understand this check. For ETMv4, here is what I find in the spec (ARM IHI 0064C)
> 
> P0 tracing support field. The permitted values are:
> 0b00  Tracing of load and store instructions as P0 elements is not
>       supported.
> 0b11  Tracing of load and store instructions as P0 elements is
>       supported, so TRCCONFIGR.INSTP0 is supported.
> 
> All other values are reserved.
> 
> So the check could simply be :
> 
>     drvdata->instrp0 = (REG_VAL(emtidr0, TRCIDR0_INSTP0) == 0b11;

Yes I can make this change, but it does make the compiler emit a slightly different binary
so we can't rely on that to check the refactor is ok.

Should I change it in this commit or stick it on the very end? Probably the end is best
in case I have to do any rebases and I still need to validate there are no mistakes.

> 
> That said, I think this is all dead code on ETMv4 as we never support
> data tracing on A class. May be there will be a use with R class in the
> future.
> 
> 
>>       /* TRCBB, bit[5] Branch broadcast tracing support bit */
>> -    if (BMVAL(etmidr0, 5, 5))
>> -        drvdata->trcbb = true;
>> -    else
>> -        drvdata->trcbb = false;
>> -
>> +    drvdata->trcbb = !!(etmidr0 & TRCIDR0_TRCBB);
>>       /* TRCCOND, bit[6] Conditional instruction tracing support bit */
>> -    if (BMVAL(etmidr0, 6, 6))
>> -        drvdata->trccond = true;
>> -    else
>> -        drvdata->trccond = false;
>> -
>> +    drvdata->trccond = !!(etmidr0 & TRCIDR0_TRCCOND);
>>       /* TRCCCI, bit[7] Cycle counting instruction bit */
>> -    if (BMVAL(etmidr0, 7, 7))
>> -        drvdata->trccci = true;
>> -    else
>> -        drvdata->trccci = false;
>> -
>> +    drvdata->trccci = !!(etmidr0 & TRCIDR0_TRCCCI);
>>       /* RETSTACK, bit[9] Return stack bit */
>> -    if (BMVAL(etmidr0, 9, 9))
>> -        drvdata->retstack = true;
>> -    else
>> -        drvdata->retstack = false;
>> -
>> +    drvdata->retstack = !!(etmidr0 & TRCIDR0_RETSTACK);
>>       /* NUMEVENT, bits[11:10] Number of events field */
>> -    drvdata->nr_event = BMVAL(etmidr0, 10, 11);
>> +    drvdata->nr_event = REG_VAL(etmidr0, TRCIDR0_NUMEVENT);
>>       /* QSUPP, bits[16:15] Q element support field */
>> -    drvdata->q_support = BMVAL(etmidr0, 15, 16);
>> +    drvdata->q_support = REG_VAL(etmidr0, TRCIDR0_QSUPP);
>>       /* TSSIZE, bits[28:24] Global timestamp size field */
>> -    drvdata->ts_size = BMVAL(etmidr0, 24, 28);
>> +    drvdata->ts_size = REG_VAL(etmidr0, TRCIDR0_TSSIZE);
>>         /* maximum size of resources */
>>       etmidr2 = etm4x_relaxed_read32(csa, TRCIDR2);
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
>> index 3c4d69b096ca..2bd8ad953b8e 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x.h
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x.h
>> @@ -130,6 +130,23 @@
>>     #define TRCRSR_TA            BIT(12)
>>   +/*
>> + * Bit positions of registers that are defined above, in the sysreg.h style
>> + * of _MASK, _SHIFT and BIT().
>> + */
>> +#define TRCIDR0_INSTP0_SHIFT            1
>> +#define TRCIDR0_INSTP0_MASK            GENMASK(1, 0)
>> +#define TRCIDR0_TRCBB                BIT(5)
>> +#define TRCIDR0_TRCCOND                BIT(6)
>> +#define TRCIDR0_TRCCCI                BIT(7)
>> +#define TRCIDR0_RETSTACK            BIT(9)
>> +#define TRCIDR0_NUMEVENT_SHIFT            10
>> +#define TRCIDR0_NUMEVENT_MASK            GENMASK(1, 0)
>> +#define TRCIDR0_QSUPP_SHIFT            15
>> +#define TRCIDR0_QSUPP_MASK            GENMASK(1, 0)
>> +#define TRCIDR0_TSSIZE_SHIFT            24
>> +#define TRCIDR0_TSSIZE_MASK            GENMASK(4, 0)
>> +
> 
> Cross checked the spec, and they look correct.
> 
>>   /*
>>    * System instructions to access ETM registers.
>>    * See ETMv4.4 spec ARM IHI0064F section 4.3.6 System instructions
>> diff --git a/drivers/hwtracing/coresight/coresight-priv.h b/drivers/hwtracing/coresight/coresight-priv.h
>> index ff1dd2092ac5..e22fa6184c6d 100644
>> --- a/drivers/hwtracing/coresight/coresight-priv.h
>> +++ b/drivers/hwtracing/coresight/coresight-priv.h
>> @@ -36,6 +36,7 @@
>>     #define TIMEOUT_US        100
>>   #define BMVAL(val, lsb, msb)    ((val & GENMASK(msb, lsb)) >> lsb)
>> +#define REG_VAL(val, name)    ((val & (name##_MASK << name##_SHIFT)) >> name##_SHIFT)
> 
> super minor nit: s/name/field ?

Will change and add a comment.

Thanks for the review.

> 
>>     #define ETM_MODE_EXCL_KERN    BIT(30)
>>   #define ETM_MODE_EXCL_USER    BIT(31)
>