All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v5] coresight: etm4x: Modify core-commit of cpu to avoid the overflow of HiSilicon ETM
@ 2020-11-26 13:34 ` Qi Liu
  0 siblings, 0 replies; 16+ messages in thread
From: Qi Liu @ 2020-11-26 13:34 UTC (permalink / raw)
  To: mathieu.poirier, suzuki.poulose, mike.leach
  Cc: coresight, linux-kernel, linux-arm-kernel, linuxarm

The ETM device can't keep up with the core pipeline when cpu core
is at full speed. This may cause overflow within core and its ETM.
This is a common phenomenon on ETM devices.

On HiSilicon Hip08 platform, a specific feature is added to set
core pipeline. So commit rate can be reduced manually to avoid ETM
overflow.

Signed-off-by: Qi Liu <liuqi115@huawei.com>
---
Change since v1:
- add CONFIG_ETM4X_IMPDEF_FEATURE and CONFIG_ETM4X_IMPDEF_HISILICON
  to keep specific feature off platforms which don't use it.
Change since v2:
- remove some unused variable.
Change since v3:
- use read/write_sysreg_s() to access register.
Change since v4:
- rename the call back function to a more generic name, and fix some
  compile warnings.

 drivers/hwtracing/coresight/Kconfig                |  9 +++
 drivers/hwtracing/coresight/coresight-etm4x-core.c | 88 ++++++++++++++++++++++
 drivers/hwtracing/coresight/coresight-etm4x.h      |  8 ++
 3 files changed, 105 insertions(+)

diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index c119824..1cc3601 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -110,6 +110,15 @@ config CORESIGHT_SOURCE_ETM4X
 	  To compile this driver as a module, choose M here: the
 	  module will be called coresight-etm4x.

+config ETM4X_IMPDEF_FEATURE
+	bool "Control overflow impdef support in CoreSight ETM 4.x driver "
+	depends on CORESIGHT_SOURCE_ETM4X
+	help
+	  This control provides overflow implement define for CoreSight
+	  ETM 4.x tracer module which could not reduce commit race
+	  automatically, and could avoid overflow within ETM tracer module
+	  and its cpu core.
+
 config CORESIGHT_STM
 	tristate "CoreSight System Trace Macrocell driver"
 	depends on (ARM && !(CPU_32v3 || CPU_32v4 || CPU_32v4T)) || ARM64
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index abd706b..fcee27a 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -3,6 +3,7 @@
  * Copyright (c) 2014, The Linux Foundation. All rights reserved.
  */

+#include <linux/bitops.h>
 #include <linux/kernel.h>
 #include <linux/moduleparam.h>
 #include <linux/init.h>
@@ -28,7 +29,9 @@
 #include <linux/perf_event.h>
 #include <linux/pm_runtime.h>
 #include <linux/property.h>
+
 #include <asm/sections.h>
+#include <asm/sysreg.h>
 #include <asm/local.h>
 #include <asm/virt.h>

@@ -103,6 +106,87 @@ struct etm4_enable_arg {
 	int rc;
 };

+#ifdef CONFIG_ETM4X_IMPDEF_FEATURE
+
+#define HISI_HIP08_AMBA_ID		0x000b6d01
+#define ETM4_AMBA_MASK			0xfffff
+#define HISI_HIP08_CORE_COMMIT_CLEAR	0x3000
+#define HISI_HIP08_CORE_COMMIT_SHIFT	12
+#define HISI_HIP08_CORE_COMMIT_REG	sys_reg(3, 1, 15, 2, 5)
+
+struct etm4_arch_features {
+	void (*arch_callback)(bool enable);
+};
+
+static bool etm4_hisi_match_pid(unsigned int id)
+{
+	return (id & ETM4_AMBA_MASK) == HISI_HIP08_AMBA_ID;
+}
+
+static void etm4_hisi_config_core_commit(bool enable)
+{
+	u64 val;
+
+	val = read_sysreg_s(HISI_HIP08_CORE_COMMIT_REG);
+	val &= ~HISI_HIP08_CORE_COMMIT_CLEAR;
+	val |= enable << HISI_HIP08_CORE_COMMIT_SHIFT;
+	write_sysreg_s(val, HISI_HIP08_CORE_COMMIT_REG);
+}
+
+static struct etm4_arch_features etm4_features[] = {
+	[ETM4_IMPDEF_HISI_CORE_COMMIT] = {
+		.arch_callback = etm4_hisi_config_core_commit,
+	},
+	{},
+};
+
+static void etm4_enable_arch_specific(struct etmv4_drvdata *drvdata)
+{
+	struct etm4_arch_features *ftr;
+	int bit;
+
+	for_each_set_bit(bit, drvdata->arch_features, ETM4_IMPDEF_FEATURE_MAX) {
+		ftr = &etm4_features[bit];
+
+		if (ftr->arch_callback)
+			ftr->arch_callback(true);
+	}
+}
+
+static void etm4_disable_arch_specific(struct etmv4_drvdata *drvdata)
+{
+	struct etm4_arch_features *ftr;
+	int bit;
+
+	for_each_set_bit(bit, drvdata->arch_features, ETM4_IMPDEF_FEATURE_MAX) {
+		ftr = &etm4_features[bit];
+
+		if (ftr->arch_callback)
+			ftr->arch_callback(false);
+	}
+}
+
+static void etm4_check_arch_features(struct etmv4_drvdata *drvdata,
+				      unsigned int id)
+{
+	if (etm4_hisi_match_pid(id))
+		set_bit(ETM4_IMPDEF_HISI_CORE_COMMIT, drvdata->arch_features);
+}
+#else
+static void etm4_enable_arch_specific(struct etmv4_drvdata *drvdata)
+{
+}
+
+static void etm4_disable_arch_specific(struct etmv4_drvdata *drvdata)
+{
+}
+
+static void etm4_check_arch_features(struct etmv4_drvdata *drvdata,
+				     unsigned int id)
+{
+}
+#endif /* CONFIG_ETM4X_IMPDEF_FEATURE */
+
 static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
 {
 	int i, rc;
@@ -110,6 +194,7 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
 	struct device *etm_dev = &drvdata->csdev->dev;

 	CS_UNLOCK(drvdata->base);
+	etm4_enable_arch_specific(drvdata);

 	etm4_os_unlock(drvdata);

@@ -476,6 +561,7 @@ static void etm4_disable_hw(void *info)
 	int i;

 	CS_UNLOCK(drvdata->base);
+	etm4_disable_arch_specific(drvdata);

 	if (!drvdata->skip_power_up) {
 		/* power can be removed from the trace unit now */
@@ -1547,6 +1633,8 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id)
 		drvdata->boot_enable = true;
 	}

+	etm4_check_arch_features(drvdata, id->id);
+
 	return 0;
 }

diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
index eefc737..3dd3e06 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.h
+++ b/drivers/hwtracing/coresight/coresight-etm4x.h
@@ -8,6 +8,7 @@

 #include <asm/local.h>
 #include <linux/spinlock.h>
+#include <linux/types.h>
 #include "coresight-priv.h"

 /*
@@ -203,6 +204,11 @@
 /* Interpretation of resource numbers change at ETM v4.3 architecture */
 #define ETM4X_ARCH_4V3	0x43

+enum etm_impdef_type {
+	ETM4_IMPDEF_HISI_CORE_COMMIT,
+	ETM4_IMPDEF_FEATURE_MAX,
+};
+
 /**
  * struct etmv4_config - configuration information related to an ETMv4
  * @mode:	Controls various modes supported by this ETM.
@@ -415,6 +421,7 @@ struct etmv4_save_state {
  * @state_needs_restore: True when there is context to restore after PM exit
  * @skip_power_up: Indicates if an implementation can skip powering up
  *		   the trace unit.
+ * @arch_features: Bitmap of arch features of etmv4 devices.
  */
 struct etmv4_drvdata {
 	void __iomem			*base;
@@ -463,6 +470,7 @@ struct etmv4_drvdata {
 	struct etmv4_save_state		*save_state;
 	bool				state_needs_restore;
 	bool				skip_power_up;
+	DECLARE_BITMAP(arch_features, ETM4_IMPDEF_FEATURE_MAX);
 };

 /* Address comparator access types */
--
2.8.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH v5] coresight: etm4x: Modify core-commit of cpu to avoid the overflow of HiSilicon ETM
@ 2020-11-26 13:34 ` Qi Liu
  0 siblings, 0 replies; 16+ messages in thread
From: Qi Liu @ 2020-11-26 13:34 UTC (permalink / raw)
  To: mathieu.poirier, suzuki.poulose, mike.leach
  Cc: coresight, linux-kernel, linux-arm-kernel, linuxarm

The ETM device can't keep up with the core pipeline when cpu core
is at full speed. This may cause overflow within core and its ETM.
This is a common phenomenon on ETM devices.

On HiSilicon Hip08 platform, a specific feature is added to set
core pipeline. So commit rate can be reduced manually to avoid ETM
overflow.

Signed-off-by: Qi Liu <liuqi115@huawei.com>
---
Change since v1:
- add CONFIG_ETM4X_IMPDEF_FEATURE and CONFIG_ETM4X_IMPDEF_HISILICON
  to keep specific feature off platforms which don't use it.
Change since v2:
- remove some unused variable.
Change since v3:
- use read/write_sysreg_s() to access register.
Change since v4:
- rename the call back function to a more generic name, and fix some
  compile warnings.

 drivers/hwtracing/coresight/Kconfig                |  9 +++
 drivers/hwtracing/coresight/coresight-etm4x-core.c | 88 ++++++++++++++++++++++
 drivers/hwtracing/coresight/coresight-etm4x.h      |  8 ++
 3 files changed, 105 insertions(+)

diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
index c119824..1cc3601 100644
--- a/drivers/hwtracing/coresight/Kconfig
+++ b/drivers/hwtracing/coresight/Kconfig
@@ -110,6 +110,15 @@ config CORESIGHT_SOURCE_ETM4X
 	  To compile this driver as a module, choose M here: the
 	  module will be called coresight-etm4x.

+config ETM4X_IMPDEF_FEATURE
+	bool "Control overflow impdef support in CoreSight ETM 4.x driver "
+	depends on CORESIGHT_SOURCE_ETM4X
+	help
+	  This control provides overflow implement define for CoreSight
+	  ETM 4.x tracer module which could not reduce commit race
+	  automatically, and could avoid overflow within ETM tracer module
+	  and its cpu core.
+
 config CORESIGHT_STM
 	tristate "CoreSight System Trace Macrocell driver"
 	depends on (ARM && !(CPU_32v3 || CPU_32v4 || CPU_32v4T)) || ARM64
diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
index abd706b..fcee27a 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
+++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
@@ -3,6 +3,7 @@
  * Copyright (c) 2014, The Linux Foundation. All rights reserved.
  */

+#include <linux/bitops.h>
 #include <linux/kernel.h>
 #include <linux/moduleparam.h>
 #include <linux/init.h>
@@ -28,7 +29,9 @@
 #include <linux/perf_event.h>
 #include <linux/pm_runtime.h>
 #include <linux/property.h>
+
 #include <asm/sections.h>
+#include <asm/sysreg.h>
 #include <asm/local.h>
 #include <asm/virt.h>

@@ -103,6 +106,87 @@ struct etm4_enable_arg {
 	int rc;
 };

+#ifdef CONFIG_ETM4X_IMPDEF_FEATURE
+
+#define HISI_HIP08_AMBA_ID		0x000b6d01
+#define ETM4_AMBA_MASK			0xfffff
+#define HISI_HIP08_CORE_COMMIT_CLEAR	0x3000
+#define HISI_HIP08_CORE_COMMIT_SHIFT	12
+#define HISI_HIP08_CORE_COMMIT_REG	sys_reg(3, 1, 15, 2, 5)
+
+struct etm4_arch_features {
+	void (*arch_callback)(bool enable);
+};
+
+static bool etm4_hisi_match_pid(unsigned int id)
+{
+	return (id & ETM4_AMBA_MASK) == HISI_HIP08_AMBA_ID;
+}
+
+static void etm4_hisi_config_core_commit(bool enable)
+{
+	u64 val;
+
+	val = read_sysreg_s(HISI_HIP08_CORE_COMMIT_REG);
+	val &= ~HISI_HIP08_CORE_COMMIT_CLEAR;
+	val |= enable << HISI_HIP08_CORE_COMMIT_SHIFT;
+	write_sysreg_s(val, HISI_HIP08_CORE_COMMIT_REG);
+}
+
+static struct etm4_arch_features etm4_features[] = {
+	[ETM4_IMPDEF_HISI_CORE_COMMIT] = {
+		.arch_callback = etm4_hisi_config_core_commit,
+	},
+	{},
+};
+
+static void etm4_enable_arch_specific(struct etmv4_drvdata *drvdata)
+{
+	struct etm4_arch_features *ftr;
+	int bit;
+
+	for_each_set_bit(bit, drvdata->arch_features, ETM4_IMPDEF_FEATURE_MAX) {
+		ftr = &etm4_features[bit];
+
+		if (ftr->arch_callback)
+			ftr->arch_callback(true);
+	}
+}
+
+static void etm4_disable_arch_specific(struct etmv4_drvdata *drvdata)
+{
+	struct etm4_arch_features *ftr;
+	int bit;
+
+	for_each_set_bit(bit, drvdata->arch_features, ETM4_IMPDEF_FEATURE_MAX) {
+		ftr = &etm4_features[bit];
+
+		if (ftr->arch_callback)
+			ftr->arch_callback(false);
+	}
+}
+
+static void etm4_check_arch_features(struct etmv4_drvdata *drvdata,
+				      unsigned int id)
+{
+	if (etm4_hisi_match_pid(id))
+		set_bit(ETM4_IMPDEF_HISI_CORE_COMMIT, drvdata->arch_features);
+}
+#else
+static void etm4_enable_arch_specific(struct etmv4_drvdata *drvdata)
+{
+}
+
+static void etm4_disable_arch_specific(struct etmv4_drvdata *drvdata)
+{
+}
+
+static void etm4_check_arch_features(struct etmv4_drvdata *drvdata,
+				     unsigned int id)
+{
+}
+#endif /* CONFIG_ETM4X_IMPDEF_FEATURE */
+
 static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
 {
 	int i, rc;
@@ -110,6 +194,7 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
 	struct device *etm_dev = &drvdata->csdev->dev;

 	CS_UNLOCK(drvdata->base);
+	etm4_enable_arch_specific(drvdata);

 	etm4_os_unlock(drvdata);

@@ -476,6 +561,7 @@ static void etm4_disable_hw(void *info)
 	int i;

 	CS_UNLOCK(drvdata->base);
+	etm4_disable_arch_specific(drvdata);

 	if (!drvdata->skip_power_up) {
 		/* power can be removed from the trace unit now */
@@ -1547,6 +1633,8 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id)
 		drvdata->boot_enable = true;
 	}

+	etm4_check_arch_features(drvdata, id->id);
+
 	return 0;
 }

diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
index eefc737..3dd3e06 100644
--- a/drivers/hwtracing/coresight/coresight-etm4x.h
+++ b/drivers/hwtracing/coresight/coresight-etm4x.h
@@ -8,6 +8,7 @@

 #include <asm/local.h>
 #include <linux/spinlock.h>
+#include <linux/types.h>
 #include "coresight-priv.h"

 /*
@@ -203,6 +204,11 @@
 /* Interpretation of resource numbers change at ETM v4.3 architecture */
 #define ETM4X_ARCH_4V3	0x43

+enum etm_impdef_type {
+	ETM4_IMPDEF_HISI_CORE_COMMIT,
+	ETM4_IMPDEF_FEATURE_MAX,
+};
+
 /**
  * struct etmv4_config - configuration information related to an ETMv4
  * @mode:	Controls various modes supported by this ETM.
@@ -415,6 +421,7 @@ struct etmv4_save_state {
  * @state_needs_restore: True when there is context to restore after PM exit
  * @skip_power_up: Indicates if an implementation can skip powering up
  *		   the trace unit.
+ * @arch_features: Bitmap of arch features of etmv4 devices.
  */
 struct etmv4_drvdata {
 	void __iomem			*base;
@@ -463,6 +470,7 @@ struct etmv4_drvdata {
 	struct etmv4_save_state		*save_state;
 	bool				state_needs_restore;
 	bool				skip_power_up;
+	DECLARE_BITMAP(arch_features, ETM4_IMPDEF_FEATURE_MAX);
 };

 /* Address comparator access types */
--
2.8.1


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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH v5] coresight: etm4x: Modify core-commit of cpu to avoid the overflow of HiSilicon ETM
  2020-11-26 13:34 ` Qi Liu
@ 2020-12-04 18:55   ` Mathieu Poirier
  -1 siblings, 0 replies; 16+ messages in thread
From: Mathieu Poirier @ 2020-12-04 18:55 UTC (permalink / raw)
  To: Qi Liu
  Cc: suzuki.poulose, mike.leach, coresight, linux-kernel,
	linux-arm-kernel, linuxarm

On Thu, Nov 26, 2020 at 09:34:30PM +0800, Qi Liu wrote:
> The ETM device can't keep up with the core pipeline when cpu core
> is at full speed. This may cause overflow within core and its ETM.
> This is a common phenomenon on ETM devices.
> 
> On HiSilicon Hip08 platform, a specific feature is added to set
> core pipeline. So commit rate can be reduced manually to avoid ETM
> overflow.
> 
> Signed-off-by: Qi Liu <liuqi115@huawei.com>
> ---
> Change since v1:
> - add CONFIG_ETM4X_IMPDEF_FEATURE and CONFIG_ETM4X_IMPDEF_HISILICON
>   to keep specific feature off platforms which don't use it.
> Change since v2:
> - remove some unused variable.
> Change since v3:
> - use read/write_sysreg_s() to access register.
> Change since v4:
> - rename the call back function to a more generic name, and fix some
>   compile warnings.
> 
>  drivers/hwtracing/coresight/Kconfig                |  9 +++
>  drivers/hwtracing/coresight/coresight-etm4x-core.c | 88 ++++++++++++++++++++++
>  drivers/hwtracing/coresight/coresight-etm4x.h      |  8 ++
>  3 files changed, 105 insertions(+)
> 
> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> index c119824..1cc3601 100644
> --- a/drivers/hwtracing/coresight/Kconfig
> +++ b/drivers/hwtracing/coresight/Kconfig
> @@ -110,6 +110,15 @@ config CORESIGHT_SOURCE_ETM4X
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called coresight-etm4x.
> 
> +config ETM4X_IMPDEF_FEATURE
> +	bool "Control overflow impdef support in CoreSight ETM 4.x driver "
> +	depends on CORESIGHT_SOURCE_ETM4X
> +	help
> +	  This control provides overflow implement define for CoreSight
> +	  ETM 4.x tracer module which could not reduce commit race
> +	  automatically, and could avoid overflow within ETM tracer module
> +	  and its cpu core.
> +
>  config CORESIGHT_STM
>  	tristate "CoreSight System Trace Macrocell driver"
>  	depends on (ARM && !(CPU_32v3 || CPU_32v4 || CPU_32v4T)) || ARM64
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index abd706b..fcee27a 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -3,6 +3,7 @@
>   * Copyright (c) 2014, The Linux Foundation. All rights reserved.
>   */
> 
> +#include <linux/bitops.h>
>  #include <linux/kernel.h>
>  #include <linux/moduleparam.h>
>  #include <linux/init.h>
> @@ -28,7 +29,9 @@
>  #include <linux/perf_event.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/property.h>
> +
>  #include <asm/sections.h>
> +#include <asm/sysreg.h>
>  #include <asm/local.h>
>  #include <asm/virt.h>
> 
> @@ -103,6 +106,87 @@ struct etm4_enable_arg {
>  	int rc;
>  };
> 
> +#ifdef CONFIG_ETM4X_IMPDEF_FEATURE
> +
> +#define HISI_HIP08_AMBA_ID		0x000b6d01
> +#define ETM4_AMBA_MASK			0xfffff
> +#define HISI_HIP08_CORE_COMMIT_CLEAR	0x3000

Here bit 12 and 13 are cleared but in etm4_hisi_config_core_commit() only bit 12
is set - is this intentional?  What is bit 13 for? 

> +#define HISI_HIP08_CORE_COMMIT_SHIFT	12
> +#define HISI_HIP08_CORE_COMMIT_REG	sys_reg(3, 1, 15, 2, 5)
> +
> +struct etm4_arch_features {
> +	void (*arch_callback)(bool enable);
> +};
> +
> +static bool etm4_hisi_match_pid(unsigned int id)
> +{
> +	return (id & ETM4_AMBA_MASK) == HISI_HIP08_AMBA_ID;
> +}
> +
> +static void etm4_hisi_config_core_commit(bool enable)
> +{
> +	u64 val;
> +
> +	val = read_sysreg_s(HISI_HIP08_CORE_COMMIT_REG);
> +	val &= ~HISI_HIP08_CORE_COMMIT_CLEAR;
> +	val |= enable << HISI_HIP08_CORE_COMMIT_SHIFT;
> +	write_sysreg_s(val, HISI_HIP08_CORE_COMMIT_REG);
> +}
> +
> +static struct etm4_arch_features etm4_features[] = {
> +	[ETM4_IMPDEF_HISI_CORE_COMMIT] = {
> +		.arch_callback = etm4_hisi_config_core_commit,
> +	},
> +	{},
> +};
> +
> +static void etm4_enable_arch_specific(struct etmv4_drvdata *drvdata)
> +{
> +	struct etm4_arch_features *ftr;
> +	int bit;
> +
> +	for_each_set_bit(bit, drvdata->arch_features, ETM4_IMPDEF_FEATURE_MAX) {
> +		ftr = &etm4_features[bit];
> +
> +		if (ftr->arch_callback)
> +			ftr->arch_callback(true);
> +	}
> +}
> +
> +static void etm4_disable_arch_specific(struct etmv4_drvdata *drvdata)
> +{
> +	struct etm4_arch_features *ftr;
> +	int bit;
> +
> +	for_each_set_bit(bit, drvdata->arch_features, ETM4_IMPDEF_FEATURE_MAX) {
> +		ftr = &etm4_features[bit];
> +
> +		if (ftr->arch_callback)
> +			ftr->arch_callback(false);
> +	}
> +}
> +
> +static void etm4_check_arch_features(struct etmv4_drvdata *drvdata,
> +				      unsigned int id)
> +{
> +	if (etm4_hisi_match_pid(id))
> +		set_bit(ETM4_IMPDEF_HISI_CORE_COMMIT, drvdata->arch_features);
> +}
> +#else
> +static void etm4_enable_arch_specific(struct etmv4_drvdata *drvdata)
> +{
> +}
> +
> +static void etm4_disable_arch_specific(struct etmv4_drvdata *drvdata)
> +{
> +}
> +
> +static void etm4_check_arch_features(struct etmv4_drvdata *drvdata,
> +				     unsigned int id)
> +{
> +}
> +#endif /* CONFIG_ETM4X_IMPDEF_FEATURE */
> +
>  static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
>  {
>  	int i, rc;
> @@ -110,6 +194,7 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
>  	struct device *etm_dev = &drvdata->csdev->dev;
> 
>  	CS_UNLOCK(drvdata->base);
> +	etm4_enable_arch_specific(drvdata);
> 
>  	etm4_os_unlock(drvdata);
> 
> @@ -476,6 +561,7 @@ static void etm4_disable_hw(void *info)
>  	int i;
> 
>  	CS_UNLOCK(drvdata->base);
> +	etm4_disable_arch_specific(drvdata);
> 
>  	if (!drvdata->skip_power_up) {
>  		/* power can be removed from the trace unit now */
> @@ -1547,6 +1633,8 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id)
>  		drvdata->boot_enable = true;
>  	}
> 
> +	etm4_check_arch_features(drvdata, id->id);
> +
>  	return 0;
>  }
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
> index eefc737..3dd3e06 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x.h
> +++ b/drivers/hwtracing/coresight/coresight-etm4x.h
> @@ -8,6 +8,7 @@
> 
>  #include <asm/local.h>
>  #include <linux/spinlock.h>
> +#include <linux/types.h>
>  #include "coresight-priv.h"
> 
>  /*
> @@ -203,6 +204,11 @@
>  /* Interpretation of resource numbers change at ETM v4.3 architecture */
>  #define ETM4X_ARCH_4V3	0x43
> 
> +enum etm_impdef_type {
> +	ETM4_IMPDEF_HISI_CORE_COMMIT,
> +	ETM4_IMPDEF_FEATURE_MAX,
> +};
> +
>  /**
>   * struct etmv4_config - configuration information related to an ETMv4
>   * @mode:	Controls various modes supported by this ETM.
> @@ -415,6 +421,7 @@ struct etmv4_save_state {
>   * @state_needs_restore: True when there is context to restore after PM exit
>   * @skip_power_up: Indicates if an implementation can skip powering up
>   *		   the trace unit.
> + * @arch_features: Bitmap of arch features of etmv4 devices.
>   */
>  struct etmv4_drvdata {
>  	void __iomem			*base;
> @@ -463,6 +470,7 @@ struct etmv4_drvdata {
>  	struct etmv4_save_state		*save_state;
>  	bool				state_needs_restore;
>  	bool				skip_power_up;
> +	DECLARE_BITMAP(arch_features, ETM4_IMPDEF_FEATURE_MAX);
>  };
> 
>  /* Address comparator access types */
> --
> 2.8.1
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v5] coresight: etm4x: Modify core-commit of cpu to avoid the overflow of HiSilicon ETM
@ 2020-12-04 18:55   ` Mathieu Poirier
  0 siblings, 0 replies; 16+ messages in thread
From: Mathieu Poirier @ 2020-12-04 18:55 UTC (permalink / raw)
  To: Qi Liu
  Cc: suzuki.poulose, coresight, linuxarm, linux-kernel,
	linux-arm-kernel, mike.leach

On Thu, Nov 26, 2020 at 09:34:30PM +0800, Qi Liu wrote:
> The ETM device can't keep up with the core pipeline when cpu core
> is at full speed. This may cause overflow within core and its ETM.
> This is a common phenomenon on ETM devices.
> 
> On HiSilicon Hip08 platform, a specific feature is added to set
> core pipeline. So commit rate can be reduced manually to avoid ETM
> overflow.
> 
> Signed-off-by: Qi Liu <liuqi115@huawei.com>
> ---
> Change since v1:
> - add CONFIG_ETM4X_IMPDEF_FEATURE and CONFIG_ETM4X_IMPDEF_HISILICON
>   to keep specific feature off platforms which don't use it.
> Change since v2:
> - remove some unused variable.
> Change since v3:
> - use read/write_sysreg_s() to access register.
> Change since v4:
> - rename the call back function to a more generic name, and fix some
>   compile warnings.
> 
>  drivers/hwtracing/coresight/Kconfig                |  9 +++
>  drivers/hwtracing/coresight/coresight-etm4x-core.c | 88 ++++++++++++++++++++++
>  drivers/hwtracing/coresight/coresight-etm4x.h      |  8 ++
>  3 files changed, 105 insertions(+)
> 
> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> index c119824..1cc3601 100644
> --- a/drivers/hwtracing/coresight/Kconfig
> +++ b/drivers/hwtracing/coresight/Kconfig
> @@ -110,6 +110,15 @@ config CORESIGHT_SOURCE_ETM4X
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called coresight-etm4x.
> 
> +config ETM4X_IMPDEF_FEATURE
> +	bool "Control overflow impdef support in CoreSight ETM 4.x driver "
> +	depends on CORESIGHT_SOURCE_ETM4X
> +	help
> +	  This control provides overflow implement define for CoreSight
> +	  ETM 4.x tracer module which could not reduce commit race
> +	  automatically, and could avoid overflow within ETM tracer module
> +	  and its cpu core.
> +
>  config CORESIGHT_STM
>  	tristate "CoreSight System Trace Macrocell driver"
>  	depends on (ARM && !(CPU_32v3 || CPU_32v4 || CPU_32v4T)) || ARM64
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> index abd706b..fcee27a 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> @@ -3,6 +3,7 @@
>   * Copyright (c) 2014, The Linux Foundation. All rights reserved.
>   */
> 
> +#include <linux/bitops.h>
>  #include <linux/kernel.h>
>  #include <linux/moduleparam.h>
>  #include <linux/init.h>
> @@ -28,7 +29,9 @@
>  #include <linux/perf_event.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/property.h>
> +
>  #include <asm/sections.h>
> +#include <asm/sysreg.h>
>  #include <asm/local.h>
>  #include <asm/virt.h>
> 
> @@ -103,6 +106,87 @@ struct etm4_enable_arg {
>  	int rc;
>  };
> 
> +#ifdef CONFIG_ETM4X_IMPDEF_FEATURE
> +
> +#define HISI_HIP08_AMBA_ID		0x000b6d01
> +#define ETM4_AMBA_MASK			0xfffff
> +#define HISI_HIP08_CORE_COMMIT_CLEAR	0x3000

Here bit 12 and 13 are cleared but in etm4_hisi_config_core_commit() only bit 12
is set - is this intentional?  What is bit 13 for? 

> +#define HISI_HIP08_CORE_COMMIT_SHIFT	12
> +#define HISI_HIP08_CORE_COMMIT_REG	sys_reg(3, 1, 15, 2, 5)
> +
> +struct etm4_arch_features {
> +	void (*arch_callback)(bool enable);
> +};
> +
> +static bool etm4_hisi_match_pid(unsigned int id)
> +{
> +	return (id & ETM4_AMBA_MASK) == HISI_HIP08_AMBA_ID;
> +}
> +
> +static void etm4_hisi_config_core_commit(bool enable)
> +{
> +	u64 val;
> +
> +	val = read_sysreg_s(HISI_HIP08_CORE_COMMIT_REG);
> +	val &= ~HISI_HIP08_CORE_COMMIT_CLEAR;
> +	val |= enable << HISI_HIP08_CORE_COMMIT_SHIFT;
> +	write_sysreg_s(val, HISI_HIP08_CORE_COMMIT_REG);
> +}
> +
> +static struct etm4_arch_features etm4_features[] = {
> +	[ETM4_IMPDEF_HISI_CORE_COMMIT] = {
> +		.arch_callback = etm4_hisi_config_core_commit,
> +	},
> +	{},
> +};
> +
> +static void etm4_enable_arch_specific(struct etmv4_drvdata *drvdata)
> +{
> +	struct etm4_arch_features *ftr;
> +	int bit;
> +
> +	for_each_set_bit(bit, drvdata->arch_features, ETM4_IMPDEF_FEATURE_MAX) {
> +		ftr = &etm4_features[bit];
> +
> +		if (ftr->arch_callback)
> +			ftr->arch_callback(true);
> +	}
> +}
> +
> +static void etm4_disable_arch_specific(struct etmv4_drvdata *drvdata)
> +{
> +	struct etm4_arch_features *ftr;
> +	int bit;
> +
> +	for_each_set_bit(bit, drvdata->arch_features, ETM4_IMPDEF_FEATURE_MAX) {
> +		ftr = &etm4_features[bit];
> +
> +		if (ftr->arch_callback)
> +			ftr->arch_callback(false);
> +	}
> +}
> +
> +static void etm4_check_arch_features(struct etmv4_drvdata *drvdata,
> +				      unsigned int id)
> +{
> +	if (etm4_hisi_match_pid(id))
> +		set_bit(ETM4_IMPDEF_HISI_CORE_COMMIT, drvdata->arch_features);
> +}
> +#else
> +static void etm4_enable_arch_specific(struct etmv4_drvdata *drvdata)
> +{
> +}
> +
> +static void etm4_disable_arch_specific(struct etmv4_drvdata *drvdata)
> +{
> +}
> +
> +static void etm4_check_arch_features(struct etmv4_drvdata *drvdata,
> +				     unsigned int id)
> +{
> +}
> +#endif /* CONFIG_ETM4X_IMPDEF_FEATURE */
> +
>  static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
>  {
>  	int i, rc;
> @@ -110,6 +194,7 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
>  	struct device *etm_dev = &drvdata->csdev->dev;
> 
>  	CS_UNLOCK(drvdata->base);
> +	etm4_enable_arch_specific(drvdata);
> 
>  	etm4_os_unlock(drvdata);
> 
> @@ -476,6 +561,7 @@ static void etm4_disable_hw(void *info)
>  	int i;
> 
>  	CS_UNLOCK(drvdata->base);
> +	etm4_disable_arch_specific(drvdata);
> 
>  	if (!drvdata->skip_power_up) {
>  		/* power can be removed from the trace unit now */
> @@ -1547,6 +1633,8 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id)
>  		drvdata->boot_enable = true;
>  	}
> 
> +	etm4_check_arch_features(drvdata, id->id);
> +
>  	return 0;
>  }
> 
> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
> index eefc737..3dd3e06 100644
> --- a/drivers/hwtracing/coresight/coresight-etm4x.h
> +++ b/drivers/hwtracing/coresight/coresight-etm4x.h
> @@ -8,6 +8,7 @@
> 
>  #include <asm/local.h>
>  #include <linux/spinlock.h>
> +#include <linux/types.h>
>  #include "coresight-priv.h"
> 
>  /*
> @@ -203,6 +204,11 @@
>  /* Interpretation of resource numbers change at ETM v4.3 architecture */
>  #define ETM4X_ARCH_4V3	0x43
> 
> +enum etm_impdef_type {
> +	ETM4_IMPDEF_HISI_CORE_COMMIT,
> +	ETM4_IMPDEF_FEATURE_MAX,
> +};
> +
>  /**
>   * struct etmv4_config - configuration information related to an ETMv4
>   * @mode:	Controls various modes supported by this ETM.
> @@ -415,6 +421,7 @@ struct etmv4_save_state {
>   * @state_needs_restore: True when there is context to restore after PM exit
>   * @skip_power_up: Indicates if an implementation can skip powering up
>   *		   the trace unit.
> + * @arch_features: Bitmap of arch features of etmv4 devices.
>   */
>  struct etmv4_drvdata {
>  	void __iomem			*base;
> @@ -463,6 +470,7 @@ struct etmv4_drvdata {
>  	struct etmv4_save_state		*save_state;
>  	bool				state_needs_restore;
>  	bool				skip_power_up;
> +	DECLARE_BITMAP(arch_features, ETM4_IMPDEF_FEATURE_MAX);
>  };
> 
>  /* Address comparator access types */
> --
> 2.8.1
> 

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v5] coresight: etm4x: Modify core-commit of cpu to avoid the overflow of HiSilicon ETM
  2020-12-04 18:55   ` Mathieu Poirier
@ 2020-12-07  2:08     ` Qi Liu
  -1 siblings, 0 replies; 16+ messages in thread
From: Qi Liu @ 2020-12-07  2:08 UTC (permalink / raw)
  To: Mathieu Poirier
  Cc: suzuki.poulose, mike.leach, coresight, linux-kernel,
	linux-arm-kernel, linuxarm

Hi Mathieu,

On 2020/12/5 2:55, Mathieu Poirier wrote:
> On Thu, Nov 26, 2020 at 09:34:30PM +0800, Qi Liu wrote:
>> The ETM device can't keep up with the core pipeline when cpu core
>> is at full speed. This may cause overflow within core and its ETM.
>> This is a common phenomenon on ETM devices.
>>
>> On HiSilicon Hip08 platform, a specific feature is added to set
>> core pipeline. So commit rate can be reduced manually to avoid ETM
>> overflow.
>>
>> Signed-off-by: Qi Liu <liuqi115@huawei.com>
>> ---
>> Change since v1:
>> - add CONFIG_ETM4X_IMPDEF_FEATURE and CONFIG_ETM4X_IMPDEF_HISILICON
>>   to keep specific feature off platforms which don't use it.
>> Change since v2:
>> - remove some unused variable.
>> Change since v3:
>> - use read/write_sysreg_s() to access register.
>> Change since v4:
>> - rename the call back function to a more generic name, and fix some
>>   compile warnings.
>>
>>  drivers/hwtracing/coresight/Kconfig                |  9 +++
>>  drivers/hwtracing/coresight/coresight-etm4x-core.c | 88 ++++++++++++++++++++++
>>  drivers/hwtracing/coresight/coresight-etm4x.h      |  8 ++
>>  3 files changed, 105 insertions(+)
>>
>> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
>> index c119824..1cc3601 100644
>> --- a/drivers/hwtracing/coresight/Kconfig
>> +++ b/drivers/hwtracing/coresight/Kconfig
>> @@ -110,6 +110,15 @@ config CORESIGHT_SOURCE_ETM4X
>>  	  To compile this driver as a module, choose M here: the
>>  	  module will be called coresight-etm4x.
>>
>> +config ETM4X_IMPDEF_FEATURE
>> +	bool "Control overflow impdef support in CoreSight ETM 4.x driver "
>> +	depends on CORESIGHT_SOURCE_ETM4X
>> +	help
>> +	  This control provides overflow implement define for CoreSight
>> +	  ETM 4.x tracer module which could not reduce commit race
>> +	  automatically, and could avoid overflow within ETM tracer module
>> +	  and its cpu core.
>> +
>>  config CORESIGHT_STM
>>  	tristate "CoreSight System Trace Macrocell driver"
>>  	depends on (ARM && !(CPU_32v3 || CPU_32v4 || CPU_32v4T)) || ARM64
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> index abd706b..fcee27a 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> @@ -3,6 +3,7 @@
>>   * Copyright (c) 2014, The Linux Foundation. All rights reserved.
>>   */
>>
>> +#include <linux/bitops.h>
>>  #include <linux/kernel.h>
>>  #include <linux/moduleparam.h>
>>  #include <linux/init.h>
>> @@ -28,7 +29,9 @@
>>  #include <linux/perf_event.h>
>>  #include <linux/pm_runtime.h>
>>  #include <linux/property.h>
>> +
>>  #include <asm/sections.h>
>> +#include <asm/sysreg.h>
>>  #include <asm/local.h>
>>  #include <asm/virt.h>
>>
>> @@ -103,6 +106,87 @@ struct etm4_enable_arg {
>>  	int rc;
>>  };
>>
>> +#ifdef CONFIG_ETM4X_IMPDEF_FEATURE
>> +
>> +#define HISI_HIP08_AMBA_ID		0x000b6d01
>> +#define ETM4_AMBA_MASK			0xfffff
>> +#define HISI_HIP08_CORE_COMMIT_CLEAR	0x3000
> 
> Here bit 12 and 13 are cleared but in etm4_hisi_config_core_commit() only bit 12
> is set - is this intentional?  What is bit 13 for? 
> 
bit 12 and 13 are used together to set core-commit, 2'b00 means cpu is at full speed,
2'b01, 2'b10, 2'b11 means reduce the speed of cpu pipeline, and 2'b01 means speed is
reduced to minimum value. So bit 12 and 13 should be cleared together in
etm4_hisi_config_core_commit().

Qi

>> +#define HISI_HIP08_CORE_COMMIT_SHIFT	12
>> +#define HISI_HIP08_CORE_COMMIT_REG	sys_reg(3, 1, 15, 2, 5)
>> +
>> +struct etm4_arch_features {
>> +	void (*arch_callback)(bool enable);
>> +};
>> +
>> +static bool etm4_hisi_match_pid(unsigned int id)
>> +{
>> +	return (id & ETM4_AMBA_MASK) == HISI_HIP08_AMBA_ID;
>> +}
>> +
>> +static void etm4_hisi_config_core_commit(bool enable)
>> +{
>> +	u64 val;
>> +
>> +	val = read_sysreg_s(HISI_HIP08_CORE_COMMIT_REG);
>> +	val &= ~HISI_HIP08_CORE_COMMIT_CLEAR;
>> +	val |= enable << HISI_HIP08_CORE_COMMIT_SHIFT;
>> +	write_sysreg_s(val, HISI_HIP08_CORE_COMMIT_REG);
>> +}
>> +
>> +static struct etm4_arch_features etm4_features[] = {
>> +	[ETM4_IMPDEF_HISI_CORE_COMMIT] = {
>> +		.arch_callback = etm4_hisi_config_core_commit,
>> +	},
>> +	{},
>> +};
>> +
>> +static void etm4_enable_arch_specific(struct etmv4_drvdata *drvdata)
>> +{
>> +	struct etm4_arch_features *ftr;
>> +	int bit;
>> +
>> +	for_each_set_bit(bit, drvdata->arch_features, ETM4_IMPDEF_FEATURE_MAX) {
>> +		ftr = &etm4_features[bit];
>> +
>> +		if (ftr->arch_callback)
>> +			ftr->arch_callback(true);
>> +	}
>> +}
>> +
>> +static void etm4_disable_arch_specific(struct etmv4_drvdata *drvdata)
>> +{
>> +	struct etm4_arch_features *ftr;
>> +	int bit;
>> +
>> +	for_each_set_bit(bit, drvdata->arch_features, ETM4_IMPDEF_FEATURE_MAX) {
>> +		ftr = &etm4_features[bit];
>> +
>> +		if (ftr->arch_callback)
>> +			ftr->arch_callback(false);
>> +	}
>> +}
>> +
>> +static void etm4_check_arch_features(struct etmv4_drvdata *drvdata,
>> +				      unsigned int id)
>> +{
>> +	if (etm4_hisi_match_pid(id))
>> +		set_bit(ETM4_IMPDEF_HISI_CORE_COMMIT, drvdata->arch_features);
>> +}
>> +#else
>> +static void etm4_enable_arch_specific(struct etmv4_drvdata *drvdata)
>> +{
>> +}
>> +
>> +static void etm4_disable_arch_specific(struct etmv4_drvdata *drvdata)
>> +{
>> +}
>> +
>> +static void etm4_check_arch_features(struct etmv4_drvdata *drvdata,
>> +				     unsigned int id)
>> +{
>> +}
>> +#endif /* CONFIG_ETM4X_IMPDEF_FEATURE */
>> +
>>  static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
>>  {
>>  	int i, rc;
>> @@ -110,6 +194,7 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
>>  	struct device *etm_dev = &drvdata->csdev->dev;
>>
>>  	CS_UNLOCK(drvdata->base);
>> +	etm4_enable_arch_specific(drvdata);
>>
>>  	etm4_os_unlock(drvdata);
>>
>> @@ -476,6 +561,7 @@ static void etm4_disable_hw(void *info)
>>  	int i;
>>
>>  	CS_UNLOCK(drvdata->base);
>> +	etm4_disable_arch_specific(drvdata);
>>
>>  	if (!drvdata->skip_power_up) {
>>  		/* power can be removed from the trace unit now */
>> @@ -1547,6 +1633,8 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id)
>>  		drvdata->boot_enable = true;
>>  	}
>>
>> +	etm4_check_arch_features(drvdata, id->id);
>> +
>>  	return 0;
>>  }
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
>> index eefc737..3dd3e06 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x.h
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x.h
>> @@ -8,6 +8,7 @@
>>
>>  #include <asm/local.h>
>>  #include <linux/spinlock.h>
>> +#include <linux/types.h>
>>  #include "coresight-priv.h"
>>
>>  /*
>> @@ -203,6 +204,11 @@
>>  /* Interpretation of resource numbers change at ETM v4.3 architecture */
>>  #define ETM4X_ARCH_4V3	0x43
>>
>> +enum etm_impdef_type {
>> +	ETM4_IMPDEF_HISI_CORE_COMMIT,
>> +	ETM4_IMPDEF_FEATURE_MAX,
>> +};
>> +
>>  /**
>>   * struct etmv4_config - configuration information related to an ETMv4
>>   * @mode:	Controls various modes supported by this ETM.
>> @@ -415,6 +421,7 @@ struct etmv4_save_state {
>>   * @state_needs_restore: True when there is context to restore after PM exit
>>   * @skip_power_up: Indicates if an implementation can skip powering up
>>   *		   the trace unit.
>> + * @arch_features: Bitmap of arch features of etmv4 devices.
>>   */
>>  struct etmv4_drvdata {
>>  	void __iomem			*base;
>> @@ -463,6 +470,7 @@ struct etmv4_drvdata {
>>  	struct etmv4_save_state		*save_state;
>>  	bool				state_needs_restore;
>>  	bool				skip_power_up;
>> +	DECLARE_BITMAP(arch_features, ETM4_IMPDEF_FEATURE_MAX);
>>  };
>>
>>  /* Address comparator access types */
>> --
>> 2.8.1
>>
> 
> .
> 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v5] coresight: etm4x: Modify core-commit of cpu to avoid the overflow of HiSilicon ETM
@ 2020-12-07  2:08     ` Qi Liu
  0 siblings, 0 replies; 16+ messages in thread
From: Qi Liu @ 2020-12-07  2:08 UTC (permalink / raw)
  To: Mathieu Poirier
  Cc: suzuki.poulose, coresight, linuxarm, linux-kernel,
	linux-arm-kernel, mike.leach

Hi Mathieu,

On 2020/12/5 2:55, Mathieu Poirier wrote:
> On Thu, Nov 26, 2020 at 09:34:30PM +0800, Qi Liu wrote:
>> The ETM device can't keep up with the core pipeline when cpu core
>> is at full speed. This may cause overflow within core and its ETM.
>> This is a common phenomenon on ETM devices.
>>
>> On HiSilicon Hip08 platform, a specific feature is added to set
>> core pipeline. So commit rate can be reduced manually to avoid ETM
>> overflow.
>>
>> Signed-off-by: Qi Liu <liuqi115@huawei.com>
>> ---
>> Change since v1:
>> - add CONFIG_ETM4X_IMPDEF_FEATURE and CONFIG_ETM4X_IMPDEF_HISILICON
>>   to keep specific feature off platforms which don't use it.
>> Change since v2:
>> - remove some unused variable.
>> Change since v3:
>> - use read/write_sysreg_s() to access register.
>> Change since v4:
>> - rename the call back function to a more generic name, and fix some
>>   compile warnings.
>>
>>  drivers/hwtracing/coresight/Kconfig                |  9 +++
>>  drivers/hwtracing/coresight/coresight-etm4x-core.c | 88 ++++++++++++++++++++++
>>  drivers/hwtracing/coresight/coresight-etm4x.h      |  8 ++
>>  3 files changed, 105 insertions(+)
>>
>> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
>> index c119824..1cc3601 100644
>> --- a/drivers/hwtracing/coresight/Kconfig
>> +++ b/drivers/hwtracing/coresight/Kconfig
>> @@ -110,6 +110,15 @@ config CORESIGHT_SOURCE_ETM4X
>>  	  To compile this driver as a module, choose M here: the
>>  	  module will be called coresight-etm4x.
>>
>> +config ETM4X_IMPDEF_FEATURE
>> +	bool "Control overflow impdef support in CoreSight ETM 4.x driver "
>> +	depends on CORESIGHT_SOURCE_ETM4X
>> +	help
>> +	  This control provides overflow implement define for CoreSight
>> +	  ETM 4.x tracer module which could not reduce commit race
>> +	  automatically, and could avoid overflow within ETM tracer module
>> +	  and its cpu core.
>> +
>>  config CORESIGHT_STM
>>  	tristate "CoreSight System Trace Macrocell driver"
>>  	depends on (ARM && !(CPU_32v3 || CPU_32v4 || CPU_32v4T)) || ARM64
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> index abd706b..fcee27a 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>> @@ -3,6 +3,7 @@
>>   * Copyright (c) 2014, The Linux Foundation. All rights reserved.
>>   */
>>
>> +#include <linux/bitops.h>
>>  #include <linux/kernel.h>
>>  #include <linux/moduleparam.h>
>>  #include <linux/init.h>
>> @@ -28,7 +29,9 @@
>>  #include <linux/perf_event.h>
>>  #include <linux/pm_runtime.h>
>>  #include <linux/property.h>
>> +
>>  #include <asm/sections.h>
>> +#include <asm/sysreg.h>
>>  #include <asm/local.h>
>>  #include <asm/virt.h>
>>
>> @@ -103,6 +106,87 @@ struct etm4_enable_arg {
>>  	int rc;
>>  };
>>
>> +#ifdef CONFIG_ETM4X_IMPDEF_FEATURE
>> +
>> +#define HISI_HIP08_AMBA_ID		0x000b6d01
>> +#define ETM4_AMBA_MASK			0xfffff
>> +#define HISI_HIP08_CORE_COMMIT_CLEAR	0x3000
> 
> Here bit 12 and 13 are cleared but in etm4_hisi_config_core_commit() only bit 12
> is set - is this intentional?  What is bit 13 for? 
> 
bit 12 and 13 are used together to set core-commit, 2'b00 means cpu is at full speed,
2'b01, 2'b10, 2'b11 means reduce the speed of cpu pipeline, and 2'b01 means speed is
reduced to minimum value. So bit 12 and 13 should be cleared together in
etm4_hisi_config_core_commit().

Qi

>> +#define HISI_HIP08_CORE_COMMIT_SHIFT	12
>> +#define HISI_HIP08_CORE_COMMIT_REG	sys_reg(3, 1, 15, 2, 5)
>> +
>> +struct etm4_arch_features {
>> +	void (*arch_callback)(bool enable);
>> +};
>> +
>> +static bool etm4_hisi_match_pid(unsigned int id)
>> +{
>> +	return (id & ETM4_AMBA_MASK) == HISI_HIP08_AMBA_ID;
>> +}
>> +
>> +static void etm4_hisi_config_core_commit(bool enable)
>> +{
>> +	u64 val;
>> +
>> +	val = read_sysreg_s(HISI_HIP08_CORE_COMMIT_REG);
>> +	val &= ~HISI_HIP08_CORE_COMMIT_CLEAR;
>> +	val |= enable << HISI_HIP08_CORE_COMMIT_SHIFT;
>> +	write_sysreg_s(val, HISI_HIP08_CORE_COMMIT_REG);
>> +}
>> +
>> +static struct etm4_arch_features etm4_features[] = {
>> +	[ETM4_IMPDEF_HISI_CORE_COMMIT] = {
>> +		.arch_callback = etm4_hisi_config_core_commit,
>> +	},
>> +	{},
>> +};
>> +
>> +static void etm4_enable_arch_specific(struct etmv4_drvdata *drvdata)
>> +{
>> +	struct etm4_arch_features *ftr;
>> +	int bit;
>> +
>> +	for_each_set_bit(bit, drvdata->arch_features, ETM4_IMPDEF_FEATURE_MAX) {
>> +		ftr = &etm4_features[bit];
>> +
>> +		if (ftr->arch_callback)
>> +			ftr->arch_callback(true);
>> +	}
>> +}
>> +
>> +static void etm4_disable_arch_specific(struct etmv4_drvdata *drvdata)
>> +{
>> +	struct etm4_arch_features *ftr;
>> +	int bit;
>> +
>> +	for_each_set_bit(bit, drvdata->arch_features, ETM4_IMPDEF_FEATURE_MAX) {
>> +		ftr = &etm4_features[bit];
>> +
>> +		if (ftr->arch_callback)
>> +			ftr->arch_callback(false);
>> +	}
>> +}
>> +
>> +static void etm4_check_arch_features(struct etmv4_drvdata *drvdata,
>> +				      unsigned int id)
>> +{
>> +	if (etm4_hisi_match_pid(id))
>> +		set_bit(ETM4_IMPDEF_HISI_CORE_COMMIT, drvdata->arch_features);
>> +}
>> +#else
>> +static void etm4_enable_arch_specific(struct etmv4_drvdata *drvdata)
>> +{
>> +}
>> +
>> +static void etm4_disable_arch_specific(struct etmv4_drvdata *drvdata)
>> +{
>> +}
>> +
>> +static void etm4_check_arch_features(struct etmv4_drvdata *drvdata,
>> +				     unsigned int id)
>> +{
>> +}
>> +#endif /* CONFIG_ETM4X_IMPDEF_FEATURE */
>> +
>>  static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
>>  {
>>  	int i, rc;
>> @@ -110,6 +194,7 @@ static int etm4_enable_hw(struct etmv4_drvdata *drvdata)
>>  	struct device *etm_dev = &drvdata->csdev->dev;
>>
>>  	CS_UNLOCK(drvdata->base);
>> +	etm4_enable_arch_specific(drvdata);
>>
>>  	etm4_os_unlock(drvdata);
>>
>> @@ -476,6 +561,7 @@ static void etm4_disable_hw(void *info)
>>  	int i;
>>
>>  	CS_UNLOCK(drvdata->base);
>> +	etm4_disable_arch_specific(drvdata);
>>
>>  	if (!drvdata->skip_power_up) {
>>  		/* power can be removed from the trace unit now */
>> @@ -1547,6 +1633,8 @@ static int etm4_probe(struct amba_device *adev, const struct amba_id *id)
>>  		drvdata->boot_enable = true;
>>  	}
>>
>> +	etm4_check_arch_features(drvdata, id->id);
>> +
>>  	return 0;
>>  }
>>
>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x.h b/drivers/hwtracing/coresight/coresight-etm4x.h
>> index eefc737..3dd3e06 100644
>> --- a/drivers/hwtracing/coresight/coresight-etm4x.h
>> +++ b/drivers/hwtracing/coresight/coresight-etm4x.h
>> @@ -8,6 +8,7 @@
>>
>>  #include <asm/local.h>
>>  #include <linux/spinlock.h>
>> +#include <linux/types.h>
>>  #include "coresight-priv.h"
>>
>>  /*
>> @@ -203,6 +204,11 @@
>>  /* Interpretation of resource numbers change at ETM v4.3 architecture */
>>  #define ETM4X_ARCH_4V3	0x43
>>
>> +enum etm_impdef_type {
>> +	ETM4_IMPDEF_HISI_CORE_COMMIT,
>> +	ETM4_IMPDEF_FEATURE_MAX,
>> +};
>> +
>>  /**
>>   * struct etmv4_config - configuration information related to an ETMv4
>>   * @mode:	Controls various modes supported by this ETM.
>> @@ -415,6 +421,7 @@ struct etmv4_save_state {
>>   * @state_needs_restore: True when there is context to restore after PM exit
>>   * @skip_power_up: Indicates if an implementation can skip powering up
>>   *		   the trace unit.
>> + * @arch_features: Bitmap of arch features of etmv4 devices.
>>   */
>>  struct etmv4_drvdata {
>>  	void __iomem			*base;
>> @@ -463,6 +470,7 @@ struct etmv4_drvdata {
>>  	struct etmv4_save_state		*save_state;
>>  	bool				state_needs_restore;
>>  	bool				skip_power_up;
>> +	DECLARE_BITMAP(arch_features, ETM4_IMPDEF_FEATURE_MAX);
>>  };
>>
>>  /* Address comparator access types */
>> --
>> 2.8.1
>>
> 
> .
> 


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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v5] coresight: etm4x: Modify core-commit of cpu to avoid the overflow of HiSilicon ETM
  2020-12-07  2:08     ` Qi Liu
@ 2020-12-07 10:38       ` Suzuki K Poulose
  -1 siblings, 0 replies; 16+ messages in thread
From: Suzuki K Poulose @ 2020-12-07 10:38 UTC (permalink / raw)
  To: Qi Liu, Mathieu Poirier
  Cc: mike.leach, coresight, linux-kernel, linux-arm-kernel, linuxarm

On 12/7/20 2:08 AM, Qi Liu wrote:
> Hi Mathieu,
> 
> On 2020/12/5 2:55, Mathieu Poirier wrote:
>> On Thu, Nov 26, 2020 at 09:34:30PM +0800, Qi Liu wrote:
>>> The ETM device can't keep up with the core pipeline when cpu core
>>> is at full speed. This may cause overflow within core and its ETM.
>>> This is a common phenomenon on ETM devices.
>>>
>>> On HiSilicon Hip08 platform, a specific feature is added to set
>>> core pipeline. So commit rate can be reduced manually to avoid ETM
>>> overflow.
>>>
>>> Signed-off-by: Qi Liu <liuqi115@huawei.com>
>>> ---
>>> Change since v1:
>>> - add CONFIG_ETM4X_IMPDEF_FEATURE and CONFIG_ETM4X_IMPDEF_HISILICON
>>>    to keep specific feature off platforms which don't use it.
>>> Change since v2:
>>> - remove some unused variable.
>>> Change since v3:
>>> - use read/write_sysreg_s() to access register.
>>> Change since v4:
>>> - rename the call back function to a more generic name, and fix some
>>>    compile warnings.
>>>
>>>   drivers/hwtracing/coresight/Kconfig                |  9 +++
>>>   drivers/hwtracing/coresight/coresight-etm4x-core.c | 88 ++++++++++++++++++++++
>>>   drivers/hwtracing/coresight/coresight-etm4x.h      |  8 ++
>>>   3 files changed, 105 insertions(+)
>>>
>>> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
>>> index c119824..1cc3601 100644
>>> --- a/drivers/hwtracing/coresight/Kconfig
>>> +++ b/drivers/hwtracing/coresight/Kconfig
>>> @@ -110,6 +110,15 @@ config CORESIGHT_SOURCE_ETM4X
>>>   	  To compile this driver as a module, choose M here: the
>>>   	  module will be called coresight-etm4x.
>>>
>>> +config ETM4X_IMPDEF_FEATURE
>>> +	bool "Control overflow impdef support in CoreSight ETM 4.x driver "
>>> +	depends on CORESIGHT_SOURCE_ETM4X
>>> +	help
>>> +	  This control provides overflow implement define for CoreSight
>>> +	  ETM 4.x tracer module which could not reduce commit race
>>> +	  automatically, and could avoid overflow within ETM tracer module
>>> +	  and its cpu core.
>>> +
>>>   config CORESIGHT_STM
>>>   	tristate "CoreSight System Trace Macrocell driver"
>>>   	depends on (ARM && !(CPU_32v3 || CPU_32v4 || CPU_32v4T)) || ARM64
>>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>> index abd706b..fcee27a 100644
>>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>> @@ -3,6 +3,7 @@
>>>    * Copyright (c) 2014, The Linux Foundation. All rights reserved.
>>>    */
>>>
>>> +#include <linux/bitops.h>
>>>   #include <linux/kernel.h>
>>>   #include <linux/moduleparam.h>
>>>   #include <linux/init.h>
>>> @@ -28,7 +29,9 @@
>>>   #include <linux/perf_event.h>
>>>   #include <linux/pm_runtime.h>
>>>   #include <linux/property.h>
>>> +
>>>   #include <asm/sections.h>
>>> +#include <asm/sysreg.h>
>>>   #include <asm/local.h>
>>>   #include <asm/virt.h>
>>>
>>> @@ -103,6 +106,87 @@ struct etm4_enable_arg {
>>>   	int rc;
>>>   };
>>>
>>> +#ifdef CONFIG_ETM4X_IMPDEF_FEATURE
>>> +
>>> +#define HISI_HIP08_AMBA_ID		0x000b6d01
>>> +#define ETM4_AMBA_MASK			0xfffff
>>> +#define HISI_HIP08_CORE_COMMIT_CLEAR	0x3000
>>
>> Here bit 12 and 13 are cleared but in etm4_hisi_config_core_commit() only bit 12
>> is set - is this intentional?  What is bit 13 for?
>>
> bit 12 and 13 are used together to set core-commit, 2'b00 means cpu is at full speed,
> 2'b01, 2'b10, 2'b11 means reduce the speed of cpu pipeline, and 2'b01 means speed is
> reduced to minimum value. So bit 12 and 13 should be cleared together in
> etm4_hisi_config_core_commit().

Please could you document this in the function.

> 
> Qi
> 
>>> +#define HISI_HIP08_CORE_COMMIT_SHIFT	12
>>> +#define HISI_HIP08_CORE_COMMIT_REG	sys_reg(3, 1, 15, 2, 5)
>>> +
>>> +struct etm4_arch_features {
>>> +	void (*arch_callback)(bool enable);
>>> +};
>>> +
>>> +static bool etm4_hisi_match_pid(unsigned int id)
>>> +{
>>> +	return (id & ETM4_AMBA_MASK) == HISI_HIP08_AMBA_ID;
>>> +}
>>> +
>>> +static void etm4_hisi_config_core_commit(bool enable)
>>> +{
>>> +	u64 val;
>>> +
>>> +	val = read_sysreg_s(HISI_HIP08_CORE_COMMIT_REG);
>>> +	val &= ~HISI_HIP08_CORE_COMMIT_CLEAR;
>>> +	val |= enable << HISI_HIP08_CORE_COMMIT_SHIFT;

I would use the explicitly masked values when you update
a register.

With the above:

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v5] coresight: etm4x: Modify core-commit of cpu to avoid the overflow of HiSilicon ETM
@ 2020-12-07 10:38       ` Suzuki K Poulose
  0 siblings, 0 replies; 16+ messages in thread
From: Suzuki K Poulose @ 2020-12-07 10:38 UTC (permalink / raw)
  To: Qi Liu, Mathieu Poirier
  Cc: coresight, linuxarm, linux-kernel, linux-arm-kernel, mike.leach

On 12/7/20 2:08 AM, Qi Liu wrote:
> Hi Mathieu,
> 
> On 2020/12/5 2:55, Mathieu Poirier wrote:
>> On Thu, Nov 26, 2020 at 09:34:30PM +0800, Qi Liu wrote:
>>> The ETM device can't keep up with the core pipeline when cpu core
>>> is at full speed. This may cause overflow within core and its ETM.
>>> This is a common phenomenon on ETM devices.
>>>
>>> On HiSilicon Hip08 platform, a specific feature is added to set
>>> core pipeline. So commit rate can be reduced manually to avoid ETM
>>> overflow.
>>>
>>> Signed-off-by: Qi Liu <liuqi115@huawei.com>
>>> ---
>>> Change since v1:
>>> - add CONFIG_ETM4X_IMPDEF_FEATURE and CONFIG_ETM4X_IMPDEF_HISILICON
>>>    to keep specific feature off platforms which don't use it.
>>> Change since v2:
>>> - remove some unused variable.
>>> Change since v3:
>>> - use read/write_sysreg_s() to access register.
>>> Change since v4:
>>> - rename the call back function to a more generic name, and fix some
>>>    compile warnings.
>>>
>>>   drivers/hwtracing/coresight/Kconfig                |  9 +++
>>>   drivers/hwtracing/coresight/coresight-etm4x-core.c | 88 ++++++++++++++++++++++
>>>   drivers/hwtracing/coresight/coresight-etm4x.h      |  8 ++
>>>   3 files changed, 105 insertions(+)
>>>
>>> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
>>> index c119824..1cc3601 100644
>>> --- a/drivers/hwtracing/coresight/Kconfig
>>> +++ b/drivers/hwtracing/coresight/Kconfig
>>> @@ -110,6 +110,15 @@ config CORESIGHT_SOURCE_ETM4X
>>>   	  To compile this driver as a module, choose M here: the
>>>   	  module will be called coresight-etm4x.
>>>
>>> +config ETM4X_IMPDEF_FEATURE
>>> +	bool "Control overflow impdef support in CoreSight ETM 4.x driver "
>>> +	depends on CORESIGHT_SOURCE_ETM4X
>>> +	help
>>> +	  This control provides overflow implement define for CoreSight
>>> +	  ETM 4.x tracer module which could not reduce commit race
>>> +	  automatically, and could avoid overflow within ETM tracer module
>>> +	  and its cpu core.
>>> +
>>>   config CORESIGHT_STM
>>>   	tristate "CoreSight System Trace Macrocell driver"
>>>   	depends on (ARM && !(CPU_32v3 || CPU_32v4 || CPU_32v4T)) || ARM64
>>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>> index abd706b..fcee27a 100644
>>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>> @@ -3,6 +3,7 @@
>>>    * Copyright (c) 2014, The Linux Foundation. All rights reserved.
>>>    */
>>>
>>> +#include <linux/bitops.h>
>>>   #include <linux/kernel.h>
>>>   #include <linux/moduleparam.h>
>>>   #include <linux/init.h>
>>> @@ -28,7 +29,9 @@
>>>   #include <linux/perf_event.h>
>>>   #include <linux/pm_runtime.h>
>>>   #include <linux/property.h>
>>> +
>>>   #include <asm/sections.h>
>>> +#include <asm/sysreg.h>
>>>   #include <asm/local.h>
>>>   #include <asm/virt.h>
>>>
>>> @@ -103,6 +106,87 @@ struct etm4_enable_arg {
>>>   	int rc;
>>>   };
>>>
>>> +#ifdef CONFIG_ETM4X_IMPDEF_FEATURE
>>> +
>>> +#define HISI_HIP08_AMBA_ID		0x000b6d01
>>> +#define ETM4_AMBA_MASK			0xfffff
>>> +#define HISI_HIP08_CORE_COMMIT_CLEAR	0x3000
>>
>> Here bit 12 and 13 are cleared but in etm4_hisi_config_core_commit() only bit 12
>> is set - is this intentional?  What is bit 13 for?
>>
> bit 12 and 13 are used together to set core-commit, 2'b00 means cpu is at full speed,
> 2'b01, 2'b10, 2'b11 means reduce the speed of cpu pipeline, and 2'b01 means speed is
> reduced to minimum value. So bit 12 and 13 should be cleared together in
> etm4_hisi_config_core_commit().

Please could you document this in the function.

> 
> Qi
> 
>>> +#define HISI_HIP08_CORE_COMMIT_SHIFT	12
>>> +#define HISI_HIP08_CORE_COMMIT_REG	sys_reg(3, 1, 15, 2, 5)
>>> +
>>> +struct etm4_arch_features {
>>> +	void (*arch_callback)(bool enable);
>>> +};
>>> +
>>> +static bool etm4_hisi_match_pid(unsigned int id)
>>> +{
>>> +	return (id & ETM4_AMBA_MASK) == HISI_HIP08_AMBA_ID;
>>> +}
>>> +
>>> +static void etm4_hisi_config_core_commit(bool enable)
>>> +{
>>> +	u64 val;
>>> +
>>> +	val = read_sysreg_s(HISI_HIP08_CORE_COMMIT_REG);
>>> +	val &= ~HISI_HIP08_CORE_COMMIT_CLEAR;
>>> +	val |= enable << HISI_HIP08_CORE_COMMIT_SHIFT;

I would use the explicitly masked values when you update
a register.

With the above:

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

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v5] coresight: etm4x: Modify core-commit of cpu to avoid the overflow of HiSilicon ETM
  2020-12-07 10:38       ` Suzuki K Poulose
@ 2020-12-07 11:21         ` Qi Liu
  -1 siblings, 0 replies; 16+ messages in thread
From: Qi Liu @ 2020-12-07 11:21 UTC (permalink / raw)
  To: Suzuki K Poulose, Mathieu Poirier
  Cc: mike.leach, coresight, linux-kernel, linux-arm-kernel, linuxarm


Hi Suzuki,
On 2020/12/7 18:38, Suzuki K Poulose wrote:
> On 12/7/20 2:08 AM, Qi Liu wrote:
>> Hi Mathieu,
>>
>> On 2020/12/5 2:55, Mathieu Poirier wrote:
>>> On Thu, Nov 26, 2020 at 09:34:30PM +0800, Qi Liu wrote:
>>>> The ETM device can't keep up with the core pipeline when cpu core
>>>> is at full speed. This may cause overflow within core and its ETM.
>>>> This is a common phenomenon on ETM devices.
>>>>
>>>> On HiSilicon Hip08 platform, a specific feature is added to set
>>>> core pipeline. So commit rate can be reduced manually to avoid ETM
>>>> overflow.
>>>>
>>>> Signed-off-by: Qi Liu <liuqi115@huawei.com>
>>>> ---
>>>> Change since v1:
>>>> - add CONFIG_ETM4X_IMPDEF_FEATURE and CONFIG_ETM4X_IMPDEF_HISILICON
>>>>    to keep specific feature off platforms which don't use it.
>>>> Change since v2:
>>>> - remove some unused variable.
>>>> Change since v3:
>>>> - use read/write_sysreg_s() to access register.
>>>> Change since v4:
>>>> - rename the call back function to a more generic name, and fix some
>>>>    compile warnings.
>>>>
>>>>   drivers/hwtracing/coresight/Kconfig                |  9 +++
>>>>   drivers/hwtracing/coresight/coresight-etm4x-core.c | 88 ++++++++++++++++++++++
>>>>   drivers/hwtracing/coresight/coresight-etm4x.h      |  8 ++
>>>>   3 files changed, 105 insertions(+)
>>>>
>>>> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
>>>> index c119824..1cc3601 100644
>>>> --- a/drivers/hwtracing/coresight/Kconfig
>>>> +++ b/drivers/hwtracing/coresight/Kconfig
>>>> @@ -110,6 +110,15 @@ config CORESIGHT_SOURCE_ETM4X
>>>>         To compile this driver as a module, choose M here: the
>>>>         module will be called coresight-etm4x.
>>>>
>>>> +config ETM4X_IMPDEF_FEATURE
>>>> +    bool "Control overflow impdef support in CoreSight ETM 4.x driver "
>>>> +    depends on CORESIGHT_SOURCE_ETM4X
>>>> +    help
>>>> +      This control provides overflow implement define for CoreSight
>>>> +      ETM 4.x tracer module which could not reduce commit race
>>>> +      automatically, and could avoid overflow within ETM tracer module
>>>> +      and its cpu core.
>>>> +
>>>>   config CORESIGHT_STM
>>>>       tristate "CoreSight System Trace Macrocell driver"
>>>>       depends on (ARM && !(CPU_32v3 || CPU_32v4 || CPU_32v4T)) || ARM64
>>>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>> index abd706b..fcee27a 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>> @@ -3,6 +3,7 @@
>>>>    * Copyright (c) 2014, The Linux Foundation. All rights reserved.
>>>>    */
>>>>
>>>> +#include <linux/bitops.h>
>>>>   #include <linux/kernel.h>
>>>>   #include <linux/moduleparam.h>
>>>>   #include <linux/init.h>
>>>> @@ -28,7 +29,9 @@
>>>>   #include <linux/perf_event.h>
>>>>   #include <linux/pm_runtime.h>
>>>>   #include <linux/property.h>
>>>> +
>>>>   #include <asm/sections.h>
>>>> +#include <asm/sysreg.h>
>>>>   #include <asm/local.h>
>>>>   #include <asm/virt.h>
>>>>
>>>> @@ -103,6 +106,87 @@ struct etm4_enable_arg {
>>>>       int rc;
>>>>   };
>>>>
>>>> +#ifdef CONFIG_ETM4X_IMPDEF_FEATURE
>>>> +
>>>> +#define HISI_HIP08_AMBA_ID        0x000b6d01
>>>> +#define ETM4_AMBA_MASK            0xfffff
>>>> +#define HISI_HIP08_CORE_COMMIT_CLEAR    0x3000
>>>
>>> Here bit 12 and 13 are cleared but in etm4_hisi_config_core_commit() only bit 12
>>> is set - is this intentional?  What is bit 13 for?
>>>
>> bit 12 and 13 are used together to set core-commit, 2'b00 means cpu is at full speed,
>> 2'b01, 2'b10, 2'b11 means reduce the speed of cpu pipeline, and 2'b01 means speed is
>> reduced to minimum value. So bit 12 and 13 should be cleared together in
>> etm4_hisi_config_core_commit().
> 
> Please could you document this in the function.
> 
of course, thanks.
>>
>> Qi
>>
>>>> +#define HISI_HIP08_CORE_COMMIT_SHIFT    12
>>>> +#define HISI_HIP08_CORE_COMMIT_REG    sys_reg(3, 1, 15, 2, 5)
>>>> +
>>>> +struct etm4_arch_features {
>>>> +    void (*arch_callback)(bool enable);
>>>> +};
>>>> +
>>>> +static bool etm4_hisi_match_pid(unsigned int id)
>>>> +{
>>>> +    return (id & ETM4_AMBA_MASK) == HISI_HIP08_AMBA_ID;
>>>> +}
>>>> +
>>>> +static void etm4_hisi_config_core_commit(bool enable)
>>>> +{
>>>> +    u64 val;
>>>> +
>>>> +    val = read_sysreg_s(HISI_HIP08_CORE_COMMIT_REG);
>>>> +    val &= ~HISI_HIP08_CORE_COMMIT_CLEAR;
>>>> +    val |= enable << HISI_HIP08_CORE_COMMIT_SHIFT;
> 
> I would use the explicitly masked values when you update
> a register.
> 
ok, how about changing these code to this:
val &= ~GENMASK(12, 13);

Thanks
Qi

> With the above:
> 
> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> 
> .
> 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v5] coresight: etm4x: Modify core-commit of cpu to avoid the overflow of HiSilicon ETM
@ 2020-12-07 11:21         ` Qi Liu
  0 siblings, 0 replies; 16+ messages in thread
From: Qi Liu @ 2020-12-07 11:21 UTC (permalink / raw)
  To: Suzuki K Poulose, Mathieu Poirier
  Cc: coresight, linuxarm, linux-kernel, linux-arm-kernel, mike.leach


Hi Suzuki,
On 2020/12/7 18:38, Suzuki K Poulose wrote:
> On 12/7/20 2:08 AM, Qi Liu wrote:
>> Hi Mathieu,
>>
>> On 2020/12/5 2:55, Mathieu Poirier wrote:
>>> On Thu, Nov 26, 2020 at 09:34:30PM +0800, Qi Liu wrote:
>>>> The ETM device can't keep up with the core pipeline when cpu core
>>>> is at full speed. This may cause overflow within core and its ETM.
>>>> This is a common phenomenon on ETM devices.
>>>>
>>>> On HiSilicon Hip08 platform, a specific feature is added to set
>>>> core pipeline. So commit rate can be reduced manually to avoid ETM
>>>> overflow.
>>>>
>>>> Signed-off-by: Qi Liu <liuqi115@huawei.com>
>>>> ---
>>>> Change since v1:
>>>> - add CONFIG_ETM4X_IMPDEF_FEATURE and CONFIG_ETM4X_IMPDEF_HISILICON
>>>>    to keep specific feature off platforms which don't use it.
>>>> Change since v2:
>>>> - remove some unused variable.
>>>> Change since v3:
>>>> - use read/write_sysreg_s() to access register.
>>>> Change since v4:
>>>> - rename the call back function to a more generic name, and fix some
>>>>    compile warnings.
>>>>
>>>>   drivers/hwtracing/coresight/Kconfig                |  9 +++
>>>>   drivers/hwtracing/coresight/coresight-etm4x-core.c | 88 ++++++++++++++++++++++
>>>>   drivers/hwtracing/coresight/coresight-etm4x.h      |  8 ++
>>>>   3 files changed, 105 insertions(+)
>>>>
>>>> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
>>>> index c119824..1cc3601 100644
>>>> --- a/drivers/hwtracing/coresight/Kconfig
>>>> +++ b/drivers/hwtracing/coresight/Kconfig
>>>> @@ -110,6 +110,15 @@ config CORESIGHT_SOURCE_ETM4X
>>>>         To compile this driver as a module, choose M here: the
>>>>         module will be called coresight-etm4x.
>>>>
>>>> +config ETM4X_IMPDEF_FEATURE
>>>> +    bool "Control overflow impdef support in CoreSight ETM 4.x driver "
>>>> +    depends on CORESIGHT_SOURCE_ETM4X
>>>> +    help
>>>> +      This control provides overflow implement define for CoreSight
>>>> +      ETM 4.x tracer module which could not reduce commit race
>>>> +      automatically, and could avoid overflow within ETM tracer module
>>>> +      and its cpu core.
>>>> +
>>>>   config CORESIGHT_STM
>>>>       tristate "CoreSight System Trace Macrocell driver"
>>>>       depends on (ARM && !(CPU_32v3 || CPU_32v4 || CPU_32v4T)) || ARM64
>>>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>> index abd706b..fcee27a 100644
>>>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>> @@ -3,6 +3,7 @@
>>>>    * Copyright (c) 2014, The Linux Foundation. All rights reserved.
>>>>    */
>>>>
>>>> +#include <linux/bitops.h>
>>>>   #include <linux/kernel.h>
>>>>   #include <linux/moduleparam.h>
>>>>   #include <linux/init.h>
>>>> @@ -28,7 +29,9 @@
>>>>   #include <linux/perf_event.h>
>>>>   #include <linux/pm_runtime.h>
>>>>   #include <linux/property.h>
>>>> +
>>>>   #include <asm/sections.h>
>>>> +#include <asm/sysreg.h>
>>>>   #include <asm/local.h>
>>>>   #include <asm/virt.h>
>>>>
>>>> @@ -103,6 +106,87 @@ struct etm4_enable_arg {
>>>>       int rc;
>>>>   };
>>>>
>>>> +#ifdef CONFIG_ETM4X_IMPDEF_FEATURE
>>>> +
>>>> +#define HISI_HIP08_AMBA_ID        0x000b6d01
>>>> +#define ETM4_AMBA_MASK            0xfffff
>>>> +#define HISI_HIP08_CORE_COMMIT_CLEAR    0x3000
>>>
>>> Here bit 12 and 13 are cleared but in etm4_hisi_config_core_commit() only bit 12
>>> is set - is this intentional?  What is bit 13 for?
>>>
>> bit 12 and 13 are used together to set core-commit, 2'b00 means cpu is at full speed,
>> 2'b01, 2'b10, 2'b11 means reduce the speed of cpu pipeline, and 2'b01 means speed is
>> reduced to minimum value. So bit 12 and 13 should be cleared together in
>> etm4_hisi_config_core_commit().
> 
> Please could you document this in the function.
> 
of course, thanks.
>>
>> Qi
>>
>>>> +#define HISI_HIP08_CORE_COMMIT_SHIFT    12
>>>> +#define HISI_HIP08_CORE_COMMIT_REG    sys_reg(3, 1, 15, 2, 5)
>>>> +
>>>> +struct etm4_arch_features {
>>>> +    void (*arch_callback)(bool enable);
>>>> +};
>>>> +
>>>> +static bool etm4_hisi_match_pid(unsigned int id)
>>>> +{
>>>> +    return (id & ETM4_AMBA_MASK) == HISI_HIP08_AMBA_ID;
>>>> +}
>>>> +
>>>> +static void etm4_hisi_config_core_commit(bool enable)
>>>> +{
>>>> +    u64 val;
>>>> +
>>>> +    val = read_sysreg_s(HISI_HIP08_CORE_COMMIT_REG);
>>>> +    val &= ~HISI_HIP08_CORE_COMMIT_CLEAR;
>>>> +    val |= enable << HISI_HIP08_CORE_COMMIT_SHIFT;
> 
> I would use the explicitly masked values when you update
> a register.
> 
ok, how about changing these code to this:
val &= ~GENMASK(12, 13);

Thanks
Qi

> With the above:
> 
> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> 
> .
> 


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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v5] coresight: etm4x: Modify core-commit of cpu to avoid the overflow of HiSilicon ETM
  2020-12-07 11:21         ` Qi Liu
@ 2020-12-07 11:27           ` Suzuki K Poulose
  -1 siblings, 0 replies; 16+ messages in thread
From: Suzuki K Poulose @ 2020-12-07 11:27 UTC (permalink / raw)
  To: Qi Liu, Mathieu Poirier
  Cc: mike.leach, coresight, linux-kernel, linux-arm-kernel, linuxarm

On 12/7/20 11:21 AM, Qi Liu wrote:
> 
> Hi Suzuki,
> On 2020/12/7 18:38, Suzuki K Poulose wrote:
>> On 12/7/20 2:08 AM, Qi Liu wrote:
>>> Hi Mathieu,
>>>
>>> On 2020/12/5 2:55, Mathieu Poirier wrote:
>>>> On Thu, Nov 26, 2020 at 09:34:30PM +0800, Qi Liu wrote:
>>>>> The ETM device can't keep up with the core pipeline when cpu core
>>>>> is at full speed. This may cause overflow within core and its ETM.
>>>>> This is a common phenomenon on ETM devices.
>>>>>
>>>>> On HiSilicon Hip08 platform, a specific feature is added to set
>>>>> core pipeline. So commit rate can be reduced manually to avoid ETM
>>>>> overflow.
>>>>>
>>>>> Signed-off-by: Qi Liu <liuqi115@huawei.com>
>>>>> ---
>>>>> Change since v1:
>>>>> - add CONFIG_ETM4X_IMPDEF_FEATURE and CONFIG_ETM4X_IMPDEF_HISILICON
>>>>>     to keep specific feature off platforms which don't use it.
>>>>> Change since v2:
>>>>> - remove some unused variable.
>>>>> Change since v3:
>>>>> - use read/write_sysreg_s() to access register.
>>>>> Change since v4:
>>>>> - rename the call back function to a more generic name, and fix some
>>>>>     compile warnings.
>>>>>
>>>>>    drivers/hwtracing/coresight/Kconfig                |  9 +++
>>>>>    drivers/hwtracing/coresight/coresight-etm4x-core.c | 88 ++++++++++++++++++++++
>>>>>    drivers/hwtracing/coresight/coresight-etm4x.h      |  8 ++
>>>>>    3 files changed, 105 insertions(+)
>>>>>
>>>>> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
>>>>> index c119824..1cc3601 100644
>>>>> --- a/drivers/hwtracing/coresight/Kconfig
>>>>> +++ b/drivers/hwtracing/coresight/Kconfig
>>>>> @@ -110,6 +110,15 @@ config CORESIGHT_SOURCE_ETM4X
>>>>>          To compile this driver as a module, choose M here: the
>>>>>          module will be called coresight-etm4x.
>>>>>
>>>>> +config ETM4X_IMPDEF_FEATURE
>>>>> +    bool "Control overflow impdef support in CoreSight ETM 4.x driver "
>>>>> +    depends on CORESIGHT_SOURCE_ETM4X
>>>>> +    help
>>>>> +      This control provides overflow implement define for CoreSight
>>>>> +      ETM 4.x tracer module which could not reduce commit race
>>>>> +      automatically, and could avoid overflow within ETM tracer module
>>>>> +      and its cpu core.
>>>>> +
>>>>>    config CORESIGHT_STM
>>>>>        tristate "CoreSight System Trace Macrocell driver"
>>>>>        depends on (ARM && !(CPU_32v3 || CPU_32v4 || CPU_32v4T)) || ARM64
>>>>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>>> index abd706b..fcee27a 100644
>>>>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>>> @@ -3,6 +3,7 @@
>>>>>     * Copyright (c) 2014, The Linux Foundation. All rights reserved.
>>>>>     */
>>>>>
>>>>> +#include <linux/bitops.h>
>>>>>    #include <linux/kernel.h>
>>>>>    #include <linux/moduleparam.h>
>>>>>    #include <linux/init.h>
>>>>> @@ -28,7 +29,9 @@
>>>>>    #include <linux/perf_event.h>
>>>>>    #include <linux/pm_runtime.h>
>>>>>    #include <linux/property.h>
>>>>> +
>>>>>    #include <asm/sections.h>
>>>>> +#include <asm/sysreg.h>
>>>>>    #include <asm/local.h>
>>>>>    #include <asm/virt.h>
>>>>>
>>>>> @@ -103,6 +106,87 @@ struct etm4_enable_arg {
>>>>>        int rc;
>>>>>    };
>>>>>
>>>>> +#ifdef CONFIG_ETM4X_IMPDEF_FEATURE
>>>>> +
>>>>> +#define HISI_HIP08_AMBA_ID        0x000b6d01
>>>>> +#define ETM4_AMBA_MASK            0xfffff
>>>>> +#define HISI_HIP08_CORE_COMMIT_CLEAR    0x3000
>>>>
>>>> Here bit 12 and 13 are cleared but in etm4_hisi_config_core_commit() only bit 12
>>>> is set - is this intentional?  What is bit 13 for?
>>>>
>>> bit 12 and 13 are used together to set core-commit, 2'b00 means cpu is at full speed,
>>> 2'b01, 2'b10, 2'b11 means reduce the speed of cpu pipeline, and 2'b01 means speed is
>>> reduced to minimum value. So bit 12 and 13 should be cleared together in
>>> etm4_hisi_config_core_commit().
>>
>> Please could you document this in the function.
>>
> of course, thanks.
>>>
>>> Qi
>>>
>>>>> +#define HISI_HIP08_CORE_COMMIT_SHIFT    12
>>>>> +#define HISI_HIP08_CORE_COMMIT_REG    sys_reg(3, 1, 15, 2, 5)
>>>>> +
>>>>> +struct etm4_arch_features {
>>>>> +    void (*arch_callback)(bool enable);
>>>>> +};
>>>>> +
>>>>> +static bool etm4_hisi_match_pid(unsigned int id)
>>>>> +{
>>>>> +    return (id & ETM4_AMBA_MASK) == HISI_HIP08_AMBA_ID;
>>>>> +}
>>>>> +
>>>>> +static void etm4_hisi_config_core_commit(bool enable)
>>>>> +{
>>>>> +    u64 val;
>>>>> +
>>>>> +    val = read_sysreg_s(HISI_HIP08_CORE_COMMIT_REG);
>>>>> +    val &= ~HISI_HIP08_CORE_COMMIT_CLEAR;
>>>>> +    val |= enable << HISI_HIP08_CORE_COMMIT_SHIFT;
>>
>> I would use the explicitly masked values when you update
>> a register.
>>
> ok, how about changing these code to this:
> val &= ~GENMASK(12, 13);

I would do :

// Rename the HISI_HIP08_CORE_COMMIT_CLEAR to HISI_HIP08_CORE_COMMIT_MASK
// above.
#define HISI_HIP08_CORE_COMMIT_MASK		0x3000

#define HISI_HIP08_CORE_COMMIT_FULL		0b00
#define HISI_HIP08_CORE_COMMIT_LVL_1		0b01


u8 commit = enable ? HISI_HIP08_CORE_COMMIT_LVL_1 : HISI_HIP08_CORE_COMMIT_FULL;

...

val |= commit << HISI_HIP08_CORE_COMMIT_SHIFT;

..


Suzuki

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v5] coresight: etm4x: Modify core-commit of cpu to avoid the overflow of HiSilicon ETM
@ 2020-12-07 11:27           ` Suzuki K Poulose
  0 siblings, 0 replies; 16+ messages in thread
From: Suzuki K Poulose @ 2020-12-07 11:27 UTC (permalink / raw)
  To: Qi Liu, Mathieu Poirier
  Cc: coresight, linuxarm, linux-kernel, linux-arm-kernel, mike.leach

On 12/7/20 11:21 AM, Qi Liu wrote:
> 
> Hi Suzuki,
> On 2020/12/7 18:38, Suzuki K Poulose wrote:
>> On 12/7/20 2:08 AM, Qi Liu wrote:
>>> Hi Mathieu,
>>>
>>> On 2020/12/5 2:55, Mathieu Poirier wrote:
>>>> On Thu, Nov 26, 2020 at 09:34:30PM +0800, Qi Liu wrote:
>>>>> The ETM device can't keep up with the core pipeline when cpu core
>>>>> is at full speed. This may cause overflow within core and its ETM.
>>>>> This is a common phenomenon on ETM devices.
>>>>>
>>>>> On HiSilicon Hip08 platform, a specific feature is added to set
>>>>> core pipeline. So commit rate can be reduced manually to avoid ETM
>>>>> overflow.
>>>>>
>>>>> Signed-off-by: Qi Liu <liuqi115@huawei.com>
>>>>> ---
>>>>> Change since v1:
>>>>> - add CONFIG_ETM4X_IMPDEF_FEATURE and CONFIG_ETM4X_IMPDEF_HISILICON
>>>>>     to keep specific feature off platforms which don't use it.
>>>>> Change since v2:
>>>>> - remove some unused variable.
>>>>> Change since v3:
>>>>> - use read/write_sysreg_s() to access register.
>>>>> Change since v4:
>>>>> - rename the call back function to a more generic name, and fix some
>>>>>     compile warnings.
>>>>>
>>>>>    drivers/hwtracing/coresight/Kconfig                |  9 +++
>>>>>    drivers/hwtracing/coresight/coresight-etm4x-core.c | 88 ++++++++++++++++++++++
>>>>>    drivers/hwtracing/coresight/coresight-etm4x.h      |  8 ++
>>>>>    3 files changed, 105 insertions(+)
>>>>>
>>>>> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
>>>>> index c119824..1cc3601 100644
>>>>> --- a/drivers/hwtracing/coresight/Kconfig
>>>>> +++ b/drivers/hwtracing/coresight/Kconfig
>>>>> @@ -110,6 +110,15 @@ config CORESIGHT_SOURCE_ETM4X
>>>>>          To compile this driver as a module, choose M here: the
>>>>>          module will be called coresight-etm4x.
>>>>>
>>>>> +config ETM4X_IMPDEF_FEATURE
>>>>> +    bool "Control overflow impdef support in CoreSight ETM 4.x driver "
>>>>> +    depends on CORESIGHT_SOURCE_ETM4X
>>>>> +    help
>>>>> +      This control provides overflow implement define for CoreSight
>>>>> +      ETM 4.x tracer module which could not reduce commit race
>>>>> +      automatically, and could avoid overflow within ETM tracer module
>>>>> +      and its cpu core.
>>>>> +
>>>>>    config CORESIGHT_STM
>>>>>        tristate "CoreSight System Trace Macrocell driver"
>>>>>        depends on (ARM && !(CPU_32v3 || CPU_32v4 || CPU_32v4T)) || ARM64
>>>>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>>> index abd706b..fcee27a 100644
>>>>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>>> @@ -3,6 +3,7 @@
>>>>>     * Copyright (c) 2014, The Linux Foundation. All rights reserved.
>>>>>     */
>>>>>
>>>>> +#include <linux/bitops.h>
>>>>>    #include <linux/kernel.h>
>>>>>    #include <linux/moduleparam.h>
>>>>>    #include <linux/init.h>
>>>>> @@ -28,7 +29,9 @@
>>>>>    #include <linux/perf_event.h>
>>>>>    #include <linux/pm_runtime.h>
>>>>>    #include <linux/property.h>
>>>>> +
>>>>>    #include <asm/sections.h>
>>>>> +#include <asm/sysreg.h>
>>>>>    #include <asm/local.h>
>>>>>    #include <asm/virt.h>
>>>>>
>>>>> @@ -103,6 +106,87 @@ struct etm4_enable_arg {
>>>>>        int rc;
>>>>>    };
>>>>>
>>>>> +#ifdef CONFIG_ETM4X_IMPDEF_FEATURE
>>>>> +
>>>>> +#define HISI_HIP08_AMBA_ID        0x000b6d01
>>>>> +#define ETM4_AMBA_MASK            0xfffff
>>>>> +#define HISI_HIP08_CORE_COMMIT_CLEAR    0x3000
>>>>
>>>> Here bit 12 and 13 are cleared but in etm4_hisi_config_core_commit() only bit 12
>>>> is set - is this intentional?  What is bit 13 for?
>>>>
>>> bit 12 and 13 are used together to set core-commit, 2'b00 means cpu is at full speed,
>>> 2'b01, 2'b10, 2'b11 means reduce the speed of cpu pipeline, and 2'b01 means speed is
>>> reduced to minimum value. So bit 12 and 13 should be cleared together in
>>> etm4_hisi_config_core_commit().
>>
>> Please could you document this in the function.
>>
> of course, thanks.
>>>
>>> Qi
>>>
>>>>> +#define HISI_HIP08_CORE_COMMIT_SHIFT    12
>>>>> +#define HISI_HIP08_CORE_COMMIT_REG    sys_reg(3, 1, 15, 2, 5)
>>>>> +
>>>>> +struct etm4_arch_features {
>>>>> +    void (*arch_callback)(bool enable);
>>>>> +};
>>>>> +
>>>>> +static bool etm4_hisi_match_pid(unsigned int id)
>>>>> +{
>>>>> +    return (id & ETM4_AMBA_MASK) == HISI_HIP08_AMBA_ID;
>>>>> +}
>>>>> +
>>>>> +static void etm4_hisi_config_core_commit(bool enable)
>>>>> +{
>>>>> +    u64 val;
>>>>> +
>>>>> +    val = read_sysreg_s(HISI_HIP08_CORE_COMMIT_REG);
>>>>> +    val &= ~HISI_HIP08_CORE_COMMIT_CLEAR;
>>>>> +    val |= enable << HISI_HIP08_CORE_COMMIT_SHIFT;
>>
>> I would use the explicitly masked values when you update
>> a register.
>>
> ok, how about changing these code to this:
> val &= ~GENMASK(12, 13);

I would do :

// Rename the HISI_HIP08_CORE_COMMIT_CLEAR to HISI_HIP08_CORE_COMMIT_MASK
// above.
#define HISI_HIP08_CORE_COMMIT_MASK		0x3000

#define HISI_HIP08_CORE_COMMIT_FULL		0b00
#define HISI_HIP08_CORE_COMMIT_LVL_1		0b01


u8 commit = enable ? HISI_HIP08_CORE_COMMIT_LVL_1 : HISI_HIP08_CORE_COMMIT_FULL;

...

val |= commit << HISI_HIP08_CORE_COMMIT_SHIFT;

..


Suzuki

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v5] coresight: etm4x: Modify core-commit of cpu to avoid the overflow of HiSilicon ETM
  2020-12-07 11:27           ` Suzuki K Poulose
@ 2020-12-07 11:32             ` Qi Liu
  -1 siblings, 0 replies; 16+ messages in thread
From: Qi Liu @ 2020-12-07 11:32 UTC (permalink / raw)
  To: Suzuki K Poulose, Mathieu Poirier
  Cc: mike.leach, coresight, linux-kernel, linux-arm-kernel, linuxarm



On 2020/12/7 19:27, Suzuki K Poulose wrote:
> On 12/7/20 11:21 AM, Qi Liu wrote:
>>
>> Hi Suzuki,
>> On 2020/12/7 18:38, Suzuki K Poulose wrote:
>>> On 12/7/20 2:08 AM, Qi Liu wrote:
>>>> Hi Mathieu,
>>>>
>>>> On 2020/12/5 2:55, Mathieu Poirier wrote:
>>>>> On Thu, Nov 26, 2020 at 09:34:30PM +0800, Qi Liu wrote:
>>>>>> The ETM device can't keep up with the core pipeline when cpu core
>>>>>> is at full speed. This may cause overflow within core and its ETM.
>>>>>> This is a common phenomenon on ETM devices.
>>>>>>
>>>>>> On HiSilicon Hip08 platform, a specific feature is added to set
>>>>>> core pipeline. So commit rate can be reduced manually to avoid ETM
>>>>>> overflow.
>>>>>>
>>>>>> Signed-off-by: Qi Liu <liuqi115@huawei.com>
>>>>>> ---
>>>>>> Change since v1:
>>>>>> - add CONFIG_ETM4X_IMPDEF_FEATURE and CONFIG_ETM4X_IMPDEF_HISILICON
>>>>>>     to keep specific feature off platforms which don't use it.
>>>>>> Change since v2:
>>>>>> - remove some unused variable.
>>>>>> Change since v3:
>>>>>> - use read/write_sysreg_s() to access register.
>>>>>> Change since v4:
>>>>>> - rename the call back function to a more generic name, and fix some
>>>>>>     compile warnings.
>>>>>>
>>>>>>    drivers/hwtracing/coresight/Kconfig                |  9 +++
>>>>>>    drivers/hwtracing/coresight/coresight-etm4x-core.c | 88 ++++++++++++++++++++++
>>>>>>    drivers/hwtracing/coresight/coresight-etm4x.h      |  8 ++
>>>>>>    3 files changed, 105 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
>>>>>> index c119824..1cc3601 100644
>>>>>> --- a/drivers/hwtracing/coresight/Kconfig
>>>>>> +++ b/drivers/hwtracing/coresight/Kconfig
>>>>>> @@ -110,6 +110,15 @@ config CORESIGHT_SOURCE_ETM4X
>>>>>>          To compile this driver as a module, choose M here: the
>>>>>>          module will be called coresight-etm4x.
>>>>>>
>>>>>> +config ETM4X_IMPDEF_FEATURE
>>>>>> +    bool "Control overflow impdef support in CoreSight ETM 4.x driver "
>>>>>> +    depends on CORESIGHT_SOURCE_ETM4X
>>>>>> +    help
>>>>>> +      This control provides overflow implement define for CoreSight
>>>>>> +      ETM 4.x tracer module which could not reduce commit race
>>>>>> +      automatically, and could avoid overflow within ETM tracer module
>>>>>> +      and its cpu core.
>>>>>> +
>>>>>>    config CORESIGHT_STM
>>>>>>        tristate "CoreSight System Trace Macrocell driver"
>>>>>>        depends on (ARM && !(CPU_32v3 || CPU_32v4 || CPU_32v4T)) || ARM64
>>>>>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>>>> index abd706b..fcee27a 100644
>>>>>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>>>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>>>> @@ -3,6 +3,7 @@
>>>>>>     * Copyright (c) 2014, The Linux Foundation. All rights reserved.
>>>>>>     */
>>>>>>
>>>>>> +#include <linux/bitops.h>
>>>>>>    #include <linux/kernel.h>
>>>>>>    #include <linux/moduleparam.h>
>>>>>>    #include <linux/init.h>
>>>>>> @@ -28,7 +29,9 @@
>>>>>>    #include <linux/perf_event.h>
>>>>>>    #include <linux/pm_runtime.h>
>>>>>>    #include <linux/property.h>
>>>>>> +
>>>>>>    #include <asm/sections.h>
>>>>>> +#include <asm/sysreg.h>
>>>>>>    #include <asm/local.h>
>>>>>>    #include <asm/virt.h>
>>>>>>
>>>>>> @@ -103,6 +106,87 @@ struct etm4_enable_arg {
>>>>>>        int rc;
>>>>>>    };
>>>>>>
>>>>>> +#ifdef CONFIG_ETM4X_IMPDEF_FEATURE
>>>>>> +
>>>>>> +#define HISI_HIP08_AMBA_ID        0x000b6d01
>>>>>> +#define ETM4_AMBA_MASK            0xfffff
>>>>>> +#define HISI_HIP08_CORE_COMMIT_CLEAR    0x3000
>>>>>
>>>>> Here bit 12 and 13 are cleared but in etm4_hisi_config_core_commit() only bit 12
>>>>> is set - is this intentional?  What is bit 13 for?
>>>>>
>>>> bit 12 and 13 are used together to set core-commit, 2'b00 means cpu is at full speed,
>>>> 2'b01, 2'b10, 2'b11 means reduce the speed of cpu pipeline, and 2'b01 means speed is
>>>> reduced to minimum value. So bit 12 and 13 should be cleared together in
>>>> etm4_hisi_config_core_commit().
>>>
>>> Please could you document this in the function.
>>>
>> of course, thanks.
>>>>
>>>> Qi
>>>>
>>>>>> +#define HISI_HIP08_CORE_COMMIT_SHIFT    12
>>>>>> +#define HISI_HIP08_CORE_COMMIT_REG    sys_reg(3, 1, 15, 2, 5)
>>>>>> +
>>>>>> +struct etm4_arch_features {
>>>>>> +    void (*arch_callback)(bool enable);
>>>>>> +};
>>>>>> +
>>>>>> +static bool etm4_hisi_match_pid(unsigned int id)
>>>>>> +{
>>>>>> +    return (id & ETM4_AMBA_MASK) == HISI_HIP08_AMBA_ID;
>>>>>> +}
>>>>>> +
>>>>>> +static void etm4_hisi_config_core_commit(bool enable)
>>>>>> +{
>>>>>> +    u64 val;
>>>>>> +
>>>>>> +    val = read_sysreg_s(HISI_HIP08_CORE_COMMIT_REG);
>>>>>> +    val &= ~HISI_HIP08_CORE_COMMIT_CLEAR;
>>>>>> +    val |= enable << HISI_HIP08_CORE_COMMIT_SHIFT;
>>>
>>> I would use the explicitly masked values when you update
>>> a register.
>>>
>> ok, how about changing these code to this:
>> val &= ~GENMASK(12, 13);
> 
> I would do :
> 
> // Rename the HISI_HIP08_CORE_COMMIT_CLEAR to HISI_HIP08_CORE_COMMIT_MASK
> // above.
> #define HISI_HIP08_CORE_COMMIT_MASK        0x3000
> 
> #define HISI_HIP08_CORE_COMMIT_FULL        0b00
> #define HISI_HIP08_CORE_COMMIT_LVL_1        0b01
> 
> 
> u8 commit = enable ? HISI_HIP08_CORE_COMMIT_LVL_1 : HISI_HIP08_CORE_COMMIT_FULL;
> 
> ...
> 
> val |= commit << HISI_HIP08_CORE_COMMIT_SHIFT;
> 
> ..
> 
> 
> Suzuki
> 
> .
ok, I'll send a new version. :)

Thanks
Qi
> 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v5] coresight: etm4x: Modify core-commit of cpu to avoid the overflow of HiSilicon ETM
@ 2020-12-07 11:32             ` Qi Liu
  0 siblings, 0 replies; 16+ messages in thread
From: Qi Liu @ 2020-12-07 11:32 UTC (permalink / raw)
  To: Suzuki K Poulose, Mathieu Poirier
  Cc: coresight, linuxarm, linux-kernel, linux-arm-kernel, mike.leach



On 2020/12/7 19:27, Suzuki K Poulose wrote:
> On 12/7/20 11:21 AM, Qi Liu wrote:
>>
>> Hi Suzuki,
>> On 2020/12/7 18:38, Suzuki K Poulose wrote:
>>> On 12/7/20 2:08 AM, Qi Liu wrote:
>>>> Hi Mathieu,
>>>>
>>>> On 2020/12/5 2:55, Mathieu Poirier wrote:
>>>>> On Thu, Nov 26, 2020 at 09:34:30PM +0800, Qi Liu wrote:
>>>>>> The ETM device can't keep up with the core pipeline when cpu core
>>>>>> is at full speed. This may cause overflow within core and its ETM.
>>>>>> This is a common phenomenon on ETM devices.
>>>>>>
>>>>>> On HiSilicon Hip08 platform, a specific feature is added to set
>>>>>> core pipeline. So commit rate can be reduced manually to avoid ETM
>>>>>> overflow.
>>>>>>
>>>>>> Signed-off-by: Qi Liu <liuqi115@huawei.com>
>>>>>> ---
>>>>>> Change since v1:
>>>>>> - add CONFIG_ETM4X_IMPDEF_FEATURE and CONFIG_ETM4X_IMPDEF_HISILICON
>>>>>>     to keep specific feature off platforms which don't use it.
>>>>>> Change since v2:
>>>>>> - remove some unused variable.
>>>>>> Change since v3:
>>>>>> - use read/write_sysreg_s() to access register.
>>>>>> Change since v4:
>>>>>> - rename the call back function to a more generic name, and fix some
>>>>>>     compile warnings.
>>>>>>
>>>>>>    drivers/hwtracing/coresight/Kconfig                |  9 +++
>>>>>>    drivers/hwtracing/coresight/coresight-etm4x-core.c | 88 ++++++++++++++++++++++
>>>>>>    drivers/hwtracing/coresight/coresight-etm4x.h      |  8 ++
>>>>>>    3 files changed, 105 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
>>>>>> index c119824..1cc3601 100644
>>>>>> --- a/drivers/hwtracing/coresight/Kconfig
>>>>>> +++ b/drivers/hwtracing/coresight/Kconfig
>>>>>> @@ -110,6 +110,15 @@ config CORESIGHT_SOURCE_ETM4X
>>>>>>          To compile this driver as a module, choose M here: the
>>>>>>          module will be called coresight-etm4x.
>>>>>>
>>>>>> +config ETM4X_IMPDEF_FEATURE
>>>>>> +    bool "Control overflow impdef support in CoreSight ETM 4.x driver "
>>>>>> +    depends on CORESIGHT_SOURCE_ETM4X
>>>>>> +    help
>>>>>> +      This control provides overflow implement define for CoreSight
>>>>>> +      ETM 4.x tracer module which could not reduce commit race
>>>>>> +      automatically, and could avoid overflow within ETM tracer module
>>>>>> +      and its cpu core.
>>>>>> +
>>>>>>    config CORESIGHT_STM
>>>>>>        tristate "CoreSight System Trace Macrocell driver"
>>>>>>        depends on (ARM && !(CPU_32v3 || CPU_32v4 || CPU_32v4T)) || ARM64
>>>>>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>>>> index abd706b..fcee27a 100644
>>>>>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>>>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
>>>>>> @@ -3,6 +3,7 @@
>>>>>>     * Copyright (c) 2014, The Linux Foundation. All rights reserved.
>>>>>>     */
>>>>>>
>>>>>> +#include <linux/bitops.h>
>>>>>>    #include <linux/kernel.h>
>>>>>>    #include <linux/moduleparam.h>
>>>>>>    #include <linux/init.h>
>>>>>> @@ -28,7 +29,9 @@
>>>>>>    #include <linux/perf_event.h>
>>>>>>    #include <linux/pm_runtime.h>
>>>>>>    #include <linux/property.h>
>>>>>> +
>>>>>>    #include <asm/sections.h>
>>>>>> +#include <asm/sysreg.h>
>>>>>>    #include <asm/local.h>
>>>>>>    #include <asm/virt.h>
>>>>>>
>>>>>> @@ -103,6 +106,87 @@ struct etm4_enable_arg {
>>>>>>        int rc;
>>>>>>    };
>>>>>>
>>>>>> +#ifdef CONFIG_ETM4X_IMPDEF_FEATURE
>>>>>> +
>>>>>> +#define HISI_HIP08_AMBA_ID        0x000b6d01
>>>>>> +#define ETM4_AMBA_MASK            0xfffff
>>>>>> +#define HISI_HIP08_CORE_COMMIT_CLEAR    0x3000
>>>>>
>>>>> Here bit 12 and 13 are cleared but in etm4_hisi_config_core_commit() only bit 12
>>>>> is set - is this intentional?  What is bit 13 for?
>>>>>
>>>> bit 12 and 13 are used together to set core-commit, 2'b00 means cpu is at full speed,
>>>> 2'b01, 2'b10, 2'b11 means reduce the speed of cpu pipeline, and 2'b01 means speed is
>>>> reduced to minimum value. So bit 12 and 13 should be cleared together in
>>>> etm4_hisi_config_core_commit().
>>>
>>> Please could you document this in the function.
>>>
>> of course, thanks.
>>>>
>>>> Qi
>>>>
>>>>>> +#define HISI_HIP08_CORE_COMMIT_SHIFT    12
>>>>>> +#define HISI_HIP08_CORE_COMMIT_REG    sys_reg(3, 1, 15, 2, 5)
>>>>>> +
>>>>>> +struct etm4_arch_features {
>>>>>> +    void (*arch_callback)(bool enable);
>>>>>> +};
>>>>>> +
>>>>>> +static bool etm4_hisi_match_pid(unsigned int id)
>>>>>> +{
>>>>>> +    return (id & ETM4_AMBA_MASK) == HISI_HIP08_AMBA_ID;
>>>>>> +}
>>>>>> +
>>>>>> +static void etm4_hisi_config_core_commit(bool enable)
>>>>>> +{
>>>>>> +    u64 val;
>>>>>> +
>>>>>> +    val = read_sysreg_s(HISI_HIP08_CORE_COMMIT_REG);
>>>>>> +    val &= ~HISI_HIP08_CORE_COMMIT_CLEAR;
>>>>>> +    val |= enable << HISI_HIP08_CORE_COMMIT_SHIFT;
>>>
>>> I would use the explicitly masked values when you update
>>> a register.
>>>
>> ok, how about changing these code to this:
>> val &= ~GENMASK(12, 13);
> 
> I would do :
> 
> // Rename the HISI_HIP08_CORE_COMMIT_CLEAR to HISI_HIP08_CORE_COMMIT_MASK
> // above.
> #define HISI_HIP08_CORE_COMMIT_MASK        0x3000
> 
> #define HISI_HIP08_CORE_COMMIT_FULL        0b00
> #define HISI_HIP08_CORE_COMMIT_LVL_1        0b01
> 
> 
> u8 commit = enable ? HISI_HIP08_CORE_COMMIT_LVL_1 : HISI_HIP08_CORE_COMMIT_FULL;
> 
> ...
> 
> val |= commit << HISI_HIP08_CORE_COMMIT_SHIFT;
> 
> ..
> 
> 
> Suzuki
> 
> .
ok, I'll send a new version. :)

Thanks
Qi
> 


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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v5] coresight: etm4x: Modify core-commit of cpu to avoid the overflow of HiSilicon ETM
  2020-12-07 11:32             ` Qi Liu
@ 2020-12-07 16:00               ` Mathieu Poirier
  -1 siblings, 0 replies; 16+ messages in thread
From: Mathieu Poirier @ 2020-12-07 16:00 UTC (permalink / raw)
  To: Qi Liu
  Cc: Suzuki K Poulose, coresight, linuxarm, linux-kernel,
	linux-arm-kernel, mike.leach

On Mon, Dec 07, 2020 at 07:32:21PM +0800, Qi Liu wrote:
> 
> 
> On 2020/12/7 19:27, Suzuki K Poulose wrote:
> > On 12/7/20 11:21 AM, Qi Liu wrote:
> >>
> >> Hi Suzuki,
> >> On 2020/12/7 18:38, Suzuki K Poulose wrote:
> >>> On 12/7/20 2:08 AM, Qi Liu wrote:
> >>>> Hi Mathieu,
> >>>>
> >>>> On 2020/12/5 2:55, Mathieu Poirier wrote:
> >>>>> On Thu, Nov 26, 2020 at 09:34:30PM +0800, Qi Liu wrote:
> >>>>>> The ETM device can't keep up with the core pipeline when cpu core
> >>>>>> is at full speed. This may cause overflow within core and its ETM.
> >>>>>> This is a common phenomenon on ETM devices.
> >>>>>>
> >>>>>> On HiSilicon Hip08 platform, a specific feature is added to set
> >>>>>> core pipeline. So commit rate can be reduced manually to avoid ETM
> >>>>>> overflow.
> >>>>>>
> >>>>>> Signed-off-by: Qi Liu <liuqi115@huawei.com>
> >>>>>> ---
> >>>>>> Change since v1:
> >>>>>> - add CONFIG_ETM4X_IMPDEF_FEATURE and CONFIG_ETM4X_IMPDEF_HISILICON
> >>>>>>     to keep specific feature off platforms which don't use it.
> >>>>>> Change since v2:
> >>>>>> - remove some unused variable.
> >>>>>> Change since v3:
> >>>>>> - use read/write_sysreg_s() to access register.
> >>>>>> Change since v4:
> >>>>>> - rename the call back function to a more generic name, and fix some
> >>>>>>     compile warnings.
> >>>>>>
> >>>>>>    drivers/hwtracing/coresight/Kconfig                |  9 +++
> >>>>>>    drivers/hwtracing/coresight/coresight-etm4x-core.c | 88 ++++++++++++++++++++++
> >>>>>>    drivers/hwtracing/coresight/coresight-etm4x.h      |  8 ++
> >>>>>>    3 files changed, 105 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> >>>>>> index c119824..1cc3601 100644
> >>>>>> --- a/drivers/hwtracing/coresight/Kconfig
> >>>>>> +++ b/drivers/hwtracing/coresight/Kconfig
> >>>>>> @@ -110,6 +110,15 @@ config CORESIGHT_SOURCE_ETM4X
> >>>>>>          To compile this driver as a module, choose M here: the
> >>>>>>          module will be called coresight-etm4x.
> >>>>>>
> >>>>>> +config ETM4X_IMPDEF_FEATURE
> >>>>>> +    bool "Control overflow impdef support in CoreSight ETM 4.x driver "
> >>>>>> +    depends on CORESIGHT_SOURCE_ETM4X
> >>>>>> +    help
> >>>>>> +      This control provides overflow implement define for CoreSight
> >>>>>> +      ETM 4.x tracer module which could not reduce commit race
> >>>>>> +      automatically, and could avoid overflow within ETM tracer module
> >>>>>> +      and its cpu core.
> >>>>>> +
> >>>>>>    config CORESIGHT_STM
> >>>>>>        tristate "CoreSight System Trace Macrocell driver"
> >>>>>>        depends on (ARM && !(CPU_32v3 || CPU_32v4 || CPU_32v4T)) || ARM64
> >>>>>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> >>>>>> index abd706b..fcee27a 100644
> >>>>>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> >>>>>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> >>>>>> @@ -3,6 +3,7 @@
> >>>>>>     * Copyright (c) 2014, The Linux Foundation. All rights reserved.
> >>>>>>     */
> >>>>>>
> >>>>>> +#include <linux/bitops.h>
> >>>>>>    #include <linux/kernel.h>
> >>>>>>    #include <linux/moduleparam.h>
> >>>>>>    #include <linux/init.h>
> >>>>>> @@ -28,7 +29,9 @@
> >>>>>>    #include <linux/perf_event.h>
> >>>>>>    #include <linux/pm_runtime.h>
> >>>>>>    #include <linux/property.h>
> >>>>>> +
> >>>>>>    #include <asm/sections.h>
> >>>>>> +#include <asm/sysreg.h>
> >>>>>>    #include <asm/local.h>
> >>>>>>    #include <asm/virt.h>
> >>>>>>
> >>>>>> @@ -103,6 +106,87 @@ struct etm4_enable_arg {
> >>>>>>        int rc;
> >>>>>>    };
> >>>>>>
> >>>>>> +#ifdef CONFIG_ETM4X_IMPDEF_FEATURE
> >>>>>> +
> >>>>>> +#define HISI_HIP08_AMBA_ID        0x000b6d01
> >>>>>> +#define ETM4_AMBA_MASK            0xfffff
> >>>>>> +#define HISI_HIP08_CORE_COMMIT_CLEAR    0x3000
> >>>>>
> >>>>> Here bit 12 and 13 are cleared but in etm4_hisi_config_core_commit() only bit 12
> >>>>> is set - is this intentional?  What is bit 13 for?
> >>>>>
> >>>> bit 12 and 13 are used together to set core-commit, 2'b00 means cpu is at full speed,
> >>>> 2'b01, 2'b10, 2'b11 means reduce the speed of cpu pipeline, and 2'b01 means speed is
> >>>> reduced to minimum value. So bit 12 and 13 should be cleared together in
> >>>> etm4_hisi_config_core_commit().
> >>>
> >>> Please could you document this in the function.
> >>>
> >> of course, thanks.
> >>>>
> >>>> Qi
> >>>>
> >>>>>> +#define HISI_HIP08_CORE_COMMIT_SHIFT    12
> >>>>>> +#define HISI_HIP08_CORE_COMMIT_REG    sys_reg(3, 1, 15, 2, 5)
> >>>>>> +
> >>>>>> +struct etm4_arch_features {
> >>>>>> +    void (*arch_callback)(bool enable);
> >>>>>> +};
> >>>>>> +
> >>>>>> +static bool etm4_hisi_match_pid(unsigned int id)
> >>>>>> +{
> >>>>>> +    return (id & ETM4_AMBA_MASK) == HISI_HIP08_AMBA_ID;
> >>>>>> +}
> >>>>>> +
> >>>>>> +static void etm4_hisi_config_core_commit(bool enable)
> >>>>>> +{
> >>>>>> +    u64 val;
> >>>>>> +
> >>>>>> +    val = read_sysreg_s(HISI_HIP08_CORE_COMMIT_REG);
> >>>>>> +    val &= ~HISI_HIP08_CORE_COMMIT_CLEAR;
> >>>>>> +    val |= enable << HISI_HIP08_CORE_COMMIT_SHIFT;
> >>>
> >>> I would use the explicitly masked values when you update
> >>> a register.
> >>>
> >> ok, how about changing these code to this:
> >> val &= ~GENMASK(12, 13);
> > 
> > I would do :
> > 
> > // Rename the HISI_HIP08_CORE_COMMIT_CLEAR to HISI_HIP08_CORE_COMMIT_MASK
> > // above.
> > #define HISI_HIP08_CORE_COMMIT_MASK        0x3000
> > 
> > #define HISI_HIP08_CORE_COMMIT_FULL        0b00
> > #define HISI_HIP08_CORE_COMMIT_LVL_1        0b01
> > 
> > 
> > u8 commit = enable ? HISI_HIP08_CORE_COMMIT_LVL_1 : HISI_HIP08_CORE_COMMIT_FULL;
> > 
> > ...
> > 
> > val |= commit << HISI_HIP08_CORE_COMMIT_SHIFT;
> > 
> > ..
> > 
> > 
> > Suzuki
> > 
> > .
> ok, I'll send a new version. :)
> 

Please do so by tomorrow morning (North America time) if you want to see this
going in the v5.11 merge window.  Otherwise it will be another 3 months.

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH v5] coresight: etm4x: Modify core-commit of cpu to avoid the overflow of HiSilicon ETM
@ 2020-12-07 16:00               ` Mathieu Poirier
  0 siblings, 0 replies; 16+ messages in thread
From: Mathieu Poirier @ 2020-12-07 16:00 UTC (permalink / raw)
  To: Qi Liu
  Cc: Suzuki K Poulose, coresight, linux-kernel, linuxarm,
	linux-arm-kernel, mike.leach

On Mon, Dec 07, 2020 at 07:32:21PM +0800, Qi Liu wrote:
> 
> 
> On 2020/12/7 19:27, Suzuki K Poulose wrote:
> > On 12/7/20 11:21 AM, Qi Liu wrote:
> >>
> >> Hi Suzuki,
> >> On 2020/12/7 18:38, Suzuki K Poulose wrote:
> >>> On 12/7/20 2:08 AM, Qi Liu wrote:
> >>>> Hi Mathieu,
> >>>>
> >>>> On 2020/12/5 2:55, Mathieu Poirier wrote:
> >>>>> On Thu, Nov 26, 2020 at 09:34:30PM +0800, Qi Liu wrote:
> >>>>>> The ETM device can't keep up with the core pipeline when cpu core
> >>>>>> is at full speed. This may cause overflow within core and its ETM.
> >>>>>> This is a common phenomenon on ETM devices.
> >>>>>>
> >>>>>> On HiSilicon Hip08 platform, a specific feature is added to set
> >>>>>> core pipeline. So commit rate can be reduced manually to avoid ETM
> >>>>>> overflow.
> >>>>>>
> >>>>>> Signed-off-by: Qi Liu <liuqi115@huawei.com>
> >>>>>> ---
> >>>>>> Change since v1:
> >>>>>> - add CONFIG_ETM4X_IMPDEF_FEATURE and CONFIG_ETM4X_IMPDEF_HISILICON
> >>>>>>     to keep specific feature off platforms which don't use it.
> >>>>>> Change since v2:
> >>>>>> - remove some unused variable.
> >>>>>> Change since v3:
> >>>>>> - use read/write_sysreg_s() to access register.
> >>>>>> Change since v4:
> >>>>>> - rename the call back function to a more generic name, and fix some
> >>>>>>     compile warnings.
> >>>>>>
> >>>>>>    drivers/hwtracing/coresight/Kconfig                |  9 +++
> >>>>>>    drivers/hwtracing/coresight/coresight-etm4x-core.c | 88 ++++++++++++++++++++++
> >>>>>>    drivers/hwtracing/coresight/coresight-etm4x.h      |  8 ++
> >>>>>>    3 files changed, 105 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/hwtracing/coresight/Kconfig b/drivers/hwtracing/coresight/Kconfig
> >>>>>> index c119824..1cc3601 100644
> >>>>>> --- a/drivers/hwtracing/coresight/Kconfig
> >>>>>> +++ b/drivers/hwtracing/coresight/Kconfig
> >>>>>> @@ -110,6 +110,15 @@ config CORESIGHT_SOURCE_ETM4X
> >>>>>>          To compile this driver as a module, choose M here: the
> >>>>>>          module will be called coresight-etm4x.
> >>>>>>
> >>>>>> +config ETM4X_IMPDEF_FEATURE
> >>>>>> +    bool "Control overflow impdef support in CoreSight ETM 4.x driver "
> >>>>>> +    depends on CORESIGHT_SOURCE_ETM4X
> >>>>>> +    help
> >>>>>> +      This control provides overflow implement define for CoreSight
> >>>>>> +      ETM 4.x tracer module which could not reduce commit race
> >>>>>> +      automatically, and could avoid overflow within ETM tracer module
> >>>>>> +      and its cpu core.
> >>>>>> +
> >>>>>>    config CORESIGHT_STM
> >>>>>>        tristate "CoreSight System Trace Macrocell driver"
> >>>>>>        depends on (ARM && !(CPU_32v3 || CPU_32v4 || CPU_32v4T)) || ARM64
> >>>>>> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-core.c b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> >>>>>> index abd706b..fcee27a 100644
> >>>>>> --- a/drivers/hwtracing/coresight/coresight-etm4x-core.c
> >>>>>> +++ b/drivers/hwtracing/coresight/coresight-etm4x-core.c
> >>>>>> @@ -3,6 +3,7 @@
> >>>>>>     * Copyright (c) 2014, The Linux Foundation. All rights reserved.
> >>>>>>     */
> >>>>>>
> >>>>>> +#include <linux/bitops.h>
> >>>>>>    #include <linux/kernel.h>
> >>>>>>    #include <linux/moduleparam.h>
> >>>>>>    #include <linux/init.h>
> >>>>>> @@ -28,7 +29,9 @@
> >>>>>>    #include <linux/perf_event.h>
> >>>>>>    #include <linux/pm_runtime.h>
> >>>>>>    #include <linux/property.h>
> >>>>>> +
> >>>>>>    #include <asm/sections.h>
> >>>>>> +#include <asm/sysreg.h>
> >>>>>>    #include <asm/local.h>
> >>>>>>    #include <asm/virt.h>
> >>>>>>
> >>>>>> @@ -103,6 +106,87 @@ struct etm4_enable_arg {
> >>>>>>        int rc;
> >>>>>>    };
> >>>>>>
> >>>>>> +#ifdef CONFIG_ETM4X_IMPDEF_FEATURE
> >>>>>> +
> >>>>>> +#define HISI_HIP08_AMBA_ID        0x000b6d01
> >>>>>> +#define ETM4_AMBA_MASK            0xfffff
> >>>>>> +#define HISI_HIP08_CORE_COMMIT_CLEAR    0x3000
> >>>>>
> >>>>> Here bit 12 and 13 are cleared but in etm4_hisi_config_core_commit() only bit 12
> >>>>> is set - is this intentional?  What is bit 13 for?
> >>>>>
> >>>> bit 12 and 13 are used together to set core-commit, 2'b00 means cpu is at full speed,
> >>>> 2'b01, 2'b10, 2'b11 means reduce the speed of cpu pipeline, and 2'b01 means speed is
> >>>> reduced to minimum value. So bit 12 and 13 should be cleared together in
> >>>> etm4_hisi_config_core_commit().
> >>>
> >>> Please could you document this in the function.
> >>>
> >> of course, thanks.
> >>>>
> >>>> Qi
> >>>>
> >>>>>> +#define HISI_HIP08_CORE_COMMIT_SHIFT    12
> >>>>>> +#define HISI_HIP08_CORE_COMMIT_REG    sys_reg(3, 1, 15, 2, 5)
> >>>>>> +
> >>>>>> +struct etm4_arch_features {
> >>>>>> +    void (*arch_callback)(bool enable);
> >>>>>> +};
> >>>>>> +
> >>>>>> +static bool etm4_hisi_match_pid(unsigned int id)
> >>>>>> +{
> >>>>>> +    return (id & ETM4_AMBA_MASK) == HISI_HIP08_AMBA_ID;
> >>>>>> +}
> >>>>>> +
> >>>>>> +static void etm4_hisi_config_core_commit(bool enable)
> >>>>>> +{
> >>>>>> +    u64 val;
> >>>>>> +
> >>>>>> +    val = read_sysreg_s(HISI_HIP08_CORE_COMMIT_REG);
> >>>>>> +    val &= ~HISI_HIP08_CORE_COMMIT_CLEAR;
> >>>>>> +    val |= enable << HISI_HIP08_CORE_COMMIT_SHIFT;
> >>>
> >>> I would use the explicitly masked values when you update
> >>> a register.
> >>>
> >> ok, how about changing these code to this:
> >> val &= ~GENMASK(12, 13);
> > 
> > I would do :
> > 
> > // Rename the HISI_HIP08_CORE_COMMIT_CLEAR to HISI_HIP08_CORE_COMMIT_MASK
> > // above.
> > #define HISI_HIP08_CORE_COMMIT_MASK        0x3000
> > 
> > #define HISI_HIP08_CORE_COMMIT_FULL        0b00
> > #define HISI_HIP08_CORE_COMMIT_LVL_1        0b01
> > 
> > 
> > u8 commit = enable ? HISI_HIP08_CORE_COMMIT_LVL_1 : HISI_HIP08_CORE_COMMIT_FULL;
> > 
> > ...
> > 
> > val |= commit << HISI_HIP08_CORE_COMMIT_SHIFT;
> > 
> > ..
> > 
> > 
> > Suzuki
> > 
> > .
> ok, I'll send a new version. :)
> 

Please do so by tomorrow morning (North America time) if you want to see this
going in the v5.11 merge window.  Otherwise it will be another 3 months.

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

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2020-12-07 16:02 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-26 13:34 [PATCH v5] coresight: etm4x: Modify core-commit of cpu to avoid the overflow of HiSilicon ETM Qi Liu
2020-11-26 13:34 ` Qi Liu
2020-12-04 18:55 ` Mathieu Poirier
2020-12-04 18:55   ` Mathieu Poirier
2020-12-07  2:08   ` Qi Liu
2020-12-07  2:08     ` Qi Liu
2020-12-07 10:38     ` Suzuki K Poulose
2020-12-07 10:38       ` Suzuki K Poulose
2020-12-07 11:21       ` Qi Liu
2020-12-07 11:21         ` Qi Liu
2020-12-07 11:27         ` Suzuki K Poulose
2020-12-07 11:27           ` Suzuki K Poulose
2020-12-07 11:32           ` Qi Liu
2020-12-07 11:32             ` Qi Liu
2020-12-07 16:00             ` Mathieu Poirier
2020-12-07 16:00               ` Mathieu Poirier

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.