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 [off-list ref] 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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel