Re: [RFC 2/4] coresight: etm4x: Support CS_MODE_API

On Tue, Sep 04, 2018 at 01:45:46PM +0300, mike@perception-point.io wrote:

From: Mike Bazov mike@perception-point.io

Adding support for CS_MODE_API mode in the ETMv4 implementation. The enable()/disable() function signatures are changed to a generic form, to support other modes in addition to perf.

The CS_MODE_API etmv4 implementation receives a etmv4 configuration structure(introduced in this commit and defined in linux/coresight.h), configures the etmv4 on the __current__ cpu according to the struct, and enables it.

When writing a changelog, please concentrate on why you're doing something rather than what you're doing.

A changelog should concentrate on _why_ rather than _what_.

In addition, the stm, etm3x are also made aware of the CS_MODE_API, without supporting it.

Here etm3x is mentionned by nothing is done about it.

Signed-off-by: Mike Bazov mike@perception-point.io

drivers/hwtracing/coresight/coresight-etm4x.c | 66 +++++++++++++++++++++++++-- drivers/hwtracing/coresight/coresight-stm.c | 4 +- include/linux/coresight.h | 22 ++++++++- 3 files changed, 83 insertions(+), 9 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etm4x.c b/drivers/hwtracing/coresight/coresight-etm4x.c index c1dcc7c289a5..f102b16617d0 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x.c +++ b/drivers/hwtracing/coresight/coresight-etm4x.c @@ -31,6 +31,7 @@ #include "coresight-etm4x.h" #include "coresight-etm-perf.h" +#include "coresight-priv.h" static int boot_enable; module_param_named(boot_enable, boot_enable, int, S_IRUGO); @@ -39,6 +40,7 @@ module_param_named(boot_enable, boot_enable, int, S_IRUGO); static int etm4_count; static struct etmv4_drvdata *etmdrvdata[NR_CPUS]; static void etm4_set_default_config(struct etmv4_config *config); +static void etm4_set_default_filter(struct etmv4_config *config); static int etm4_set_event_filters(struct etmv4_drvdata *drvdata, struct perf_event *event); @@ -226,6 +228,42 @@ static int etm4_parse_event_config(struct etmv4_drvdata *drvdata, return ret; } +static void etm4_apply_config(struct coresight_device *csdev,

• 	      struct etm_config *etm_config)
  

+{

• struct etmv4_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
• struct etmv4_config *config = &drvdata->config;
• /* Clear configuration from previous run */
• memset(config, 0, sizeof(struct etmv4_config));
• if (!etm_config->kernel)

• config->mode = ETM_MODE_EXCL_KERN;
  

• if (!etm_config->user)

• config->mode = ETM_MODE_EXCL_USER;
  

• /* Always start from the default config */
• etm4_set_default_config(config);
• etm4_set_default_filter(config);
• if (etm_config->cycacc) {

• config->cfg |= BIT(4);
  

• /* TRM: Must program this for cycacc to work */
  

• config->ccctlr = ETM_CYC_THRESHOLD_DEFAULT;
  

• }
• if (etm_config->timestamp)

• /* bit[11], Global timestamp tracing bit */
  

• config->cfg |= BIT(11);
  

• /* return stack - enable if selected and supported */
• if (etm_config->retstack && drvdata->retstack)

• /* bit[12], Return stack enable bit */
  

• config->cfg |= BIT(12);
  

• drvdata->trcid = etm_config->trace_id;

+}

Please reuse the current etm4 configuration routine rather than spinning off another one. If the original one doesn't fit your need split it up in smaller chunks. That way we have only one configuration scheme to maintain.

static int etm4_enable_perf(struct coresight_device *csdev, struct perf_event *event) { @@ -275,8 +313,23 @@ static int etm4_enable_sysfs(struct coresight_device *csdev) return ret; } +static int etm4_enable_api(struct coresight_device *csdev,

• 	   struct etm_config *config)
  

+{

• struct etmv4_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent);
• if (WARN_ON_ONCE(drvdata->cpu != smp_processor_id()))

• return -EINVAL;
  

• /* Apply the request config. */
• etm4_apply_config(csdev, config);
• etm4_enable_hw(drvdata);
• return 0;

+}

static int etm4_enable(struct coresight_device *csdev,

•        struct perf_event *event, u32 mode)
  

•        void *config, u32 mode)
  

{ int ret; u32 val; @@ -293,7 +346,10 @@ static int etm4_enable(struct coresight_device *csdev, ret = etm4_enable_sysfs(csdev); break; case CS_MODE_PERF:

• ret = etm4_enable_perf(csdev, event);
  

• ret = etm4_enable_perf(csdev, config);
  

• break;
  

• case CS_MODE_API:

• ret = etm4_enable_api(csdev, config);
  

  break; default: ret = -EINVAL;

@@ -384,7 +440,7 @@ static void etm4_disable_sysfs(struct coresight_device *csdev) } static void etm4_disable(struct coresight_device *csdev,

• 	 struct perf_event *event)
  

• 	 void *config)
  

{ u32 mode; struct etmv4_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); @@ -400,10 +456,11 @@ static void etm4_disable(struct coresight_device *csdev, case CS_MODE_DISABLED: break; case CS_MODE_SYSFS:

• case CS_MODE_API: /* Disabling via API mode is the same as sysfs mode */ etm4_disable_sysfs(csdev); break; case CS_MODE_PERF:

• etm4_disable_perf(csdev, event);
  

• etm4_disable_perf(csdev, config);
  

  break; }

@@ -826,7 +883,6 @@ static int etm4_set_event_filters(struct etmv4_drvdata *drvdata, address = (type == ETM_ADDR_TYPE_START ? filter->start_addr : filter->stop_addr);

Extra cosmetic repair

	/* Configure comparator */
	etm4_set_start_stop_filter(config, address,
				   comparator, type);

diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c index 35d6f9709274..7cb1d7525dde 100644 --- a/drivers/hwtracing/coresight/coresight-stm.c +++ b/drivers/hwtracing/coresight/coresight-stm.c @@ -191,7 +191,7 @@ static void stm_enable_hw(struct stm_drvdata *drvdata) } static int stm_enable(struct coresight_device *csdev,

•       struct perf_event *event, u32 mode)
  

•       void *config, u32 mode)
  

{ u32 val; struct stm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); @@ -254,7 +254,7 @@ static void stm_disable_hw(struct stm_drvdata *drvdata) } static void stm_disable(struct coresight_device *csdev,

• 	struct perf_event *event)
  

• 	void *config)
  

{ struct stm_drvdata *drvdata = dev_get_drvdata(csdev->dev.parent); diff --git a/include/linux/coresight.h b/include/linux/coresight.h index 8f7f98570afd..c29a3bb4c386 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -227,9 +227,9 @@ struct coresight_ops_source { int (*cpu_id)(struct coresight_device *csdev); int (*trace_id)(struct coresight_device *csdev); int (*enable)(struct coresight_device *csdev,

•       struct perf_event *event,  u32 mode);
  

•       void *config,  u32 mode);
  

  void (*disable)(struct coresight_device *csdev,

• 	struct perf_event *event);
  

• 	void *config);
  

}; /** @@ -253,6 +253,24 @@ struct coresight_ops { const struct coresight_ops_helper *helper_ops; }; +/**

• • struct etm_config - Generic etm configuration.
• • @retstack: enable retstack.
• • @user: trace user mode.
• • @kernel: trace kernel mode.
• • @timestamp: enable timestamp packets.
• • @cycacc: enable acc
• • @trace_id: trace id.
• */

+struct etm_config {

• bool retstack; /* enable retstack */
• bool user; /* trace user mode */
• bool kernel; /* trace kernel mode */
• bool timestamp; /* timestamp packets */
• bool cycacc; /* cyc packets */
• int trace_id; /* etm trace id */

+};

This part is very problematic. First it is bound to change as more options are required by users. This is the issue we currently have for perf were we need to expand the configuration option to reflect what the HW can do. Ultimately I'd like to have the same structure for both the API and perf mode.

A word of warning here... Fixing this will be difficult and require a fair amount of discussion.

#ifdef CONFIG_CORESIGHT extern struct coresight_device * coresight_register(struct coresight_desc *desc); -- 2.16.2