[PATCH v3 0/4] platform/x86: intel_telemetry: Fix logs and formatting

[PATCH v3 0/4] platform/x86: intel_telemetry: Fix logs and formatting

[Souvik Kumar Chakravarty]

This patchset fixes https://bugzilla.kernel.org/show_bug.cgi?id=197833, and
other issues related to telemetry counters. It also cleans up formatting
and removes redundant code.

It is rebased on top of the TESTING branch.

Changes since v2:
 * Changes in GCR read API name and adding back static qualifier to account
   for case of missing logs during suspend/resume 

Changes since v1:
 * Code-Review comments have been incorporated.

Souvik Kumar Chakravarty (4):
  platform/x86: intel_pmc_ipc: Add read64 API
  platform/x86: intel_telemetry: Fix suspend stats
  platform/x86: intel_telemetry: Improve S0ix logs
  platform/x86: intel_telemetry: Remove redundancies

 arch/x86/include/asm/intel_pmc_ipc.h           |  6 ++
 drivers/platform/x86/intel_pmc_ipc.c           | 33 +++++++++-
 drivers/platform/x86/intel_telemetry_debugfs.c | 83 +++++++++++---------------
 3 files changed, 72 insertions(+), 50 deletions(-)

-- 
2.7.4

[PATCH v1 1/4] platform/x86: intel_pmc_ipc: Add read64 API

[Souvik Kumar Chakravarty]

Add intel_pmc_gcr_read64() API for reading from 64-bit GCR registers.
This API will be called from intel_telemetry. Update description of
intel_pmc_gcr_read().

Signed-off-by: Souvik Kumar Chakravarty <souvik.k.chakravarty@intel.com>
---
 arch/x86/include/asm/intel_pmc_ipc.h |  6 ++++++
 drivers/platform/x86/intel_pmc_ipc.c | 33 +++++++++++++++++++++++++++++++--
 2 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/intel_pmc_ipc.h b/arch/x86/include/asm/intel_pmc_ipc.h
index fac89eb..19c9369 100644
--- a/arch/x86/include/asm/intel_pmc_ipc.h
+++ b/arch/x86/include/asm/intel_pmc_ipc.h
@@ -37,6 +37,7 @@ int intel_pmc_ipc_command(u32 cmd, u32 sub, u8 *in, u32 inlen,
 		u32 *out, u32 outlen);
 int intel_pmc_s0ix_counter_read(u64 *data);
 int intel_pmc_gcr_read(u32 offset, u32 *data);
+int intel_pmc_gcr_read64(u32 offset, u64 *data);
 int intel_pmc_gcr_write(u32 offset, u32 data);
 int intel_pmc_gcr_update(u32 offset, u32 mask, u32 val);
 
@@ -69,6 +70,11 @@ static inline int intel_pmc_gcr_read(u32 offset, u32 *data)
 	return -EINVAL;
 }
 
+static inline int intel_pmc_gcr_read64(u32 offset, u64 *data)
+{
+	return -EINVAL;
+}
+
 static inline int intel_pmc_gcr_write(u32 offset, u32 data)
 {
 	return -EINVAL;
diff --git a/drivers/platform/x86/intel_pmc_ipc.c b/drivers/platform/x86/intel_pmc_ipc.c
index e03fa314..e7edc8c 100644
--- a/drivers/platform/x86/intel_pmc_ipc.c
+++ b/drivers/platform/x86/intel_pmc_ipc.c
@@ -215,11 +215,11 @@ static inline int is_gcr_valid(u32 offset)
 }
 
 /**
- * intel_pmc_gcr_read() - Read PMC GCR register
+ * intel_pmc_gcr_read() - Read a 32-bit PMC GCR register
  * @offset:	offset of GCR register from GCR address base
  * @data:	data pointer for storing the register output
  *
- * Reads the PMC GCR register of given offset.
+ * Reads the 32-bit PMC GCR register at given offset.
  *
  * Return:	negative value on error or 0 on success.
  */
@@ -244,6 +244,35 @@ int intel_pmc_gcr_read(u32 offset, u32 *data)
 EXPORT_SYMBOL_GPL(intel_pmc_gcr_read);
 
 /**
+ * intel_pmc_gcr_read64() - Read a 64-bit PMC GCR register
+ * @offset:	offset of GCR register from GCR address base
+ * @data:	data pointer for storing the register output
+ *
+ * Reads the 64-bit PMC GCR register at given offset.
+ *
+ * Return:	negative value on error or 0 on success.
+ */
+int intel_pmc_gcr_read64(u32 offset, u64 *data)
+{
+	int ret;
+
+	spin_lock(&ipcdev.gcr_lock);
+
+	ret = is_gcr_valid(offset);
+	if (ret < 0) {
+		spin_unlock(&ipcdev.gcr_lock);
+		return ret;
+	}
+
+	*data = readq(ipcdev.gcr_mem_base + offset);
+
+	spin_unlock(&ipcdev.gcr_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(intel_pmc_gcr_read64);
+
+/**
  * intel_pmc_gcr_write() - Write PMC GCR register
  * @offset:	offset of GCR register from GCR address base
  * @data:	register update value
-- 
2.7.4

[PATCH v3 2/4] platform/x86: intel_telemetry: Fix suspend stats

[Souvik Kumar Chakravarty]

Suspend stats are not reported consistently due to a limitation in the PMC
firmware. This limitation causes a delay in updating the s0ix counters and
residencies in the telemetry log upon s0ix exit. As a consequence, reading
these counters from the suspend-exit notifier may result in zero read.

This patch fixes this issue by cross-verifying the s0ix residencies from
the GCR TELEM registers in case the counters are not incremented in the
telemetry log after suspend.

This fixes https://bugzilla.kernel.org/show_bug.cgi?id=197833

Reported-and-tested-by: Rajneesh Bhardwaj <rajneesh.bhardwaj@intel.com>
Signed-off-by: Souvik Kumar Chakravarty <souvik.k.chakravarty@intel.com>
---
 drivers/platform/x86/intel_telemetry_debugfs.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

Changes since v2:
 * Use pmc_ipc_gcr_read64() API to read 64-bits at a time
 * Re-introduce the static qualifier to handle the case where the telemetry
   suspend/resume log does not contain the s0ix residencies. In that case,
   we would like to retain the values from the previous/initial cycle.

Changes since v1:
 * Use pmc_ipc_gcr_readq API to read 64-bits at a time

diff --git a/drivers/platform/x86/intel_telemetry_debugfs.c b/drivers/platform/x86/intel_telemetry_debugfs.c
index 4249e826..5bc4f20 100644
--- a/drivers/platform/x86/intel_telemetry_debugfs.c
+++ b/drivers/platform/x86/intel_telemetry_debugfs.c
@@ -890,6 +890,31 @@ static int pm_suspend_exit_cb(void)
 		goto out;
 	}
 
+	/*
+	 * Due to some design limitations in the firmware, sometimes the
+	 * counters do not get updated by the time we reach here. As a
+	 * workaround, we try to see if this was a genuine case of sleep
+	 * failure or not by cross-checking from PMC GCR registers directly.
+	 */
+	if (suspend_shlw_ctr_exit == suspend_shlw_ctr_temp &&
+	    suspend_deep_ctr_exit == suspend_deep_ctr_temp) {
+		ret = intel_pmc_gcr_read64(PMC_GCR_TELEM_SHLW_S0IX_REG,
+					  &suspend_shlw_res_exit);
+		if (ret < 0)
+			goto out;
+
+		ret = intel_pmc_gcr_read64(PMC_GCR_TELEM_DEEP_S0IX_REG,
+					  &suspend_deep_res_exit);
+		if (ret < 0)
+			goto out;
+
+		if (suspend_shlw_res_exit > suspend_shlw_res_temp)
+			suspend_shlw_ctr_exit++;
+
+		if (suspend_deep_res_exit > suspend_deep_res_temp)
+			suspend_deep_ctr_exit++;
+	}
+
 	suspend_shlw_ctr_exit -= suspend_shlw_ctr_temp;
 	suspend_deep_ctr_exit -= suspend_deep_ctr_temp;
 	suspend_shlw_res_exit -= suspend_shlw_res_temp;
-- 
2.7.4

[PATCH v2 3/4] platform/x86: intel_telemetry: Improve S0ix logs

[Souvik Kumar Chakravarty]

Suspend with shallow wakes is not a useful parameter since the phenomena
does not exist on deployed devices and is only a parameter of use during
device power-on phase. The field always reads zero. Additionally there
are other easier methods to detect it, e.g., if the S0ix counter
increments by more than one during suspend. Hence the field is superfluous
and can be removed.

This patch also slightly renames the S0ix total field for better
viewability.

Signed-off-by: Souvik Kumar Chakravarty <souvik.k.chakravarty@intel.com>
---
 drivers/platform/x86/intel_telemetry_debugfs.c | 45 ++++----------------------
 1 file changed, 7 insertions(+), 38 deletions(-)

Changes since v1:
 * Remove alignment changes from this patch

diff --git a/drivers/platform/x86/intel_telemetry_debugfs.c b/drivers/platform/x86/intel_telemetry_debugfs.c
index 5bc4f20..97aae98 100644
--- a/drivers/platform/x86/intel_telemetry_debugfs.c
+++ b/drivers/platform/x86/intel_telemetry_debugfs.c
@@ -98,10 +98,6 @@ static u32 suspend_shlw_ctr_temp, suspend_deep_ctr_temp;
 static u64 suspend_shlw_res_temp, suspend_deep_res_temp;
 
 struct telemetry_susp_stats {
-	u32 shlw_swake_ctr;
-	u32 deep_swake_ctr;
-	u64 shlw_swake_res;
-	u64 deep_swake_res;
 	u32 shlw_ctr;
 	u32 deep_ctr;
 	u64 shlw_res;
@@ -598,19 +594,15 @@ static int telem_soc_states_show(struct seq_file *s, void *unused)
 
 	seq_printf(s, "S0IX Shallow\t\t\t %10u\t %10llu\n",
 		   s0ix_shlw_ctr -
-		   conf->suspend_stats.shlw_ctr -
-		   conf->suspend_stats.shlw_swake_ctr,
+		   conf->suspend_stats.shlw_ctr,
 		   (u64)((s0ix_shlw_res -
-		   conf->suspend_stats.shlw_res -
-		   conf->suspend_stats.shlw_swake_res)*10/192));
+		   conf->suspend_stats.shlw_res)*10/192));
 
 	seq_printf(s, "S0IX Deep\t\t\t %10u\t %10llu\n",
 		   s0ix_deep_ctr -
-		   conf->suspend_stats.deep_ctr -
-		   conf->suspend_stats.deep_swake_ctr,
+		   conf->suspend_stats.deep_ctr,
 		   (u64)((s0ix_deep_res -
-		   conf->suspend_stats.deep_res -
-		   conf->suspend_stats.deep_swake_res)*10/192));
+		   conf->suspend_stats.deep_res)*10/192));
 
 	seq_printf(s, "Suspend(With S0ixShallow)\t %10u\t %10llu\n",
 		   conf->suspend_stats.shlw_ctr,
@@ -620,13 +612,7 @@ static int telem_soc_states_show(struct seq_file *s, void *unused)
 		   conf->suspend_stats.deep_ctr,
 		   (u64)(conf->suspend_stats.deep_res*10)/192);
 
-	seq_printf(s, "Suspend(With Shallow-Wakes)\t %10u\t %10llu\n",
-		   conf->suspend_stats.shlw_swake_ctr +
-		   conf->suspend_stats.deep_swake_ctr,
-		   (u64)((conf->suspend_stats.shlw_swake_res +
-		   conf->suspend_stats.deep_swake_res)*10/192));
-
-	seq_printf(s, "S0IX+Suspend Total\t\t %10u\t %10llu\n", s0ix_total_ctr,
+	seq_printf(s, "TOTAL S0IX\t\t\t %10u\t %10llu\n", s0ix_total_ctr,
 				(u64)(s0ix_total_res*10/192));
 	seq_puts(s, "\n-------------------------------------------------\n");
 	seq_puts(s, "\t\tDEVICE STATES\n");
@@ -920,23 +906,15 @@ static int pm_suspend_exit_cb(void)
 	suspend_shlw_res_exit -= suspend_shlw_res_temp;
 	suspend_deep_res_exit -= suspend_deep_res_temp;
 
-	if (suspend_shlw_ctr_exit == 1) {
+	if (suspend_shlw_ctr_exit != 0) {
 		conf->suspend_stats.shlw_ctr +=
 		suspend_shlw_ctr_exit;
 
 		conf->suspend_stats.shlw_res +=
 		suspend_shlw_res_exit;
 	}
-	/* Shallow Wakes Case */
-	else if (suspend_shlw_ctr_exit > 1) {
-		conf->suspend_stats.shlw_swake_ctr +=
-		suspend_shlw_ctr_exit;
-
-		conf->suspend_stats.shlw_swake_res +=
-		suspend_shlw_res_exit;
-	}
 
-	if (suspend_deep_ctr_exit == 1) {
+	if (suspend_deep_ctr_exit != 0) {
 		conf->suspend_stats.deep_ctr +=
 		suspend_deep_ctr_exit;
 
@@ -944,15 +922,6 @@ static int pm_suspend_exit_cb(void)
 		suspend_deep_res_exit;
 	}
 
-	/* Shallow Wakes Case */
-	else if (suspend_deep_ctr_exit > 1) {
-		conf->suspend_stats.deep_swake_ctr +=
-		suspend_deep_ctr_exit;
-
-		conf->suspend_stats.deep_swake_res +=
-		suspend_deep_res_exit;
-	}
-
 out:
 	suspend_prep_ok = 0;
 	return NOTIFY_OK;
-- 
2.7.4

[PATCH v2 4/4] platform/x86: intel_telemetry: Remove redundancies

[Souvik Kumar Chakravarty]

This patch removes unnecessary header files and newlines.
It also fixes some alignment issues.

Signed-off-by: Souvik Kumar Chakravarty <souvik.k.chakravarty@intel.com>
---
 drivers/platform/x86/intel_telemetry_debugfs.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

Changes since v1:
 * Consolidated alignment changes into this patch

diff --git a/drivers/platform/x86/intel_telemetry_debugfs.c b/drivers/platform/x86/intel_telemetry_debugfs.c
index 97aae98..ffd0474 100644
--- a/drivers/platform/x86/intel_telemetry_debugfs.c
+++ b/drivers/platform/x86/intel_telemetry_debugfs.c
@@ -23,7 +23,6 @@
  */
 #include <linux/debugfs.h>
 #include <linux/device.h>
-#include <linux/io.h>
 #include <linux/module.h>
 #include <linux/pci.h>
 #include <linux/seq_file.h>
@@ -32,11 +31,10 @@
 #include <asm/cpu_device_id.h>
 #include <asm/intel-family.h>
 #include <asm/intel_pmc_ipc.h>
-#include <asm/intel_punit_ipc.h>
 #include <asm/intel_telemetry.h>
 
-#define DRIVER_NAME	"telemetry_soc_debugfs"
-#define DRIVER_VERSION	"1.0.0"
+#define DRIVER_NAME			"telemetry_soc_debugfs"
+#define DRIVER_VERSION			"1.0.0"
 
 /* ApolloLake SoC Event-IDs */
 #define TELEM_APL_PSS_PSTATES_ID	0x2802
@@ -246,7 +244,6 @@ static struct telem_ioss_pg_info telem_apl_ioss_pg_data[] = {
 	{"PRTC",	25},
 };
 
-
 struct telemetry_debugfs_conf {
 	struct telemetry_susp_stats suspend_stats;
 	struct dentry *telemetry_dbg_dir;
@@ -381,7 +378,6 @@ static int telem_pss_states_show(struct seq_file *s, void *unused)
 			TELEM_APL_MASK_PCS_STATE;
 		}
 
-
 		TELEM_CHECK_AND_PARSE_EVTS(conf->pss_idle_id,
 					   conf->pss_idle_evts - 1,
 					   pss_idle, evtlog[index].telem_evtlog,
@@ -401,7 +397,6 @@ static int telem_pss_states_show(struct seq_file *s, void *unused)
 					   conf->pcs_s0ix_blkd_data,
 					   TELEM_MASK_BYTE);
 
-
 		TELEM_CHECK_AND_PARSE_EVTS(conf->pss_wakeup_id,
 					   conf->pss_wakeup_evts,
 					   pss_s0ix_wakeup,
@@ -494,7 +489,6 @@ static const struct file_operations telem_pss_ops = {
 	.release	= single_release,
 };
 
-
 static int telem_ioss_states_show(struct seq_file *s, void *unused)
 {
 	struct telemetry_evtlog evtlog[TELEM_MAX_OS_ALLOCATED_EVENTS];
@@ -613,7 +607,7 @@ static int telem_soc_states_show(struct seq_file *s, void *unused)
 		   (u64)(conf->suspend_stats.deep_res*10)/192);
 
 	seq_printf(s, "TOTAL S0IX\t\t\t %10u\t %10llu\n", s0ix_total_ctr,
-				(u64)(s0ix_total_res*10/192));
+		   (u64)(s0ix_total_res*10/192));
 	seq_puts(s, "\n-------------------------------------------------\n");
 	seq_puts(s, "\t\tDEVICE STATES\n");
 	seq_puts(s, "-------------------------------------------------\n");
@@ -758,7 +752,6 @@ static const struct file_operations telem_pss_trc_verb_ops = {
 	.release	= single_release,
 };
 
-
 static int telem_ioss_trc_verb_show(struct seq_file *s, void *unused)
 {
 	u32 verbosity;
-- 
2.7.4

Re: [PATCH v3 0/4] platform/x86: intel_telemetry: Fix logs and formatting

[Andy Shevchenko]

On Fri, Nov 24, 2017 at 3:34 PM, Souvik Kumar Chakravarty
<souvik.k.chakravarty@intel.com> wrote:
> This patchset fixes https://bugzilla.kernel.org/show_bug.cgi?id=197833, and
> other issues related to telemetry counters. It also cleans up formatting
> and removes redundant code.
>
> It is rebased on top of the TESTING branch.
>

Pushed to my review and testing queue, thanks!

> Changes since v2:
>  * Changes in GCR read API name and adding back static qualifier to account
>    for case of missing logs during suspend/resume
>
> Changes since v1:
>  * Code-Review comments have been incorporated.
>
> Souvik Kumar Chakravarty (4):
>   platform/x86: intel_pmc_ipc: Add read64 API
>   platform/x86: intel_telemetry: Fix suspend stats
>   platform/x86: intel_telemetry: Improve S0ix logs
>   platform/x86: intel_telemetry: Remove redundancies
>
>  arch/x86/include/asm/intel_pmc_ipc.h           |  6 ++
>  drivers/platform/x86/intel_pmc_ipc.c           | 33 +++++++++-
>  drivers/platform/x86/intel_telemetry_debugfs.c | 83 +++++++++++---------------
>  3 files changed, 72 insertions(+), 50 deletions(-)
>
> --
> 2.7.4
>



-- 
With Best Regards,
Andy Shevchenko

[PATCH v2 3/4] platform/x86: intel_telemetry: Improve S0ix logs

[Souvik Kumar Chakravarty]

Suspend with shallow wakes is not a useful parameter since the phenomena
does not exist on deployed devices and is only a parameter of use during
device power-on phase. The field always reads zero. Additionally there
are other easier methods to detect it, e.g., if the S0ix counter
increments by more than one during suspend. Hence the field is superfluous
and can be removed.

This patch also slightly renames the S0ix total field for better
viewability.

Signed-off-by: Souvik Kumar Chakravarty <souvik.k.chakravarty@intel.com>
---
 drivers/platform/x86/intel_telemetry_debugfs.c | 45 ++++----------------------
 1 file changed, 7 insertions(+), 38 deletions(-)

Changes since v1:
 * Remove alignment changes from this patch

diff --git a/drivers/platform/x86/intel_telemetry_debugfs.c b/drivers/platform/x86/intel_telemetry_debugfs.c
index d7e90fd..ab4a20f 100644
--- a/drivers/platform/x86/intel_telemetry_debugfs.c
+++ b/drivers/platform/x86/intel_telemetry_debugfs.c
@@ -98,10 +98,6 @@ static u32 suspend_shlw_ctr_temp, suspend_deep_ctr_temp;
 static u64 suspend_shlw_res_temp, suspend_deep_res_temp;
 
 struct telemetry_susp_stats {
-	u32 shlw_swake_ctr;
-	u32 deep_swake_ctr;
-	u64 shlw_swake_res;
-	u64 deep_swake_res;
 	u32 shlw_ctr;
 	u32 deep_ctr;
 	u64 shlw_res;
@@ -598,19 +594,15 @@ static int telem_soc_states_show(struct seq_file *s, void *unused)
 
 	seq_printf(s, "S0IX Shallow\t\t\t %10u\t %10llu\n",
 		   s0ix_shlw_ctr -
-		   conf->suspend_stats.shlw_ctr -
-		   conf->suspend_stats.shlw_swake_ctr,
+		   conf->suspend_stats.shlw_ctr,
 		   (u64)((s0ix_shlw_res -
-		   conf->suspend_stats.shlw_res -
-		   conf->suspend_stats.shlw_swake_res)*10/192));
+		   conf->suspend_stats.shlw_res)*10/192));
 
 	seq_printf(s, "S0IX Deep\t\t\t %10u\t %10llu\n",
 		   s0ix_deep_ctr -
-		   conf->suspend_stats.deep_ctr -
-		   conf->suspend_stats.deep_swake_ctr,
+		   conf->suspend_stats.deep_ctr,
 		   (u64)((s0ix_deep_res -
-		   conf->suspend_stats.deep_res -
-		   conf->suspend_stats.deep_swake_res)*10/192));
+		   conf->suspend_stats.deep_res)*10/192));
 
 	seq_printf(s, "Suspend(With S0ixShallow)\t %10u\t %10llu\n",
 		   conf->suspend_stats.shlw_ctr,
@@ -620,13 +612,7 @@ static int telem_soc_states_show(struct seq_file *s, void *unused)
 		   conf->suspend_stats.deep_ctr,
 		   (u64)(conf->suspend_stats.deep_res*10)/192);
 
-	seq_printf(s, "Suspend(With Shallow-Wakes)\t %10u\t %10llu\n",
-		   conf->suspend_stats.shlw_swake_ctr +
-		   conf->suspend_stats.deep_swake_ctr,
-		   (u64)((conf->suspend_stats.shlw_swake_res +
-		   conf->suspend_stats.deep_swake_res)*10/192));
-
-	seq_printf(s, "S0IX+Suspend Total\t\t %10u\t %10llu\n", s0ix_total_ctr,
+	seq_printf(s, "TOTAL S0IX\t\t\t %10u\t %10llu\n", s0ix_total_ctr,
 				(u64)(s0ix_total_res*10/192));
 	seq_puts(s, "\n-------------------------------------------------\n");
 	seq_puts(s, "\t\tDEVICE STATES\n");
@@ -920,23 +906,15 @@ static int pm_suspend_exit_cb(void)
 	suspend_shlw_res_exit -= suspend_shlw_res_temp;
 	suspend_deep_res_exit -= suspend_deep_res_temp;
 
-	if (suspend_shlw_ctr_exit == 1) {
+	if (suspend_shlw_ctr_exit != 0) {
 		conf->suspend_stats.shlw_ctr +=
 		suspend_shlw_ctr_exit;
 
 		conf->suspend_stats.shlw_res +=
 		suspend_shlw_res_exit;
 	}
-	/* Shallow Wakes Case */
-	else if (suspend_shlw_ctr_exit > 1) {
-		conf->suspend_stats.shlw_swake_ctr +=
-		suspend_shlw_ctr_exit;
-
-		conf->suspend_stats.shlw_swake_res +=
-		suspend_shlw_res_exit;
-	}
 
-	if (suspend_deep_ctr_exit == 1) {
+	if (suspend_deep_ctr_exit != 0) {
 		conf->suspend_stats.deep_ctr +=
 		suspend_deep_ctr_exit;
 
@@ -944,15 +922,6 @@ static int pm_suspend_exit_cb(void)
 		suspend_deep_res_exit;
 	}
 
-	/* Shallow Wakes Case */
-	else if (suspend_deep_ctr_exit > 1) {
-		conf->suspend_stats.deep_swake_ctr +=
-		suspend_deep_ctr_exit;
-
-		conf->suspend_stats.deep_swake_res +=
-		suspend_deep_res_exit;
-	}
-
 out:
 	suspend_prep_ok = 0;
 	return NOTIFY_OK;
-- 
2.7.4