On 09/18/2020 04:35 PM, Mike Leach wrote:

Hi Suzuki

On Fri, 11 Sep 2020 at 09:41, Suzuki K Poulose [off-list ref] wrote:

quoted

We have been using TRCIDR1 for detecting the ETM version. As
we are about to discover the trace unit on a CPU, let us use a
CoreSight architected register, instead of an ETM architected
register for accurate detection on a CPU. e.g, a CPU might
implement a custom trace unit, not an ETM.

Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>

quoted

  static int etm4_cpu_id(struct coresight_device *csdev)
  {
         struct etmv4_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);

@@ -694,10 +693,23 @@ static void etm_detect_lock_status(struct etmv4_drvdata *drvdata,
         drvdata->os_lock_model = TRCOSLSR_OSM(os_lsr);
  }

+static inline bool trace_unit_supported(u32 devarch)
+{
+       return (devarch & ETM_DEVARCH_ID_MASK) == ETM_DEVARCH_ETMv4x_ARCH;
+}
+

This is fine for system reg access - but imposes additional
constraints on memory mapped devices that previously may have worked
just by matching CID/PID. Can you be certain there are no devices out
there that have omitted this register (it does have a present bit
after all)

Very good point and I agree. I could restrict this to system instruction
based devices and use the TRCIDR1 for

quoted

  static bool etm_init_iomem_access(struct etmv4_drvdata *drvdata,
                                   struct csdev_access *csa)
  {
+       u32 devarch;
+
+       devarch = readl_relaxed(drvdata->base + TRCDEVARCH);
+       if (!trace_unit_supported(devarch))
+               return false;
+
         *csa = CSDEV_ACCESS_IOMEM(drvdata->base);
+       drvdata->arch = devarch;
+
         return true;
  }

@@ -713,7 +725,6 @@ static bool etm_init_csdev_access(struct etmv4_drvdata *drvdata,
  static void etm4_init_arch_data(void *info)

The primary task of this routine is to read all the ID registers and
set up the data in regards to resources etc.
We should not be mixing in a secondary task of detection of a valid device.

I agree that we have to read up the registers. However, for system instructions
based devices, we shouldn't try to access random register space, if they are not
ETM. Moreover, it kind of simplifies the logic, where you don't have to read up
all the registers if this is not a supported device in the first place.

Or the other way around, I think the priority is to make sure we are dealing with
a valid device we support, before we tread into reading the register space to avoid
unknown side effects of the operations.

Thanks

Suzuki

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