Hi Suzuki,
I've looked at the set and have only one real gripe - the implementation and timing of component detection on the sysreg path. I've summarised my thoughts here, but as the changes are found across multiple patches I may well have repeated myself a little in the individual places.
On Fri, 11 Sep 2020 at 09:41, Suzuki K Poulose suzuki.poulose@arm.com wrote:
CoreSight ETMv4.4 introduced system instructions for accessing the ETM. This also implies that they may not be on the amba bus.
System instructions have always been an option - but we have never supported them up to now. In fact both memory access and system instructions can live side by side - but the driver really needs to choose just one! What did happen is that a PE that supports Arm Trace 8.4 mandates ETM 4.4, and ETM 4.4 mandates system instruction access for PEs with Arm Trace 8.4, and deprecates memory access.
But there is nothing to stop other variants having the system instruction interface. So there is no need to describe this as a purely 4.4. onwards support - it will support any version of the ETM that has sysreg access.
The spec permits aarch32 / armv7 register access via CP14 - but I assume this is omitted deliberately & not intended to be supported at this time.
Right now all the CoreSight components are accessed via memory map. Also, we have some common routines in coresight generic code driver (e.g, CS_LOCK, claim/disclaim), which assume the mmio. In order to preserve the generic algorithms at a single place and to allow dynamic switch for ETMs, this series introduces an abstraction layer for accessing a coresight device. It is designed such that the mmio access are fast tracked (i.e, without an indirect function call).
This will also help us to get rid of the driver+attribute specific sysfs show/store routines and replace them with a single routine to access a given register offset (which can be embedded in the dev_ext_attribute). This is not currently implemented in the series, but can be achieved.
Further we switch the generic routines to work with the abstraction. With this in place, we refactor the etm4x code a bit to allow for supporting the system instructions with very little new code. The changes also switch to using the system instructions by default even when we may have an MMIO.
We use TRCDEVARCH for the detection of the ETM component, which is a standard register as per CoreSight architecture, rather than the etm specific id register TRCIDR3. This is for making sure that we are able to detect the ETM via system instructions accurately, when the the trace unit could be anything (etm or a custom trace unit).
I'm assuming you mean TRCIDR1 here -- which in part, defines the etm architecture version. TRCIDR3 does something else entirely. Not sure I agree with this though - the driver is designed to match the ETM spec so there is no problem with using TRCIDR1 to spot functional variants according to ETM version,
The etm4_init_arch_data() function is not about detecting the presence of an ETMv4 component, but about exploring the capabilities it has. We check 4 bits of the version as a sanity check, but at this point we should be pretty sure we are dealing with an ETM of some kind.
TRCDEVARCH is already used for detection in the AMBA matching code - assuming the table includes the optional CoreSIght UCI. I would imagine that similar detection needs to go on for instruction access - but once we have detected an ETM, then ETM architected registers are sufficient. If the device is not an ETM then it should be detected and rejected early - and the bindings examined to determine why this driver was attached!
The act of adding in a check against TRCDEVARCH as part of the etm4_init_arch_data() function adds new and hidden checks to AMBA devices where it was sufficient to have an entry in the probe match table before. Most recent additions include the UCI matching, but older ones don't. I am concerned that this changes may trip up older existing implementations which for some reason may not have TRCDEVCARCH, or have set it to not present.
For this reason, I beleive that the TRCDEVARCH check for the sys reg access should occur on the sysreg specific probe - balancing what happens on the AMBA side. That way the common code remains common. Further the setup of the CSA for the device can happen immediately in the common etm_probe() function, based on *base being NULL or not, rather than as a side effect of the etm4_init_arch_data() call.
Regards
Mike
The series has been mildly tested on a model. I would really appreciate any testing on real hardware.
Applies on coresight/next
Changes since V1:
- Flip the switch for iomem from no_iomem to io_mem in csdev_access.
- Split patches for claim/disclaim and CS_LOCK/UNLOCK conversions.
- Move device access initialisation for etm4x to the target CPU
- Cleanup secure exception level mask handling.
- Switch to use TRCDEVARCH for ETM component discovery. This is for making
- Check the availability of OS/Software Locks before using them.
Suzuki K Poulose (19): coresight: Introduce device access abstraction coresight: tpiu: Prepare for using coresight device access abstraction coresight: Convert coresight_timeout to use access abstraction coresight: Convert claim/disclaim operations to use access wrappers coresight: Use device access layer for Software lock/unlock operations coresight: etm4x: Always read the registers on the host CPU coresight: etm4x: Convert all register accesses coresight: etm4x: Add commentary on the registers coresight: etm4x: Add sysreg access helpers coresight: etm4x: Define DEVARCH register fields coresight: etm4x: Check for OS and Software Lock coresight: etm4x: Cleanup secure exception level masks coresight: etm4x: Clean up exception level masks coresight: etm4x: Detect access early on the target CPU coresight: etm4x: Use TRCDEVARCH for component discovery coresight: etm4x: Detect system instructions support coresight: etm4x: Refactor probing routine coresight: etm4x: Add support for sysreg only devices dts: bindings: coresight: ETMv4.4 system register access only units
.../devicetree/bindings/arm/coresight.txt | 6 +- drivers/hwtracing/coresight/coresight-catu.c | 24 +- .../hwtracing/coresight/coresight-cpu-debug.c | 22 +- .../hwtracing/coresight/coresight-cti-sysfs.c | 5 +- drivers/hwtracing/coresight/coresight-cti.c | 34 +- drivers/hwtracing/coresight/coresight-etb10.c | 29 +- .../coresight/coresight-etm3x-sysfs.c | 10 +- drivers/hwtracing/coresight/coresight-etm3x.c | 35 +- .../coresight/coresight-etm4x-sysfs.c | 44 +- drivers/hwtracing/coresight/coresight-etm4x.c | 716 +++++++++++------- drivers/hwtracing/coresight/coresight-etm4x.h | 440 ++++++++++- .../hwtracing/coresight/coresight-funnel.c | 22 +- drivers/hwtracing/coresight/coresight-priv.h | 9 +- .../coresight/coresight-replicator.c | 31 +- drivers/hwtracing/coresight/coresight-stm.c | 50 +- .../hwtracing/coresight/coresight-tmc-etf.c | 38 +- .../hwtracing/coresight/coresight-tmc-etr.c | 20 +- drivers/hwtracing/coresight/coresight-tmc.c | 16 +- drivers/hwtracing/coresight/coresight-tpiu.c | 32 +- drivers/hwtracing/coresight/coresight.c | 130 +++- include/linux/coresight.h | 230 +++++- 21 files changed, 1449 insertions(+), 494 deletions(-)
-- 2.24.1
-- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK