Messages in this thread | | | Date | Thu, 5 Aug 2021 18:59:26 +0800 | From | Leo Yan <> | Subject | Re: [PATCH 5/6] perf cs-etm: Create ETE decoder |
| |
On Tue, Aug 03, 2021 at 02:09:38PM +0100, James Clark wrote:
[...]
> >> -static enum _ocsd_arch_version cs_etm_decoder__get_arch_ver(u32 reg_idr1) > >> +static enum _ocsd_arch_version cs_etm_decoder__get_arch_ver(u32 reg_idr1, u32 reg_devarch) > >> { > >> + /* ETE has to be v9 so set arch version to v8.3+ (ARCH__AA64) */ > >> + if (cs_etm__is_ete(reg_devarch)) > >> + return ARCH_AA64; > >> + > > > > Based on values used in below change, I think we can unify the ETM > > versio number like: > > > > ARCH_V8R3 : REVISION, bits[19:16] is 0x3 > > ARCH_V8R4 : REVISION, bits[19:16] is 0x4 > > ARCH_V8R5 : REVISION, bits[19:16] is 0x5 > > Do you mean make this change in OpenCSD? At the moment it understands these > values so I'm not sure if the extra ones would be useful:
Yes. As Mike said, these new macros will cause big changes in OpenCSD, so I don't have strong opinion to add more macros for tracer versions.
> >> +struct cs_ete_trace_params { > >> + struct cs_etmv4_trace_params base_params; > >> + u32 reg_devarch; > > > > As we have said, can we directly support ETMv4.5, so that it can > > smoothly support ETE features? If so, we don't need to add a new > > structure "cs_ete_trace_params" at here. > > > > I think with the new magic number change this is more likely to stay, > what are your thoughts?
Agreed. Just wander if need to define the struct cs_ete_trace_params as below?
struct cs_ete_trace_params { u32 reg_idr0; u32 reg_idr1; u32 reg_idr2; u32 reg_idr8; u32 reg_configr; u32 reg_traceidr; u32 reg_devarch; }
> >> + > >> +#define TRCDEVARCH_ARCHPART_SHIFT 0 > >> +#define TRCDEVARCH_ARCHPART_MASK GENMASK(11, 0) > >> +#define TRCDEVARCH_ARCHPART(x) (((x) & TRCDEVARCH_ARCHPART_MASK) >> TRCDEVARCH_ARCHPART_SHIFT) > >> + > >> +#define TRCDEVARCH_ARCHVER_SHIFT 12 > >> +#define TRCDEVARCH_ARCHVER_MASK GENMASK(15, 12) > >> +#define TRCDEVARCH_ARCHVER(x) (((x) & TRCDEVARCH_ARCHVER_MASK) >> TRCDEVARCH_ARCHVER_SHIFT) > >> + > >> +bool cs_etm__is_ete(u32 trcdevarch) > >> +{ > >> + /* > >> + * ETE if ARCHVER is 5 (ARCHVER is 4 for ETM) and ARCHPART is 0xA13. > >> + * See ETM_DEVARCH_ETE_ARCH in coresight-etm4x.h > >> + */ > >> + return TRCDEVARCH_ARCHVER(trcdevarch) == 5 && TRCDEVARCH_ARCHPART(trcdevarch) == 0xA13; > > > > I think this is incorrect. > > > > Here should check the bit field "REVISION, bits[19:16]". If it's > > field value is >= 5, then we can say it supports ETE. I checked the > > spec for ETMv4.4 and ETMv4.6, both use the same values for the > > Bits[15:12] = 0x4, so the architecture ID is same for ETMv4.x IPs. > > > > I tried to copy this as closely as possible from the ETE driver. See in coresight-etm4x.h > > #define ETM_DEVARCH_ETE_ARCH \ > (ETM_DEVARCH_ARCHITECT_ARM | ETM_DEVARCH_ARCHID_ETE | ETM_DEVARCH_PRESENT) > > Where ETM_DEVARCH_ARCHID_ETE is ARCHVER == 5 and ARCHPART == 0xA13. I didn't check > ETM_DEVARCH_ARCHITECT_ARM because I thought that wouldn't be necessary. If we want to make > the change do detect >= 5 then I think this should be made in the driver first. @Suzuki, > what do you think?
The tracer has two fields:
- ARCHID bits[15:12] - REVISION, bits[19:16]
For ETE its ARCHID[15:12] is 0x5 and ETMv4.x's ARCHID[15:12] is 0x4. So checking ARCHID[15:12] is the right way to distinguish if the tracer is ETE and creates corresponding decoder for it.
When reviewed this patch I assumed we also need to create ETE decoder if ETMv4.x has supported packet extension. As Mike confirmed, all ETMv4.x tracers keep to use existed way to create decoder; so it's not necessary to check REVISION bit field.
So please ignore my this comment.
Thanks, Leo
| |