[PATCH 00/27] tools/power/x86/intel-speed-select: Enhancements

[PATCH 00/27] tools/power/x86/intel-speed-select: Enhancements

[Srinivas Pandruvada]

These are not urgent fixes, but improves usability based on user
feedback. This series addresses:
- Check validaity of user options for a command
- Print helpful errors
- Improves help
- Enhance existing options like --info
- Fixes issues with json keys
- Take care of handling offline CPUs


Srinivas Pandruvada (27):
  tools/power/x86/intel-speed-select: Warn for invalid package id
  tools/power/x86/intel-speed-select: Make target CPU optional for
    core-power info
  tools/power/x86/intel-speed-select: Enhance core-power info command
  tools/power/x86/intel-speed-select: Use more verbiage for clos
    information
  tools/power/x86/intel-speed-select: Special handling for CPU 0
    online/offline
  tools/power/x86/intel-speed-select: Max CPU count calculation when
    CPU0 is offline
  tools/power/x86/intel-speed-select: Store topology information
  tools/power/x86/intel-speed-select: Helpful warning for missing kernel
    interface
  tools/power/x86/intel-speed-select: Enhance help
  tools/power/x86/intel-speed-select: Enhance --info option
  tools/power/x86/intel-speed-select: Add an API for error/information
    print
  tools/power/x86/intel-speed-select: Improve error display for
    perf-profile feature
  tools/power/x86/intel-speed-select: Check feature status first
  tools/power/x86/intel-speed-select: Display error for invalid priority
    type
  tools/power/x86/intel-speed-select: Enhance help for core-power assoc
  tools/power/x86/intel-speed-select: Improve output of perf-profile
    commands
  tools/power/x86/intel-speed-select: Improve error display for
    base-freq feature
  tools/power/x86/intel-speed-select: Improve error display for
    turbo-freq feature
  tools/power/x86/intel-speed-select: Kernel interface error handling
  tools/power/x86/intel-speed-select: Improve core-power result and
    error display
  tools/power/x86/intel-speed-select: Show error for invalid CPUs in the
    options
  tools/power/x86/intel-speed-select: Improve CLX commands
  tools/power/x86/intel-speed-select: Fix avx options for turbo-freq
    feature
  tools/power/x86/intel-speed-select: Print friendly warning for bad
    command line
  tools/power/x86/intel-speed-select: Add display for enabled cpus count
  tools/power/x86/intel-speed-select: Avoid duplicate Package strings
    for json
  tools/power/x86/intel-speed-select: Update version

 .../x86/intel-speed-select/isst-config.c      | 574 ++++++++++++++----
 .../power/x86/intel-speed-select/isst-core.c  | 117 +++-
 .../x86/intel-speed-select/isst-display.c     | 274 ++++++---
 tools/power/x86/intel-speed-select/isst.h     |  12 +-
 4 files changed, 752 insertions(+), 225 deletions(-)

-- 
2.24.1

[PATCH 01/27] tools/power/x86/intel-speed-select: Warn for invalid package id

[Srinivas Pandruvada]

When CPU is offline, we can't get package id. So print error for this
and don't use output.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 tools/power/x86/intel-speed-select/isst-config.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/power/x86/intel-speed-select/isst-config.c b/tools/power/x86/intel-speed-select/isst-config.c
index c922cfd7ba50..3f019f4f90df 100644
--- a/tools/power/x86/intel-speed-select/isst-config.c
+++ b/tools/power/x86/intel-speed-select/isst-config.c
@@ -261,6 +261,10 @@ static void for_each_online_package_in_set(void (*callback)(int, void *, void *,
 		if (die_id < 0)
 			die_id = 0;
 		pkg_id = get_physical_package_id(i);
+		if (pkg_id < 0) {
+			fprintf(stderr, "Failed to get package id, CPU %d may be offline\n", i);
+			continue;
+		}
 		/* Create an unique id for package, die combination to store */
 		pkg_id = (MAX_PACKAGE_COUNT * pkg_id + die_id);
 
@@ -362,6 +366,10 @@ static void set_cpu_present_cpu_mask(void)
 				die_id = 0;
 
 			pkg_id = get_physical_package_id(i);
+			if (pkg_id < 0) {
+				fprintf(stderr, "Failed to get package id, CPU %d may be offline\n", i);
+				continue;
+			}
 			if (pkg_id < MAX_PACKAGE_COUNT &&
 			    die_id < MAX_DIE_PER_PACKAGE) {
 				int core_id = get_physical_core_id(i);
-- 
2.24.1

[PATCH 02/27] tools/power/x86/intel-speed-select: Make target CPU optional for core-power info

[Srinivas Pandruvada]

Currently "-c" is a mandatory option for "core-power info" command. Make
this optional as this is a per package/die property. When not specified,
it will print info for every package/die.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 tools/power/x86/intel-speed-select/isst-config.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/tools/power/x86/intel-speed-select/isst-config.c b/tools/power/x86/intel-speed-select/isst-config.c
index 3f019f4f90df..c0de18e10cda 100644
--- a/tools/power/x86/intel-speed-select/isst-config.c
+++ b/tools/power/x86/intel-speed-select/isst-config.c
@@ -1763,19 +1763,17 @@ static void dump_clos_info(int arg)
 	if (cmd_help) {
 		fprintf(stderr,
 			"Print Intel Speed Select Technology core power information\n");
-		fprintf(stderr, "\tSpecify targeted cpu id with [--cpu|-c]\n");
-		exit(0);
-	}
-
-	if (!max_target_cpus) {
-		fprintf(stderr,
-			"Invalid target cpu. Specify with [-c|--cpu]\n");
+		fprintf(stderr, "\t Optionally specify targeted cpu id with [--cpu|-c]\n");
 		exit(0);
 	}
 
 	isst_ctdp_display_information_start(outf);
-	for_each_online_target_cpu_in_set(get_clos_info_for_cpu, NULL,
-					  NULL, NULL, NULL);
+	if (max_target_cpus)
+		for_each_online_target_cpu_in_set(get_clos_info_for_cpu, NULL,
+						  NULL, NULL, NULL);
+	else
+		for_each_online_package_in_set(get_clos_info_for_cpu, NULL,
+					       NULL, NULL, NULL);
 	isst_ctdp_display_information_end(outf);
 
 }
-- 
2.24.1

[PATCH 03/27] tools/power/x86/intel-speed-select: Enhance core-power info command

[Srinivas Pandruvada]

In addition to CLOS enable status, also show the core-power feature
status. This will help why clos enable status didn't give desired
results as the core-power feature may be disabled or unsupported.

The new display looks as follows:

$intel-speed-select core-power info
Intel(R) Speed Select Technology
..
 package-0
  die-0
    cpu-0
      core-power
        support-status:supported
        enable-status:enabled
        clos-enable-status:1
        priority-type:0

In the above display "support-status" and "enable-status", shows the
status of the core-power feature and "clos-enable-status", shows the
status of the clos.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 .../power/x86/intel-speed-select/isst-config.c  |  9 +++++++--
 .../power/x86/intel-speed-select/isst-display.c | 17 ++++++++++++++++-
 tools/power/x86/intel-speed-select/isst.h       |  4 +++-
 3 files changed, 26 insertions(+), 4 deletions(-)

diff --git a/tools/power/x86/intel-speed-select/isst-config.c b/tools/power/x86/intel-speed-select/isst-config.c
index c0de18e10cda..405b03cba248 100644
--- a/tools/power/x86/intel-speed-select/isst-config.c
+++ b/tools/power/x86/intel-speed-select/isst-config.c
@@ -1754,8 +1754,13 @@ static void get_clos_info_for_cpu(int cpu, void *arg1, void *arg2, void *arg3,
 	ret = isst_clos_get_clos_information(cpu, &enable, &prio_type);
 	if (ret)
 		perror("isst_clos_get_info");
-	else
-		isst_clos_display_clos_information(cpu, outf, enable, prio_type);
+	else {
+		int cp_state, cp_cap;
+
+		isst_read_pm_config(cpu, &cp_state, &cp_cap);
+		isst_clos_display_clos_information(cpu, outf, enable, prio_type,
+						   cp_state, cp_cap);
+	}
 }
 
 static void dump_clos_info(int arg)
diff --git a/tools/power/x86/intel-speed-select/isst-display.c b/tools/power/x86/intel-speed-select/isst-display.c
index 3d70be2a9f61..30377acb909d 100644
--- a/tools/power/x86/intel-speed-select/isst-display.c
+++ b/tools/power/x86/intel-speed-select/isst-display.c
@@ -589,7 +589,8 @@ void isst_clos_display_information(int cpu, FILE *outf, int clos,
 }
 
 void isst_clos_display_clos_information(int cpu, FILE *outf,
-					int clos_enable, int type)
+					int clos_enable, int type,
+					int state, int cap)
 {
 	char header[256];
 	char value[256];
@@ -605,7 +606,21 @@ void isst_clos_display_clos_information(int cpu, FILE *outf,
 	snprintf(header, sizeof(header), "core-power");
 	format_and_print(outf, 4, header, NULL);
 
+	snprintf(header, sizeof(header), "support-status");
+	if (cap)
+		snprintf(value, sizeof(value), "supported");
+	else
+		snprintf(value, sizeof(value), "unsupported");
+	format_and_print(outf, 5, header, value);
+
 	snprintf(header, sizeof(header), "enable-status");
+	if (state)
+		snprintf(value, sizeof(value), "enabled");
+	else
+		snprintf(value, sizeof(value), "disabled");
+	format_and_print(outf, 5, header, value);
+
+	snprintf(header, sizeof(header), "clos-enable-status");
 	snprintf(value, sizeof(value), "%d", clos_enable);
 	format_and_print(outf, 5, header, value);
 
diff --git a/tools/power/x86/intel-speed-select/isst.h b/tools/power/x86/intel-speed-select/isst.h
index ad5aa6341d0f..53e147a9a295 100644
--- a/tools/power/x86/intel-speed-select/isst.h
+++ b/tools/power/x86/intel-speed-select/isst.h
@@ -245,7 +245,9 @@ extern void isst_display_result(int cpu, FILE *outf, char *feature, char *cmd,
 
 extern int isst_clos_get_clos_information(int cpu, int *enable, int *type);
 extern void isst_clos_display_clos_information(int cpu, FILE *outf,
-					       int clos_enable, int type);
+					       int clos_enable, int type,
+					       int state, int cap);
 extern int is_clx_n_platform(void);
 extern int get_cpufreq_base_freq(int cpu);
+extern int isst_read_pm_config(int cpu, int *cp_state, int *cp_cap);
 #endif
-- 
2.24.1

[PATCH 04/27] tools/power/x86/intel-speed-select: Use more verbiage for clos information

[Srinivas Pandruvada]

Instead of displaying 0 and 1 for enable status, display "disabled"
and "enabled" respectively.

Similarly for priority type, display "ordered or proportional" instead
of 0 and 1.

An example display:

$intel-speed-select -c 1 core-power info
Intel(R) Speed Select Technology
..
 package-0
  die-0
    cpu-1
      core-power
        support-status:supported
        enable-status:enabled
        clos-enable-status:enabled
        priority-type:proportional

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 tools/power/x86/intel-speed-select/isst-display.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/tools/power/x86/intel-speed-select/isst-display.c b/tools/power/x86/intel-speed-select/isst-display.c
index 30377acb909d..ec737e101e52 100644
--- a/tools/power/x86/intel-speed-select/isst-display.c
+++ b/tools/power/x86/intel-speed-select/isst-display.c
@@ -621,11 +621,17 @@ void isst_clos_display_clos_information(int cpu, FILE *outf,
 	format_and_print(outf, 5, header, value);
 
 	snprintf(header, sizeof(header), "clos-enable-status");
-	snprintf(value, sizeof(value), "%d", clos_enable);
+	if (clos_enable)
+		snprintf(value, sizeof(value), "enabled");
+	else
+		snprintf(value, sizeof(value), "disabled");
 	format_and_print(outf, 5, header, value);
 
 	snprintf(header, sizeof(header), "priority-type");
-	snprintf(value, sizeof(value), "%d", type);
+	if (type)
+		snprintf(value, sizeof(value), "ordered");
+	else
+		snprintf(value, sizeof(value), "proportional");
 	format_and_print(outf, 5, header, value);
 
 	format_and_print(outf, 1, NULL, NULL);
-- 
2.24.1

[PATCH 05/27] tools/power/x86/intel-speed-select: Special handling for CPU 0 online/offline

[Srinivas Pandruvada]

When "-o" option for force online/offline is used with command:
perf-profile set-config-level

If the config level calls for CPU 0 online/offline, then call fails
as there is special kernel setup required for CPU 0 online/offline
and the currently not setup for that.

But when call is for online CPU 0, then don't fail. Just warn that
this system is not setup for CPU 0 online/offline.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 tools/power/x86/intel-speed-select/isst-config.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/tools/power/x86/intel-speed-select/isst-config.c b/tools/power/x86/intel-speed-select/isst-config.c
index 405b03cba248..b8f4846d7c78 100644
--- a/tools/power/x86/intel-speed-select/isst-config.c
+++ b/tools/power/x86/intel-speed-select/isst-config.c
@@ -220,8 +220,14 @@ static void set_cpu_online_offline(int cpu, int state)
 		 "/sys/devices/system/cpu/cpu%d/online", cpu);
 
 	fd = open(buffer, O_WRONLY);
-	if (fd < 0)
+	if (fd < 0) {
+		if (!cpu && state) {
+			fprintf(stderr, "This system is not configured for CPU 0 online/offline\n");
+			fprintf(stderr, "Ignoring online request for CPU 0 as this is already online\n");
+			return;
+		}
 		err(-1, "%s open failed", buffer);
+	}
 
 	if (state)
 		ret = write(fd, "1\n", 2);
-- 
2.24.1

[PATCH 06/27] tools/power/x86/intel-speed-select: Max CPU count calculation when CPU0 is offline

[Srinivas Pandruvada]

Currently /sys/devices/system/cpu/cpu0/topology/thread_siblings is used to
get the max CPU count. But when CPU0 is offline, then this file will be
absent.

So add processing so that we can get count from any first CPU in the
system. which is online.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 .../power/x86/intel-speed-select/isst-config.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/tools/power/x86/intel-speed-select/isst-config.c b/tools/power/x86/intel-speed-select/isst-config.c
index b8f4846d7c78..c773c6cc02a5 100644
--- a/tools/power/x86/intel-speed-select/isst-config.c
+++ b/tools/power/x86/intel-speed-select/isst-config.c
@@ -316,10 +316,24 @@ static void set_max_cpu_num(void)
 {
 	FILE *filep;
 	unsigned long dummy;
+	int i;
 
 	topo_max_cpus = 0;
-	filep = fopen_or_exit(
-		"/sys/devices/system/cpu/cpu0/topology/thread_siblings", "r");
+	for (i = 0; i < 256; ++i) {
+		char path[256];
+
+		snprintf(path, sizeof(path),
+			 "/sys/devices/system/cpu/cpu%d/topology/thread_siblings", i);
+		filep = fopen(path, "r");
+		if (filep)
+			break;
+	}
+
+	if (!filep) {
+		fprintf(stderr, "Can't get max cpu number\n");
+		exit(0);
+	}
+
 	while (fscanf(filep, "%lx,", &dummy) == 1)
 		topo_max_cpus += BITMASK_SIZE;
 	fclose(filep);
-- 
2.24.1

[PATCH 07/27] tools/power/x86/intel-speed-select: Store topology information

[Srinivas Pandruvada]

Once the CPU is offline, the topology information (core-id, package-id,
die-id) is not accessible via sysfs. So when user selects a config level
more than base config 0 and offlined CPUs to match the config level,
to return to base config he has to manually online CPUs before. Without
this CPUs information mapping from Punit CPU numbering will lot work
as it needs atlest package id for each CPU.

To avoid this additional steps store the topology information in a file
, which is created on the very first run after boot. Since system boots
in base config and all CPUs are online, we can get information about
every CPU.

Once any of the APIs like get_physical_package_id(),
get_physical_core_id() or get_physical_die_id() fails to read from
sysfs, read from the stored mapping file.

This mapping file is stored in /tmp file system. so on every boot
it is recreated to make sure that any new CPUs are added to the
system before boot are taken into account.

But don't use the stored physical device id  when trying to get
information for CPU to send message in for_each_online_package_in_set().
Here use the real value from syfs and in case fails try the next CPU.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 .../x86/intel-speed-select/isst-config.c      | 143 ++++++++++++++++--
 1 file changed, 132 insertions(+), 11 deletions(-)

diff --git a/tools/power/x86/intel-speed-select/isst-config.c b/tools/power/x86/intel-speed-select/isst-config.c
index c773c6cc02a5..4773970cda9b 100644
--- a/tools/power/x86/intel-speed-select/isst-config.c
+++ b/tools/power/x86/intel-speed-select/isst-config.c
@@ -62,6 +62,13 @@ struct _cpu_map {
 };
 struct _cpu_map *cpu_map;
 
+struct cpu_topology {
+	short cpu;
+	short core_id;
+	short pkg_id;
+	short die_id;
+};
+
 void debug_printf(const char *format, ...)
 {
 	va_list args;
@@ -176,25 +183,137 @@ int out_format_is_json(void)
 	return out_format_json;
 }
 
+static int get_stored_topology_info(int cpu, int *core_id, int *pkg_id, int *die_id)
+{
+	const char *pathname = "/tmp/isst_cpu_topology.dat";
+	struct cpu_topology cpu_top;
+	FILE *fp;
+	int ret;
+
+	fp = fopen(pathname, "rb");
+	if (!fp)
+		return -1;
+
+	ret = fseek(fp, cpu * sizeof(cpu_top), SEEK_SET);
+	if (ret)
+		goto err_ret;
+
+	ret = fread(&cpu_top, sizeof(cpu_top), 1, fp);
+	if (ret != 1) {
+		ret = -1;
+		goto err_ret;
+	}
+
+	*pkg_id = cpu_top.pkg_id;
+	*core_id = cpu_top.core_id;
+	*die_id = cpu_top.die_id;
+	ret = 0;
+
+err_ret:
+	fclose(fp);
+
+	return ret;
+}
+
+static void store_cpu_topology(void)
+{
+	const char *pathname = "/tmp/isst_cpu_topology.dat";
+	FILE *fp;
+	int i;
+
+	fp = fopen(pathname, "rb");
+	if (fp) {
+		/* Mapping already exists */
+		fclose(fp);
+		return;
+	}
+
+	fp = fopen(pathname, "wb");
+	if (!fp) {
+		fprintf(stderr, "Can't create file:%s\n", pathname);
+		return;
+	}
+
+	for (i = 0; i < topo_max_cpus; ++i) {
+		struct cpu_topology cpu_top;
+
+		cpu_top.core_id = parse_int_file(0,
+			"/sys/devices/system/cpu/cpu%d/topology/core_id", i);
+		if (cpu_top.core_id < 0)
+			cpu_top.core_id = -1;
+
+		cpu_top.pkg_id = parse_int_file(0,
+			"/sys/devices/system/cpu/cpu%d/topology/physical_package_id", i);
+		if (cpu_top.pkg_id < 0)
+			cpu_top.pkg_id = -1;
+
+		cpu_top.die_id = parse_int_file(0,
+			"/sys/devices/system/cpu/cpu%d/topology/die_id", i);
+		if (cpu_top.die_id < 0)
+			cpu_top.die_id = -1;
+
+		cpu_top.cpu = i;
+
+		if (fwrite(&cpu_top, sizeof(cpu_top), 1, fp) != 1) {
+			fprintf(stderr, "Can't write to:%s\n", pathname);
+			break;
+		}
+	}
+
+	fclose(fp);
+}
+
 int get_physical_package_id(int cpu)
 {
-	return parse_int_file(
-		0, "/sys/devices/system/cpu/cpu%d/topology/physical_package_id",
-		cpu);
+	int ret;
+
+	ret = parse_int_file(0,
+			"/sys/devices/system/cpu/cpu%d/topology/physical_package_id",
+			cpu);
+	if (ret < 0) {
+		int core_id, pkg_id, die_id;
+
+		ret = get_stored_topology_info(cpu, &core_id, &pkg_id, &die_id);
+		if (!ret)
+			return pkg_id;
+	}
+
+	return ret;
 }
 
 int get_physical_core_id(int cpu)
 {
-	return parse_int_file(
-		0, "/sys/devices/system/cpu/cpu%d/topology/core_id", cpu);
+	int ret;
+
+	ret = parse_int_file(0,
+			"/sys/devices/system/cpu/cpu%d/topology/core_id",
+			cpu);
+	if (ret < 0) {
+		int core_id, pkg_id, die_id;
+
+		ret = get_stored_topology_info(cpu, &core_id, &pkg_id, &die_id);
+		if (!ret)
+			return core_id;
+	}
+
+	return ret;
 }
 
 int get_physical_die_id(int cpu)
 {
 	int ret;
 
-	ret = parse_int_file(0, "/sys/devices/system/cpu/cpu%d/topology/die_id",
-			     cpu);
+	ret = parse_int_file(0,
+			"/sys/devices/system/cpu/cpu%d/topology/die_id",
+			cpu);
+	if (ret < 0) {
+		int core_id, pkg_id, die_id;
+
+		ret = get_stored_topology_info(cpu, &core_id, &pkg_id, &die_id);
+		if (!ret)
+			return die_id;
+	}
+
 	if (ret < 0)
 		ret = 0;
 
@@ -266,11 +385,12 @@ static void for_each_online_package_in_set(void (*callback)(int, void *, void *,
 		die_id = get_physical_die_id(i);
 		if (die_id < 0)
 			die_id = 0;
-		pkg_id = get_physical_package_id(i);
-		if (pkg_id < 0) {
-			fprintf(stderr, "Failed to get package id, CPU %d may be offline\n", i);
+
+		pkg_id = parse_int_file(0,
+			"/sys/devices/system/cpu/cpu%d/topology/physical_package_id", i);
+		if (pkg_id < 0)
 			continue;
-		}
+
 		/* Create an unique id for package, die combination to store */
 		pkg_id = (MAX_PACKAGE_COUNT * pkg_id + die_id);
 
@@ -2352,6 +2472,7 @@ static void cmdline(int argc, char **argv)
 	printf("Intel(R) Speed Select Technology\n");
 	printf("Executing on CPU model:%d[0x%x]\n", cpu_model, cpu_model);
 	set_max_cpu_num();
+	store_cpu_topology();
 	set_cpu_present_cpu_mask();
 	set_cpu_target_cpu_mask();
 
-- 
2.24.1

[PATCH 08/27] tools/power/x86/intel-speed-select: Helpful warning for missing kernel interface

[Srinivas Pandruvada]

When the device file "/dev/isst_interface" is not present, instead of
failing on access, check at the start and print a helpful warning.

Here CLX platform is an exception, which doesn't depend on the device
file. So continue for CLX platform.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 .../x86/intel-speed-select/isst-config.c      | 34 +++++++++++++------
 1 file changed, 24 insertions(+), 10 deletions(-)

diff --git a/tools/power/x86/intel-speed-select/isst-config.c b/tools/power/x86/intel-speed-select/isst-config.c
index 4773970cda9b..9bf461e57a34 100644
--- a/tools/power/x86/intel-speed-select/isst-config.c
+++ b/tools/power/x86/intel-speed-select/isst-config.c
@@ -2408,6 +2408,8 @@ static void print_version(void)
 
 static void cmdline(int argc, char **argv)
 {
+	const char *pathname = "/dev/isst_interface";
+	FILE *fp;
 	int opt;
 	int option_index = 0;
 	int ret;
@@ -2423,6 +2425,28 @@ static void cmdline(int argc, char **argv)
 		{ 0, 0, 0, 0 }
 	};
 
+	if (geteuid() != 0) {
+		fprintf(stderr, "Must run as root\n");
+		exit(0);
+	}
+
+	ret = update_cpu_model();
+	if (ret)
+		err(-1, "Invalid CPU model (%d)\n", cpu_model);
+	printf("Intel(R) Speed Select Technology\n");
+	printf("Executing on CPU model:%d[0x%x]\n", cpu_model, cpu_model);
+
+	if (!is_clx_n_platform()) {
+		fp = fopen(pathname, "rb");
+		if (!fp) {
+			fprintf(stderr, "Intel speed select drivers are not loaded on this system.\n");
+			fprintf(stderr, "Verify that kernel config includes CONFIG_INTEL_SPEED_SELECT_INTERFACE.\n");
+			fprintf(stderr, "If the config is included then this is not a supported platform.\n");
+			exit(0);
+		}
+		fclose(fp);
+	}
+
 	progname = argv[0];
 	while ((opt = getopt_long_only(argc, argv, "+c:df:hio:v", long_options,
 				       &option_index)) != -1) {
@@ -2457,20 +2481,10 @@ static void cmdline(int argc, char **argv)
 		}
 	}
 
-	if (geteuid() != 0) {
-		fprintf(stderr, "Must run as root\n");
-		exit(0);
-	}
-
 	if (optind > (argc - 2)) {
 		fprintf(stderr, "Feature name and|or command not specified\n");
 		exit(0);
 	}
-	ret = update_cpu_model();
-	if (ret)
-		err(-1, "Invalid CPU model (%d)\n", cpu_model);
-	printf("Intel(R) Speed Select Technology\n");
-	printf("Executing on CPU model:%d[0x%x]\n", cpu_model, cpu_model);
 	set_max_cpu_num();
 	store_cpu_topology();
 	set_cpu_present_cpu_mask();
-- 
2.24.1

[PATCH 09/27] tools/power/x86/intel-speed-select: Enhance help

[Srinivas Pandruvada]

Enhance help message which adds some example. The changes include:
- Print help when options are not recognized.
- For CLX, display only options which are applicable.
- Sort options in alphatical order.
- Disply help() instead of error:
"Feature name and|or command not specified"
- Remove duplicate display of
Intel(R) Speed Select Technology

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 .../x86/intel-speed-select/isst-config.c      | 29 +++++++++++++++----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/tools/power/x86/intel-speed-select/isst-config.c b/tools/power/x86/intel-speed-select/isst-config.c
index 9bf461e57a34..5302a552f87f 100644
--- a/tools/power/x86/intel-speed-select/isst-config.c
+++ b/tools/power/x86/intel-speed-select/isst-config.c
@@ -2368,11 +2368,18 @@ void process_command(int argc, char **argv,
 
 static void usage(void)
 {
-	printf("Intel(R) Speed Select Technology\n");
+	if (is_clx_n_platform()) {
+		fprintf(stderr, "\nThere is limited support of Intel Speed Select features on this platform.\n");
+		fprintf(stderr, "Everything is pre-configured using BIOS options, this tool can't enable any feature in the hardware.\n\n");
+	}
+
 	printf("\nUsage:\n");
 	printf("intel-speed-select [OPTIONS] FEATURE COMMAND COMMAND_ARGUMENTS\n");
-	printf("\nUse this tool to enumerate and control the Intel Speed Select Technology features,\n");
-	printf("\nFEATURE : [perf-profile|base-freq|turbo-freq|core-power]\n");
+	printf("\nUse this tool to enumerate and control the Intel Speed Select Technology features:\n");
+	if (is_clx_n_platform())
+		printf("\nFEATURE : [perf-profile|base-freq]\n");
+	else
+		printf("\nFEATURE : [perf-profile|base-freq|turbo-freq|core-power]\n");
 	printf("\nFor help on each feature, use -h|--help\n");
 	printf("\tFor example:  intel-speed-select perf-profile -h\n");
 
@@ -2385,17 +2392,29 @@ static void usage(void)
 	printf("\t\tDefault: Die scoped for all dies in the system with multiple dies/package\n");
 	printf("\t\t\t Or Package scoped for all Packages when each package contains one die\n");
 	printf("\t[-d|--debug] : Debug mode\n");
+	printf("\t[-f|--format] : output format [json|text]. Default: text\n");
 	printf("\t[-h|--help] : Print help\n");
 	printf("\t[-i|--info] : Print platform information\n");
 	printf("\t[-o|--out] : Output file\n");
 	printf("\t\t\tDefault : stderr\n");
-	printf("\t[-f|--format] : output format [json|text]. Default: text\n");
 	printf("\t[-v|--version] : Print version\n");
 
 	printf("\nResult format\n");
 	printf("\tResult display uses a common format for each command:\n");
 	printf("\tResults are formatted in text/JSON with\n");
 	printf("\t\tPackage, Die, CPU, and command specific results.\n");
+
+	printf("\nExamples\n");
+	printf("\tTo get platform information:\n");
+	printf("\t\tintel-speed-select --info\n");
+	printf("\tTo get full perf-profile information dump:\n");
+	printf("\t\tintel-speed-select perf-profile info\n");
+	printf("\tTo get full base-freq information dump:\n");
+	printf("\t\tintel-speed-select base-freq info -l 0\n");
+	if (!is_clx_n_platform()) {
+		printf("\tTo get full turbo-freq information dump:\n");
+		printf("\t\tintel-speed-select turbo-freq info -l 0\n");
+	}
 	exit(1);
 }
 
@@ -2482,7 +2501,7 @@ static void cmdline(int argc, char **argv)
 	}
 
 	if (optind > (argc - 2)) {
-		fprintf(stderr, "Feature name and|or command not specified\n");
+		usage();
 		exit(0);
 	}
 	set_max_cpu_num();
-- 
2.24.1

[PATCH 10/27] tools/power/x86/intel-speed-select: Enhance --info option

[Srinivas Pandruvada]

Add additional information, which will allow user to detect available
features. This will allow users to check presence of features before
continue to test.
A sample output:

$sudo ./intel-speed-select --info

Intel(R) Speed Select Technology
Executing on CPU model:85[0x55]
Platform: API version : 1
Platform: Driver version : 1
Platform: mbox supported : 1
Platform: mmio supported : 0
Intel(R) SST-PP (feature perf-profile) is not supported
Only performance level 0 (base level) is present
TDP level change control is locked
Intel(R) SST-TF (feature turbo-freq) is supported
Intel(R) SST-BF (feature base-freq) is supported
Intel(R) SST-CP (feature core-power) is supported

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 .../x86/intel-speed-select/isst-config.c      | 74 +++++++++++++++++++
 tools/power/x86/intel-speed-select/isst.h     |  2 +
 2 files changed, 76 insertions(+)

diff --git a/tools/power/x86/intel-speed-select/isst-config.c b/tools/power/x86/intel-speed-select/isst-config.c
index 5302a552f87f..65110d06394f 100644
--- a/tools/power/x86/intel-speed-select/isst-config.c
+++ b/tools/power/x86/intel-speed-select/isst-config.c
@@ -846,12 +846,85 @@ static int isst_fill_platform_info(void)
 	return 0;
 }
 
+static void isst_print_extended_platform_info(void)
+{
+	int cp_state, cp_cap, fact_support = 0, pbf_support = 0;
+	struct isst_pkg_ctdp_level_info ctdp_level;
+	struct isst_pkg_ctdp pkg_dev;
+	int ret, i, j;
+	FILE *filep;
+
+	for (i = 0; i < 256; ++i) {
+		char path[256];
+
+		snprintf(path, sizeof(path),
+			 "/sys/devices/system/cpu/cpu%d/topology/thread_siblings", i);
+		filep = fopen(path, "r");
+		if (filep)
+			break;
+	}
+
+	if (!filep)
+		return;
+
+	fclose(filep);
+
+	ret = isst_get_ctdp_levels(i, &pkg_dev);
+	if (ret)
+		return;
+
+	if (pkg_dev.enabled) {
+		fprintf(outf, "Intel(R) SST-PP (feature perf-profile) is supported\n");
+	} else {
+		fprintf(outf, "Intel(R) SST-PP (feature perf-profile) is not supported\n");
+		fprintf(outf, "Only performance level 0 (base level) is present\n");
+	}
+
+	if (pkg_dev.locked)
+		fprintf(outf, "TDP level change control is locked\n");
+	else
+		fprintf(outf, "TDP level change control is unlocked, max level: %d \n", pkg_dev.levels);
+
+	for (j = 0; j <= pkg_dev.levels; ++j) {
+		ret = isst_get_ctdp_control(i, j, &ctdp_level);
+		if (ret)
+			continue;
+
+		if (!fact_support && ctdp_level.fact_support)
+			fact_support = 1;
+
+		if (!pbf_support && ctdp_level.pbf_support)
+			pbf_support = 1;
+	}
+
+	if (fact_support)
+		fprintf(outf, "Intel(R) SST-TF (feature turbo-freq) is supported\n");
+	else
+		fprintf(outf, "Intel(R) SST-TF (feature turbo-freq) is not supported\n");
+
+	if (pbf_support)
+		fprintf(outf, "Intel(R) SST-BF (feature base-freq) is supported\n");
+	else
+		fprintf(outf, "Intel(R) SST-BF (feature base-freq) is not supported\n");
+
+	ret = isst_read_pm_config(i, &cp_state, &cp_cap);
+	if (cp_cap)
+		fprintf(outf, "Intel(R) SST-CP (feature core-power) is supported\n");
+	else
+		fprintf(outf, "Intel(R) SST-CP (feature core-power) is not supported\n");
+}
+
 static void isst_print_platform_information(void)
 {
 	struct isst_if_platform_info platform_info;
 	const char *pathname = "/dev/isst_interface";
 	int fd;
 
+	if (is_clx_n_platform()) {
+		fprintf(stderr, "\nThis option in not supported on this platform\n");
+		exit(0);
+	}
+
 	fd = open(pathname, O_RDWR);
 	if (fd < 0)
 		err(-1, "%s open failed", pathname);
@@ -867,6 +940,7 @@ static void isst_print_platform_information(void)
 			platform_info.mbox_supported);
 		fprintf(outf, "Platform: mmio supported : %d\n",
 			platform_info.mmio_supported);
+		isst_print_extended_platform_info();
 	}
 
 	close(fd);
diff --git a/tools/power/x86/intel-speed-select/isst.h b/tools/power/x86/intel-speed-select/isst.h
index 53e147a9a295..639d3d649480 100644
--- a/tools/power/x86/intel-speed-select/isst.h
+++ b/tools/power/x86/intel-speed-select/isst.h
@@ -196,6 +196,8 @@ extern int isst_send_msr_command(unsigned int cpu, unsigned int command,
 				 int write, unsigned long long *req_resp);
 
 extern int isst_get_ctdp_levels(int cpu, struct isst_pkg_ctdp *pkg_dev);
+extern int isst_get_ctdp_control(int cpu, int config_index,
+				 struct isst_pkg_ctdp_level_info *ctdp_level);
 extern int isst_get_coremask_info(int cpu, int config_index,
 			   struct isst_pkg_ctdp_level_info *ctdp_level);
 extern int isst_get_process_ctdp(int cpu, int tdp_level,
-- 
2.24.1

[PATCH 11/27] tools/power/x86/intel-speed-select: Add an API for error/information print

[Srinivas Pandruvada]

Add a common API which can be used to print all error and information
messages. In this way a common format can be used.

For json output an error index in suffixed to make unique error key.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 .../x86/intel-speed-select/isst-config.c      |  5 +++
 .../x86/intel-speed-select/isst-display.c     | 44 +++++++++++++++++++
 tools/power/x86/intel-speed-select/isst.h     |  2 +
 3 files changed, 51 insertions(+)

diff --git a/tools/power/x86/intel-speed-select/isst-config.c b/tools/power/x86/intel-speed-select/isst-config.c
index 65110d06394f..360fa00f9c7a 100644
--- a/tools/power/x86/intel-speed-select/isst-config.c
+++ b/tools/power/x86/intel-speed-select/isst-config.c
@@ -69,6 +69,11 @@ struct cpu_topology {
 	short die_id;
 };
 
+FILE *get_output_file(void)
+{
+	return outf;
+}
+
 void debug_printf(const char *format, ...)
 {
 	va_list args;
diff --git a/tools/power/x86/intel-speed-select/isst-display.c b/tools/power/x86/intel-speed-select/isst-display.c
index ec737e101e52..943226d1dd4e 100644
--- a/tools/power/x86/intel-speed-select/isst-display.c
+++ b/tools/power/x86/intel-speed-select/isst-display.c
@@ -515,15 +515,18 @@ void isst_ctdp_display_information(int cpu, FILE *outf, int tdp_level,
 	format_and_print(outf, 1, NULL, NULL);
 }
 
+static int start;
 void isst_ctdp_display_information_start(FILE *outf)
 {
 	last_level = 0;
 	format_and_print(outf, 0, "start", NULL);
+	start = 1;
 }
 
 void isst_ctdp_display_information_end(FILE *outf)
 {
 	format_and_print(outf, 0, NULL, NULL);
+	start = 0;
 }
 
 void isst_pbf_display_information(int cpu, FILE *outf, int level,
@@ -686,3 +689,44 @@ void isst_display_result(int cpu, FILE *outf, char *feature, char *cmd,
 
 	format_and_print(outf, 1, NULL, NULL);
 }
+
+void isst_display_error_info_message(int error, char *msg, int arg_valid, int arg)
+{
+	FILE *outf = get_output_file();
+	static int error_index;
+	char header[256];
+	char value[256];
+
+	if (!out_format_is_json()) {
+		if (arg_valid)
+			snprintf(value, sizeof(value), "%s %d", msg, arg);
+		else
+			snprintf(value, sizeof(value), "%s", msg);
+
+		if (error)
+			fprintf(outf, "Error: %s\n", value);
+		else
+			fprintf(outf, "Information: %s\n", value);
+		return;
+	}
+
+	if (!start)
+		format_and_print(outf, 0, "start", NULL);
+
+	if (error)
+		snprintf(header, sizeof(header), "Error%d", error_index++);
+	else
+		snprintf(header, sizeof(header), "Information:%d", error_index++);
+	format_and_print(outf, 1, header, NULL);
+
+	snprintf(header, sizeof(header), "message");
+	if (arg_valid)
+		snprintf(value, sizeof(value), "%s %d", msg, arg);
+	else
+		snprintf(value, sizeof(value), "%s", msg);
+
+	format_and_print(outf, 2, header, value);
+	format_and_print(outf, 1, NULL, NULL);
+	if (!start)
+		format_and_print(outf, 0, NULL, NULL);
+}
diff --git a/tools/power/x86/intel-speed-select/isst.h b/tools/power/x86/intel-speed-select/isst.h
index 639d3d649480..4950d2368ff8 100644
--- a/tools/power/x86/intel-speed-select/isst.h
+++ b/tools/power/x86/intel-speed-select/isst.h
@@ -172,6 +172,7 @@ extern int get_cpu_count(int pkg_id, int die_id);
 extern int get_core_count(int pkg_id, int die_id);
 
 /* Common interfaces */
+FILE *get_output_file(void);
 extern void debug_printf(const char *format, ...);
 extern int out_format_is_json(void);
 extern int get_physical_package_id(int cpu);
@@ -252,4 +253,5 @@ extern void isst_clos_display_clos_information(int cpu, FILE *outf,
 extern int is_clx_n_platform(void);
 extern int get_cpufreq_base_freq(int cpu);
 extern int isst_read_pm_config(int cpu, int *cp_state, int *cp_cap);
+extern void isst_display_error_info_message(int error, char *msg, int arg_valid, int arg);
 #endif
-- 
2.24.1

[PATCH 12/27] tools/power/x86/intel-speed-select: Improve error display for perf-profile feature

[Srinivas Pandruvada]

This change adds improved error display and handling for commands related
to perf-profile feature. The changes include:
- When invalid TDP level is passed. display error and exit
- Replace perror with helpful error message
- Show error when TDP level can't be set
- Print error when information can't be read for a level
- Validate user options for invalid level
- Display error for TDP lock status

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 .../x86/intel-speed-select/isst-config.c      | 21 +++++++++---
 .../power/x86/intel-speed-select/isst-core.c  | 33 ++++++++++++++++---
 2 files changed, 44 insertions(+), 10 deletions(-)

diff --git a/tools/power/x86/intel-speed-select/isst-config.c b/tools/power/x86/intel-speed-select/isst-config.c
index 360fa00f9c7a..4230a19664d3 100644
--- a/tools/power/x86/intel-speed-select/isst-config.c
+++ b/tools/power/x86/intel-speed-select/isst-config.c
@@ -1130,6 +1130,11 @@ static void dump_clx_n_config_for_cpu(int cpu, void *arg1, void *arg2,
 {
 	int ret;
 
+	if (tdp_level != 0xff && tdp_level != 0) {
+		isst_display_error_info_message(1, "Invalid level", 1, tdp_level);
+		exit(0);
+	}
+
 	ret = clx_n_config(cpu);
 	if (ret) {
 		perror("isst_get_process_ctdp");
@@ -1154,7 +1159,9 @@ static void dump_isst_config_for_cpu(int cpu, void *arg1, void *arg2,
 	memset(&pkg_dev, 0, sizeof(pkg_dev));
 	ret = isst_get_process_ctdp(cpu, tdp_level, &pkg_dev);
 	if (ret) {
-		perror("isst_get_process_ctdp");
+		isst_display_error_info_message(1, "Failed to get perf-profile info on cpu", 1, cpu);
+		isst_ctdp_display_information_end(outf);
+		exit(1);
 	} else {
 		isst_ctdp_display_information(cpu, outf, tdp_level, &pkg_dev);
 		isst_get_process_ctdp_complete(cpu, &pkg_dev);
@@ -1197,9 +1204,11 @@ static void set_tdp_level_for_cpu(int cpu, void *arg1, void *arg2, void *arg3,
 	int ret;
 
 	ret = isst_set_tdp_level(cpu, tdp_level);
-	if (ret)
-		perror("set_tdp_level_for_cpu");
-	else {
+	if (ret) {
+		isst_display_error_info_message(1, "Set TDP level failed", 0, 0);
+		isst_ctdp_display_information_end(outf);
+		exit(1);
+	} else {
 		isst_display_result(cpu, outf, "perf-profile", "set_tdp_level",
 				    ret);
 		if (force_online_offline) {
@@ -1237,11 +1246,13 @@ static void set_tdp_level(int arg)
 			"\t Arguments: -l|--level : Specify tdp level\n");
 		fprintf(stderr,
 			"\t Optional Arguments: -o | online : online/offline for the tdp level\n");
+		fprintf(stderr,
+			"\t  online/offline operation has limitations, refer to Linux hotplug documentation\n");
 		exit(0);
 	}
 
 	if (tdp_level == 0xff) {
-		fprintf(outf, "Invalid command: specify tdp_level\n");
+		isst_display_error_info_message(1, "Invalid command: specify tdp_level", 0, 0);
 		exit(1);
 	}
 	isst_ctdp_display_information_start(outf);
diff --git a/tools/power/x86/intel-speed-select/isst-core.c b/tools/power/x86/intel-speed-select/isst-core.c
index 81a119f688a3..2f3921cd87f3 100644
--- a/tools/power/x86/intel-speed-select/isst-core.c
+++ b/tools/power/x86/intel-speed-select/isst-core.c
@@ -114,8 +114,10 @@ int isst_get_tdp_info(int cpu, int config_index,
 
 	ret = isst_send_mbox_command(cpu, CONFIG_TDP, CONFIG_TDP_GET_TDP_INFO,
 				     0, config_index, &resp);
-	if (ret)
+	if (ret) {
+		isst_display_error_info_message(1, "Invalid level, Can't get TDP information at level", 1, config_index);
 		return ret;
+	}
 
 	ctdp_level->pkg_tdp = resp & GENMASK(14, 0);
 	ctdp_level->tdp_ratio = (resp & GENMASK(23, 16)) >> 16;
@@ -352,7 +354,7 @@ int isst_set_tdp_level_msr(int cpu, int tdp_level)
 	debug_printf("cpu: tdp_level via MSR %d\n", cpu, tdp_level);
 
 	if (isst_get_config_tdp_lock_status(cpu)) {
-		debug_printf("cpu: tdp_locked %d\n", cpu);
+		isst_display_error_info_message(1, "tdp_locked", 0, 0);
 		return -1;
 	}
 
@@ -373,10 +375,19 @@ int isst_set_tdp_level(int cpu, int tdp_level)
 	unsigned int resp;
 	int ret;
 
+
+	if (isst_get_config_tdp_lock_status(cpu)) {
+		isst_display_error_info_message(1, "TDP is locked", 0, 0);
+		return -1;
+
+	}
+
 	ret = isst_send_mbox_command(cpu, CONFIG_TDP, CONFIG_TDP_SET_LEVEL, 0,
 				     tdp_level, &resp);
-	if (ret)
-		return isst_set_tdp_level_msr(cpu, tdp_level);
+	if (ret) {
+		isst_display_error_info_message(1, "Set TDP level failed for level", 1, tdp_level);
+		return ret;
+	}
 
 	return 0;
 }
@@ -671,7 +682,7 @@ void isst_get_process_ctdp_complete(int cpu, struct isst_pkg_ctdp *pkg_dev)
 
 int isst_get_process_ctdp(int cpu, int tdp_level, struct isst_pkg_ctdp *pkg_dev)
 {
-	int i, ret;
+	int i, ret, valid = 0;
 
 	if (pkg_dev->processed)
 		return 0;
@@ -684,6 +695,14 @@ int isst_get_process_ctdp(int cpu, int tdp_level, struct isst_pkg_ctdp *pkg_dev)
 		     cpu, pkg_dev->enabled, pkg_dev->current_level,
 		     pkg_dev->levels);
 
+	if (tdp_level != 0xff && tdp_level > pkg_dev->levels) {
+		isst_display_error_info_message(1, "Invalid level", 0, 0);
+		return -1;
+	}
+
+	if (!pkg_dev->enabled)
+		isst_display_error_info_message(0, "perf-profile feature is not supported, just base-config level 0 is valid", 0, 0);
+
 	for (i = 0; i <= pkg_dev->levels; ++i) {
 		struct isst_pkg_ctdp_level_info *ctdp_level;
 
@@ -703,6 +722,7 @@ int isst_get_process_ctdp(int cpu, int tdp_level, struct isst_pkg_ctdp *pkg_dev)
 		if (ret)
 			continue;
 
+		valid = 1;
 		pkg_dev->processed = 1;
 		ctdp_level->processed = 1;
 
@@ -775,6 +795,9 @@ int isst_get_process_ctdp(int cpu, int tdp_level, struct isst_pkg_ctdp *pkg_dev)
 		isst_get_uncore_mem_freq(cpu, i, ctdp_level);
 	}
 
+	if (!valid)
+		isst_display_error_info_message(0, "Invalid level, Can't get TDP control information at specified levels on cpu", 1, cpu);
+
 	return 0;
 }
 
-- 
2.24.1

[PATCH 13/27] tools/power/x86/intel-speed-select: Check feature status first

[Srinivas Pandruvada]

Before looking for information about the base-freq or turbo-freq details,
first check if the feature is supported at that level. If not print error
and return.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 .../power/x86/intel-speed-select/isst-core.c  | 24 +++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/tools/power/x86/intel-speed-select/isst-core.c b/tools/power/x86/intel-speed-select/isst-core.c
index 2f3921cd87f3..732d9a5eacf1 100644
--- a/tools/power/x86/intel-speed-select/isst-core.c
+++ b/tools/power/x86/intel-speed-select/isst-core.c
@@ -394,9 +394,19 @@ int isst_set_tdp_level(int cpu, int tdp_level)
 
 int isst_get_pbf_info(int cpu, int level, struct isst_pbf_info *pbf_info)
 {
+	struct isst_pkg_ctdp_level_info ctdp_level;
 	int i, ret, core_cnt, max;
 	unsigned int req, resp;
 
+	ret = isst_get_ctdp_control(cpu, level, &ctdp_level);
+	if (ret)
+		return ret;
+
+	if (!ctdp_level.pbf_support) {
+		fprintf(stderr, "base-freq feature is not present at this level:%d\n", level);
+		return -1;
+	}
+
 	pbf_info->core_cpumask_size = alloc_cpu_set(&pbf_info->core_cpumask);
 
 	core_cnt = get_core_count(get_physical_package_id(cpu), get_physical_die_id(cpu));
@@ -492,6 +502,10 @@ int isst_set_pbf_fact_status(int cpu, int pbf, int enable)
 		else
 			req &= ~BIT(17);
 	} else {
+
+		if (enable && !ctdp_level.sst_cp_enabled)
+			fprintf(stderr, "Make sure to execute before: core-power enable\n");
+
 		if (ctdp_level.pbf_enabled)
 			req = BIT(17);
 
@@ -579,9 +593,19 @@ int isst_get_fact_bucket_info(int cpu, int level,
 
 int isst_get_fact_info(int cpu, int level, struct isst_fact_info *fact_info)
 {
+	struct isst_pkg_ctdp_level_info ctdp_level;
 	unsigned int resp;
 	int ret;
 
+	ret = isst_get_ctdp_control(cpu, level, &ctdp_level);
+	if (ret)
+		return ret;
+
+	if (!ctdp_level.fact_support) {
+		fprintf(stderr, "turbo-freq feature is not present at this level:%d\n", level);
+		return -1;
+	}
+
 	ret = isst_send_mbox_command(cpu, CONFIG_TDP,
 				     CONFIG_TDP_GET_FACT_LP_CLIPPING_RATIO, 0,
 				     level, &resp);
-- 
2.24.1

[PATCH 14/27] tools/power/x86/intel-speed-select: Display error for invalid priority type

[Srinivas Pandruvada]

When priority type for core-power enable command is anything more than 1
display error before change to 1, which is ordered priority.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 tools/power/x86/intel-speed-select/isst-core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/power/x86/intel-speed-select/isst-core.c b/tools/power/x86/intel-speed-select/isst-core.c
index 732d9a5eacf1..f69c009ef6f6 100644
--- a/tools/power/x86/intel-speed-select/isst-core.c
+++ b/tools/power/x86/intel-speed-select/isst-core.c
@@ -897,6 +897,9 @@ int isst_pm_qos_config(int cpu, int enable_clos, int priority_type)
 	else
 		req = req & ~BIT(1);
 
+	if (priority_type > 1)
+		fprintf(stderr, "Invalid priority type: Changing type to ordered\n");
+
 	if (priority_type)
 		req = req | BIT(2);
 	else
-- 
2.24.1

[PATCH 15/27] tools/power/x86/intel-speed-select: Enhance help for core-power assoc

[Srinivas Pandruvada]

Enhance help to specify CPU and clos by an example.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 tools/power/x86/intel-speed-select/isst-config.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tools/power/x86/intel-speed-select/isst-config.c b/tools/power/x86/intel-speed-select/isst-config.c
index 4230a19664d3..75c8e1a933ef 100644
--- a/tools/power/x86/intel-speed-select/isst-config.c
+++ b/tools/power/x86/intel-speed-select/isst-config.c
@@ -2104,6 +2104,10 @@ static void set_clos_assoc(int arg)
 		fprintf(stderr, "Associate a clos id to a CPU\n");
 		fprintf(stderr,
 			"\tSpecify targeted clos id with [--clos|-c]\n");
+		fprintf(stderr,
+			"\tFor example to associate clos 1 to CPU 0: issue\n");
+		fprintf(stderr,
+			"\tintel-speed-select --cpu 0 core-power assoc --clos 1\n");
 		exit(0);
 	}
 
-- 
2.24.1

[PATCH 16/27] tools/power/x86/intel-speed-select: Improve output of perf-profile commands

[Srinivas Pandruvada]

Improve output of perf-profile commands:
get-config-enabled
get-lock-status

Instead of showing 0/1, show meaningful strings. Also show error when
command is failed.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 .../x86/intel-speed-select/isst-config.c      | 20 +++++++++++--------
 .../x86/intel-speed-select/isst-display.c     | 10 +++++++---
 tools/power/x86/intel-speed-select/isst.h     |  2 +-
 3 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/tools/power/x86/intel-speed-select/isst-config.c b/tools/power/x86/intel-speed-select/isst-config.c
index 75c8e1a933ef..362a352e6266 100644
--- a/tools/power/x86/intel-speed-select/isst-config.c
+++ b/tools/power/x86/intel-speed-select/isst-config.c
@@ -953,6 +953,7 @@ static void isst_print_platform_information(void)
 	exit(0);
 }
 
+static char *local_str0, *local_str1;
 static void exec_on_get_ctdp_cpu(int cpu, void *arg1, void *arg2, void *arg3,
 				 void *arg4)
 {
@@ -962,13 +963,14 @@ static void exec_on_get_ctdp_cpu(int cpu, void *arg1, void *arg2, void *arg3,
 	fn_ptr = arg1;
 	ret = fn_ptr(cpu, arg2);
 	if (ret)
-		perror("get_tdp_*");
+		isst_display_error_info_message(1, "get_tdp_* failed", 0, 0);
 	else
 		isst_ctdp_display_core_info(cpu, outf, arg3,
-					    *(unsigned int *)arg4);
+					    *(unsigned int *)arg4,
+					    local_str0, local_str1);
 }
 
-#define _get_tdp_level(desc, suffix, object, help)                                \
+#define _get_tdp_level(desc, suffix, object, help, str0, str1)			\
 	static void get_tdp_##object(int arg)                                    \
 	{                                                                         \
 		struct isst_pkg_ctdp ctdp;                                        \
@@ -979,6 +981,8 @@ static void exec_on_get_ctdp_cpu(int cpu, void *arg1, void *arg2, void *arg3,
 				help);                                            \
 			exit(0);                                                  \
 		}                                                                 \
+		local_str0 = str0;						  \
+		local_str1 = str1;						  \
 		isst_ctdp_display_information_start(outf);                        \
 		if (max_target_cpus)                                              \
 			for_each_online_target_cpu_in_set(                        \
@@ -992,12 +996,12 @@ static void exec_on_get_ctdp_cpu(int cpu, void *arg1, void *arg2, void *arg3,
 		isst_ctdp_display_information_end(outf);                          \
 	}
 
-_get_tdp_level("get-config-levels", levels, levels, "TDP levels");
-_get_tdp_level("get-config-version", levels, version, "TDP version");
-_get_tdp_level("get-config-enabled", levels, enabled, "TDP enable status");
+_get_tdp_level("get-config-levels", levels, levels, "Max TDP level", NULL, NULL);
+_get_tdp_level("get-config-version", levels, version, "TDP version", NULL, NULL);
+_get_tdp_level("get-config-enabled", levels, enabled, "perf-profile enable status", "disabled", "enabled");
 _get_tdp_level("get-config-current_level", levels, current_level,
-	       "Current TDP Level");
-_get_tdp_level("get-lock-status", levels, locked, "TDP lock status");
+	       "Current TDP Level", NULL, NULL);
+_get_tdp_level("get-lock-status", levels, locked, "TDP lock status", "unlocked", "locked");
 
 struct isst_pkg_ctdp clx_n_pkg_dev;
 
diff --git a/tools/power/x86/intel-speed-select/isst-display.c b/tools/power/x86/intel-speed-select/isst-display.c
index 943226d1dd4e..0667b405f17f 100644
--- a/tools/power/x86/intel-speed-select/isst-display.c
+++ b/tools/power/x86/intel-speed-select/isst-display.c
@@ -289,7 +289,7 @@ static void _isst_fact_display_information(int cpu, FILE *outf, int level,
 }
 
 void isst_ctdp_display_core_info(int cpu, FILE *outf, char *prefix,
-				 unsigned int val)
+				 unsigned int val, char *str0, char *str1)
 {
 	char header[256];
 	char value[256];
@@ -301,8 +301,12 @@ void isst_ctdp_display_core_info(int cpu, FILE *outf, char *prefix,
 	format_and_print(outf, 2, header, NULL);
 	snprintf(header, sizeof(header), "cpu-%d", cpu);
 	format_and_print(outf, 3, header, NULL);
-
-	snprintf(value, sizeof(value), "%u", val);
+	if (str0 && !val)
+		snprintf(value, sizeof(value), "%s", str0);
+	else if (str1 && val)
+		snprintf(value, sizeof(value), "%s", str1);
+	else
+		snprintf(value, sizeof(value), "%u", val);
 	format_and_print(outf, 4, prefix, value);
 
 	format_and_print(outf, 1, NULL, NULL);
diff --git a/tools/power/x86/intel-speed-select/isst.h b/tools/power/x86/intel-speed-select/isst.h
index 4950d2368ff8..69fa2c3c70c3 100644
--- a/tools/power/x86/intel-speed-select/isst.h
+++ b/tools/power/x86/intel-speed-select/isst.h
@@ -208,7 +208,7 @@ extern void isst_get_process_ctdp_complete(int cpu,
 extern void isst_ctdp_display_information(int cpu, FILE *outf, int tdp_level,
 					  struct isst_pkg_ctdp *pkg_dev);
 extern void isst_ctdp_display_core_info(int cpu, FILE *outf, char *prefix,
-					unsigned int val);
+					unsigned int val, char *str0, char *str1);
 extern void isst_ctdp_display_information_start(FILE *outf);
 extern void isst_ctdp_display_information_end(FILE *outf);
 extern void isst_pbf_display_information(int cpu, FILE *outf, int level,
-- 
2.24.1

[PATCH 17/27] tools/power/x86/intel-speed-select: Improve error display for base-freq feature

[Srinivas Pandruvada]

This change adds improved error display and handling for commands related
to base-freq feature. The changes include:
- Replace perror/fprintf with helpful error message
- Error for not specifying TDP level when required
- For CLX show help which shows limitation

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 .../x86/intel-speed-select/isst-config.c      | 25 ++++++++++++++++---
 .../power/x86/intel-speed-select/isst-core.c  | 14 ++++++++++-
 2 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/tools/power/x86/intel-speed-select/isst-config.c b/tools/power/x86/intel-speed-select/isst-config.c
index 362a352e6266..95602f98a138 100644
--- a/tools/power/x86/intel-speed-select/isst-config.c
+++ b/tools/power/x86/intel-speed-select/isst-config.c
@@ -1276,7 +1276,7 @@ static void clx_n_dump_pbf_config_for_cpu(int cpu, void *arg1, void *arg2,
 
 	ret = clx_n_config(cpu);
 	if (ret) {
-		perror("isst_get_process_ctdp");
+		isst_display_error_info_message(1, "clx_n_config failed", 0, 0);
 	} else {
 		struct isst_pkg_ctdp_level_info *ctdp_level;
 		struct isst_pbf_info *pbf_info;
@@ -1297,7 +1297,9 @@ static void dump_pbf_config_for_cpu(int cpu, void *arg1, void *arg2, void *arg3,
 
 	ret = isst_get_pbf_info(cpu, tdp_level, &pbf_info);
 	if (ret) {
-		perror("isst_get_pbf_info");
+		isst_display_error_info_message(1, "Failed to get base-freq info at this level", 1, tdp_level);
+		isst_ctdp_display_information_end(outf);
+		exit(1);
 	} else {
 		isst_pbf_display_information(cpu, outf, tdp_level, &pbf_info);
 		isst_get_pbf_info_complete(&pbf_info);
@@ -1317,7 +1319,7 @@ static void dump_pbf_config(int arg)
 	}
 
 	if (tdp_level == 0xff) {
-		fprintf(outf, "Invalid command: specify tdp_level\n");
+		isst_display_error_info_message(1, "Invalid command: specify tdp_level", 0, 0);
 		exit(1);
 	}
 
@@ -1635,7 +1637,7 @@ static void set_pbf_for_cpu(int cpu, void *arg1, void *arg2, void *arg3,
 
 	ret = isst_set_pbf_fact_status(cpu, 1, status);
 	if (ret) {
-		perror("isst_set_pbf");
+		debug_printf("isst_set_pbf_fact_status failed");
 		if (auto_mode)
 			isst_pm_qos_config(cpu, 0, 0);
 	} else {
@@ -1667,10 +1669,25 @@ static void set_pbf_enable(int arg)
 		if (enable) {
 			fprintf(stderr,
 				"Enable Intel Speed Select Technology base frequency feature\n");
+			if (is_clx_n_platform()) {
+				fprintf(stderr,
+					"\tOn this platform this command doesn't enable feature in the hardware.\n");
+				fprintf(stderr,
+					"\tIt updates the cpufreq scaling_min_freq to match cpufreq base_frequency.\n");
+				exit(0);
+
+			}
 			fprintf(stderr,
 				"\tOptional Arguments: -a|--auto : Use priority of cores to set core-power associations\n");
 		} else {
 
+			if (is_clx_n_platform()) {
+				fprintf(stderr,
+					"\tOn this platform this command doesn't disable feature in the hardware.\n");
+				fprintf(stderr,
+					"\tIt updates the cpufreq scaling_min_freq to match cpuinfo_min_freq\n");
+				exit(0);
+			}
 			fprintf(stderr,
 				"Disable Intel Speed Select Technology base frequency feature\n");
 			fprintf(stderr,
diff --git a/tools/power/x86/intel-speed-select/isst-core.c b/tools/power/x86/intel-speed-select/isst-core.c
index f69c009ef6f6..7836f9f08af1 100644
--- a/tools/power/x86/intel-speed-select/isst-core.c
+++ b/tools/power/x86/intel-speed-select/isst-core.c
@@ -395,15 +395,27 @@ int isst_set_tdp_level(int cpu, int tdp_level)
 int isst_get_pbf_info(int cpu, int level, struct isst_pbf_info *pbf_info)
 {
 	struct isst_pkg_ctdp_level_info ctdp_level;
+	struct isst_pkg_ctdp pkg_dev;
 	int i, ret, core_cnt, max;
 	unsigned int req, resp;
 
+	ret = isst_get_ctdp_levels(cpu, &pkg_dev);
+	if (ret) {
+		isst_display_error_info_message(1, "Failed to get number of levels", 0, 0);
+		return ret;
+	}
+
+	if (level > pkg_dev.levels) {
+		isst_display_error_info_message(1, "Invalid level", 1, level);
+		return -1;
+	}
+
 	ret = isst_get_ctdp_control(cpu, level, &ctdp_level);
 	if (ret)
 		return ret;
 
 	if (!ctdp_level.pbf_support) {
-		fprintf(stderr, "base-freq feature is not present at this level:%d\n", level);
+		isst_display_error_info_message(1, "base-freq feature is not present at this level", 1, level);
 		return -1;
 	}
 
-- 
2.24.1

[PATCH 18/27] tools/power/x86/intel-speed-select: Improve error display for turbo-freq feature

[Srinivas Pandruvada]

This change adds improved error display and handling for commands related
to turbo-freq feature. The changes include:
- Replace perror/fprintf with helpful error message
- Error for not specifying TDP level when required
- Show error for invalid bucket number
- Show message to enable core-power before enabling turbo-freq feature

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 .../x86/intel-speed-select/isst-config.c      | 15 ++++---
 .../power/x86/intel-speed-select/isst-core.c  | 41 ++++++++++++++++---
 .../x86/intel-speed-select/isst-display.c     | 16 +++++++-
 tools/power/x86/intel-speed-select/isst.h     |  2 +-
 4 files changed, 60 insertions(+), 14 deletions(-)

diff --git a/tools/power/x86/intel-speed-select/isst-config.c b/tools/power/x86/intel-speed-select/isst-config.c
index 95602f98a138..48915470c572 100644
--- a/tools/power/x86/intel-speed-select/isst-config.c
+++ b/tools/power/x86/intel-speed-select/isst-config.c
@@ -1712,12 +1712,15 @@ static void dump_fact_config_for_cpu(int cpu, void *arg1, void *arg2,
 	struct isst_fact_info fact_info;
 	int ret;
 
-	ret = isst_get_fact_info(cpu, tdp_level, &fact_info);
-	if (ret)
-		perror("isst_get_fact_bucket_info");
-	else
+	ret = isst_get_fact_info(cpu, tdp_level, fact_bucket, &fact_info);
+	if (ret) {
+		isst_display_error_info_message(1, "Failed to get turbo-freq info at this level", 1, tdp_level);
+		isst_ctdp_display_information_end(outf);
+		exit(1);
+	} else {
 		isst_fact_display_information(cpu, outf, tdp_level, fact_bucket,
 					      fact_avx, &fact_info);
+	}
 }
 
 static void dump_fact_config(int arg)
@@ -1735,7 +1738,7 @@ static void dump_fact_config(int arg)
 	}
 
 	if (tdp_level == 0xff) {
-		fprintf(outf, "Invalid command: specify tdp_level\n");
+		isst_display_error_info_message(1, "Invalid command: specify tdp_level\n", 0, 0);
 		exit(1);
 	}
 
@@ -1763,7 +1766,7 @@ static void set_fact_for_cpu(int cpu, void *arg1, void *arg2, void *arg3,
 
 	ret = isst_set_pbf_fact_status(cpu, 0, status);
 	if (ret) {
-		perror("isst_set_fact");
+		debug_printf("isst_set_pbf_fact_status failed");
 		if (auto_mode)
 			isst_pm_qos_config(cpu, 0, 0);
 
diff --git a/tools/power/x86/intel-speed-select/isst-core.c b/tools/power/x86/intel-speed-select/isst-core.c
index 7836f9f08af1..89b3068a2685 100644
--- a/tools/power/x86/intel-speed-select/isst-core.c
+++ b/tools/power/x86/intel-speed-select/isst-core.c
@@ -516,7 +516,7 @@ int isst_set_pbf_fact_status(int cpu, int pbf, int enable)
 	} else {
 
 		if (enable && !ctdp_level.sst_cp_enabled)
-			fprintf(stderr, "Make sure to execute before: core-power enable\n");
+			isst_display_error_info_message(0, "Make sure to execute before: core-power enable", 0, 0);
 
 		if (ctdp_level.pbf_enabled)
 			req = BIT(17);
@@ -603,18 +603,30 @@ int isst_get_fact_bucket_info(int cpu, int level,
 	return 0;
 }
 
-int isst_get_fact_info(int cpu, int level, struct isst_fact_info *fact_info)
+int isst_get_fact_info(int cpu, int level, int fact_bucket, struct isst_fact_info *fact_info)
 {
 	struct isst_pkg_ctdp_level_info ctdp_level;
+	struct isst_pkg_ctdp pkg_dev;
 	unsigned int resp;
-	int ret;
+	int j, ret, print;
+
+	ret = isst_get_ctdp_levels(cpu, &pkg_dev);
+	if (ret) {
+		isst_display_error_info_message(1, "Failed to get number of levels", 0, 0);
+		return ret;
+	}
+
+	if (level > pkg_dev.levels) {
+		isst_display_error_info_message(1, "Invalid level", 1, level);
+		return -1;
+	}
 
 	ret = isst_get_ctdp_control(cpu, level, &ctdp_level);
 	if (ret)
 		return ret;
 
 	if (!ctdp_level.fact_support) {
-		fprintf(stderr, "turbo-freq feature is not present at this level:%d\n", level);
+		isst_display_error_info_message(1, "turbo-freq feature is not present at this level", 1, level);
 		return -1;
 	}
 
@@ -632,8 +644,25 @@ int isst_get_fact_info(int cpu, int level, struct isst_fact_info *fact_info)
 	fact_info->lp_clipping_ratio_license_avx512 = (resp >> 16) & 0xff;
 
 	ret = isst_get_fact_bucket_info(cpu, level, fact_info->bucket_info);
+	if (ret)
+		return ret;
 
-	return ret;
+	print = 0;
+	for (j = 0; j < ISST_FACT_MAX_BUCKETS; ++j) {
+		if (fact_bucket != 0xff && fact_bucket != j)
+			continue;
+
+		if (!fact_info->bucket_info[j].high_priority_cores_count)
+			break;
+
+		print = 1;
+	}
+	if (!print) {
+		isst_display_error_info_message(1, "Invalid bucket", 0, 0);
+		return -1;
+	}
+
+	return 0;
 }
 
 int isst_set_trl(int cpu, unsigned long long trl)
@@ -769,7 +798,7 @@ int isst_get_process_ctdp(int cpu, int tdp_level, struct isst_pkg_ctdp *pkg_dev)
 		}
 
 		if (ctdp_level->fact_support) {
-			ret = isst_get_fact_info(cpu, i,
+			ret = isst_get_fact_info(cpu, i, 0xff,
 						 &ctdp_level->fact_info);
 			if (ret)
 				return ret;
diff --git a/tools/power/x86/intel-speed-select/isst-display.c b/tools/power/x86/intel-speed-select/isst-display.c
index 0667b405f17f..bf9422336fab 100644
--- a/tools/power/x86/intel-speed-select/isst-display.c
+++ b/tools/power/x86/intel-speed-select/isst-display.c
@@ -222,7 +222,21 @@ static void _isst_fact_display_information(int cpu, FILE *outf, int level,
 	struct isst_fact_bucket_info *bucket_info = fact_info->bucket_info;
 	char header[256];
 	char value[256];
-	int j;
+	int print = 0, j;
+
+	for (j = 0; j < ISST_FACT_MAX_BUCKETS; ++j) {
+		if (fact_bucket != 0xff && fact_bucket != j)
+			continue;
+
+		if (!bucket_info[j].high_priority_cores_count)
+			break;
+
+		print = 1;
+	}
+	if (!print) {
+		fprintf(stderr, "Invalid bucket\n");
+		return;
+	}
 
 	snprintf(header, sizeof(header), "speed-select-turbo-freq-properties");
 	format_and_print(outf, base_level, header, NULL);
diff --git a/tools/power/x86/intel-speed-select/isst.h b/tools/power/x86/intel-speed-select/isst.h
index 69fa2c3c70c3..2e1afd856a78 100644
--- a/tools/power/x86/intel-speed-select/isst.h
+++ b/tools/power/x86/intel-speed-select/isst.h
@@ -219,7 +219,7 @@ extern int isst_set_pbf_fact_status(int cpu, int pbf, int enable);
 extern int isst_get_pbf_info(int cpu, int level,
 			     struct isst_pbf_info *pbf_info);
 extern void isst_get_pbf_info_complete(struct isst_pbf_info *pbf_info);
-extern int isst_get_fact_info(int cpu, int level,
+extern int isst_get_fact_info(int cpu, int level, int fact_bucket,
 			      struct isst_fact_info *fact_info);
 extern int isst_get_fact_bucket_info(int cpu, int level,
 				     struct isst_fact_bucket_info *bucket_info);
-- 
2.24.1

[PATCH 19/27] tools/power/x86/intel-speed-select: Kernel interface error handling

[Srinivas Pandruvada]

Treat a case when mailbox/mmio command can't be handled by the kernel
drivers when the module is removed or send a command which no driver can
handle. In this case ENOTTY result is returned, so print error.

Also when the isst_if_mmio module is removed, we can't send CLOS message
messages via Mailbox on non SKX based platforms. When this module is
removed,  isst_platform_info.mmio_supported is set to 0. So it can't be
used as a condition to send via mailbox. Here replace check for Skylake
based platform to send via mailbox, other platforms can't use mailbox in
lieu of MMIO.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 .../x86/intel-speed-select/isst-config.c      | 28 +++++++++++++++----
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/tools/power/x86/intel-speed-select/isst-config.c b/tools/power/x86/intel-speed-select/isst-config.c
index 48915470c572..2ab902c18bcc 100644
--- a/tools/power/x86/intel-speed-select/isst-config.c
+++ b/tools/power/x86/intel-speed-select/isst-config.c
@@ -95,6 +95,14 @@ int is_clx_n_platform(void)
 	return 0;
 }
 
+int is_skx_based_platform(void)
+{
+	if (cpu_model == 0x55)
+		return 1;
+
+	return 0;
+}
+
 static int update_cpu_model(void)
 {
 	unsigned int ebx, ecx, edx;
@@ -695,7 +703,11 @@ static int isst_send_mmio_command(unsigned int cpu, unsigned int reg, int write,
 	}
 
 	if (ioctl(fd, cmd, &io_regs) == -1) {
-		perror("ISST_IF_IO_CMD");
+		if (errno == ENOTTY) {
+			perror("ISST_IF_IO_COMMAND\n");
+			fprintf(stderr, "Check presence of kernel modules: isst_if_mmio\n");
+			exit(0);
+		}
 		fprintf(outf, "Error: mmio_cmd cpu:%d reg:%x read_write:%x\n",
 			cpu, reg, write);
 	} else {
@@ -724,7 +736,7 @@ int isst_send_mbox_command(unsigned int cpu, unsigned char command,
 		"mbox_send: cpu:%d command:%x sub_command:%x parameter:%x req_data:%x\n",
 		cpu, command, sub_command, parameter, req_data);
 
-	if (isst_platform_info.mmio_supported && command == CONFIG_CLOS &&
+	if (!is_skx_based_platform() && command == CONFIG_CLOS &&
 	    sub_command != CLOS_PM_QOS_CONFIG) {
 		unsigned int value;
 		int write = 0;
@@ -774,10 +786,14 @@ int isst_send_mbox_command(unsigned int cpu, unsigned char command,
 		err(-1, "%s open failed", pathname);
 
 	if (ioctl(fd, ISST_IF_MBOX_COMMAND, &mbox_cmds) == -1) {
-		perror("ISST_IF_MBOX_COMMAND");
-		fprintf(outf,
-			"Error: mbox_cmd cpu:%d command:%x sub_command:%x parameter:%x req_data:%x\n",
-			cpu, command, sub_command, parameter, req_data);
+		if (errno == ENOTTY) {
+			perror("ISST_IF_MBOX_COMMAND\n");
+			fprintf(stderr, "Check presence of kernel modules: isst_if_mbox_pci or isst_if_mbox_msr\n");
+			exit(0);
+		}
+		debug_printf(
+			"Error: mbox_cmd cpu:%d command:%x sub_command:%x parameter:%x req_data:%x errorno:%d\n",
+			cpu, command, sub_command, parameter, req_data, errno);
 		return -1;
 	} else {
 		*resp = mbox_cmds.mbox_cmd[0].resp_data;
-- 
2.24.1

[PATCH 20/27] tools/power/x86/intel-speed-select: Improve core-power result and error display

[Srinivas Pandruvada]

This change adds improved error display and handling for commands related
to core-power feature. The changes include:
- Replace perror with helpful error message
- Use ordered priority for SKX based platform by default as the
proportional priority is not supported
- Don't show weight and epp in help and also give error when user
tries to set them in SKX based platforms
- Range check for epp and weights and display error

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 .../x86/intel-speed-select/isst-config.c      | 87 +++++++++++--------
 .../power/x86/intel-speed-select/isst-core.c  | 10 ++-
 .../x86/intel-speed-select/isst-display.c     |  5 +-
 3 files changed, 62 insertions(+), 40 deletions(-)

diff --git a/tools/power/x86/intel-speed-select/isst-config.c b/tools/power/x86/intel-speed-select/isst-config.c
index 2ab902c18bcc..7a8a315ac8d1 100644
--- a/tools/power/x86/intel-speed-select/isst-config.c
+++ b/tools/power/x86/intel-speed-select/isst-config.c
@@ -1361,7 +1361,7 @@ static int set_clos_param(int cpu, int clos, int epp, int wt, int min, int max)
 
 	ret = isst_pm_get_clos(cpu, clos, &clos_config);
 	if (ret) {
-		perror("isst_pm_get_clos");
+		isst_display_error_info_message(1, "isst_pm_get_clos failed", 0, 0);
 		return ret;
 	}
 	clos_config.clos_min = min;
@@ -1370,7 +1370,7 @@ static int set_clos_param(int cpu, int clos, int epp, int wt, int min, int max)
 	clos_config.clos_prop_prio = wt;
 	ret = isst_set_clos(cpu, clos, &clos_config);
 	if (ret) {
-		perror("isst_pm_set_clos");
+		isst_display_error_info_message(1, "isst_set_clos failed", 0, 0);
 		return ret;
 	}
 
@@ -1577,7 +1577,7 @@ static int set_core_priority_and_min(int cpu, int mask_size,
 		debug_printf("Associate cpu: %d clos: %d\n", i, clos);
 		ret = isst_clos_associate(i, clos);
 		if (ret) {
-			perror("isst_clos_associate");
+			isst_display_error_info_message(1, "isst_clos_associate failed", 0, 0);
 			return ret;
 		}
 	}
@@ -1593,14 +1593,14 @@ static int set_pbf_core_power(int cpu)
 
 	ret = isst_get_ctdp_levels(cpu, &pkg_dev);
 	if (ret) {
-		perror("isst_get_ctdp_levels");
+		debug_printf("isst_get_ctdp_levels failed");
 		return ret;
 	}
 	debug_printf("Current_level: %d\n", pkg_dev.current_level);
 
 	ret = isst_get_pbf_info(cpu, pkg_dev.current_level, &pbf_info);
 	if (ret) {
-		perror("isst_get_pbf_info");
+		debug_printf("isst_get_pbf_info failed");
 		return ret;
 	}
 	debug_printf("p1_high: %d p1_low: %d\n", pbf_info.p1_high,
@@ -1610,13 +1610,13 @@ static int set_pbf_core_power(int cpu)
 					pbf_info.core_cpumask,
 					pbf_info.p1_high, pbf_info.p1_low);
 	if (ret) {
-		perror("set_core_priority_and_min");
+		debug_printf("set_core_priority_and_min failed");
 		return ret;
 	}
 
 	ret = isst_pm_qos_config(cpu, 1, 1);
 	if (ret) {
-		perror("isst_pm_qos_config");
+		debug_printf("isst_pm_qos_config failed");
 		return ret;
 	}
 
@@ -1666,7 +1666,7 @@ static void set_pbf_for_cpu(int cpu, void *arg1, void *arg2, void *arg3,
 	}
 
 	if (auto_mode && !status)
-		isst_pm_qos_config(cpu, 0, 0);
+		isst_pm_qos_config(cpu, 0, 1);
 
 disp_result:
 	if (status)
@@ -1933,9 +1933,12 @@ static void enable_clos_qos_config(int cpu, void *arg1, void *arg2, void *arg3,
 	int ret;
 	int status = *(int *)arg4;
 
+	if (is_skx_based_platform())
+		clos_priority_type = 1;
+
 	ret = isst_pm_qos_config(cpu, status, clos_priority_type);
 	if (ret)
-		perror("isst_pm_qos_config");
+		isst_display_error_info_message(1, "isst_pm_qos_config failed", 0, 0);
 
 	if (status)
 		isst_display_result(cpu, outf, "core-power", "enable",
@@ -1953,9 +1956,11 @@ static void set_clos_enable(int arg)
 		if (enable) {
 			fprintf(stderr,
 				"Enable core-power for a package/die\n");
-			fprintf(stderr,
-				"\tClos Enable: Specify priority type with [--priority|-p]\n");
-			fprintf(stderr, "\t\t 0: Proportional, 1: Ordered\n");
+			if (!is_skx_based_platform()) {
+				fprintf(stderr,
+					"\tClos Enable: Specify priority type with [--priority|-p]\n");
+				fprintf(stderr, "\t\t 0: Proportional, 1: Ordered\n");
+			}
 		} else {
 			fprintf(stderr,
 				"Disable core-power: [No command arguments are required]\n");
@@ -1986,7 +1991,7 @@ static void dump_clos_config_for_cpu(int cpu, void *arg1, void *arg2,
 
 	ret = isst_pm_get_clos(cpu, current_clos, &clos_config);
 	if (ret)
-		perror("isst_pm_get_clos");
+		isst_display_error_info_message(1, "isst_pm_get_clos failed", 0, 0);
 	else
 		isst_clos_display_information(cpu, outf, current_clos,
 					      &clos_config);
@@ -2002,7 +2007,8 @@ static void dump_clos_config(int arg)
 		exit(0);
 	}
 	if (current_clos < 0 || current_clos > 3) {
-		fprintf(stderr, "Invalid clos id\n");
+		isst_display_error_info_message(1, "Invalid clos id\n", 0, 0);
+		isst_ctdp_display_information_end(outf);
 		exit(0);
 	}
 
@@ -2023,7 +2029,7 @@ static void get_clos_info_for_cpu(int cpu, void *arg1, void *arg2, void *arg3,
 
 	ret = isst_clos_get_clos_information(cpu, &enable, &prio_type);
 	if (ret)
-		perror("isst_clos_get_info");
+		isst_display_error_info_message(1, "isst_clos_get_info failed", 0, 0);
 	else {
 		int cp_state, cp_cap;
 
@@ -2069,7 +2075,7 @@ static void set_clos_config_for_cpu(int cpu, void *arg1, void *arg2, void *arg3,
 	clos_config.clos_desired = clos_desired;
 	ret = isst_set_clos(cpu, current_clos, &clos_config);
 	if (ret)
-		perror("isst_set_clos");
+		isst_display_error_info_message(1, "isst_set_clos failed", 0, 0);
 	else
 		isst_display_result(cpu, outf, "core-power", "config", ret);
 }
@@ -2081,26 +2087,27 @@ static void set_clos_config(int arg)
 			"Set core-power configuration for one of the four clos ids\n");
 		fprintf(stderr,
 			"\tSpecify targeted clos id with [--clos|-c]\n");
-		fprintf(stderr, "\tSpecify clos EPP with [--epp|-e]\n");
-		fprintf(stderr,
-			"\tSpecify clos Proportional Priority [--weight|-w]\n");
+		if (!is_skx_based_platform()) {
+			fprintf(stderr, "\tSpecify clos EPP with [--epp|-e]\n");
+			fprintf(stderr,
+				"\tSpecify clos Proportional Priority [--weight|-w]\n");
+		}
 		fprintf(stderr, "\tSpecify clos min in MHz with [--min|-n]\n");
 		fprintf(stderr, "\tSpecify clos max in MHz with [--max|-m]\n");
-		fprintf(stderr, "\tSpecify clos desired in MHz with [--desired|-d]\n");
 		exit(0);
 	}
 
 	if (current_clos < 0 || current_clos > 3) {
-		fprintf(stderr, "Invalid clos id\n");
+		isst_display_error_info_message(1, "Invalid clos id\n", 0, 0);
 		exit(0);
 	}
-	if (clos_epp < 0 || clos_epp > 0x0F) {
-		fprintf(stderr, "clos epp is not specified, default: 0\n");
+	if (!is_skx_based_platform() && (clos_epp < 0 || clos_epp > 0x0F)) {
+		fprintf(stderr, "clos epp is not specified or invalid, default: 0\n");
 		clos_epp = 0;
 	}
-	if (clos_prop_prio < 0 || clos_prop_prio > 0x0F) {
+	if (!is_skx_based_platform() && (clos_prop_prio < 0 || clos_prop_prio > 0x0F)) {
 		fprintf(stderr,
-			"clos frequency weight is not specified, default: 0\n");
+			"clos frequency weight is not specified or invalid, default: 0\n");
 		clos_prop_prio = 0;
 	}
 	if (clos_min < 0) {
@@ -2108,11 +2115,11 @@ static void set_clos_config(int arg)
 		clos_min = 0;
 	}
 	if (clos_max < 0) {
-		fprintf(stderr, "clos max is not specified, default: 25500 MHz\n");
+		fprintf(stderr, "clos max is not specified, default: Max frequency (ratio 0xff)\n");
 		clos_max = 0xff;
 	}
-	if (clos_desired < 0) {
-		fprintf(stderr, "clos desired is not specified, default: 0\n");
+	if (clos_desired) {
+		fprintf(stderr, "clos desired is not supported on this platform\n");
 		clos_desired = 0x00;
 	}
 
@@ -2133,7 +2140,7 @@ static void set_clos_assoc_for_cpu(int cpu, void *arg1, void *arg2, void *arg3,
 
 	ret = isst_clos_associate(cpu, current_clos);
 	if (ret)
-		perror("isst_clos_associate");
+		debug_printf("isst_clos_associate failed");
 	else
 		isst_display_result(cpu, outf, "core-power", "assoc", ret);
 }
@@ -2152,15 +2159,14 @@ static void set_clos_assoc(int arg)
 	}
 
 	if (current_clos < 0 || current_clos > 3) {
-		fprintf(stderr, "Invalid clos id\n");
+		isst_display_error_info_message(1, "Invalid clos id\n", 0, 0);
 		exit(0);
 	}
 	if (max_target_cpus)
 		for_each_online_target_cpu_in_set(set_clos_assoc_for_cpu, NULL,
 						  NULL, NULL, NULL);
 	else {
-		fprintf(stderr,
-			"Invalid target cpu. Specify with [-c|--cpu]\n");
+		isst_display_error_info_message(1, "Invalid target cpu. Specify with [-c|--cpu]", 0, 0);
 	}
 }
 
@@ -2171,7 +2177,7 @@ static void get_clos_assoc_for_cpu(int cpu, void *arg1, void *arg2, void *arg3,
 
 	ret = isst_clos_get_assoc_status(cpu, &clos);
 	if (ret)
-		perror("isst_clos_get_assoc_status");
+		isst_display_error_info_message(1, "isst_clos_get_assoc_status failed", 0, 0);
 	else
 		isst_clos_display_assoc_information(cpu, outf, clos);
 }
@@ -2185,8 +2191,7 @@ static void get_clos_assoc(int arg)
 	}
 
 	if (!max_target_cpus) {
-		fprintf(stderr,
-			"Invalid target cpu. Specify with [-c|--cpu]\n");
+		isst_display_error_info_message(1, "Invalid target cpu. Specify with [-c|--cpu]", 0, 0);
 		exit(0);
 	}
 
@@ -2366,6 +2371,10 @@ static void parse_cmd_args(int argc, int start, char **argv)
 			break;
 		case 'e':
 			clos_epp = atoi(optarg);
+			if (is_skx_based_platform()) {
+				isst_display_error_info_message(1, "epp can't be specified on this platform", 0, 0);
+				exit(0);
+			}
 			break;
 		case 'n':
 			clos_min = atoi(optarg);
@@ -2377,9 +2386,17 @@ static void parse_cmd_args(int argc, int start, char **argv)
 			break;
 		case 'p':
 			clos_priority_type = atoi(optarg);
+			if (is_skx_based_platform() && !clos_priority_type) {
+				isst_display_error_info_message(1, "Invalid clos priority type: proportional for this platform", 0, 0);
+				exit(0);
+			}
 			break;
 		case 'w':
 			clos_prop_prio = atoi(optarg);
+			if (is_skx_based_platform()) {
+				isst_display_error_info_message(1, "weight can't be specified on this platform", 0, 0);
+				exit(0);
+			}
 			break;
 		default:
 			printf("no match\n");
diff --git a/tools/power/x86/intel-speed-select/isst-core.c b/tools/power/x86/intel-speed-select/isst-core.c
index 89b3068a2685..67c9b1139631 100644
--- a/tools/power/x86/intel-speed-select/isst-core.c
+++ b/tools/power/x86/intel-speed-select/isst-core.c
@@ -917,17 +917,19 @@ int isst_pm_qos_config(int cpu, int enable_clos, int priority_type)
 		}
 		ret = isst_write_pm_config(cpu, 0);
 		if (ret)
-			perror("isst_write_pm_config\n");
+			isst_display_error_info_message(0, "WRITE_PM_CONFIG command failed, ignoring error\n", 0, 0);
 	} else {
 		ret = isst_write_pm_config(cpu, 1);
 		if (ret)
-			perror("isst_write_pm_config\n");
+			isst_display_error_info_message(0, "WRITE_PM_CONFIG command failed, ignoring error\n", 0, 0);
 	}
 
 	ret = isst_send_mbox_command(cpu, CONFIG_CLOS, CLOS_PM_QOS_CONFIG, 0, 0,
 				     &resp);
-	if (ret)
+	if (ret) {
+		isst_display_error_info_message(1, "CLOS_PM_QOS_CONFIG command failed", 0, 0);
 		return ret;
+	}
 
 	debug_printf("cpu:%d CLOS_PM_QOS_CONFIG resp:%x\n", cpu, resp);
 
@@ -939,7 +941,7 @@ int isst_pm_qos_config(int cpu, int enable_clos, int priority_type)
 		req = req & ~BIT(1);
 
 	if (priority_type > 1)
-		fprintf(stderr, "Invalid priority type: Changing type to ordered\n");
+		isst_display_error_info_message(1, "Invalid priority type: Changing type to ordered", 0, 0);
 
 	if (priority_type)
 		req = req | BIT(2);
diff --git a/tools/power/x86/intel-speed-select/isst-display.c b/tools/power/x86/intel-speed-select/isst-display.c
index bf9422336fab..ed51d8b79c41 100644
--- a/tools/power/x86/intel-speed-select/isst-display.c
+++ b/tools/power/x86/intel-speed-select/isst-display.c
@@ -599,7 +599,10 @@ void isst_clos_display_information(int cpu, FILE *outf, int clos,
 	format_and_print(outf, 5, header, value);
 
 	snprintf(header, sizeof(header), "clos-max");
-	snprintf(value, sizeof(value), "%d MHz", clos_config->clos_max * DISP_FREQ_MULTIPLIER);
+	if (clos_config->clos_max == 0xff)
+		snprintf(value, sizeof(value), "Max Turbo frequency");
+	else
+		snprintf(value, sizeof(value), "%d MHz", clos_config->clos_max * DISP_FREQ_MULTIPLIER);
 	format_and_print(outf, 5, header, value);
 
 	snprintf(header, sizeof(header), "clos-desired");
-- 
2.24.1

[PATCH 21/27] tools/power/x86/intel-speed-select: Show error for invalid CPUs in the options

[Srinivas Pandruvada]

When --cpu or -c is used to specify target CPUs and non of them are valid,
display error.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 tools/power/x86/intel-speed-select/isst-config.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/power/x86/intel-speed-select/isst-config.c b/tools/power/x86/intel-speed-select/isst-config.c
index 7a8a315ac8d1..386791aaeca1 100644
--- a/tools/power/x86/intel-speed-select/isst-config.c
+++ b/tools/power/x86/intel-speed-select/isst-config.c
@@ -425,7 +425,7 @@ static void for_each_online_target_cpu_in_set(
 	void (*callback)(int, void *, void *, void *, void *), void *arg1,
 	void *arg2, void *arg3, void *arg4)
 {
-	int i;
+	int i, found = 0;
 
 	for (i = 0; i < topo_max_cpus; ++i) {
 		int online;
@@ -439,9 +439,14 @@ static void for_each_online_target_cpu_in_set(
 			online =
 				1; /* online entry for CPU 0 needs some special configs */
 
-		if (online && callback)
+		if (online && callback) {
 			callback(i, arg1, arg2, arg3, arg4);
+			found = 1;
+		}
 	}
+
+	if (!found)
+		fprintf(stderr, "No valid CPU in the list\n");
 }
 
 #define BITMASK_SIZE 32
-- 
2.24.1

[PATCH 22/27] tools/power/x86/intel-speed-select: Improve CLX commands

[Srinivas Pandruvada]

CLX doesn't have capability to change the feature in the hardware, but
this acts as "--auto | -a" option. So even if user didn't specify the
option, use this as --auto | -a to set cpufreq scaling frequency limits.

Also remove perror with debug_printf as they don't bring any value.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 tools/power/x86/intel-speed-select/isst-config.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/tools/power/x86/intel-speed-select/isst-config.c b/tools/power/x86/intel-speed-select/isst-config.c
index 386791aaeca1..a44ec2f76f52 100644
--- a/tools/power/x86/intel-speed-select/isst-config.c
+++ b/tools/power/x86/intel-speed-select/isst-config.c
@@ -1162,7 +1162,7 @@ static void dump_clx_n_config_for_cpu(int cpu, void *arg1, void *arg2,
 
 	ret = clx_n_config(cpu);
 	if (ret) {
-		perror("isst_get_process_ctdp");
+		debug_printf("clx_n_config failed");
 	} else {
 		struct isst_pkg_ctdp_level_info *ctdp_level;
 		struct isst_pbf_info *pbf_info;
@@ -1419,7 +1419,7 @@ static int set_clx_pbf_cpufreq_scaling_min_max(int cpu)
 
 	ret = clx_n_config(cpu);
 	if (ret) {
-		perror("set_clx_pbf_cpufreq_scaling_min_max");
+		debug_printf("cpufreq_scaling_min_max failed for CLX");
 		return ret;
 	}
 
@@ -1635,17 +1635,13 @@ static void set_pbf_for_cpu(int cpu, void *arg1, void *arg2, void *arg3,
 	int status = *(int *)arg4;
 
 	if (is_clx_n_platform()) {
+		ret = 0;
 		if (status) {
-			ret = 0;
-			if (auto_mode)
-				set_clx_pbf_cpufreq_scaling_min_max(cpu);
+			set_clx_pbf_cpufreq_scaling_min_max(cpu);
 
 		} else {
-			ret = -1;
-			if (auto_mode) {
-				set_scaling_max_to_cpuinfo_max(cpu);
-				set_scaling_min_to_cpuinfo_min(cpu);
-			}
+			set_scaling_max_to_cpuinfo_max(cpu);
+			set_scaling_min_to_cpuinfo_min(cpu);
 		}
 		goto disp_result;
 	}
-- 
2.24.1

[PATCH 23/27] tools/power/x86/intel-speed-select: Fix avx options for turbo-freq feature

[Srinivas Pandruvada]

Specifying "avx2" and "avx512" option for display filter doesn't work
with short option "-r", only works with --try-type. Also compare full
6 characters for "avx512" string.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 tools/power/x86/intel-speed-select/isst-config.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/power/x86/intel-speed-select/isst-config.c b/tools/power/x86/intel-speed-select/isst-config.c
index a44ec2f76f52..2a81fb881f04 100644
--- a/tools/power/x86/intel-speed-select/isst-config.c
+++ b/tools/power/x86/intel-speed-select/isst-config.c
@@ -2329,7 +2329,7 @@ static void parse_cmd_args(int argc, int start, char **argv)
 	option_index = start;
 
 	optind = start + 1;
-	while ((opt = getopt_long(argc, argv, "b:l:t:c:d:e:n:m:p:w:hoa",
+	while ((opt = getopt_long(argc, argv, "b:l:t:c:d:e:n:m:p:w:r:hoa",
 				  long_options, &option_index)) != -1) {
 		switch (opt) {
 		case 'a':
@@ -2355,7 +2355,7 @@ static void parse_cmd_args(int argc, int start, char **argv)
 				fact_avx = 0x01;
 			} else if (!strncmp(optarg, "avx2", 4)) {
 				fact_avx = 0x02;
-			} else if (!strncmp(optarg, "avx512", 4)) {
+			} else if (!strncmp(optarg, "avx512", 6)) {
 				fact_avx = 0x04;
 			} else {
 				fprintf(outf, "Invalid sse,avx options\n");
-- 
2.24.1

[PATCH 24/27] tools/power/x86/intel-speed-select: Print friendly warning for bad command line

[Srinivas Pandruvada]

When user specifies invalid option, display "Unknown Option: ignore",
instead of "no match". Also display error for garbage on the command
line.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 tools/power/x86/intel-speed-select/isst-config.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/power/x86/intel-speed-select/isst-config.c b/tools/power/x86/intel-speed-select/isst-config.c
index 2a81fb881f04..924cb871d6d2 100644
--- a/tools/power/x86/intel-speed-select/isst-config.c
+++ b/tools/power/x86/intel-speed-select/isst-config.c
@@ -2400,9 +2400,12 @@ static void parse_cmd_args(int argc, int start, char **argv)
 			}
 			break;
 		default:
-			printf("no match\n");
+			printf("Unknown option: ignore\n");
 		}
 	}
+
+	if (argv[optind])
+		printf("Garbage at the end of command: ignore\n");
 }
 
 static void isst_help(void)
-- 
2.24.1

[PATCH 25/27] tools/power/x86/intel-speed-select: Add display for enabled cpus count

[Srinivas Pandruvada]

In addition to total CPU count also display "enabled-cpu-count" for
perf-profile info command. This will show number of CPUs in the
"enable-cpu-mask".

For example:

 perf-profile-level-4
        cpu-count:32
        enable-cpu-count:16
        enable-cpu-mask:e42d0000,e42d0000

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 tools/power/x86/intel-speed-select/isst-display.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/power/x86/intel-speed-select/isst-display.c b/tools/power/x86/intel-speed-select/isst-display.c
index ed51d8b79c41..4f4c42147a90 100644
--- a/tools/power/x86/intel-speed-select/isst-display.c
+++ b/tools/power/x86/intel-speed-select/isst-display.c
@@ -354,6 +354,14 @@ void isst_ctdp_display_information(int cpu, FILE *outf, int tdp_level,
 		snprintf(value, sizeof(value), "%d", j);
 		format_and_print(outf, base_level + 4, header, value);
 
+		j = CPU_COUNT_S(ctdp_level->core_cpumask_size,
+				ctdp_level->core_cpumask);
+		if (j) {
+			snprintf(header, sizeof(header), "enable-cpu-count");
+			snprintf(value, sizeof(value), "%d", j);
+			format_and_print(outf, base_level + 4, header, value);
+		}
+
 		if (ctdp_level->core_cpumask_size) {
 			snprintf(header, sizeof(header), "enable-cpu-mask");
 			printcpumask(sizeof(value), value,
-- 
2.24.1

[PATCH 26/27] tools/power/x86/intel-speed-select: Avoid duplicate Package strings for json

[Srinivas Pandruvada]

For platforms where multiple packages/die, this makes "Package-" key
duplicate. To make unique, add die and cpu id to key name.

So "Package-0" key name will change to "Package-0-die-x:cpu-x".

For example:
$sudo ./intel-speed-select -f json  perf-profile info
Intel(R) Speed Select Technology
Executing on CPU model:106[0x6a]
{
  "package-0:die-0:cpu-0": {
    "perf-profile-level-0": {
      "cpu-count": "32",
      "enable-cpu-count": "32",
...
...
 "package-1:die-0:cpu-16": {

    "perf-profile-level-0": {
      "cpu-count": "32",
      "enable-cpu-count": "32",
      "enable-cpu-mask": "ffff0000,ffff0000",
...
...

For non json format, there is no change. Here when print_package_info()
is called, it will return the level to print for other information.
This level is used formatting. Also in some function duplicate code
was there to print package,die and CPU information. Replace all that
code with a call to print_package_info().

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 .../x86/intel-speed-select/isst-display.c     | 172 +++++++++---------
 1 file changed, 83 insertions(+), 89 deletions(-)

diff --git a/tools/power/x86/intel-speed-select/isst-display.c b/tools/power/x86/intel-speed-select/isst-display.c
index 4f4c42147a90..51dbaa5f02ec 100644
--- a/tools/power/x86/intel-speed-select/isst-display.c
+++ b/tools/power/x86/intel-speed-select/isst-display.c
@@ -158,10 +158,17 @@ static void format_and_print(FILE *outf, int level, char *header, char *value)
 	last_level = level;
 }
 
-static void print_package_info(int cpu, FILE *outf)
+static int print_package_info(int cpu, FILE *outf)
 {
 	char header[256];
 
+	if (out_format_is_json()) {
+		snprintf(header, sizeof(header), "package-%d:die-%d:cpu-%d",
+			 get_physical_package_id(cpu), get_physical_die_id(cpu),
+			 cpu);
+		format_and_print(outf, 1, header, NULL);
+		return 1;
+	}
 	snprintf(header, sizeof(header), "package-%d",
 		 get_physical_package_id(cpu));
 	format_and_print(outf, 1, header, NULL);
@@ -169,6 +176,8 @@ static void print_package_info(int cpu, FILE *outf)
 	format_and_print(outf, 2, header, NULL);
 	snprintf(header, sizeof(header), "cpu-%d", cpu);
 	format_and_print(outf, 3, header, NULL);
+
+	return 3;
 }
 
 static void _isst_pbf_display_information(int cpu, FILE *outf, int level,
@@ -331,10 +340,11 @@ void isst_ctdp_display_information(int cpu, FILE *outf, int tdp_level,
 {
 	char header[256];
 	char value[256];
-	int i, base_level = 1;
+	static int level;
+	int i;
 
 	if (pkg_dev->processed)
-		print_package_info(cpu, outf);
+		level = print_package_info(cpu, outf);
 
 	for (i = 0; i <= pkg_dev->levels; ++i) {
 		struct isst_pkg_ctdp_level_info *ctdp_level;
@@ -346,20 +356,20 @@ void isst_ctdp_display_information(int cpu, FILE *outf, int tdp_level,
 
 		snprintf(header, sizeof(header), "perf-profile-level-%d",
 			 ctdp_level->level);
-		format_and_print(outf, base_level + 3, header, NULL);
+		format_and_print(outf, level + 1, header, NULL);
 
 		snprintf(header, sizeof(header), "cpu-count");
 		j = get_cpu_count(get_physical_die_id(cpu),
 				  get_physical_die_id(cpu));
 		snprintf(value, sizeof(value), "%d", j);
-		format_and_print(outf, base_level + 4, header, value);
+		format_and_print(outf, level + 2, header, value);
 
 		j = CPU_COUNT_S(ctdp_level->core_cpumask_size,
 				ctdp_level->core_cpumask);
 		if (j) {
 			snprintf(header, sizeof(header), "enable-cpu-count");
 			snprintf(value, sizeof(value), "%d", j);
-			format_and_print(outf, base_level + 4, header, value);
+			format_and_print(outf, level + 2, header, value);
 		}
 
 		if (ctdp_level->core_cpumask_size) {
@@ -367,59 +377,59 @@ void isst_ctdp_display_information(int cpu, FILE *outf, int tdp_level,
 			printcpumask(sizeof(value), value,
 				     ctdp_level->core_cpumask_size,
 				     ctdp_level->core_cpumask);
-			format_and_print(outf, base_level + 4, header, value);
+			format_and_print(outf, level + 2, header, value);
 
 			snprintf(header, sizeof(header), "enable-cpu-list");
 			printcpulist(sizeof(value), value,
 				     ctdp_level->core_cpumask_size,
 				     ctdp_level->core_cpumask);
-			format_and_print(outf, base_level + 4, header, value);
+			format_and_print(outf, level + 2, header, value);
 		}
 
 		snprintf(header, sizeof(header), "thermal-design-power-ratio");
 		snprintf(value, sizeof(value), "%d", ctdp_level->tdp_ratio);
-		format_and_print(outf, base_level + 4, header, value);
+		format_and_print(outf, level + 2, header, value);
 
 		snprintf(header, sizeof(header), "base-frequency(MHz)");
 		if (!ctdp_level->sse_p1)
 			ctdp_level->sse_p1 = ctdp_level->tdp_ratio;
 		snprintf(value, sizeof(value), "%d",
 			  ctdp_level->sse_p1 * DISP_FREQ_MULTIPLIER);
-		format_and_print(outf, base_level + 4, header, value);
+		format_and_print(outf, level + 2, header, value);
 
 		if (ctdp_level->avx2_p1) {
 			snprintf(header, sizeof(header), "base-frequency-avx2(MHz)");
 			snprintf(value, sizeof(value), "%d",
 				 ctdp_level->avx2_p1 * DISP_FREQ_MULTIPLIER);
-			format_and_print(outf, base_level + 4, header, value);
+			format_and_print(outf, level + 2, header, value);
 		}
 
 		if (ctdp_level->avx512_p1) {
 			snprintf(header, sizeof(header), "base-frequency-avx512(MHz)");
 			snprintf(value, sizeof(value), "%d",
 				 ctdp_level->avx512_p1 * DISP_FREQ_MULTIPLIER);
-			format_and_print(outf, base_level + 4, header, value);
+			format_and_print(outf, level + 2, header, value);
 		}
 
 		if (ctdp_level->uncore_p1) {
 			snprintf(header, sizeof(header), "uncore-frequency-min(MHz)");
 			snprintf(value, sizeof(value), "%d",
 				 ctdp_level->uncore_p1 * DISP_FREQ_MULTIPLIER);
-			format_and_print(outf, base_level + 4, header, value);
+			format_and_print(outf, level + 2, header, value);
 		}
 
 		if (ctdp_level->uncore_p0) {
 			snprintf(header, sizeof(header), "uncore-frequency-max(MHz)");
 			snprintf(value, sizeof(value), "%d",
 				 ctdp_level->uncore_p0 * DISP_FREQ_MULTIPLIER);
-			format_and_print(outf, base_level + 4, header, value);
+			format_and_print(outf, level + 2, header, value);
 		}
 
 		if (ctdp_level->mem_freq) {
 			snprintf(header, sizeof(header), "mem-frequency(MHz)");
 			snprintf(value, sizeof(value), "%d",
 				 ctdp_level->mem_freq * DISP_FREQ_MULTIPLIER);
-			format_and_print(outf, base_level + 4, header, value);
+			format_and_print(outf, level + 2, header, value);
 		}
 
 		snprintf(header, sizeof(header),
@@ -431,7 +441,7 @@ void isst_ctdp_display_information(int cpu, FILE *outf, int tdp_level,
 				snprintf(value, sizeof(value), "disabled");
 		} else
 			snprintf(value, sizeof(value), "unsupported");
-		format_and_print(outf, base_level + 4, header, value);
+		format_and_print(outf, level + 2, header, value);
 
 		snprintf(header, sizeof(header),
 			 "speed-select-base-freq");
@@ -442,7 +452,7 @@ void isst_ctdp_display_information(int cpu, FILE *outf, int tdp_level,
 				snprintf(value, sizeof(value), "disabled");
 		} else
 			snprintf(value, sizeof(value), "unsupported");
-		format_and_print(outf, base_level + 4, header, value);
+		format_and_print(outf, level + 2, header, value);
 
 		snprintf(header, sizeof(header),
 			 "speed-select-core-power");
@@ -453,89 +463,89 @@ void isst_ctdp_display_information(int cpu, FILE *outf, int tdp_level,
 				snprintf(value, sizeof(value), "disabled");
 		} else
 			snprintf(value, sizeof(value), "unsupported");
-		format_and_print(outf, base_level + 4, header, value);
+		format_and_print(outf, level + 2, header, value);
 
 		if (is_clx_n_platform()) {
 			if (ctdp_level->pbf_support)
 				_isst_pbf_display_information(cpu, outf,
 							      tdp_level,
 							  &ctdp_level->pbf_info,
-							      base_level + 4);
+							      level + 1);
 			continue;
 		}
 
 		if (ctdp_level->pkg_tdp) {
 			snprintf(header, sizeof(header), "thermal-design-power(W)");
 			snprintf(value, sizeof(value), "%d", ctdp_level->pkg_tdp);
-			format_and_print(outf, base_level + 4, header, value);
+			format_and_print(outf, level + 2, header, value);
 		}
 
 		if (ctdp_level->t_proc_hot) {
 			snprintf(header, sizeof(header), "tjunction-max(C)");
 			snprintf(value, sizeof(value), "%d", ctdp_level->t_proc_hot);
-			format_and_print(outf, base_level + 4, header, value);
+			format_and_print(outf, level + 2, header, value);
 		}
 
 		snprintf(header, sizeof(header), "turbo-ratio-limits-sse");
-		format_and_print(outf, base_level + 4, header, NULL);
+		format_and_print(outf, level + 2, header, NULL);
 		for (j = 0; j < 8; ++j) {
 			snprintf(header, sizeof(header), "bucket-%d", j);
-			format_and_print(outf, base_level + 5, header, NULL);
+			format_and_print(outf, level + 3, header, NULL);
 
 			snprintf(header, sizeof(header), "core-count");
 			snprintf(value, sizeof(value), "%llu", (ctdp_level->buckets_info >> (j * 8)) & 0xff);
-			format_and_print(outf, base_level + 6, header, value);
+			format_and_print(outf, level + 4, header, value);
 
 			snprintf(header, sizeof(header),
 				"max-turbo-frequency(MHz)");
 			snprintf(value, sizeof(value), "%d",
 				 ctdp_level->trl_sse_active_cores[j] *
 				  DISP_FREQ_MULTIPLIER);
-			format_and_print(outf, base_level + 6, header, value);
+			format_and_print(outf, level + 4, header, value);
 		}
 
 		if (ctdp_level->trl_avx_active_cores[0]) {
 			snprintf(header, sizeof(header), "turbo-ratio-limits-avx2");
-			format_and_print(outf, base_level + 4, header, NULL);
+			format_and_print(outf, level + 2, header, NULL);
 			for (j = 0; j < 8; ++j) {
 				snprintf(header, sizeof(header), "bucket-%d", j);
-				format_and_print(outf, base_level + 5, header, NULL);
+				format_and_print(outf, level + 3, header, NULL);
 
 				snprintf(header, sizeof(header), "core-count");
 				snprintf(value, sizeof(value), "%llu", (ctdp_level->buckets_info >> (j * 8)) & 0xff);
-				format_and_print(outf, base_level + 6, header, value);
+				format_and_print(outf, level + 4, header, value);
 
 				snprintf(header, sizeof(header), "max-turbo-frequency(MHz)");
 				snprintf(value, sizeof(value), "%d", ctdp_level->trl_avx_active_cores[j] * DISP_FREQ_MULTIPLIER);
-				format_and_print(outf, base_level + 6, header, value);
+				format_and_print(outf, level + 4, header, value);
 			}
 		}
 
 		if (ctdp_level->trl_avx_512_active_cores[0]) {
 			snprintf(header, sizeof(header), "turbo-ratio-limits-avx512");
-			format_and_print(outf, base_level + 4, header, NULL);
+			format_and_print(outf, level + 2, header, NULL);
 			for (j = 0; j < 8; ++j) {
 				snprintf(header, sizeof(header), "bucket-%d", j);
-				format_and_print(outf, base_level + 5, header, NULL);
+				format_and_print(outf, level + 3, header, NULL);
 
 				snprintf(header, sizeof(header), "core-count");
 				snprintf(value, sizeof(value), "%llu", (ctdp_level->buckets_info >> (j * 8)) & 0xff);
-				format_and_print(outf, base_level + 6, header, value);
+				format_and_print(outf, level + 4, header, value);
 
 				snprintf(header, sizeof(header), "max-turbo-frequency(MHz)");
 				snprintf(value, sizeof(value), "%d", ctdp_level->trl_avx_512_active_cores[j] * DISP_FREQ_MULTIPLIER);
-			format_and_print(outf, base_level + 6, header, value);
+				format_and_print(outf, level + 4, header, value);
 			}
 		}
 
 		if (ctdp_level->pbf_support)
 			_isst_pbf_display_information(cpu, outf, i,
 						      &ctdp_level->pbf_info,
-						      base_level + 4);
+						      level + 2);
 		if (ctdp_level->fact_support)
 			_isst_fact_display_information(cpu, outf, i, 0xff, 0xff,
 						       &ctdp_level->fact_info,
-						       base_level + 4);
+						       level + 2);
 	}
 
 	format_and_print(outf, 1, NULL, NULL);
@@ -558,8 +568,10 @@ void isst_ctdp_display_information_end(FILE *outf)
 void isst_pbf_display_information(int cpu, FILE *outf, int level,
 				  struct isst_pbf_info *pbf_info)
 {
-	print_package_info(cpu, outf);
-	_isst_pbf_display_information(cpu, outf, level, pbf_info, 4);
+	int _level;
+
+	_level = print_package_info(cpu, outf);
+	_isst_pbf_display_information(cpu, outf, level, pbf_info, _level + 1);
 	format_and_print(outf, 1, NULL, NULL);
 }
 
@@ -567,9 +579,11 @@ void isst_fact_display_information(int cpu, FILE *outf, int level,
 				   int fact_bucket, int fact_avx,
 				   struct isst_fact_info *fact_info)
 {
-	print_package_info(cpu, outf);
+	int _level;
+
+	_level = print_package_info(cpu, outf);
 	_isst_fact_display_information(cpu, outf, level, fact_bucket, fact_avx,
-				       fact_info, 4);
+				       fact_info, _level + 1);
 	format_and_print(outf, 1, NULL, NULL);
 }
 
@@ -578,46 +592,41 @@ void isst_clos_display_information(int cpu, FILE *outf, int clos,
 {
 	char header[256];
 	char value[256];
+	int level;
 
-	snprintf(header, sizeof(header), "package-%d",
-		 get_physical_package_id(cpu));
-	format_and_print(outf, 1, header, NULL);
-	snprintf(header, sizeof(header), "die-%d", get_physical_die_id(cpu));
-	format_and_print(outf, 2, header, NULL);
-	snprintf(header, sizeof(header), "cpu-%d", cpu);
-	format_and_print(outf, 3, header, NULL);
+	level = print_package_info(cpu, outf);
 
 	snprintf(header, sizeof(header), "core-power");
-	format_and_print(outf, 4, header, NULL);
+	format_and_print(outf, level + 1, header, NULL);
 
 	snprintf(header, sizeof(header), "clos");
 	snprintf(value, sizeof(value), "%d", clos);
-	format_and_print(outf, 5, header, value);
+	format_and_print(outf, level + 2, header, value);
 
 	snprintf(header, sizeof(header), "epp");
 	snprintf(value, sizeof(value), "%d", clos_config->epp);
-	format_and_print(outf, 5, header, value);
+	format_and_print(outf, level + 2, header, value);
 
 	snprintf(header, sizeof(header), "clos-proportional-priority");
 	snprintf(value, sizeof(value), "%d", clos_config->clos_prop_prio);
-	format_and_print(outf, 5, header, value);
+	format_and_print(outf, level + 2, header, value);
 
 	snprintf(header, sizeof(header), "clos-min");
 	snprintf(value, sizeof(value), "%d MHz", clos_config->clos_min * DISP_FREQ_MULTIPLIER);
-	format_and_print(outf, 5, header, value);
+	format_and_print(outf, level + 2, header, value);
 
 	snprintf(header, sizeof(header), "clos-max");
 	if (clos_config->clos_max == 0xff)
 		snprintf(value, sizeof(value), "Max Turbo frequency");
 	else
 		snprintf(value, sizeof(value), "%d MHz", clos_config->clos_max * DISP_FREQ_MULTIPLIER);
-	format_and_print(outf, 5, header, value);
+	format_and_print(outf, level + 2, header, value);
 
 	snprintf(header, sizeof(header), "clos-desired");
 	snprintf(value, sizeof(value), "%d MHz", clos_config->clos_desired * DISP_FREQ_MULTIPLIER);
-	format_and_print(outf, 5, header, value);
+	format_and_print(outf, level + 2, header, value);
 
-	format_and_print(outf, 1, NULL, NULL);
+	format_and_print(outf, level, NULL, NULL);
 }
 
 void isst_clos_display_clos_information(int cpu, FILE *outf,
@@ -626,70 +635,60 @@ void isst_clos_display_clos_information(int cpu, FILE *outf,
 {
 	char header[256];
 	char value[256];
+	int level;
 
-	snprintf(header, sizeof(header), "package-%d",
-		 get_physical_package_id(cpu));
-	format_and_print(outf, 1, header, NULL);
-	snprintf(header, sizeof(header), "die-%d", get_physical_die_id(cpu));
-	format_and_print(outf, 2, header, NULL);
-	snprintf(header, sizeof(header), "cpu-%d", cpu);
-	format_and_print(outf, 3, header, NULL);
+	level = print_package_info(cpu, outf);
 
 	snprintf(header, sizeof(header), "core-power");
-	format_and_print(outf, 4, header, NULL);
+	format_and_print(outf, level + 1, header, NULL);
 
 	snprintf(header, sizeof(header), "support-status");
 	if (cap)
 		snprintf(value, sizeof(value), "supported");
 	else
 		snprintf(value, sizeof(value), "unsupported");
-	format_and_print(outf, 5, header, value);
+	format_and_print(outf, level + 2, header, value);
 
 	snprintf(header, sizeof(header), "enable-status");
 	if (state)
 		snprintf(value, sizeof(value), "enabled");
 	else
 		snprintf(value, sizeof(value), "disabled");
-	format_and_print(outf, 5, header, value);
+	format_and_print(outf, level + 2, header, value);
 
 	snprintf(header, sizeof(header), "clos-enable-status");
 	if (clos_enable)
 		snprintf(value, sizeof(value), "enabled");
 	else
 		snprintf(value, sizeof(value), "disabled");
-	format_and_print(outf, 5, header, value);
+	format_and_print(outf, level + 2, header, value);
 
 	snprintf(header, sizeof(header), "priority-type");
 	if (type)
 		snprintf(value, sizeof(value), "ordered");
 	else
 		snprintf(value, sizeof(value), "proportional");
-	format_and_print(outf, 5, header, value);
+	format_and_print(outf, level + 2, header, value);
 
-	format_and_print(outf, 1, NULL, NULL);
+	format_and_print(outf, level, NULL, NULL);
 }
 
 void isst_clos_display_assoc_information(int cpu, FILE *outf, int clos)
 {
 	char header[256];
 	char value[256];
+	int level;
 
-	snprintf(header, sizeof(header), "package-%d",
-		 get_physical_package_id(cpu));
-	format_and_print(outf, 1, header, NULL);
-	snprintf(header, sizeof(header), "die-%d", get_physical_die_id(cpu));
-	format_and_print(outf, 2, header, NULL);
-	snprintf(header, sizeof(header), "cpu-%d", cpu);
-	format_and_print(outf, 3, header, NULL);
+	level = print_package_info(cpu, outf);
 
 	snprintf(header, sizeof(header), "get-assoc");
-	format_and_print(outf, 4, header, NULL);
+	format_and_print(outf, level + 1, header, NULL);
 
 	snprintf(header, sizeof(header), "clos");
 	snprintf(value, sizeof(value), "%d", clos);
-	format_and_print(outf, 5, header, value);
+	format_and_print(outf, level + 2, header, value);
 
-	format_and_print(outf, 1, NULL, NULL);
+	format_and_print(outf, level, NULL, NULL);
 }
 
 void isst_display_result(int cpu, FILE *outf, char *feature, char *cmd,
@@ -697,26 +696,21 @@ void isst_display_result(int cpu, FILE *outf, char *feature, char *cmd,
 {
 	char header[256];
 	char value[256];
+	int level = 3;
+
+	if (cpu >= 0)
+		level = print_package_info(cpu, outf);
 
-	if (cpu >= 0) {
-		snprintf(header, sizeof(header), "package-%d",
-			 get_physical_package_id(cpu));
-		format_and_print(outf, 1, header, NULL);
-		snprintf(header, sizeof(header), "die-%d", get_physical_die_id(cpu));
-		format_and_print(outf, 2, header, NULL);
-		snprintf(header, sizeof(header), "cpu-%d", cpu);
-		format_and_print(outf, 3, header, NULL);
-	}
 	snprintf(header, sizeof(header), "%s", feature);
-	format_and_print(outf, 4, header, NULL);
+	format_and_print(outf, level + 1, header, NULL);
 	snprintf(header, sizeof(header), "%s", cmd);
 	if (!result)
 		snprintf(value, sizeof(value), "success");
 	else
 		snprintf(value, sizeof(value), "failed(error %d)", result);
-	format_and_print(outf, 5, header, value);
+	format_and_print(outf, level + 2, header, value);
 
-	format_and_print(outf, 1, NULL, NULL);
+	format_and_print(outf, level, NULL, NULL);
 }
 
 void isst_display_error_info_message(int error, char *msg, int arg_valid, int arg)
-- 
2.24.1

[PATCH 27/27] tools/power/x86/intel-speed-select: Update version

[Srinivas Pandruvada]

Fair number of changes including bug fixes done to change version.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 tools/power/x86/intel-speed-select/isst-config.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/power/x86/intel-speed-select/isst-config.c b/tools/power/x86/intel-speed-select/isst-config.c
index 924cb871d6d2..cd803a40ee40 100644
--- a/tools/power/x86/intel-speed-select/isst-config.c
+++ b/tools/power/x86/intel-speed-select/isst-config.c
@@ -15,7 +15,7 @@ struct process_cmd_struct {
 	int arg;
 };
 
-static const char *version_str = "v1.2";
+static const char *version_str = "v1.3";
 static const int supported_api_ver = 1;
 static struct isst_if_platform_info isst_platform_info;
 static char *progname;
-- 
2.24.1