Re: [RFC PATCH v2 7/8] coresight: etm4x: Add complex configuration handlers to etmv4

Hi Mathieu,

On Tue, 13 Oct 2020 at 21:35, Mathieu Poirier mathieu.poirier@linaro.org wrote:

On Thu, Aug 27, 2020 at 03:34:10PM +0100, Mike Leach wrote:

...

Adds in handlers to allow the ETMv4 to use the complex configuration support. Features and configurations can be loaded and selected in the device.

Signed-off-by: Mike Leach mike.leach@linaro.org

drivers/hwtracing/coresight/Makefile | 3 +- .../hwtracing/coresight/coresight-etm4x-cfg.c | 226 ++++++++++++++++++ .../hwtracing/coresight/coresight-etm4x-cfg.h | 29 +++ .../coresight/coresight-etm4x-core.c | 30 ++- .../coresight/coresight-etm4x-sysfs.c | 3 + 5 files changed, 288 insertions(+), 3 deletions(-) create mode 100644 drivers/hwtracing/coresight/coresight-etm4x-cfg.c create mode 100644 drivers/hwtracing/coresight/coresight-etm4x-cfg.h

diff --git a/drivers/hwtracing/coresight/Makefile b/drivers/hwtracing/coresight/Makefile index 177bc6338312..5723094b2b69 100644 --- a/drivers/hwtracing/coresight/Makefile +++ b/drivers/hwtracing/coresight/Makefile @@ -16,7 +16,8 @@ obj-$(CONFIG_CORESIGHT_SOURCE_ETM3X) += coresight-etm3x.o coresight-etm3x-y := coresight-etm3x-core.o coresight-etm-cp14.o \ coresight-etm3x-sysfs.o obj-$(CONFIG_CORESIGHT_SOURCE_ETM4X) += coresight-etm4x.o -coresight-etm4x-y := coresight-etm4x-core.o coresight-etm4x-sysfs.o +coresight-etm4x-y := coresight-etm4x-core.o coresight-etm4x-sysfs.o \

•                 coresight-etm4x-cfg.o
  

obj-$(CONFIG_CORESIGHT_STM) += coresight-stm.o obj-$(CONFIG_CORESIGHT_CPU_DEBUG) += coresight-cpu-debug.o obj-$(CONFIG_CORESIGHT_CATU) += coresight-catu.o diff --git a/drivers/hwtracing/coresight/coresight-etm4x-cfg.c b/drivers/hwtracing/coresight/coresight-etm4x-cfg.c new file mode 100644 index 000000000000..c355003d3e05 --- /dev/null +++ b/drivers/hwtracing/coresight/coresight-etm4x-cfg.c @@ -0,0 +1,226 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright(C) 2020 Linaro Limited. All rights reserved.
• • Author: Mike Leach mike.leach@linaro.org
• */

+#include "coresight-etm4x.h" +#include "coresight-etm4x-cfg.h" +#include "coresight-priv.h" +#include "coresight-syscfg.h"

+/**

• • etm4_cfg_map_reg_offset - validate and map the register offset into a

• •                    location in the driver config struct.
    

• • Limits the number of registers that can be accessed and programmed in
• • features, to those which are used to control the trace capture parameters.
• • Omits or limits access to those which the driver must use exclusively.
• • Invalid offsets will result in fail code return and feature load failure.
• • @drvdata: driver data to map into.
• • @reg: register to map.
• • @offset: device offset for the register
• */

+static int etm4_cfg_map_reg_offset(struct etmv4_drvdata *drvdata,

•                            struct cs_cfg_reg *reg, u32 offset)
  

+{

• int err = 0, idx;
  

• struct etmv4_config *drvcfg = &drvdata->config;
  

• #define MAPREG(cval, elem) \
  

•         case cval: \
  

•                 reg->drv_store = &drvcfg->elem; \
  

•                 break;
  

• #define MAPREGIDX(cval, elem, off_idx) \
  

•         case cval: \
  

•                 reg->drv_store = &drvcfg->elem[off_idx]; \
  

•                 break;
  

• if (((offset >= TRCEVENTCTL0R) && (offset <= TRCVIPCSSCTLR)) ||
  

•     ((offset >= TRCSEQRSTEVR) && (offset <= TRCEXTINSELR)) ||
  

•     ((offset >= TRCCIDCCTLR0) && (offset <= TRCVMIDCCTLR1))) {
  

•         switch (offset) {
  

•                 /* 32 bit single control and filter registers */
  

•                 MAPREG(TRCEVENTCTL0R, eventctrl0);
  

•                 MAPREG(TRCEVENTCTL1R, eventctrl1);
  

•                 MAPREG(TRCSTALLCTLR, stall_ctrl);
  

•                 MAPREG(TRCTSCTLR, ts_ctrl);
  

•                 MAPREG(TRCSYNCPR, syncfreq);
  

•                 MAPREG(TRCCCCTLR, ccctlr);
  

•                 MAPREG(TRCBBCTLR, bb_ctrl);
  

•                 MAPREG(TRCVICTLR, vinst_ctrl);
  

•                 MAPREG(TRCVIIECTLR, viiectlr);
  

•                 MAPREG(TRCVISSCTLR, vissctlr);
  

•                 MAPREG(TRCVIPCSSCTLR, vipcssctlr);
  

•                 MAPREG(TRCSEQRSTEVR, seq_rst);
  

•                 MAPREG(TRCSEQSTR, seq_state);
  

•                 MAPREG(TRCEXTINSELR, ext_inp);
  

•                 MAPREG(TRCCIDCCTLR0, ctxid_mask0);
  

•                 MAPREG(TRCCIDCCTLR1, ctxid_mask1);
  

•                 MAPREG(TRCVMIDCCTLR0, vmid_mask0);
  

•                 MAPREG(TRCVMIDCCTLR1, vmid_mask1);
  

•         default:
  

•                 err = -EINVAL;
  

•                 break;
  

•         }
  

• } else if ((offset & GENMASK(11, 4)) == TRCSEQEVRn(0)) {
  

•         /* sequencer state control registers */
  

•         idx = (offset & GENMASK(3, 0)) / 4;
  

•         if (idx < ETM_MAX_SEQ_STATES)
  

•                 reg->drv_store = &drvcfg->seq_ctrl[idx];
  

•         else
  

•                 err = -EINVAL;
  

• } else if ((offset >= TRCSSCCRn(0)) && (offset <= TRCSSPCICRn(7))) {
  

•         /* 32 bit, 8 off indexed register sets */
  

•         idx = (offset & GENMASK(4, 0)) / 4;
  

•         switch (offset & GENMASK(11, 5)) {
  

•                 MAPREGIDX(TRCSSCCRn(0), ss_ctrl, idx);
  

•                 MAPREGIDX(TRCSSCSRn(0), ss_status, idx);
  

•                 MAPREGIDX(TRCSSPCICRn(0), ss_pe_cmp, idx);
  

•         default:
  

•                 err = -EINVAL;
  

•                 break;
  

•         }
  

• } else if ((offset >= TRCCIDCVRn(0)) && (offset <= TRCVMIDCVRn(7))) {
  

•         /* 64 bit, 8 off indexed register sets */
  

•         idx = (offset & GENMASK(5, 0)) / 8;
  

•         switch (offset & GENMASK(11, 6)) {
  

•                 MAPREGIDX(TRCCIDCVRn(0), ctxid_pid, idx);
  

•                 MAPREGIDX(TRCVMIDCVRn(0), vmid_val, idx);
  

•         default:
  

•                 err = -EINVAL;
  

•                 break;
  

•         }
  

• } else if ((offset >= TRCRSCTLRn(2)) &&
  

•            (offset <= TRCRSCTLRn((ETM_MAX_RES_SEL - 1)))) {
  

•         /* 32 bit resource selection regs, 32 off, skip fixed 0,1 */
  

•         idx = (offset & GENMASK(6, 0)) / 4;
  

•         if (idx < ETM_MAX_RES_SEL)
  

•                 reg->drv_store = &drvcfg->res_ctrl[idx];
  

•         else
  

•                 err = -EINVAL;
  

• } else if ((offset >= TRCACVRn(0)) &&
  

•            (offset <= TRCACATRn((ETM_MAX_SINGLE_ADDR_CMP - 1)))) {
  

•         /* 64 bit addr cmp regs, 16 off */
  

•         idx = (offset & GENMASK(6, 0)) / 8;
  

•         switch (offset & GENMASK(11, 7)) {
  

•                 MAPREGIDX(TRCACVRn(0), addr_val, idx);
  

•                 MAPREGIDX(TRCACATRn(0), addr_acc, idx);
  

•         default:
  

•                 err = -EINVAL;
  

•                 break;
  

•         }
  

• } else if ((offset >= TRCCNTRLDVRn(0)) &&
  

•            (offset <= TRCCNTVRn((ETMv4_MAX_CNTR - 1)))) {
  

•         /* 32 bit counter regs, 4 off (ETMv4_MAX_CNTR - 1) */
  

•         idx = (offset &  GENMASK(3, 0)) / 4;
  

•         switch (offset &  GENMASK(11, 4)) {
  

•                 MAPREGIDX(TRCCNTRLDVRn(0), cntrldvr, idx);
  

•                 MAPREGIDX(TRCCNTCTLRn(0), cntr_ctrl, idx);
  

•                 MAPREGIDX(TRCCNTVRn(0), cntr_val, idx);
  

•         default:
  

•                 err = -EINVAL;
  

•                 break;
  

•         }
  

• } else {
  

•         err = -EINVAL;
  

• }
  

• return err;
  

+}

+/**

• • etm4_cfg_load_feature - load a feature into a device instance.
• • @csdev: An ETMv4 CoreSight device.
• • @feat_desc: The static feature descriptor to be loaded.
• • The function will load a feature instance into the device, based on the
• • supplied descriptor. The descriptor will be checked to ensure a valid
• • feature can be loaded for this ETMv4 device.
• • Parameter and register definitions will be converted into internal
• • structures that are used to set the values in the driver when the
• • feature is enabled for the device.
• */

+static int etm4_cfg_load_feature(struct coresight_device *csdev,

•                          const struct cs_cfg_feature_desc *feat_desc)
  

+{

• struct device *dev = csdev->dev.parent;
  

• struct etmv4_drvdata *drvdata = dev_get_drvdata(dev);
  

• struct cs_cfg_feature *feat;
  

• u32 offset;
  

• int i = 0, err = 0;
  

• feat = devm_kzalloc(dev, sizeof(struct cs_cfg_feature), GFP_KERNEL);
  

• if (!feat)
  

•         return -ENOMEM;
  

• feat->desc = feat_desc;
  

• feat->dev_spinlock = &drvdata->spinlock;
  

• feat->csdev = csdev;
  

• /* count the params regs and allocate space.  */
  

• while (feat_desc->params[i++].name)
  

•         feat->nr_params++;
  

• i = 0;
  

• while (feat_desc->regs[i++].flags)
  

•         feat->nr_regs++;
  

• feat->params = devm_kcalloc(dev, feat->nr_params,
  

•                             sizeof(struct cs_cfg_parameter),
  

•                             GFP_KERNEL);
  

• if (!feat->params)
  

•         return -ENOMEM;
  

• feat->regs = devm_kcalloc(dev, feat->nr_regs,
  

•                           sizeof(struct cs_cfg_reg), GFP_KERNEL);
  

• if (!feat->regs)
  

•         return -ENOMEM;
  

• /* load the parameters default values */
  

• for (i = 0; i < feat->nr_params; i++)
  

•         feat->params[i].feat = feat;
  

• /* process the register descriptions */
  

• for (i = 0; i < feat->nr_regs; i++) {
  

•         offset = feat_desc->regs[i].flags & CS_CFG_REG_ID_MASK;
  

•         err = etm4_cfg_map_reg_offset(drvdata, &feat->regs[i], offset);
  

•         if (err)
  

•                 return err;
  

• }
  

• /* set sysfs representation of the feature */
  

• err = coresight_cfg_sysfs_add_grp(feat);
  

• /* add the feature to the device list and init default values */
  

• if (!err) {
  

•         coresight_cfg_list_add_feat(feat);
  

•         coresight_cfg_set_def_ops(feat);
  

•         feat->ops.reset(feat);
  

• }
  

• return err;
  

+}

+/* match information when loading configurations */ +#define CS_CFG_ETM4_MATCH_FLAGS (CS_CFG_MATCH_CLASS_SRC_ALL | \

•                          CS_CFG_MATCH_CLASS_SRC_ETM4)
  

+int etm4_cs_cfg_register(struct coresight_device *csdev, const char *dev_name) +{

• struct cs_cfg_match_info cfg_info;
  

• struct cs_cfg_dev_feat_ops ops;
  

• cfg_info.dev_name = dev_name;
  

• cfg_info.flags = CS_CFG_ETM4_MATCH_FLAGS;
  

• ops.load_feat = &etm4_cfg_load_feature;
  

• return cscfg_register_csdev(csdev, &cfg_info, &ops);
  

+} diff --git a/drivers/hwtracing/coresight/coresight-etm4x-cfg.h b/drivers/hwtracing/coresight/coresight-etm4x-cfg.h new file mode 100644 index 000000000000..bf33c720b5e9 --- /dev/null +++ b/drivers/hwtracing/coresight/coresight-etm4x-cfg.h @@ -0,0 +1,29 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*

• • Copyright (c) 2014-2015, The Linux Foundation. All rights reserved.
• */

+#ifndef _CORESIGHT_CORESIGHT_ETM4X_CFG_H +#define _CORESIGHT_CORESIGHT_ETM4X_CFG_H

+#include "coresight-config.h" +#include "coresight-etm4x.h"

+/* ETMv4 specific config defines */

+/* resource IDs */

+#define ETM4_CFG_RES_CTR 0x00001000 +#define ETM4_CFG_RES_CMP 0x00002000 +#define ETM4_CFG_RES_CMP_PAIR0 0x00003000 +#define ETM4_CFG_RES_CMP_PAIR1 0x00004000 +#define ETM4_CFG_RES_SEL 0x00005000 +#define ETM4_CFG_RES_SEL_PAIR0 0x00006000 +#define ETM4_CFG_RES_SEL_PAIR1 0x00007000 +#define ETM4_CFG_RES_SEQ 0x00008000 +#define ETM4_CFG_RES_TS 0x00009000 +#define ETM4_CFG_RES_MASK 0x0000F000

+int etm4_cs_cfg_register(struct coresight_device *csdev, const char *dev_name);

+#endif /* _CORESIGHT_CORESIGHT_ETM4X_CFG_H */ diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c index 56aed38cd0fa..3a58f835eb63 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c @@ -34,6 +34,8 @@

#include "coresight-etm4x.h" #include "coresight-etm-perf.h" +#include "coresight-etm4x-cfg.h" +#include "coresight-syscfg.h"

static int boot_enable; module_param(boot_enable, int, 0444); @@ -320,12 +322,15 @@ static int etm4_config_timestamp_event(struct etmv4_drvdata *drvdata) return ret; }

-static int etm4_parse_event_config(struct etmv4_drvdata *drvdata, +static int etm4_parse_event_config(struct coresight_device *csdev, struct perf_event *event) { int ret = 0;

• struct etmv4_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
  struct etmv4_config *config = &drvdata->config;
  struct perf_event_attr *attr = &event->attr;
  

• unsigned long cfg_id;
  

• int preset;
  
  if (!attr) {
          ret = -EINVAL;
  

@@ -383,6 +388,13 @@ static int etm4_parse_event_config(struct etmv4_drvdata *drvdata, /* bit[12], Return stack enable bit */ config->cfg |= BIT(12);

• /* set any selected configuration */
  

• if (attr->config2 & BIT(CSSYS_BIT_CONFIG_SEL)) {
  

•         cfg_id = (u32)attr->config2;
  

•         preset = (attr->config2 >> 32) & 0xF;
  

If we follow my previous suggestion then the above would look like:

    preset = (u32)attr->config1 & 0xF;
    cfg_id = attr->config2 >> 32;

Just shifting by 32 is obvious when you have this set in mind but insanely difficult to understand for someone trying to figure out what we did. You can use a hard coded value but add a comment that clearly describe what is happening and also reference the PMU_FORMAT_ATTR(configid) at the top of the file.

OK will comment / adjust accordingly & have followed your previous idea..

...

•         ret = coresight_cfg_enable_dev_config(csdev, cfg_id, preset);
  

• }
  

out: return ret; } @@ -399,7 +411,7 @@ static int etm4_enable_perf(struct coresight_device *csdev, }

  /* Configure the tracer based on the session's specifics */

• ret = etm4_parse_event_config(drvdata, event);
  

• ret = etm4_parse_event_config(csdev, event);
  if (ret)
          goto out;
  /* And enable it */
  

@@ -415,6 +427,8 @@ static int etm4_enable_sysfs(struct coresight_device *csdev) struct etm4_enable_arg arg = { }; int ret;

• coresight_cfg_set_enabled_feats(csdev);
  

Above in etm4_parse_event_config() function coresight_cfg_enable_dev_config() is used, which implies that when using the perf interface configurations are used. But here coresight_cfg_set_enabled_feats() is called and as such from sysfs features are used.

I think it would be best to use configurations for both perf and sysfs to avoid confusion.

The key difference here is that perf can only ever select a configuration. Because sysfs./ configfs is more flexible it is possible for sysfs to activate a feature on an individual device outside of the concept of a system configuration. The thought here being that a feature can be used, perhaps alongside some other specific programming in sysfs to find a combination that works for a given task. This allows a user to try out things which may end up being used later as a proper configuration.

Thanks

Mike

Thanks, Mathieu

...

  spin_lock(&drvdata->spinlock);

  /*

@@ -529,11 +543,14 @@ static int etm4_disable_perf(struct coresight_device *csdev, u32 control; struct etm_filters *filters = event->hw.addr_filters; struct etmv4_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);

• struct perf_event_attr *attr = &event->attr;
  
  if (WARN_ON_ONCE(drvdata->cpu != smp_processor_id()))
          return -EINVAL;
  
  etm4_disable_hw(drvdata);
  

• if (attr->config2 & BIT(CSSYS_BIT_CONFIG_SEL))
  

•         coresight_cfg_disable_dev_config(csdev);
  
  /*
   * Check if the start/stop logic was active when the unit was stopped.
  

@@ -568,6 +585,7 @@ static void etm4_disable_sysfs(struct coresight_device *csdev) smp_call_function_single(drvdata->cpu, etm4_disable_hw, drvdata, 1);

  spin_unlock(&drvdata->spinlock);

• coresight_cfg_save_enabled_feats(csdev);
  cpus_read_unlock();
  
  dev_dbg(&csdev->dev, "ETM tracing disabled\n");
  

@@ -1519,6 +1537,13 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id) return ret; }

• /* register with config infrastructure & load any current features */
  

• ret = etm4_cs_cfg_register(drvdata->csdev, dev_name(dev));
  

• if (ret) {
  

•         coresight_unregister(drvdata->csdev);
  

•         return ret;
  

• }
  

• etmdrvdata[drvdata->cpu] = drvdata;
  
  pm_runtime_put(&adev->dev);
  

@@ -1571,6 +1596,7 @@ static int __exit etm4_remove(struct amba_device *adev)

  cpus_read_unlock();

• cscfg_unregister_csdev(drvdata->csdev);
  coresight_unregister(drvdata->csdev);
  
  return 0;
  

diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c index b673e738bc9a..ec182544a064 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c @@ -9,6 +9,7 @@ #include <linux/sysfs.h> #include "coresight-etm4x.h" #include "coresight-priv.h" +#include "coresight-config.h"

static int etm4_set_mode_exclude(struct etmv4_drvdata *drvdata, bool exclude) { @@ -269,6 +270,8 @@ static ssize_t reset_store(struct device *dev,

  spin_unlock(&drvdata->spinlock);

• coresight_cfg_reset_feats(to_coresight_device(dev));
  

• return size;
  

} static DEVICE_ATTR_WO(reset); -- 2.17.1