[PATCH 0/7] platform/x86/inte-uncore-freq: Cleanups

[PATCH 0/7] platform/x86/inte-uncore-freq: Cleanups

[Tero Kristo]

Hi,

This series introduces some cleanups for the code and driver level APIs.
The purpose is to improve the maintainability of the code and make it
easier to add new features in future.

No functional changes expected from any of the patches in this series.

-Tero

[PATCH 1/7] platform/x86/intel-uncore-freq: Re-arrange bit masks

[Tero Kristo]

Rename the various bitmasks from the 'UNCORE_GENMASK_*' to
'UNCORE_*_MASK', and re-order them based on the register they reside in.

No functional change intended.

Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
Reviewed-by: Ilpo Jarvinen <ilpo.jarvinen@linux.intel.com>
---
 .../uncore-frequency/uncore-frequency-tpmi.c  | 25 +++++++++++--------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
index bb8e72deb354..b58294498921 100644
--- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
+++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
@@ -69,9 +69,12 @@ struct tpmi_uncore_struct {
 	bool write_blocked;
 };
 
-#define UNCORE_GENMASK_MIN_RATIO	GENMASK_ULL(21, 15)
-#define UNCORE_GENMASK_MAX_RATIO	GENMASK_ULL(14, 8)
-#define UNCORE_GENMASK_CURRENT_RATIO	GENMASK_ULL(6, 0)
+/* Bit definitions for STATUS register */
+#define UNCORE_CURRENT_RATIO_MASK			GENMASK_ULL(6, 0)
+
+/* Bit definitions for CONTROL register */
+#define UNCORE_MAX_RATIO_MASK				GENMASK_ULL(14, 8)
+#define UNCORE_MIN_RATIO_MASK				GENMASK_ULL(21, 15)
 
 /* Helper function to read MMIO offset for max/min control frequency */
 static void read_control_freq(struct tpmi_uncore_cluster_info *cluster_info,
@@ -80,11 +83,11 @@ static void read_control_freq(struct tpmi_uncore_cluster_info *cluster_info,
 	u64 control;
 
 	control = readq(cluster_info->cluster_base + UNCORE_CONTROL_INDEX);
-	*max = FIELD_GET(UNCORE_GENMASK_MAX_RATIO, control) * UNCORE_FREQ_KHZ_MULTIPLIER;
-	*min = FIELD_GET(UNCORE_GENMASK_MIN_RATIO, control) * UNCORE_FREQ_KHZ_MULTIPLIER;
+	*max = FIELD_GET(UNCORE_MAX_RATIO_MASK, control) * UNCORE_FREQ_KHZ_MULTIPLIER;
+	*min = FIELD_GET(UNCORE_MIN_RATIO_MASK, control) * UNCORE_FREQ_KHZ_MULTIPLIER;
 }
 
-#define UNCORE_MAX_RATIO	FIELD_MAX(UNCORE_GENMASK_MAX_RATIO)
+#define UNCORE_MAX_RATIO	FIELD_MAX(UNCORE_MAX_RATIO_MASK)
 
 /* Callback for sysfs read for max/min frequencies. Called under mutex locks */
 static int uncore_read_control_freq(struct uncore_data *data, unsigned int *min,
@@ -134,11 +137,11 @@ static void write_control_freq(struct tpmi_uncore_cluster_info *cluster_info, un
 	control = readq(cluster_info->cluster_base + UNCORE_CONTROL_INDEX);
 
 	if (min_max) {
-		control &= ~UNCORE_GENMASK_MAX_RATIO;
-		control |= FIELD_PREP(UNCORE_GENMASK_MAX_RATIO, input);
+		control &= ~UNCORE_MAX_RATIO_MASK;
+		control |= FIELD_PREP(UNCORE_MAX_RATIO_MASK, input);
 	} else {
-		control &= ~UNCORE_GENMASK_MIN_RATIO;
-		control |= FIELD_PREP(UNCORE_GENMASK_MIN_RATIO, input);
+		control &= ~UNCORE_MIN_RATIO_MASK;
+		control |= FIELD_PREP(UNCORE_MIN_RATIO_MASK, input);
 	}
 
 	writeq(control, (cluster_info->cluster_base + UNCORE_CONTROL_INDEX));
@@ -204,7 +207,7 @@ static int uncore_read_freq(struct uncore_data *data, unsigned int *freq)
 		return -ENODATA;
 
 	status = readq((u8 __iomem *)cluster_info->cluster_base + UNCORE_STATUS_INDEX);
-	*freq = FIELD_GET(UNCORE_GENMASK_CURRENT_RATIO, status) * UNCORE_FREQ_KHZ_MULTIPLIER;
+	*freq = FIELD_GET(UNCORE_CURRENT_RATIO_MASK, status) * UNCORE_FREQ_KHZ_MULTIPLIER;
 
 	return 0;
 }
-- 
2.43.0

[PATCH 2/7] platform/x86/intel-uncore-freq: Get rid of magic values

[Tero Kristo]

Get rid of any magic bitmasks from the code. Define proper macros for
these, and use the bitfield operations to access them.

No functional change intended.

Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
---
 .../intel/uncore-frequency/uncore-frequency.c | 22 ++++++++++++-------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency.c b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency.c
index b89c0dda9e5d..d3fdae695bbd 100644
--- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency.c
+++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency.c
@@ -14,6 +14,7 @@
  * Author: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
  */
 
+#include <linux/bitfield.h>
 #include <linux/cpu.h>
 #include <linux/module.h>
 #include <linux/slab.h>
@@ -36,6 +37,11 @@ static enum cpuhp_state uncore_hp_state __read_mostly;
 #define MSR_UNCORE_PERF_STATUS	0x621
 #define UNCORE_FREQ_KHZ_MULTIPLIER	100000
 
+#define UNCORE_MAX_RATIO_MASK	GENMASK_ULL(6, 0)
+#define UNCORE_MIN_RATIO_MASK	GENMASK_ULL(14, 8)
+
+#define UNCORE_CURRENT_RATIO_MASK	GENMASK_ULL(6, 0)
+
 static int uncore_read_control_freq(struct uncore_data *data, unsigned int *min,
 				    unsigned int *max)
 {
@@ -49,8 +55,8 @@ static int uncore_read_control_freq(struct uncore_data *data, unsigned int *min,
 	if (ret)
 		return ret;
 
-	*max = (cap & 0x7F) * UNCORE_FREQ_KHZ_MULTIPLIER;
-	*min = ((cap & GENMASK(14, 8)) >> 8) * UNCORE_FREQ_KHZ_MULTIPLIER;
+	*max = FIELD_GET(UNCORE_MAX_RATIO_MASK, cap) * UNCORE_FREQ_KHZ_MULTIPLIER;
+	*min = FIELD_GET(UNCORE_MIN_RATIO_MASK, cap) * UNCORE_FREQ_KHZ_MULTIPLIER;
 
 	return 0;
 }
@@ -62,7 +68,7 @@ static int uncore_write_control_freq(struct uncore_data *data, unsigned int inpu
 	u64 cap;
 
 	input /= UNCORE_FREQ_KHZ_MULTIPLIER;
-	if (!input || input > 0x7F)
+	if (!input || input > FIELD_MAX(UNCORE_MAX_RATIO_MASK))
 		return -EINVAL;
 
 	if (data->control_cpu < 0)
@@ -73,11 +79,11 @@ static int uncore_write_control_freq(struct uncore_data *data, unsigned int inpu
 		return ret;
 
 	if (min_max) {
-		cap &= ~0x7F;
-		cap |= input;
+		cap &= ~UNCORE_MAX_RATIO_MASK;
+		cap |= FIELD_PREP(UNCORE_MAX_RATIO_MASK, input);
 	} else  {
-		cap &= ~GENMASK(14, 8);
-		cap |= (input << 8);
+		cap &= ~UNCORE_MIN_RATIO_MASK;
+		cap |= FIELD_PREP(UNCORE_MIN_RATIO_MASK, input);
 	}
 
 	ret = wrmsrl_on_cpu(data->control_cpu, MSR_UNCORE_RATIO_LIMIT, cap);
@@ -101,7 +107,7 @@ static int uncore_read_freq(struct uncore_data *data, unsigned int *freq)
 	if (ret)
 		return ret;
 
-	*freq = (ratio & 0x7F) * UNCORE_FREQ_KHZ_MULTIPLIER;
+	*freq = FIELD_GET(UNCORE_CURRENT_RATIO_MASK, ratio) * UNCORE_FREQ_KHZ_MULTIPLIER;
 
 	return 0;
 }
-- 
2.43.0

[PATCH 3/7] platform/x86/intel-uncore-freq: Get rid of magic min_max argument

[Tero Kristo]

Get rid of the hardcoded / magic min_max argument from internal APIs.
Instead, use an enumerated index value for it.

No functional change intended.

Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
---
 .../uncore-frequency-common.c                 | 21 ++++++++++---------
 .../uncore-frequency-common.h                 |  8 ++++++-
 .../uncore-frequency/uncore-frequency-tpmi.c  | 18 +++++++++-------
 .../intel/uncore-frequency/uncore-frequency.c |  4 ++--
 4 files changed, 30 insertions(+), 21 deletions(-)

diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c
index 33bb58dc3f78..7daca46f9235 100644
--- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c
+++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c
@@ -20,7 +20,7 @@ static DEFINE_IDA(intel_uncore_ida);
 
 /* callbacks for actual HW read/write */
 static int (*uncore_read)(struct uncore_data *data, unsigned int *min, unsigned int *max);
-static int (*uncore_write)(struct uncore_data *data, unsigned int input, unsigned int min_max);
+static int (*uncore_write)(struct uncore_data *data, unsigned int input, enum uncore_index index);
 static int (*uncore_read_freq)(struct uncore_data *data, unsigned int *freq);
 
 static ssize_t show_domain_id(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
@@ -45,7 +45,7 @@ static ssize_t show_package_id(struct kobject *kobj, struct kobj_attribute *attr
 }
 
 static ssize_t show_min_max_freq_khz(struct uncore_data *data,
-				      char *buf, int min_max)
+				      char *buf, enum uncore_index index)
 {
 	unsigned int min, max;
 	int ret;
@@ -56,7 +56,7 @@ static ssize_t show_min_max_freq_khz(struct uncore_data *data,
 	if (ret)
 		return ret;
 
-	if (min_max)
+	if (index == UNCORE_INDEX_MAX_FREQ)
 		return sprintf(buf, "%u\n", max);
 
 	return sprintf(buf, "%u\n", min);
@@ -64,7 +64,7 @@ static ssize_t show_min_max_freq_khz(struct uncore_data *data,
 
 static ssize_t store_min_max_freq_khz(struct uncore_data *data,
 				      const char *buf, ssize_t count,
-				      int min_max)
+				      enum uncore_index index)
 {
 	unsigned int input;
 	int ret;
@@ -73,7 +73,7 @@ static ssize_t store_min_max_freq_khz(struct uncore_data *data,
 		return -EINVAL;
 
 	mutex_lock(&uncore_lock);
-	ret = uncore_write(data, input, min_max);
+	ret = uncore_write(data, input, index);
 	mutex_unlock(&uncore_lock);
 
 	if (ret)
@@ -125,11 +125,11 @@ static ssize_t show_perf_status_freq_khz(struct uncore_data *data, char *buf)
 		return show_perf_status_freq_khz(data, buf); \
 	}
 
-store_uncore_min_max(min_freq_khz, 0);
-store_uncore_min_max(max_freq_khz, 1);
+store_uncore_min_max(min_freq_khz, UNCORE_INDEX_MIN_FREQ);
+store_uncore_min_max(max_freq_khz, UNCORE_INDEX_MAX_FREQ);
 
-show_uncore_min_max(min_freq_khz, 0);
-show_uncore_min_max(max_freq_khz, 1);
+show_uncore_min_max(min_freq_khz, UNCORE_INDEX_MIN_FREQ);
+show_uncore_min_max(max_freq_khz, UNCORE_INDEX_MAX_FREQ);
 
 show_uncore_perf_status(current_freq_khz);
 
@@ -270,7 +270,8 @@ void uncore_freq_remove_die_entry(struct uncore_data *data)
 EXPORT_SYMBOL_NS_GPL(uncore_freq_remove_die_entry, INTEL_UNCORE_FREQUENCY);
 
 int uncore_freq_common_init(int (*read_control_freq)(struct uncore_data *data, unsigned int *min, unsigned int *max),
-			     int (*write_control_freq)(struct uncore_data *data, unsigned int input, unsigned int set_max),
+			     int (*write_control_freq)(struct uncore_data *data, unsigned int input,
+						       enum uncore_index index),
 			     int (*read_freq)(struct uncore_data *data, unsigned int *freq))
 {
 	mutex_lock(&uncore_lock);
diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.h b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.h
index 0e5bf507e555..c6ceeda3f2bf 100644
--- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.h
+++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.h
@@ -66,8 +66,14 @@ struct uncore_data {
 
 #define UNCORE_DOMAIN_ID_INVALID	-1
 
+enum uncore_index {
+	UNCORE_INDEX_MIN_FREQ,
+	UNCORE_INDEX_MAX_FREQ,
+};
+
 int uncore_freq_common_init(int (*read_control_freq)(struct uncore_data *data, unsigned int *min, unsigned int *max),
-			     int (*write_control_freq)(struct uncore_data *data, unsigned int input, unsigned int min_max),
+			     int (*write_control_freq)(struct uncore_data *data, unsigned int input,
+						       enum uncore_index index),
 			     int (*uncore_read_freq)(struct uncore_data *data, unsigned int *freq));
 void uncore_freq_common_exit(void);
 int uncore_freq_add_entry(struct uncore_data *data, int cpu);
diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
index b58294498921..c8e8c8087812 100644
--- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
+++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
@@ -130,13 +130,13 @@ static int uncore_read_control_freq(struct uncore_data *data, unsigned int *min,
 
 /* Helper function to write MMIO offset for max/min control frequency */
 static void write_control_freq(struct tpmi_uncore_cluster_info *cluster_info, unsigned int input,
-			      unsigned int min_max)
+			      unsigned int index)
 {
 	u64 control;
 
 	control = readq(cluster_info->cluster_base + UNCORE_CONTROL_INDEX);
 
-	if (min_max) {
+	if (index == UNCORE_INDEX_MAX_FREQ) {
 		control &= ~UNCORE_MAX_RATIO_MASK;
 		control |= FIELD_PREP(UNCORE_MAX_RATIO_MASK, input);
 	} else {
@@ -149,7 +149,7 @@ static void write_control_freq(struct tpmi_uncore_cluster_info *cluster_info, un
 
 /* Callback for sysfs write for max/min frequencies. Called under mutex locks */
 static int uncore_write_control_freq(struct uncore_data *data, unsigned int input,
-				     unsigned int min_max)
+				     enum uncore_index index)
 {
 	struct tpmi_uncore_cluster_info *cluster_info;
 	struct tpmi_uncore_struct *uncore_root;
@@ -174,10 +174,10 @@ static int uncore_write_control_freq(struct uncore_data *data, unsigned int inpu
 
 			for (j = 0; j < uncore_root->pd_info[i].cluster_count; ++j)
 				write_control_freq(&uncore_root->pd_info[i].cluster_infos[j],
-						  input, min_max);
+						  input, index);
 		}
 
-		if (min_max)
+		if (index == UNCORE_INDEX_MAX_FREQ)
 			uncore_root->max_ratio = input;
 		else
 			uncore_root->min_ratio = input;
@@ -185,13 +185,15 @@ static int uncore_write_control_freq(struct uncore_data *data, unsigned int inpu
 		return 0;
 	}
 
-	if (min_max && uncore_root->max_ratio && uncore_root->max_ratio < input)
+	if (index == UNCORE_INDEX_MAX_FREQ && uncore_root->max_ratio &&
+	    uncore_root->max_ratio < input)
 		return -EINVAL;
 
-	if (!min_max && uncore_root->min_ratio && uncore_root->min_ratio > input)
+	if (index == UNCORE_INDEX_MIN_FREQ && uncore_root->min_ratio &&
+	    uncore_root->min_ratio > input)
 		return -EINVAL;
 
-	write_control_freq(cluster_info, input, min_max);
+	write_control_freq(cluster_info, input, index);
 
 	return 0;
 }
diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency.c b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency.c
index d3fdae695bbd..18ebd95080ed 100644
--- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency.c
+++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency.c
@@ -62,7 +62,7 @@ static int uncore_read_control_freq(struct uncore_data *data, unsigned int *min,
 }
 
 static int uncore_write_control_freq(struct uncore_data *data, unsigned int input,
-				     unsigned int min_max)
+				     enum uncore_index index)
 {
 	int ret;
 	u64 cap;
@@ -78,7 +78,7 @@ static int uncore_write_control_freq(struct uncore_data *data, unsigned int inpu
 	if (ret)
 		return ret;
 
-	if (min_max) {
+	if (index == UNCORE_INDEX_MAX_FREQ) {
 		cap &= ~UNCORE_MAX_RATIO_MASK;
 		cap |= FIELD_PREP(UNCORE_MAX_RATIO_MASK, input);
 	} else  {
-- 
2.43.0

[PATCH 4/7] platform/x86/intel-uncore-freq: Use uncore_index with read_control_freq

[Tero Kristo]

Use the enumerated index for selecting the uncore driver parameter to
read, instead of reading everything. This is done in preparation to
expand the API to access more parameters later.

No functional change intended.

Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
---
 .../uncore-frequency-common.c                 | 23 ++++++------
 .../uncore-frequency-common.h                 |  9 +++--
 .../uncore-frequency/uncore-frequency-tpmi.c  | 37 ++++++++++++-------
 .../intel/uncore-frequency/uncore-frequency.c | 10 +++--
 4 files changed, 45 insertions(+), 34 deletions(-)

diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c
index 7daca46f9235..8d9817d70e27 100644
--- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c
+++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c
@@ -19,7 +19,7 @@ static int uncore_instance_count;
 static DEFINE_IDA(intel_uncore_ida);
 
 /* callbacks for actual HW read/write */
-static int (*uncore_read)(struct uncore_data *data, unsigned int *min, unsigned int *max);
+static int (*uncore_read)(struct uncore_data *data, unsigned int *value, enum uncore_index index);
 static int (*uncore_write)(struct uncore_data *data, unsigned int input, enum uncore_index index);
 static int (*uncore_read_freq)(struct uncore_data *data, unsigned int *freq);
 
@@ -47,19 +47,16 @@ static ssize_t show_package_id(struct kobject *kobj, struct kobj_attribute *attr
 static ssize_t show_min_max_freq_khz(struct uncore_data *data,
 				      char *buf, enum uncore_index index)
 {
-	unsigned int min, max;
+	unsigned int value;
 	int ret;
 
 	mutex_lock(&uncore_lock);
-	ret = uncore_read(data, &min, &max);
+	ret = uncore_read(data, &value, index);
 	mutex_unlock(&uncore_lock);
 	if (ret)
 		return ret;
 
-	if (index == UNCORE_INDEX_MAX_FREQ)
-		return sprintf(buf, "%u\n", max);
-
-	return sprintf(buf, "%u\n", min);
+	return sprintf(buf, "%u\n", value);
 }
 
 static ssize_t store_min_max_freq_khz(struct uncore_data *data,
@@ -238,7 +235,8 @@ int uncore_freq_add_entry(struct uncore_data *data, int cpu)
 		sprintf(data->name, "package_%02d_die_%02d", data->package_id, data->die_id);
 	}
 
-	uncore_read(data, &data->initial_min_freq_khz, &data->initial_max_freq_khz);
+	uncore_read(data, &data->initial_min_freq_khz, UNCORE_INDEX_MIN_FREQ);
+	uncore_read(data, &data->initial_max_freq_khz, UNCORE_INDEX_MAX_FREQ);
 
 	ret = create_attr_group(data, data->name);
 	if (ret) {
@@ -269,10 +267,11 @@ void uncore_freq_remove_die_entry(struct uncore_data *data)
 }
 EXPORT_SYMBOL_NS_GPL(uncore_freq_remove_die_entry, INTEL_UNCORE_FREQUENCY);
 
-int uncore_freq_common_init(int (*read_control_freq)(struct uncore_data *data, unsigned int *min, unsigned int *max),
-			     int (*write_control_freq)(struct uncore_data *data, unsigned int input,
-						       enum uncore_index index),
-			     int (*read_freq)(struct uncore_data *data, unsigned int *freq))
+int uncore_freq_common_init(int (*read_control_freq)(struct uncore_data *data, unsigned int *value,
+						     enum uncore_index index),
+			    int (*write_control_freq)(struct uncore_data *data, unsigned int input,
+						      enum uncore_index index),
+			    int (*read_freq)(struct uncore_data *data, unsigned int *freq))
 {
 	mutex_lock(&uncore_lock);
 
diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.h b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.h
index c6ceeda3f2bf..2d98931cd8e2 100644
--- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.h
+++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.h
@@ -71,10 +71,11 @@ enum uncore_index {
 	UNCORE_INDEX_MAX_FREQ,
 };
 
-int uncore_freq_common_init(int (*read_control_freq)(struct uncore_data *data, unsigned int *min, unsigned int *max),
-			     int (*write_control_freq)(struct uncore_data *data, unsigned int input,
-						       enum uncore_index index),
-			     int (*uncore_read_freq)(struct uncore_data *data, unsigned int *freq));
+int uncore_freq_common_init(int (*read_control_freq)(struct uncore_data *data, unsigned int *value,
+						     enum uncore_index index),
+			    int (*write_control_freq)(struct uncore_data *data, unsigned int input,
+						      enum uncore_index index),
+			    int (*uncore_read_freq)(struct uncore_data *data, unsigned int *freq));
 void uncore_freq_common_exit(void);
 int uncore_freq_add_entry(struct uncore_data *data, int cpu);
 void uncore_freq_remove_die_entry(struct uncore_data *data);
diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
index c8e8c8087812..e6ec66eb2f03 100644
--- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
+++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
@@ -78,20 +78,22 @@ struct tpmi_uncore_struct {
 
 /* Helper function to read MMIO offset for max/min control frequency */
 static void read_control_freq(struct tpmi_uncore_cluster_info *cluster_info,
-			     unsigned int *min, unsigned int *max)
+			     unsigned int *value, enum uncore_index index)
 {
 	u64 control;
 
 	control = readq(cluster_info->cluster_base + UNCORE_CONTROL_INDEX);
-	*max = FIELD_GET(UNCORE_MAX_RATIO_MASK, control) * UNCORE_FREQ_KHZ_MULTIPLIER;
-	*min = FIELD_GET(UNCORE_MIN_RATIO_MASK, control) * UNCORE_FREQ_KHZ_MULTIPLIER;
+	if (index == UNCORE_INDEX_MAX_FREQ)
+		*value = FIELD_GET(UNCORE_MAX_RATIO_MASK, control) * UNCORE_FREQ_KHZ_MULTIPLIER;
+	else
+		*value = FIELD_GET(UNCORE_MIN_RATIO_MASK, control) * UNCORE_FREQ_KHZ_MULTIPLIER;
 }
 
 #define UNCORE_MAX_RATIO	FIELD_MAX(UNCORE_MAX_RATIO_MASK)
 
 /* Callback for sysfs read for max/min frequencies. Called under mutex locks */
-static int uncore_read_control_freq(struct uncore_data *data, unsigned int *min,
-				    unsigned int *max)
+static int uncore_read_control_freq(struct uncore_data *data, unsigned int *value,
+				    enum uncore_index index)
 {
 	struct tpmi_uncore_cluster_info *cluster_info;
 
@@ -99,10 +101,11 @@ static int uncore_read_control_freq(struct uncore_data *data, unsigned int *min,
 
 	if (cluster_info->root_domain) {
 		struct tpmi_uncore_struct *uncore_root = cluster_info->uncore_root;
-		int i, _min = 0, _max = 0;
+		unsigned int _min, _max, _value;
+		int i;
 
-		*min = UNCORE_MAX_RATIO * UNCORE_FREQ_KHZ_MULTIPLIER;
-		*max = 0;
+		_min = UNCORE_MAX_RATIO * UNCORE_FREQ_KHZ_MULTIPLIER;
+		_max = 0;
 
 		/*
 		 * Get the max/min by looking at each cluster. Get the lowest
@@ -113,17 +116,23 @@ static int uncore_read_control_freq(struct uncore_data *data, unsigned int *min,
 
 			for (j = 0; j < uncore_root->pd_info[i].cluster_count; ++j) {
 				read_control_freq(&uncore_root->pd_info[i].cluster_infos[j],
-						  &_min, &_max);
-				if (*min > _min)
-					*min = _min;
-				if (*max < _max)
-					*max = _max;
+						  &_value, index);
+				if (_value < _min)
+					_min = _value;
+				if (_value > _max)
+					_max = _value;
 			}
 		}
+
+		if (index == UNCORE_INDEX_MIN_FREQ)
+			*value = _min;
+		else
+			*value = _max;
+
 		return 0;
 	}
 
-	read_control_freq(cluster_info, min, max);
+	read_control_freq(cluster_info, value, index);
 
 	return 0;
 }
diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency.c b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency.c
index 18ebd95080ed..433ddba4608e 100644
--- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency.c
+++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency.c
@@ -42,8 +42,8 @@ static enum cpuhp_state uncore_hp_state __read_mostly;
 
 #define UNCORE_CURRENT_RATIO_MASK	GENMASK_ULL(6, 0)
 
-static int uncore_read_control_freq(struct uncore_data *data, unsigned int *min,
-				    unsigned int *max)
+static int uncore_read_control_freq(struct uncore_data *data, unsigned int *value,
+				    enum uncore_index index)
 {
 	u64 cap;
 	int ret;
@@ -55,8 +55,10 @@ static int uncore_read_control_freq(struct uncore_data *data, unsigned int *min,
 	if (ret)
 		return ret;
 
-	*max = FIELD_GET(UNCORE_MAX_RATIO_MASK, cap) * UNCORE_FREQ_KHZ_MULTIPLIER;
-	*min = FIELD_GET(UNCORE_MIN_RATIO_MASK, cap) * UNCORE_FREQ_KHZ_MULTIPLIER;
+	if (index == UNCORE_INDEX_MAX_FREQ)
+		*value = FIELD_GET(UNCORE_MAX_RATIO_MASK, cap) * UNCORE_FREQ_KHZ_MULTIPLIER;
+	else
+		*value = FIELD_GET(UNCORE_MIN_RATIO_MASK, cap) * UNCORE_FREQ_KHZ_MULTIPLIER;
 
 	return 0;
 }
-- 
2.43.0

[PATCH 5/7] platform/x86/intel-uncore-freq: Get rid of uncore_read_freq driver API

[Tero Kristo]

Get rid of uncore_read_freq driver API. Instead, add a new entry to the
enumerated read interface and use this.

No functional change intended.

Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
---
 .../uncore-frequency-common.c                 | 19 ++++++--------
 .../uncore-frequency-common.h                 | 10 ++++----
 .../uncore-frequency/uncore-frequency-tpmi.c  | 25 ++++++++++++++++---
 .../intel/uncore-frequency/uncore-frequency.c | 20 +++++++++++++--
 4 files changed, 52 insertions(+), 22 deletions(-)

diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c
index 8d9817d70e27..cfbff057458d 100644
--- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c
+++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c
@@ -21,7 +21,6 @@ static DEFINE_IDA(intel_uncore_ida);
 /* callbacks for actual HW read/write */
 static int (*uncore_read)(struct uncore_data *data, unsigned int *value, enum uncore_index index);
 static int (*uncore_write)(struct uncore_data *data, unsigned int input, enum uncore_index index);
-static int (*uncore_read_freq)(struct uncore_data *data, unsigned int *freq);
 
 static ssize_t show_domain_id(struct kobject *kobj, struct kobj_attribute *attr, char *buf)
 {
@@ -85,7 +84,7 @@ static ssize_t show_perf_status_freq_khz(struct uncore_data *data, char *buf)
 	int ret;
 
 	mutex_lock(&uncore_lock);
-	ret = uncore_read_freq(data, &freq);
+	ret = uncore_read(data, &freq, UNCORE_INDEX_CURRENT_FREQ);
 	mutex_unlock(&uncore_lock);
 	if (ret)
 		return ret;
@@ -195,7 +194,7 @@ static int create_attr_group(struct uncore_data *data, char *name)
 	data->uncore_attrs[index++] = &data->initial_min_freq_khz_kobj_attr.attr;
 	data->uncore_attrs[index++] = &data->initial_max_freq_khz_kobj_attr.attr;
 
-	ret = uncore_read_freq(data, &freq);
+	ret = uncore_read(data, &freq, UNCORE_INDEX_CURRENT_FREQ);
 	if (!ret)
 		data->uncore_attrs[index++] = &data->current_freq_khz_kobj_attr.attr;
 
@@ -267,17 +266,15 @@ void uncore_freq_remove_die_entry(struct uncore_data *data)
 }
 EXPORT_SYMBOL_NS_GPL(uncore_freq_remove_die_entry, INTEL_UNCORE_FREQUENCY);
 
-int uncore_freq_common_init(int (*read_control_freq)(struct uncore_data *data, unsigned int *value,
-						     enum uncore_index index),
-			    int (*write_control_freq)(struct uncore_data *data, unsigned int input,
-						      enum uncore_index index),
-			    int (*read_freq)(struct uncore_data *data, unsigned int *freq))
+int uncore_freq_common_init(int (*read)(struct uncore_data *data, unsigned int *value,
+					enum uncore_index index),
+			    int (*write)(struct uncore_data *data, unsigned int input,
+					 enum uncore_index index))
 {
 	mutex_lock(&uncore_lock);
 
-	uncore_read = read_control_freq;
-	uncore_write = write_control_freq;
-	uncore_read_freq = read_freq;
+	uncore_read = read;
+	uncore_write = write;
 
 	if (!uncore_root_kobj) {
 		struct device *dev_root = bus_get_dev_root(&cpu_subsys);
diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.h b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.h
index 2d98931cd8e2..4c245b945e4e 100644
--- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.h
+++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.h
@@ -69,13 +69,13 @@ struct uncore_data {
 enum uncore_index {
 	UNCORE_INDEX_MIN_FREQ,
 	UNCORE_INDEX_MAX_FREQ,
+	UNCORE_INDEX_CURRENT_FREQ,
 };
 
-int uncore_freq_common_init(int (*read_control_freq)(struct uncore_data *data, unsigned int *value,
-						     enum uncore_index index),
-			    int (*write_control_freq)(struct uncore_data *data, unsigned int input,
-						      enum uncore_index index),
-			    int (*uncore_read_freq)(struct uncore_data *data, unsigned int *freq));
+int uncore_freq_common_init(int (*read)(struct uncore_data *data, unsigned int *value,
+					enum uncore_index index),
+			    int (*write)(struct uncore_data *data, unsigned int input,
+					 enum uncore_index index));
 void uncore_freq_common_exit(void);
 int uncore_freq_add_entry(struct uncore_data *data, int cpu);
 void uncore_freq_remove_die_entry(struct uncore_data *data);
diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
index e6ec66eb2f03..8d8260781cf0 100644
--- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
+++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-tpmi.c
@@ -91,7 +91,7 @@ static void read_control_freq(struct tpmi_uncore_cluster_info *cluster_info,
 
 #define UNCORE_MAX_RATIO	FIELD_MAX(UNCORE_MAX_RATIO_MASK)
 
-/* Callback for sysfs read for max/min frequencies. Called under mutex locks */
+/* Helper for sysfs read for max/min frequencies. Called under mutex locks */
 static int uncore_read_control_freq(struct uncore_data *data, unsigned int *value,
 				    enum uncore_index index)
 {
@@ -207,7 +207,7 @@ static int uncore_write_control_freq(struct uncore_data *data, unsigned int inpu
 	return 0;
 }
 
-/* Callback for sysfs read for the current uncore frequency. Called under mutex locks */
+/* Helper for sysfs read for the current uncore frequency. Called under mutex locks */
 static int uncore_read_freq(struct uncore_data *data, unsigned int *freq)
 {
 	struct tpmi_uncore_cluster_info *cluster_info;
@@ -223,6 +223,24 @@ static int uncore_read_freq(struct uncore_data *data, unsigned int *freq)
 	return 0;
 }
 
+/* Callback for sysfs read for TPMI uncore values. Called under mutex locks. */
+static int uncore_read(struct uncore_data *data, unsigned int *value, enum uncore_index index)
+{
+	switch (index) {
+	case UNCORE_INDEX_MIN_FREQ:
+	case UNCORE_INDEX_MAX_FREQ:
+		return uncore_read_control_freq(data, value, index);
+
+	case UNCORE_INDEX_CURRENT_FREQ:
+		return uncore_read_freq(data, value);
+
+	default:
+		break;
+	}
+
+	return -EOPNOTSUPP;
+}
+
 static void remove_cluster_entries(struct tpmi_uncore_struct *tpmi_uncore)
 {
 	int i;
@@ -273,8 +291,7 @@ static int uncore_probe(struct auxiliary_device *auxdev, const struct auxiliary_
 		return -EINVAL;
 
 	/* Register callbacks to uncore core */
-	ret = uncore_freq_common_init(uncore_read_control_freq, uncore_write_control_freq,
-				      uncore_read_freq);
+	ret = uncore_freq_common_init(uncore_read, uncore_write_control_freq);
 	if (ret)
 		return ret;
 
diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency.c b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency.c
index 433ddba4608e..f31a8912bb1f 100644
--- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency.c
+++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency.c
@@ -114,6 +114,23 @@ static int uncore_read_freq(struct uncore_data *data, unsigned int *freq)
 	return 0;
 }
 
+static int uncore_read(struct uncore_data *data, unsigned int *value, enum uncore_index index)
+{
+	switch (index) {
+	case UNCORE_INDEX_MIN_FREQ:
+	case UNCORE_INDEX_MAX_FREQ:
+		return uncore_read_control_freq(data, value, index);
+
+	case UNCORE_INDEX_CURRENT_FREQ:
+		return uncore_read_freq(data, value);
+
+	default:
+		break;
+	}
+
+	return -EOPNOTSUPP;
+}
+
 /* Caller provides protection */
 static struct uncore_data *uncore_get_instance(unsigned int cpu)
 {
@@ -256,8 +273,7 @@ static int __init intel_uncore_init(void)
 	if (!uncore_instances)
 		return -ENOMEM;
 
-	ret = uncore_freq_common_init(uncore_read_control_freq, uncore_write_control_freq,
-				      uncore_read_freq);
+	ret = uncore_freq_common_init(uncore_read, uncore_write_control_freq);
 	if (ret)
 		goto err_free;
 
-- 
2.43.0

[PATCH 6/7] platform/x86/intel-uncore-freq: Rename the sysfs helper macro names

[Tero Kristo]

The macros to create sysfs entries are going to be used for other
attributes in addition to current min/max frequencies only, so rename
these to be more generic.

No functional change intended.

Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
---
 .../uncore-frequency-common.c                 | 25 ++++++++-----------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c
index cfbff057458d..9356a0f322e6 100644
--- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c
+++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c
@@ -43,8 +43,7 @@ static ssize_t show_package_id(struct kobject *kobj, struct kobj_attribute *attr
 	return sprintf(buf, "%u\n", data->package_id);
 }
 
-static ssize_t show_min_max_freq_khz(struct uncore_data *data,
-				      char *buf, enum uncore_index index)
+static ssize_t show_attr(struct uncore_data *data, char *buf, enum uncore_index index)
 {
 	unsigned int value;
 	int ret;
@@ -58,9 +57,8 @@ static ssize_t show_min_max_freq_khz(struct uncore_data *data,
 	return sprintf(buf, "%u\n", value);
 }
 
-static ssize_t store_min_max_freq_khz(struct uncore_data *data,
-				      const char *buf, ssize_t count,
-				      enum uncore_index index)
+static ssize_t store_attr(struct uncore_data *data, const char *buf, ssize_t count,
+			  enum uncore_index index)
 {
 	unsigned int input;
 	int ret;
@@ -92,24 +90,23 @@ static ssize_t show_perf_status_freq_khz(struct uncore_data *data, char *buf)
 	return sprintf(buf, "%u\n", freq);
 }
 
-#define store_uncore_min_max(name, min_max)				\
+#define store_uncore_attr(name, index)					\
 	static ssize_t store_##name(struct kobject *kobj,		\
 				     struct kobj_attribute *attr,	\
 				     const char *buf, size_t count)	\
 	{								\
 		struct uncore_data *data = container_of(attr, struct uncore_data, name##_kobj_attr);\
 									\
-		return store_min_max_freq_khz(data, buf, count,	\
-					      min_max);		\
+		return store_attr(data, buf, count, index);		\
 	}
 
-#define show_uncore_min_max(name, min_max)				\
+#define show_uncore_attr(name, index)					\
 	static ssize_t show_##name(struct kobject *kobj,		\
 				    struct kobj_attribute *attr, char *buf)\
 	{                                                               \
 		struct uncore_data *data = container_of(attr, struct uncore_data, name##_kobj_attr);\
 									\
-		return show_min_max_freq_khz(data, buf, min_max);	\
+		return show_attr(data, buf, index);			\
 	}
 
 #define show_uncore_perf_status(name)					\
@@ -121,11 +118,11 @@ static ssize_t show_perf_status_freq_khz(struct uncore_data *data, char *buf)
 		return show_perf_status_freq_khz(data, buf); \
 	}
 
-store_uncore_min_max(min_freq_khz, UNCORE_INDEX_MIN_FREQ);
-store_uncore_min_max(max_freq_khz, UNCORE_INDEX_MAX_FREQ);
+store_uncore_attr(min_freq_khz, UNCORE_INDEX_MIN_FREQ);
+store_uncore_attr(max_freq_khz, UNCORE_INDEX_MAX_FREQ);
 
-show_uncore_min_max(min_freq_khz, UNCORE_INDEX_MIN_FREQ);
-show_uncore_min_max(max_freq_khz, UNCORE_INDEX_MAX_FREQ);
+show_uncore_attr(min_freq_khz, UNCORE_INDEX_MIN_FREQ);
+show_uncore_attr(max_freq_khz, UNCORE_INDEX_MAX_FREQ);
 
 show_uncore_perf_status(current_freq_khz);
 
-- 
2.43.0

[PATCH 7/7] platform/x86/intel-uncore-freq: Use generic helpers for current frequency

[Tero Kristo]

Use the generic sysfs helpers for reading the current frequency also,
and remove the custom ones.

No functional change intended.

Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
---
 .../uncore-frequency-common.c                 | 25 +------------------
 1 file changed, 1 insertion(+), 24 deletions(-)

diff --git a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c
index 9356a0f322e6..4e880585cbe4 100644
--- a/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c
+++ b/drivers/platform/x86/intel/uncore-frequency/uncore-frequency-common.c
@@ -76,20 +76,6 @@ static ssize_t store_attr(struct uncore_data *data, const char *buf, ssize_t cou
 	return count;
 }
 
-static ssize_t show_perf_status_freq_khz(struct uncore_data *data, char *buf)
-{
-	unsigned int freq;
-	int ret;
-
-	mutex_lock(&uncore_lock);
-	ret = uncore_read(data, &freq, UNCORE_INDEX_CURRENT_FREQ);
-	mutex_unlock(&uncore_lock);
-	if (ret)
-		return ret;
-
-	return sprintf(buf, "%u\n", freq);
-}
-
 #define store_uncore_attr(name, index)					\
 	static ssize_t store_##name(struct kobject *kobj,		\
 				     struct kobj_attribute *attr,	\
@@ -109,22 +95,13 @@ static ssize_t show_perf_status_freq_khz(struct uncore_data *data, char *buf)
 		return show_attr(data, buf, index);			\
 	}
 
-#define show_uncore_perf_status(name)					\
-	static ssize_t show_##name(struct kobject *kobj,		\
-				   struct kobj_attribute *attr, char *buf)\
-	{                                                               \
-		struct uncore_data *data = container_of(attr, struct uncore_data, name##_kobj_attr);\
-									\
-		return show_perf_status_freq_khz(data, buf); \
-	}
-
 store_uncore_attr(min_freq_khz, UNCORE_INDEX_MIN_FREQ);
 store_uncore_attr(max_freq_khz, UNCORE_INDEX_MAX_FREQ);
 
 show_uncore_attr(min_freq_khz, UNCORE_INDEX_MIN_FREQ);
 show_uncore_attr(max_freq_khz, UNCORE_INDEX_MAX_FREQ);
 
-show_uncore_perf_status(current_freq_khz);
+show_uncore_attr(current_freq_khz, UNCORE_INDEX_CURRENT_FREQ);
 
 #define show_uncore_data(member_name)					\
 	static ssize_t show_##member_name(struct kobject *kobj,	\
-- 
2.43.0

Re: [PATCH 0/7] platform/x86/inte-uncore-freq: Cleanups

[srinivas pandruvada]

On Mon, 2024-06-17 at 09:04 +0300, Tero Kristo wrote:
> Hi,
> 
> This series introduces some cleanups for the code and driver level
> APIs.
> The purpose is to improve the maintainability of the code and make it
> easier to add new features in future.
> 
> No functional changes expected from any of the patches in this
> series.

    Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

Thanks,
Srinivas

> 

> -Tero
> 

Re: [PATCH 0/7] platform/x86/inte-uncore-freq: Cleanups

[Ilpo Järvinen]

On Mon, 17 Jun 2024, Tero Kristo wrote:

> Hi,
> 
> This series introduces some cleanups for the code and driver level APIs.
> The purpose is to improve the maintainability of the code and make it
> easier to add new features in future.
> 
> No functional changes expected from any of the patches in this series.

Nice cleanups, thank you.

I've applied these into review-ilpo branch with the following 
modifications:

- I fixed my name on Reviewed-by line. Please don't corrupt my name if
  handwriting a tag instead of copying it. While I don't care that much
  if my name is not always written 100% correctly, an incorrect rev-by 
  line causes my tools to add the correct form one so the rev-by line 
  gets duplicated (luckily I was paying attention to the non-critical 
  part of the output of my tool this time around so I caught it).

- I renamed _min, _max, and _value variables to min, max, and v, 
  respectively, because the reason for using underscores got removed by
  the change.

-- 
 i.