Messages in this thread | | | Date | Thu, 3 Feb 2022 12:08:03 +0000 | Subject | Re: [PATCH 01/15] coresight: Make ETM4x TRCIDR0 register accesses consistent with sysreg.h | From | James Clark <> |
| |
On 03/02/2022 10:54, Suzuki K Poulose wrote: > On 03/02/2022 10:40, James Clark wrote: >> >> >> 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@arm.com> >>>> --- >>>> .../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. > > I would say, fix the existing check first and then convert to use the > updated symbols. > > That way we could queue the fix separately and you may be able to rebase > your next version on the updated tree ?
Good idea, I've resubmitted v2 with that and the other comments you left.
> > > Cheers > Suzuki
| |