[PATCH 0/7] platform/x86/amd/pmf: Updates to amd-pmf driver

[PATCH 0/7] platform/x86/amd/pmf: Updates to amd-pmf driver

[Shyam Sundar S K]

This patch series includes:
-Add support to get sbios requests and static slider as per new APMF
specification
-Add support for the APTS (AMD Performance & Thermal State) method
-Disable debugfs support for 1AH family series
-Add support for heartbeat notify event to OEM BIOS

Shyam Sundar S K (7):
  platform/x86/amd/pmf: Differentiate PMF ACPI versions
  platform/x86/amd/pmf: Disable debugfs support for querying power
    thermals
  platform/x86/amd/pmf: Add support to get sbios requests in PMF driver
  platform/x86/amd/pmf: Add support to notify sbios heart beat event
  platform/x86/amd/pmf: Add support to get APTS index numbers for static
    slider
  platform/x86/amd/pmf: Add support to get sps default APTS index values
  platform/x86/amd/pmf: Update sps power thermals according to the
    platform-profiles

 drivers/platform/x86/amd/pmf/acpi.c | 138 +++++++++++++++++++++++++-
 drivers/platform/x86/amd/pmf/core.c |  15 ++-
 drivers/platform/x86/amd/pmf/pmf.h  |  86 ++++++++++++++++-
 drivers/platform/x86/amd/pmf/sps.c  | 144 +++++++++++++++++++++++++++-
 4 files changed, 375 insertions(+), 8 deletions(-)

-- 
2.25.1

[PATCH 1/7] platform/x86/amd/pmf: Differentiate PMF ACPI versions

[Shyam Sundar S K]

For family 1AH, certain PMF features have been enhanced - leading to a
newer APMF (AMD PMF) spec (BIOS and PMF driver interface) called v2.

This information would be fed into the if_version field of the
verify_interface method of the APMF call from the BIOS.

Use this information to store the version number to differentiate
between v1 or v2 and also store the information into the PMF private
data structure, as this information would be required for further code
branching to support the latest silicon.

Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/platform/x86/amd/pmf/acpi.c | 6 ++++--
 drivers/platform/x86/amd/pmf/pmf.h  | 1 +
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
index f2eb07ef855a..1f287a147c57 100644
--- a/drivers/platform/x86/amd/pmf/acpi.c
+++ b/drivers/platform/x86/amd/pmf/acpi.c
@@ -218,8 +218,10 @@ static int apmf_if_verify_interface(struct amd_pmf_dev *pdev)
 		return err;
 
 	pdev->supported_func = output.supported_functions;
-	dev_dbg(pdev->dev, "supported functions:0x%x notifications:0x%x\n",
-		output.supported_functions, output.notification_mask);
+	dev_dbg(pdev->dev, "supported functions:0x%x notifications:0x%x version:%u\n",
+		output.supported_functions, output.notification_mask, output.version);
+
+	pdev->pmf_if_version = output.version;
 
 	return 0;
 }
diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
index 66cae1cca73c..e51ac981af73 100644
--- a/drivers/platform/x86/amd/pmf/pmf.h
+++ b/drivers/platform/x86/amd/pmf/pmf.h
@@ -231,6 +231,7 @@ struct amd_pmf_dev {
 	u64 policy_addr;
 	void *policy_base;
 	bool smart_pc_enabled;
+	u16 pmf_if_version;
 };
 
 struct apmf_sps_prop_granular {
-- 
2.25.1

[PATCH 2/7] platform/x86/amd/pmf: Disable debugfs support for querying power thermals

[Shyam Sundar S K]

The GET interface to receive the active power thermal information from
the PMFW has been depricated. Hence drop the debugfs support for the
newer platforms.

Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/platform/x86/amd/pmf/core.c | 5 +++--
 drivers/platform/x86/amd/pmf/pmf.h  | 2 ++
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
index 4f734e049f4a..2230bc2b1f59 100644
--- a/drivers/platform/x86/amd/pmf/core.c
+++ b/drivers/platform/x86/amd/pmf/core.c
@@ -113,8 +113,9 @@ static void amd_pmf_dbgfs_unregister(struct amd_pmf_dev *dev)
 static void amd_pmf_dbgfs_register(struct amd_pmf_dev *dev)
 {
 	dev->dbgfs_dir = debugfs_create_dir("amd_pmf", NULL);
-	debugfs_create_file("current_power_limits", 0644, dev->dbgfs_dir, dev,
-			    &current_power_limits_fops);
+	if (dev->pmf_if_version == PMF_IF_V1)
+		debugfs_create_file("current_power_limits", 0644, dev->dbgfs_dir, dev,
+				    &current_power_limits_fops);
 }
 
 int amd_pmf_get_power_source(void)
diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
index e51ac981af73..4364af72a7a3 100644
--- a/drivers/platform/x86/amd/pmf/pmf.h
+++ b/drivers/platform/x86/amd/pmf/pmf.h
@@ -83,6 +83,8 @@
 #define TA_OUTPUT_RESERVED_MEM				906
 #define MAX_OPERATION_PARAMS					4
 
+#define PMF_IF_V1		1
+
 /* AMD PMF BIOS interfaces */
 struct apmf_verify_interface {
 	u16 size;
-- 
2.25.1

[PATCH 3/7] platform/x86/amd/pmf: Add support to get sbios requests in PMF driver

[Shyam Sundar S K]

Update the APMF function index 2 for family 1Ah, that gets the
information of  SBIOS requests (like the pending requests from BIOS,
custom notifications, updation of power limits etc).

Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/platform/x86/amd/pmf/acpi.c |  6 ++++++
 drivers/platform/x86/amd/pmf/pmf.h  | 13 +++++++++++++
 2 files changed, 19 insertions(+)

diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
index 1f287a147c57..1b2a099c0cef 100644
--- a/drivers/platform/x86/amd/pmf/acpi.c
+++ b/drivers/platform/x86/amd/pmf/acpi.c
@@ -166,6 +166,12 @@ int apmf_get_auto_mode_def(struct amd_pmf_dev *pdev, struct apmf_auto_mode *data
 	return apmf_if_call_store_buffer(pdev, APMF_FUNC_AUTO_MODE, data, sizeof(*data));
 }
 
+int apmf_get_sbios_requests_v2(struct amd_pmf_dev *pdev, struct apmf_sbios_req_v2 *req)
+{
+	return apmf_if_call_store_buffer(pdev, APMF_FUNC_SBIOS_REQUESTS,
+									req, sizeof(*req));
+}
+
 int apmf_get_sbios_requests(struct amd_pmf_dev *pdev, struct apmf_sbios_req *req)
 {
 	return apmf_if_call_store_buffer(pdev, APMF_FUNC_SBIOS_REQUESTS,
diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
index 4364af72a7a3..f11d2a348696 100644
--- a/drivers/platform/x86/amd/pmf/pmf.h
+++ b/drivers/platform/x86/amd/pmf/pmf.h
@@ -116,6 +116,18 @@ struct apmf_sbios_req {
 	u8 skin_temp_hs2;
 } __packed;
 
+struct apmf_sbios_req_v2 {
+	u16 size;
+	u32 pending_req;
+	u8 rsvd;
+	u32 update_ppt_pmf;
+	u32 update_ppt_pmf_apu_only;
+	u32 update_stt_min;
+	u8 update_stt_apu;
+	u8 update_stt_hs2;
+	u32 custom_policy[10];
+} __packed;
+
 struct apmf_fan_idx {
 	u16 size;
 	u8 fan_ctl_mode;
@@ -612,6 +624,7 @@ void amd_pmf_init_auto_mode(struct amd_pmf_dev *dev);
 void amd_pmf_deinit_auto_mode(struct amd_pmf_dev *dev);
 void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t time_elapsed_ms);
 int apmf_get_sbios_requests(struct amd_pmf_dev *pdev, struct apmf_sbios_req *req);
+int apmf_get_sbios_requests_v2(struct amd_pmf_dev *pdev, struct apmf_sbios_req_v2 *req);
 
 void amd_pmf_update_2_cql(struct amd_pmf_dev *dev, bool is_cql_event);
 int amd_pmf_reset_amt(struct amd_pmf_dev *dev);
-- 
2.25.1

[PATCH 4/7] platform/x86/amd/pmf: Add support to notify sbios heart beat event

[Shyam Sundar S K]

Add support for newer revision of the heart beat notify events.
This event is used to notify to the OEM BIOS on driver
load/unload/suspend/resume scenarios.

If OEM BIOS does not receive the heart beat event from PMF driver, OEM
BIOS shall conclude that PMF driver is no more active and BIOS will
update to the legacy system power thermals.

Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/platform/x86/amd/pmf/acpi.c | 42 +++++++++++++++++++++++++++--
 drivers/platform/x86/amd/pmf/core.c | 10 +++++++
 drivers/platform/x86/amd/pmf/pmf.h  | 17 ++++++++++++
 3 files changed, 67 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
index 1b2a099c0cef..0fc8ad0ac3e9 100644
--- a/drivers/platform/x86/amd/pmf/acpi.c
+++ b/drivers/platform/x86/amd/pmf/acpi.c
@@ -140,6 +140,44 @@ static void apmf_sbios_heartbeat_notify(struct work_struct *work)
 	kfree(info);
 }
 
+int amd_pmf_notify_sbios_heartbeat_event_v2(struct amd_pmf_dev *dev, u8 flag)
+{
+	struct sbios_hb_event_v2 args = { };
+	struct acpi_buffer params;
+	union acpi_object *info;
+
+	args.size = sizeof(args);
+
+	switch (flag) {
+	case ON_LOAD:
+		args.load = 1;
+		break;
+	case ON_UNLOAD:
+		args.unload = 1;
+		break;
+	case ON_SUSPEND:
+		args.suspend = 1;
+		break;
+	case ON_RESUME:
+		args.resume = 1;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	params.length = sizeof(args);
+	params.pointer = (void *)&args;
+
+	info = apmf_if_call(dev, APMF_FUNC_SBIOS_HEARTBEAT_V2, &params);
+	if (!info)
+		return -EIO;
+
+	dev_dbg(dev->dev, "Sending v2 heartbeat event to SBIOS\n");
+
+	kfree(info);
+	return 0;
+}
+
 int apmf_update_fan_idx(struct amd_pmf_dev *pdev, bool manual, u32 idx)
 {
 	union acpi_object *info;
@@ -328,7 +366,7 @@ void apmf_acpi_deinit(struct amd_pmf_dev *pmf_dev)
 {
 	acpi_handle ahandle = ACPI_HANDLE(pmf_dev->dev);
 
-	if (pmf_dev->hb_interval)
+	if (pmf_dev->hb_interval && pmf_dev->pmf_if_version == PMF_IF_V1)
 		cancel_delayed_work_sync(&pmf_dev->heart_beat);
 
 	if (is_apmf_func_supported(pmf_dev, APMF_FUNC_AUTO_MODE) &&
@@ -352,7 +390,7 @@ int apmf_acpi_init(struct amd_pmf_dev *pmf_dev)
 		goto out;
 	}
 
-	if (pmf_dev->hb_interval) {
+	if (pmf_dev->hb_interval && pmf_dev->pmf_if_version == PMF_IF_V1) {
 		/* send heartbeats only if the interval is not zero */
 		INIT_DELAYED_WORK(&pmf_dev->heart_beat, apmf_sbios_heartbeat_notify);
 		schedule_delayed_work(&pmf_dev->heart_beat, 0);
diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
index 2230bc2b1f59..5e52eb32ab2b 100644
--- a/drivers/platform/x86/amd/pmf/core.c
+++ b/drivers/platform/x86/amd/pmf/core.c
@@ -300,6 +300,9 @@ static int amd_pmf_suspend_handler(struct device *dev)
 	if (pdev->smart_pc_enabled)
 		cancel_delayed_work_sync(&pdev->pb_work);
 
+	if (is_apmf_func_supported(pdev, APMF_FUNC_SBIOS_HEARTBEAT_V2))
+		amd_pmf_notify_sbios_heartbeat_event_v2(pdev, ON_SUSPEND);
+
 	return 0;
 }
 
@@ -317,6 +320,9 @@ static int amd_pmf_resume_handler(struct device *dev)
 	if (pdev->smart_pc_enabled)
 		schedule_delayed_work(&pdev->pb_work, msecs_to_jiffies(2000));
 
+	if (is_apmf_func_supported(pdev, APMF_FUNC_SBIOS_HEARTBEAT_V2))
+		amd_pmf_notify_sbios_heartbeat_event_v2(pdev, ON_RESUME);
+
 	return 0;
 }
 
@@ -444,6 +450,8 @@ static int amd_pmf_probe(struct platform_device *pdev)
 	amd_pmf_dbgfs_register(dev);
 	amd_pmf_init_features(dev);
 	apmf_install_handler(dev);
+	if (is_apmf_func_supported(dev, APMF_FUNC_SBIOS_HEARTBEAT_V2))
+		amd_pmf_notify_sbios_heartbeat_event_v2(dev, ON_LOAD);
 
 	dev_info(dev->dev, "registered PMF device successfully\n");
 
@@ -455,6 +463,8 @@ static void amd_pmf_remove(struct platform_device *pdev)
 	struct amd_pmf_dev *dev = platform_get_drvdata(pdev);
 
 	amd_pmf_deinit_features(dev);
+	if (is_apmf_func_supported(dev, APMF_FUNC_SBIOS_HEARTBEAT_V2))
+		amd_pmf_notify_sbios_heartbeat_event_v2(dev, ON_UNLOAD);
 	apmf_acpi_deinit(dev);
 	amd_pmf_dbgfs_unregister(dev);
 	mutex_destroy(&dev->lock);
diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
index f11d2a348696..5cad11369697 100644
--- a/drivers/platform/x86/amd/pmf/pmf.h
+++ b/drivers/platform/x86/amd/pmf/pmf.h
@@ -30,6 +30,7 @@
 #define APMF_FUNC_STATIC_SLIDER_GRANULAR       9
 #define APMF_FUNC_DYN_SLIDER_AC				11
 #define APMF_FUNC_DYN_SLIDER_DC				12
+#define APMF_FUNC_SBIOS_HEARTBEAT_V2			16
 
 /* Message Definitions */
 #define SET_SPL				0x03 /* SPL: Sustained Power Limit */
@@ -85,6 +86,21 @@
 
 #define PMF_IF_V1		1
 
+struct sbios_hb_event_v2 {
+	u16 size;
+	u8 load;
+	u8 unload;
+	u8 suspend;
+	u8 resume;
+} __packed;
+
+enum sbios_hb_v2 {
+	ON_LOAD,
+	ON_UNLOAD,
+	ON_SUSPEND,
+	ON_RESUME,
+};
+
 /* AMD PMF BIOS interfaces */
 struct apmf_verify_interface {
 	u16 size;
@@ -600,6 +616,7 @@ int amd_pmf_get_power_source(void);
 int apmf_install_handler(struct amd_pmf_dev *pmf_dev);
 int apmf_os_power_slider_update(struct amd_pmf_dev *dev, u8 flag);
 int amd_pmf_set_dram_addr(struct amd_pmf_dev *dev, bool alloc_buffer);
+int amd_pmf_notify_sbios_heartbeat_event_v2(struct amd_pmf_dev *dev, u8 flag);
 
 /* SPS Layer */
 int amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf);
-- 
2.25.1

[PATCH 5/7] platform/x86/amd/pmf: Add support to get APTS index numbers for static slider

[Shyam Sundar S K]

APMF spec has a newer section called the APTS (AMD Performance and
Thermal State) information, where each slider/power mode is associated
with an index number.

Add support to get these indices for the Static Slider.

Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/platform/x86/amd/pmf/acpi.c | 10 +++++++
 drivers/platform/x86/amd/pmf/pmf.h  | 24 +++++++++++++++++
 drivers/platform/x86/amd/pmf/sps.c  | 42 ++++++++++++++++++++++++++++-
 3 files changed, 75 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
index 0fc8ad0ac3e9..28df45c058db 100644
--- a/drivers/platform/x86/amd/pmf/acpi.c
+++ b/drivers/platform/x86/amd/pmf/acpi.c
@@ -96,6 +96,16 @@ int is_apmf_func_supported(struct amd_pmf_dev *pdev, unsigned long index)
 	return !!(pdev->supported_func & BIT(index - 1));
 }
 
+int apmf_get_static_slider_granular_v2(struct amd_pmf_dev *pdev,
+				       struct apmf_static_slider_granular_output_v2 *data)
+{
+	if (!is_apmf_func_supported(pdev, APMF_FUNC_STATIC_SLIDER_GRANULAR))
+		return -EINVAL;
+
+	return apmf_if_call_store_buffer(pdev, APMF_FUNC_STATIC_SLIDER_GRANULAR,
+								data, sizeof(*data));
+}
+
 int apmf_get_static_slider_granular(struct amd_pmf_dev *pdev,
 				    struct apmf_static_slider_granular_output *data)
 {
diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
index 5cad11369697..b27e96aeac23 100644
--- a/drivers/platform/x86/amd/pmf/pmf.h
+++ b/drivers/platform/x86/amd/pmf/pmf.h
@@ -85,6 +85,7 @@
 #define MAX_OPERATION_PARAMS					4
 
 #define PMF_IF_V1		1
+#define PMF_IF_V2		2
 
 struct sbios_hb_event_v2 {
 	u16 size;
@@ -264,6 +265,17 @@ struct amd_pmf_dev {
 	u16 pmf_if_version;
 };
 
+struct apmf_sps_prop_granular_v2 {
+	u8 ac_best_perf;
+	u8 ac_balanced;
+	u8 ac_best_pwr_efficiency;
+	u8 ac_energy_saver;
+	u8 dc_best_perf;
+	u8 dc_balanced;
+	u8 dc_best_pwr_efficiency;
+	u8 dc_battery_saver;
+} __packed;
+
 struct apmf_sps_prop_granular {
 	u32 fppt;
 	u32 sppt;
@@ -285,6 +297,16 @@ struct amd_pmf_static_slider_granular {
 	struct apmf_sps_prop_granular prop[POWER_SOURCE_MAX][POWER_MODE_MAX];
 };
 
+struct apmf_static_slider_granular_output_v2 {
+	u16 size;
+	struct apmf_sps_prop_granular_v2 sps_idx;
+} __packed;
+
+struct amd_pmf_static_slider_granular_v2 {
+	u16 size;
+	struct apmf_sps_prop_granular_v2 sps_idx;
+};
+
 struct os_power_slider {
 	u16 size;
 	u8 slider_event;
@@ -634,6 +656,8 @@ const char *amd_pmf_source_as_str(unsigned int state);
 
 int apmf_update_fan_idx(struct amd_pmf_dev *pdev, bool manual, u32 idx);
 int amd_pmf_set_sps_power_limits(struct amd_pmf_dev *pmf);
+int apmf_get_static_slider_granular_v2(struct amd_pmf_dev *dev,
+				       struct apmf_static_slider_granular_output_v2 *data);
 
 /* Auto Mode Layer */
 int apmf_get_auto_mode_def(struct amd_pmf_dev *pdev, struct apmf_auto_mode *data);
diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c
index 33e23e25c8b1..dc4c7ccd4c43 100644
--- a/drivers/platform/x86/amd/pmf/sps.c
+++ b/drivers/platform/x86/amd/pmf/sps.c
@@ -10,6 +10,7 @@
 
 #include "pmf.h"
 
+static struct amd_pmf_static_slider_granular_v2 config_store_v2;
 static struct amd_pmf_static_slider_granular config_store;
 
 #ifdef CONFIG_AMD_PMF_DEBUG
@@ -63,10 +64,46 @@ static void amd_pmf_dump_sps_defaults(struct amd_pmf_static_slider_granular *dat
 
 	pr_debug("Static Slider Data - END\n");
 }
+
+static void amd_pmf_dump_sps_defaults_v2(struct amd_pmf_static_slider_granular_v2 *data)
+{
+	pr_debug("Static Slider APTS state index data - BEGIN");
+	pr_debug("size: %u\n", data->size);
+	pr_debug("ac_best_perf: %u\n", data->sps_idx.ac_best_perf);
+	pr_debug("ac_balanced: %u\n", data->sps_idx.ac_balanced);
+	pr_debug("ac_best_pwr_efficiency: %u\n", data->sps_idx.ac_best_pwr_efficiency);
+	pr_debug("ac_energy_saver: %u\n", data->sps_idx.ac_energy_saver);
+	pr_debug("dc_best_perf: %u\n", data->sps_idx.dc_best_perf);
+	pr_debug("dc_balanced: %u\n", data->sps_idx.dc_balanced);
+	pr_debug("dc_best_pwr_efficiency: %u\n", data->sps_idx.dc_best_pwr_efficiency);
+	pr_debug("dc_battery_saver: %u\n", data->sps_idx.dc_battery_saver);
+	pr_debug("Static Slider APTS state index data - END\n");
+}
 #else
 static void amd_pmf_dump_sps_defaults(struct amd_pmf_static_slider_granular *data) {}
+static void amd_pmf_dump_sps_defaults_v2(struct amd_pmf_static_slider_granular_v2 *data) {}
 #endif
 
+static void amd_pmf_load_defaults_sps_v2(struct amd_pmf_dev *dev)
+{
+	struct apmf_static_slider_granular_output_v2 output;
+
+	memset(&config_store_v2, 0, sizeof(config_store_v2));
+	apmf_get_static_slider_granular_v2(dev, &output);
+
+	config_store_v2.size = output.size;
+	config_store_v2.sps_idx.ac_best_perf = output.sps_idx.ac_best_perf;
+	config_store_v2.sps_idx.ac_balanced = output.sps_idx.ac_balanced;
+	config_store_v2.sps_idx.ac_best_pwr_efficiency = output.sps_idx.ac_best_pwr_efficiency;
+	config_store_v2.sps_idx.ac_energy_saver = output.sps_idx.ac_energy_saver;
+	config_store_v2.sps_idx.dc_best_perf = output.sps_idx.dc_best_perf;
+	config_store_v2.sps_idx.dc_balanced = output.sps_idx.dc_balanced;
+	config_store_v2.sps_idx.dc_best_pwr_efficiency = output.sps_idx.dc_best_pwr_efficiency;
+	config_store_v2.sps_idx.dc_battery_saver = output.sps_idx.dc_battery_saver;
+
+	amd_pmf_dump_sps_defaults_v2(&config_store_v2);
+}
+
 static void amd_pmf_load_defaults_sps(struct amd_pmf_dev *dev)
 {
 	struct apmf_static_slider_granular_output output;
@@ -256,7 +293,10 @@ int amd_pmf_init_sps(struct amd_pmf_dev *dev)
 	dev->current_profile = PLATFORM_PROFILE_BALANCED;
 
 	if (is_apmf_func_supported(dev, APMF_FUNC_STATIC_SLIDER_GRANULAR)) {
-		amd_pmf_load_defaults_sps(dev);
+		if (dev->pmf_if_version == PMF_IF_V2)
+			amd_pmf_load_defaults_sps_v2(dev);
+		else
+			amd_pmf_load_defaults_sps(dev);
 
 		/* update SPS balanced power mode thermals */
 		amd_pmf_set_sps_power_limits(dev);
-- 
2.25.1

[PATCH 6/7] platform/x86/amd/pmf: Add support to get sps default APTS index values

[Shyam Sundar S K]

During the driver probe, the default cache values for the static slider
would be obtained by evaluating the APTS method. Add support to use
these values as the thermal settings to be updated on the system based
on the changing platform-profiles.

Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/platform/x86/amd/pmf/acpi.c | 74 +++++++++++++++++++++++++++++
 drivers/platform/x86/amd/pmf/pmf.h  | 25 ++++++++++
 drivers/platform/x86/amd/pmf/sps.c  | 52 +++++++++++++++++++-
 3 files changed, 149 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
index 28df45c058db..ea9b3871a374 100644
--- a/drivers/platform/x86/amd/pmf/acpi.c
+++ b/drivers/platform/x86/amd/pmf/acpi.c
@@ -90,12 +90,86 @@ static int apmf_if_call_store_buffer(struct amd_pmf_dev *pdev, int fn, void *des
 	return err;
 }
 
+static union acpi_object *apts_if_call(struct amd_pmf_dev *pdev, u32 state_index)
+{
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	acpi_handle ahandle = ACPI_HANDLE(pdev->dev);
+	struct acpi_object_list apts_if_arg_list;
+	union acpi_object apts_if_args[3];
+	acpi_status status;
+
+	apts_if_arg_list.count = 3;
+	apts_if_arg_list.pointer = &apts_if_args[0];
+
+	apts_if_args[0].type = ACPI_TYPE_INTEGER;
+	apts_if_args[0].integer.value = 1;
+	apts_if_args[1].type = ACPI_TYPE_INTEGER;
+	apts_if_args[1].integer.value = state_index;
+	apts_if_args[2].type = ACPI_TYPE_INTEGER;
+	apts_if_args[2].integer.value = 0;
+
+	status = acpi_evaluate_object(ahandle, "APTS", &apts_if_arg_list, &buffer);
+	if (ACPI_FAILURE(status)) {
+		dev_err(pdev->dev, "APTS state_idx:%u call failed\n", state_index);
+		kfree(buffer.pointer);
+		return NULL;
+	}
+
+	return buffer.pointer;
+}
+
+static int apts_if_call_store_buffer(struct amd_pmf_dev *pdev,
+				     u32 index, void *data, size_t out_sz)
+{
+	union acpi_object *info;
+	size_t size;
+	int err = 0;
+
+	info = apts_if_call(pdev, index);
+	if (!info)
+		return -EIO;
+
+	if (info->type != ACPI_TYPE_BUFFER) {
+		dev_err(pdev->dev, "object is not a buffer\n");
+		err = -EINVAL;
+		goto out;
+	}
+
+	size = *(u16 *)info->buffer.pointer;
+	if (info->buffer.length < size) {
+		dev_err(pdev->dev, "buffer smaller than header size %u < %zu\n",
+			info->buffer.length, size);
+		err = -EINVAL;
+		goto out;
+	}
+
+	if (size < out_sz) {
+		dev_err(pdev->dev, "buffer too small %zu\n", size);
+		err = -EINVAL;
+		goto out;
+	}
+
+	memcpy(data, info->buffer.pointer, out_sz);
+out:
+	kfree(info);
+	return err;
+}
+
 int is_apmf_func_supported(struct amd_pmf_dev *pdev, unsigned long index)
 {
 	/* If bit-n is set, that indicates function n+1 is supported */
 	return !!(pdev->supported_func & BIT(index - 1));
 }
 
+int apts_get_static_slider_granular_v2(struct amd_pmf_dev *pdev,
+				       struct amd_pmf_apts_granular_output *data, u32 apts_idx)
+{
+	if (!is_apmf_func_supported(pdev, APMF_FUNC_STATIC_SLIDER_GRANULAR))
+		return -EINVAL;
+
+	return apts_if_call_store_buffer(pdev, apts_idx, data, sizeof(*data));
+}
+
 int apmf_get_static_slider_granular_v2(struct amd_pmf_dev *pdev,
 				       struct apmf_static_slider_granular_output_v2 *data)
 {
diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
index b27e96aeac23..f99805827510 100644
--- a/drivers/platform/x86/amd/pmf/pmf.h
+++ b/drivers/platform/x86/amd/pmf/pmf.h
@@ -87,6 +87,29 @@
 #define PMF_IF_V1		1
 #define PMF_IF_V2		2
 
+#define APTS_MAX_STATES		16
+
+/* APTS PMF BIOS Interface */
+struct amd_pmf_apts_output {
+	u16 table_version;
+	u32 fan_table_idx;
+	u32 pmf_ppt;
+	u32 ppt_pmf_apu_only;
+	u32 stt_min_limit;
+	u8 stt_skin_temp_limit_apu;
+	u8 stt_skin_temp_limit_hs2;
+} __packed;
+
+struct amd_pmf_apts_granular_output {
+	u16 size;
+	struct amd_pmf_apts_output val;
+} __packed;
+
+struct amd_pmf_apts_granular {
+	u16 size;
+	struct amd_pmf_apts_output val[APTS_MAX_STATES];
+};
+
 struct sbios_hb_event_v2 {
 	u16 size;
 	u8 load;
@@ -658,6 +681,8 @@ int apmf_update_fan_idx(struct amd_pmf_dev *pdev, bool manual, u32 idx);
 int amd_pmf_set_sps_power_limits(struct amd_pmf_dev *pmf);
 int apmf_get_static_slider_granular_v2(struct amd_pmf_dev *dev,
 				       struct apmf_static_slider_granular_output_v2 *data);
+int apts_get_static_slider_granular_v2(struct amd_pmf_dev *pdev,
+				       struct amd_pmf_apts_granular_output *data, u32 apts_idx);
 
 /* Auto Mode Layer */
 int apmf_get_auto_mode_def(struct amd_pmf_dev *pdev, struct apmf_auto_mode *data);
diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c
index dc4c7ccd4c43..067fabdaa503 100644
--- a/drivers/platform/x86/amd/pmf/sps.c
+++ b/drivers/platform/x86/amd/pmf/sps.c
@@ -12,6 +12,7 @@
 
 static struct amd_pmf_static_slider_granular_v2 config_store_v2;
 static struct amd_pmf_static_slider_granular config_store;
+static struct amd_pmf_apts_granular apts_config_store;
 
 #ifdef CONFIG_AMD_PMF_DEBUG
 static const char *slider_as_str(unsigned int state)
@@ -79,11 +80,56 @@ static void amd_pmf_dump_sps_defaults_v2(struct amd_pmf_static_slider_granular_v
 	pr_debug("dc_battery_saver: %u\n", data->sps_idx.dc_battery_saver);
 	pr_debug("Static Slider APTS state index data - END\n");
 }
+
+static void amd_pmf_dump_apts_sps_defaults(struct amd_pmf_apts_granular *info)
+{
+	int i;
+
+	pr_debug("Static Slider APTS index default values data - BEGIN");
+
+	for (i = 0; i < APTS_MAX_STATES; i++) {
+		pr_debug("index[%d]: table_version = %u\n", i, info->val[i].table_version);
+		pr_debug("index[%d]: fan_table_idx = %u\n", i, info->val[i].fan_table_idx);
+		pr_debug("index[%d]: pmf_ppt = %u\n", i, info->val[i].pmf_ppt);
+		pr_debug("index[%d]: ppt_pmf_apu_only = %u\n", i, info->val[i].ppt_pmf_apu_only);
+		pr_debug("index[%d]: stt_min_limit = %u\n", i, info->val[i].stt_min_limit);
+		pr_debug("index[%d]: stt_skin_temp_limit_apu = %u\n",
+			 i, info->val[i].stt_skin_temp_limit_apu);
+		pr_debug("index[%d]: stt_skin_temp_limit_hs2 = %u\n",
+			 i, info->val[i].stt_skin_temp_limit_hs2);
+	}
+
+	pr_debug("Static Slider APTS index default values data - END");
+}
 #else
 static void amd_pmf_dump_sps_defaults(struct amd_pmf_static_slider_granular *data) {}
 static void amd_pmf_dump_sps_defaults_v2(struct amd_pmf_static_slider_granular_v2 *data) {}
+static void amd_pmf_dump_apts_sps_defaults(struct amd_pmf_apts_granular *info) {}
 #endif
 
+static void amd_pmf_load_apts_defaults_sps_v2(struct amd_pmf_dev *pdev)
+{
+	struct amd_pmf_apts_granular_output output;
+	int i;
+
+	memset(&apts_config_store, 0, sizeof(apts_config_store));
+
+	for (i = 0; i < APTS_MAX_STATES; i++) {
+		apts_get_static_slider_granular_v2(pdev, &output, i);
+		apts_config_store.val[i].table_version = output.val.table_version;
+		apts_config_store.val[i].fan_table_idx = output.val.fan_table_idx;
+		apts_config_store.val[i].pmf_ppt = output.val.pmf_ppt;
+		apts_config_store.val[i].ppt_pmf_apu_only = output.val.ppt_pmf_apu_only;
+		apts_config_store.val[i].stt_min_limit = output.val.stt_min_limit;
+		apts_config_store.val[i].stt_skin_temp_limit_apu =
+							output.val.stt_skin_temp_limit_apu;
+		apts_config_store.val[i].stt_skin_temp_limit_hs2 =
+							output.val.stt_skin_temp_limit_hs2;
+	}
+
+	amd_pmf_dump_apts_sps_defaults(&apts_config_store);
+}
+
 static void amd_pmf_load_defaults_sps_v2(struct amd_pmf_dev *dev)
 {
 	struct apmf_static_slider_granular_output_v2 output;
@@ -293,10 +339,12 @@ int amd_pmf_init_sps(struct amd_pmf_dev *dev)
 	dev->current_profile = PLATFORM_PROFILE_BALANCED;
 
 	if (is_apmf_func_supported(dev, APMF_FUNC_STATIC_SLIDER_GRANULAR)) {
-		if (dev->pmf_if_version == PMF_IF_V2)
+		if (dev->pmf_if_version == PMF_IF_V2) {
 			amd_pmf_load_defaults_sps_v2(dev);
-		else
+			amd_pmf_load_apts_defaults_sps_v2(dev);
+		} else {
 			amd_pmf_load_defaults_sps(dev);
+		}
 
 		/* update SPS balanced power mode thermals */
 		amd_pmf_set_sps_power_limits(dev);
-- 
2.25.1

[PATCH 7/7] platform/x86/amd/pmf: Update sps power thermals according to the platform-profiles

[Shyam Sundar S K]

Update power thermals according to the platform-profiles selected by the
user.

Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/platform/x86/amd/pmf/pmf.h |  2 ++
 drivers/platform/x86/amd/pmf/sps.c | 54 ++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+)

diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
index f99805827510..09600b612964 100644
--- a/drivers/platform/x86/amd/pmf/pmf.h
+++ b/drivers/platform/x86/amd/pmf/pmf.h
@@ -51,6 +51,8 @@
 #define GET_STT_LIMIT_APU	0x20
 #define GET_STT_LIMIT_HS2	0x21
 #define SET_P3T				0x23 /* P3T: Peak Package Power Limit */
+#define SET_PMF_PPT            0x25
+#define SET_PMF_PPT_APU_ONLY   0x26
 
 /* OS slider update notification */
 #define DC_BEST_PERF		0
diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c
index 067fabdaa503..1ae06a4743a8 100644
--- a/drivers/platform/x86/amd/pmf/sps.c
+++ b/drivers/platform/x86/amd/pmf/sps.c
@@ -177,6 +177,19 @@ static void amd_pmf_load_defaults_sps(struct amd_pmf_dev *dev)
 	amd_pmf_dump_sps_defaults(&config_store);
 }
 
+static void amd_pmf_update_slider_v2(struct amd_pmf_dev *dev, int idx)
+{
+	amd_pmf_send_cmd(dev, SET_PMF_PPT, false, apts_config_store.val[idx].pmf_ppt, NULL);
+	amd_pmf_send_cmd(dev, SET_PMF_PPT_APU_ONLY, false,
+			 apts_config_store.val[idx].ppt_pmf_apu_only, NULL);
+	amd_pmf_send_cmd(dev, SET_STT_MIN_LIMIT, false,
+			 apts_config_store.val[idx].stt_min_limit, NULL);
+	amd_pmf_send_cmd(dev, SET_STT_LIMIT_APU, false,
+			 apts_config_store.val[idx].stt_skin_temp_limit_apu, NULL);
+	amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false,
+			 apts_config_store.val[idx].stt_skin_temp_limit_hs2, NULL);
+}
+
 void amd_pmf_update_slider(struct amd_pmf_dev *dev, bool op, int idx,
 			   struct amd_pmf_static_slider_granular *table)
 {
@@ -209,6 +222,44 @@ void amd_pmf_update_slider(struct amd_pmf_dev *dev, bool op, int idx,
 	}
 }
 
+static int amd_pmf_update_sps_power_limits_v2(struct amd_pmf_dev *pdev, int pwr_mode)
+{
+	int src = amd_pmf_get_power_source();
+
+	if (src == POWER_SOURCE_AC) {
+		switch (pwr_mode) {
+		case POWER_MODE_PERFORMANCE:
+			amd_pmf_update_slider_v2(pdev, config_store_v2.sps_idx.ac_best_perf);
+			break;
+		case POWER_MODE_BALANCED_POWER:
+			amd_pmf_update_slider_v2(pdev, config_store_v2.sps_idx.ac_balanced);
+			break;
+		case POWER_MODE_POWER_SAVER:
+			amd_pmf_update_slider_v2(pdev,
+						 config_store_v2.sps_idx.ac_best_pwr_efficiency);
+			break;
+		default:
+			return -EINVAL;
+		}
+	} else if (src == POWER_SOURCE_DC) {
+		switch (pwr_mode) {
+		case POWER_MODE_PERFORMANCE:
+			amd_pmf_update_slider_v2(pdev, config_store_v2.sps_idx.dc_best_perf);
+			break;
+		case POWER_MODE_BALANCED_POWER:
+			amd_pmf_update_slider_v2(pdev, config_store_v2.sps_idx.dc_balanced);
+			break;
+		case POWER_MODE_POWER_SAVER:
+			amd_pmf_update_slider_v2(pdev,
+						 config_store_v2.sps_idx.dc_best_pwr_efficiency);
+			break;
+		default:
+			return -EINVAL;
+		}
+	}
+	return 0;
+}
+
 int amd_pmf_set_sps_power_limits(struct amd_pmf_dev *pmf)
 {
 	int mode;
@@ -217,6 +268,9 @@ int amd_pmf_set_sps_power_limits(struct amd_pmf_dev *pmf)
 	if (mode < 0)
 		return mode;
 
+	if (pmf->pmf_if_version == PMF_IF_V2)
+		return amd_pmf_update_sps_power_limits_v2(pmf, mode);
+
 	amd_pmf_update_slider(pmf, SLIDER_OP_SET, mode, NULL);
 
 	return 0;
-- 
2.25.1

Re: [PATCH 3/7] platform/x86/amd/pmf: Add support to get sbios requests in PMF driver

[Ilpo Järvinen]

On Tue, 27 Feb 2024, Shyam Sundar S K wrote:

> Update the APMF function index 2 for family 1Ah, that gets the
> information of  SBIOS requests (like the pending requests from BIOS,

extra space.

> custom notifications, updation of power limits etc).
> 
> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>  drivers/platform/x86/amd/pmf/acpi.c |  6 ++++++
>  drivers/platform/x86/amd/pmf/pmf.h  | 13 +++++++++++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
> index 1f287a147c57..1b2a099c0cef 100644
> --- a/drivers/platform/x86/amd/pmf/acpi.c
> +++ b/drivers/platform/x86/amd/pmf/acpi.c
> @@ -166,6 +166,12 @@ int apmf_get_auto_mode_def(struct amd_pmf_dev *pdev, struct apmf_auto_mode *data
>  	return apmf_if_call_store_buffer(pdev, APMF_FUNC_AUTO_MODE, data, sizeof(*data));
>  }
>  
> +int apmf_get_sbios_requests_v2(struct amd_pmf_dev *pdev, struct apmf_sbios_req_v2 *req)
> +{
> +	return apmf_if_call_store_buffer(pdev, APMF_FUNC_SBIOS_REQUESTS,
> +									req, sizeof(*req));

Fix the alignment please.

> +}
> +
>  int apmf_get_sbios_requests(struct amd_pmf_dev *pdev, struct apmf_sbios_req *req)
>  {
>  	return apmf_if_call_store_buffer(pdev, APMF_FUNC_SBIOS_REQUESTS,
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index 4364af72a7a3..f11d2a348696 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -116,6 +116,18 @@ struct apmf_sbios_req {
>  	u8 skin_temp_hs2;
>  } __packed;
>  
> +struct apmf_sbios_req_v2 {
> +	u16 size;
> +	u32 pending_req;
> +	u8 rsvd;
> +	u32 update_ppt_pmf;
> +	u32 update_ppt_pmf_apu_only;
> +	u32 update_stt_min;
> +	u8 update_stt_apu;
> +	u8 update_stt_hs2;

Is it intentional that these do not match with the names in struct 
apmf_sbios_req? I mean some of the fields look suspiciously close in name 
so is the purpose still the same and somebody just invented new names for the 
same field?


-- 
 i.

Re: [PATCH 4/7] platform/x86/amd/pmf: Add support to notify sbios heart beat event

[Ilpo Järvinen]

On Tue, 27 Feb 2024, Shyam Sundar S K wrote:

> Add support for newer revision of the heart beat notify events.
> This event is used to notify to the OEM BIOS on driver
> load/unload/suspend/resume scenarios.
> 
> If OEM BIOS does not receive the heart beat event from PMF driver, OEM
> BIOS shall conclude that PMF driver is no more active and BIOS will
> update to the legacy system power thermals.
> 
> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>  drivers/platform/x86/amd/pmf/acpi.c | 42 +++++++++++++++++++++++++++--
>  drivers/platform/x86/amd/pmf/core.c | 10 +++++++
>  drivers/platform/x86/amd/pmf/pmf.h  | 17 ++++++++++++
>  3 files changed, 67 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
> index 1b2a099c0cef..0fc8ad0ac3e9 100644
> --- a/drivers/platform/x86/amd/pmf/acpi.c
> +++ b/drivers/platform/x86/amd/pmf/acpi.c
> @@ -140,6 +140,44 @@ static void apmf_sbios_heartbeat_notify(struct work_struct *work)
>  	kfree(info);
>  }
>  
> +int amd_pmf_notify_sbios_heartbeat_event_v2(struct amd_pmf_dev *dev, u8 flag)
> +{
> +	struct sbios_hb_event_v2 args = { };
> +	struct acpi_buffer params;
> +	union acpi_object *info;
> +
> +	args.size = sizeof(args);
> +
> +	switch (flag) {
> +	case ON_LOAD:
> +		args.load = 1;
> +		break;
> +	case ON_UNLOAD:
> +		args.unload = 1;
> +		break;
> +	case ON_SUSPEND:
> +		args.suspend = 1;
> +		break;
> +	case ON_RESUME:
> +		args.resume = 1;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	params.length = sizeof(args);
> +	params.pointer = (void *)&args;

Casting type pointer to void pointer (and vice-versa) is not necessary as 
it's implicitly done for you by the compiler.

> +	info = apmf_if_call(dev, APMF_FUNC_SBIOS_HEARTBEAT_V2, &params);
> +	if (!info)
> +		return -EIO;
> +
> +	dev_dbg(dev->dev, "Sending v2 heartbeat event to SBIOS\n");

Perhaps including the flag would make this more useful. It's a bit odd 
though you dev_dbg the success but not the failure case. I'd tend to think 
the failure is more useful to know than things working normally.

-- 
 i.

Re: [PATCH 5/7] platform/x86/amd/pmf: Add support to get APTS index numbers for static slider

[Ilpo Järvinen]

On Tue, 27 Feb 2024, Shyam Sundar S K wrote:

> APMF spec has a newer section called the APTS (AMD Performance and
> Thermal State) information, where each slider/power mode is associated
> with an index number.
> 
> Add support to get these indices for the Static Slider.
> 
> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>  drivers/platform/x86/amd/pmf/acpi.c | 10 +++++++
>  drivers/platform/x86/amd/pmf/pmf.h  | 24 +++++++++++++++++
>  drivers/platform/x86/amd/pmf/sps.c  | 42 ++++++++++++++++++++++++++++-
>  3 files changed, 75 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
> index 0fc8ad0ac3e9..28df45c058db 100644
> --- a/drivers/platform/x86/amd/pmf/acpi.c
> +++ b/drivers/platform/x86/amd/pmf/acpi.c
> @@ -96,6 +96,16 @@ int is_apmf_func_supported(struct amd_pmf_dev *pdev, unsigned long index)
>  	return !!(pdev->supported_func & BIT(index - 1));
>  }
>  
> +int apmf_get_static_slider_granular_v2(struct amd_pmf_dev *pdev,
> +				       struct apmf_static_slider_granular_output_v2 *data)
> +{
> +	if (!is_apmf_func_supported(pdev, APMF_FUNC_STATIC_SLIDER_GRANULAR))
> +		return -EINVAL;
> +
> +	return apmf_if_call_store_buffer(pdev, APMF_FUNC_STATIC_SLIDER_GRANULAR,
> +								data, sizeof(*data));

Wrong aligment. Please go through all the patches to check these.

> diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c
> index 33e23e25c8b1..dc4c7ccd4c43 100644
> --- a/drivers/platform/x86/amd/pmf/sps.c
> +++ b/drivers/platform/x86/amd/pmf/sps.c
> @@ -10,6 +10,7 @@
>  
>  #include "pmf.h"
>  
> +static struct amd_pmf_static_slider_granular_v2 config_store_v2;
>  static struct amd_pmf_static_slider_granular config_store;
>  
>  #ifdef CONFIG_AMD_PMF_DEBUG
> @@ -63,10 +64,46 @@ static void amd_pmf_dump_sps_defaults(struct amd_pmf_static_slider_granular *dat
>  
>  	pr_debug("Static Slider Data - END\n");
>  }
> +
> +static void amd_pmf_dump_sps_defaults_v2(struct amd_pmf_static_slider_granular_v2 *data)
> +{
> +	pr_debug("Static Slider APTS state index data - BEGIN");
> +	pr_debug("size: %u\n", data->size);
> +	pr_debug("ac_best_perf: %u\n", data->sps_idx.ac_best_perf);
> +	pr_debug("ac_balanced: %u\n", data->sps_idx.ac_balanced);
> +	pr_debug("ac_best_pwr_efficiency: %u\n", data->sps_idx.ac_best_pwr_efficiency);
> +	pr_debug("ac_energy_saver: %u\n", data->sps_idx.ac_energy_saver);
> +	pr_debug("dc_best_perf: %u\n", data->sps_idx.dc_best_perf);
> +	pr_debug("dc_balanced: %u\n", data->sps_idx.dc_balanced);
> +	pr_debug("dc_best_pwr_efficiency: %u\n", data->sps_idx.dc_best_pwr_efficiency);
> +	pr_debug("dc_battery_saver: %u\n", data->sps_idx.dc_battery_saver);

I know these are debug only but what is the advantage of having the 
underscores in them? I think they'd read & match just fine without them 
(perhaps pwr->power would be better but it's up to you) and ac/dc can then 
be capitalized.


-- 
 i.

Re: [PATCH 6/7] platform/x86/amd/pmf: Add support to get sps default APTS index values

[Ilpo Järvinen]

On Tue, 27 Feb 2024, Shyam Sundar S K wrote:

> During the driver probe, the default cache values for the static slider
> would be obtained by evaluating the APTS method. Add support to use
> these values as the thermal settings to be updated on the system based
> on the changing platform-profiles.
> 
> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>

> @@ -79,11 +80,56 @@ static void amd_pmf_dump_sps_defaults_v2(struct amd_pmf_static_slider_granular_v
>  	pr_debug("dc_battery_saver: %u\n", data->sps_idx.dc_battery_saver);
>  	pr_debug("Static Slider APTS state index data - END\n");
>  }
> +
> +static void amd_pmf_dump_apts_sps_defaults(struct amd_pmf_apts_granular *info)
> +{
> +	int i;
> +
> +	pr_debug("Static Slider APTS index default values data - BEGIN");
> +
> +	for (i = 0; i < APTS_MAX_STATES; i++) {
> +		pr_debug("index[%d]: table_version = %u\n", i, info->val[i].table_version);
> +		pr_debug("index[%d]: fan_table_idx = %u\n", i, info->val[i].fan_table_idx);
> +		pr_debug("index[%d]: pmf_ppt = %u\n", i, info->val[i].pmf_ppt);
> +		pr_debug("index[%d]: ppt_pmf_apu_only = %u\n", i, info->val[i].ppt_pmf_apu_only);
> +		pr_debug("index[%d]: stt_min_limit = %u\n", i, info->val[i].stt_min_limit);
> +		pr_debug("index[%d]: stt_skin_temp_limit_apu = %u\n",
> +			 i, info->val[i].stt_skin_temp_limit_apu);
> +		pr_debug("index[%d]: stt_skin_temp_limit_hs2 = %u\n",
> +			 i, info->val[i].stt_skin_temp_limit_hs2);

Again, underscores seem mostly unnecessary. There's also inconsistency in 
capitalization and space/underscore usage compared with the existing 
printouts now that I looked also the existing pr_debug()s. Please try to 
make things mostly consistent.

> +	}
> +
> +	pr_debug("Static Slider APTS index default values data - END");
> +}
>  #else
>  static void amd_pmf_dump_sps_defaults(struct amd_pmf_static_slider_granular *data) {}
>  static void amd_pmf_dump_sps_defaults_v2(struct amd_pmf_static_slider_granular_v2 *data) {}
> +static void amd_pmf_dump_apts_sps_defaults(struct amd_pmf_apts_granular *info) {}
>  #endif
>  
> +static void amd_pmf_load_apts_defaults_sps_v2(struct amd_pmf_dev *pdev)
> +{
> +	struct amd_pmf_apts_granular_output output;
> +	int i;
> +
> +	memset(&apts_config_store, 0, sizeof(apts_config_store));
> +
> +	for (i = 0; i < APTS_MAX_STATES; i++) {
> +		apts_get_static_slider_granular_v2(pdev, &output, i);
> +		apts_config_store.val[i].table_version = output.val.table_version;
> +		apts_config_store.val[i].fan_table_idx = output.val.fan_table_idx;
> +		apts_config_store.val[i].pmf_ppt = output.val.pmf_ppt;
> +		apts_config_store.val[i].ppt_pmf_apu_only = output.val.ppt_pmf_apu_only;
> +		apts_config_store.val[i].stt_min_limit = output.val.stt_min_limit;
> +		apts_config_store.val[i].stt_skin_temp_limit_apu =
> +							output.val.stt_skin_temp_limit_apu;
> +		apts_config_store.val[i].stt_skin_temp_limit_hs2 =
> +							output.val.stt_skin_temp_limit_hs2;

Add a temporary variabled for apts_config_store.val[i] to make these 
shorter?


-- 
 i.

Re: [PATCH 5/7] platform/x86/amd/pmf: Add support to get APTS index numbers for static slider

[Ilpo Järvinen]

On Tue, 27 Feb 2024, Shyam Sundar S K wrote:

> APMF spec has a newer section called the APTS (AMD Performance and
> Thermal State) information, where each slider/power mode is associated
> with an index number.
> 
> Add support to get these indices for the Static Slider.
> 
> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>  drivers/platform/x86/amd/pmf/acpi.c | 10 +++++++
>  drivers/platform/x86/amd/pmf/pmf.h  | 24 +++++++++++++++++
>  drivers/platform/x86/amd/pmf/sps.c  | 42 ++++++++++++++++++++++++++++-
>  3 files changed, 75 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
> index 0fc8ad0ac3e9..28df45c058db 100644
> --- a/drivers/platform/x86/amd/pmf/acpi.c
> +++ b/drivers/platform/x86/amd/pmf/acpi.c
> @@ -96,6 +96,16 @@ int is_apmf_func_supported(struct amd_pmf_dev *pdev, unsigned long index)
>  	return !!(pdev->supported_func & BIT(index - 1));
>  }
>  
> +int apmf_get_static_slider_granular_v2(struct amd_pmf_dev *pdev,
> +				       struct apmf_static_slider_granular_output_v2 *data)
> +{
> +	if (!is_apmf_func_supported(pdev, APMF_FUNC_STATIC_SLIDER_GRANULAR))
> +		return -EINVAL;
> +
> +	return apmf_if_call_store_buffer(pdev, APMF_FUNC_STATIC_SLIDER_GRANULAR,
> +								data, sizeof(*data));
> +}
> +
>  int apmf_get_static_slider_granular(struct amd_pmf_dev *pdev,
>  				    struct apmf_static_slider_granular_output *data)
>  {
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index 5cad11369697..b27e96aeac23 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -85,6 +85,7 @@
>  #define MAX_OPERATION_PARAMS					4
>  
>  #define PMF_IF_V1		1
> +#define PMF_IF_V2		2
>  
>  struct sbios_hb_event_v2 {
>  	u16 size;
> @@ -264,6 +265,17 @@ struct amd_pmf_dev {
>  	u16 pmf_if_version;
>  };
>  
> +struct apmf_sps_prop_granular_v2 {
> +	u8 ac_best_perf;
> +	u8 ac_balanced;
> +	u8 ac_best_pwr_efficiency;
> +	u8 ac_energy_saver;
> +	u8 dc_best_perf;
> +	u8 dc_balanced;
> +	u8 dc_best_pwr_efficiency;
> +	u8 dc_battery_saver;

I started to wonder if these could be made into an two element array with 
4xu8 in each, one for AC and DC because it would simplify some other code 
in the subsequent patches (and perhaps even in this patch)?

-- 
 i.

Re: [PATCH 2/7] platform/x86/amd/pmf: Disable debugfs support for querying power thermals

[Kuppuswamy Sathyanarayanan]

On 2/27/24 4:55 AM, Shyam Sundar S K wrote:
> The GET interface to receive the active power thermal information from
> the PMFW has been depricated. Hence drop the debugfs support for the
> newer platforms.

Instead of "newer platforms", I recommend to use version numbers.

IIUC, you don't want to expose it for ver > 1.

>
> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>  drivers/platform/x86/amd/pmf/core.c | 5 +++--
>  drivers/platform/x86/amd/pmf/pmf.h  | 2 ++
>  2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
> index 4f734e049f4a..2230bc2b1f59 100644
> --- a/drivers/platform/x86/amd/pmf/core.c
> +++ b/drivers/platform/x86/amd/pmf/core.c
> @@ -113,8 +113,9 @@ static void amd_pmf_dbgfs_unregister(struct amd_pmf_dev *dev)
>  static void amd_pmf_dbgfs_register(struct amd_pmf_dev *dev)
>  {
>  	dev->dbgfs_dir = debugfs_create_dir("amd_pmf", NULL);
> -	debugfs_create_file("current_power_limits", 0644, dev->dbgfs_dir, dev,
> -			    &current_power_limits_fops);
> +	if (dev->pmf_if_version == PMF_IF_V1)
> +		debugfs_create_file("current_power_limits", 0644, dev->dbgfs_dir, dev,
> +				    &current_power_limits_fops);
>  }
>  
>  int amd_pmf_get_power_source(void)
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index e51ac981af73..4364af72a7a3 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -83,6 +83,8 @@
>  #define TA_OUTPUT_RESERVED_MEM				906
>  #define MAX_OPERATION_PARAMS					4
>  
> +#define PMF_IF_V1		1
> +
>  /* AMD PMF BIOS interfaces */
>  struct apmf_verify_interface {
>  	u16 size;

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH 1/7] platform/x86/amd/pmf: Differentiate PMF ACPI versions

[Kuppuswamy Sathyanarayanan]

Hi,

On 2/27/24 4:55 AM, Shyam Sundar S K wrote:
> For family 1AH, certain PMF features have been enhanced - leading to a
> newer APMF (AMD PMF) spec (BIOS and PMF driver interface) called v2.
>
> This information would be fed into the if_version field of the
> verify_interface method of the APMF call from the BIOS.
>
> Use this information to store the version number to differentiate
> between v1 or v2 and also store the information into the PMF private
> data structure, as this information would be required for further code
> branching to support the latest silicon.
>
> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
Looks good to me.

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

>  drivers/platform/x86/amd/pmf/acpi.c | 6 ++++--
>  drivers/platform/x86/amd/pmf/pmf.h  | 1 +
>  2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
> index f2eb07ef855a..1f287a147c57 100644
> --- a/drivers/platform/x86/amd/pmf/acpi.c
> +++ b/drivers/platform/x86/amd/pmf/acpi.c
> @@ -218,8 +218,10 @@ static int apmf_if_verify_interface(struct amd_pmf_dev *pdev)
>  		return err;
>  
>  	pdev->supported_func = output.supported_functions;
> -	dev_dbg(pdev->dev, "supported functions:0x%x notifications:0x%x\n",
> -		output.supported_functions, output.notification_mask);
> +	dev_dbg(pdev->dev, "supported functions:0x%x notifications:0x%x version:%u\n",
> +		output.supported_functions, output.notification_mask, output.version);
> +
> +	pdev->pmf_if_version = output.version;
>  
>  	return 0;
>  }
> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> index 66cae1cca73c..e51ac981af73 100644
> --- a/drivers/platform/x86/amd/pmf/pmf.h
> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> @@ -231,6 +231,7 @@ struct amd_pmf_dev {
>  	u64 policy_addr;
>  	void *policy_base;
>  	bool smart_pc_enabled;
> +	u16 pmf_if_version;
>  };
>  
>  struct apmf_sps_prop_granular {

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH 3/7] platform/x86/amd/pmf: Add support to get sbios requests in PMF driver

[Shyam Sundar S K]

On 2/27/2024 18:58, Ilpo Järvinen wrote:
> On Tue, 27 Feb 2024, Shyam Sundar S K wrote:
> 
>> Update the APMF function index 2 for family 1Ah, that gets the
>> information of  SBIOS requests (like the pending requests from BIOS,
> 
> extra space.
> 
>> custom notifications, updation of power limits etc).
>>
>> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>> ---
>>  drivers/platform/x86/amd/pmf/acpi.c |  6 ++++++
>>  drivers/platform/x86/amd/pmf/pmf.h  | 13 +++++++++++++
>>  2 files changed, 19 insertions(+)
>>
>> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
>> index 1f287a147c57..1b2a099c0cef 100644
>> --- a/drivers/platform/x86/amd/pmf/acpi.c
>> +++ b/drivers/platform/x86/amd/pmf/acpi.c
>> @@ -166,6 +166,12 @@ int apmf_get_auto_mode_def(struct amd_pmf_dev *pdev, struct apmf_auto_mode *data
>>  	return apmf_if_call_store_buffer(pdev, APMF_FUNC_AUTO_MODE, data, sizeof(*data));
>>  }
>>  
>> +int apmf_get_sbios_requests_v2(struct amd_pmf_dev *pdev, struct apmf_sbios_req_v2 *req)
>> +{
>> +	return apmf_if_call_store_buffer(pdev, APMF_FUNC_SBIOS_REQUESTS,
>> +									req, sizeof(*req));
> 
> Fix the alignment please.
> 
>> +}
>> +
>>  int apmf_get_sbios_requests(struct amd_pmf_dev *pdev, struct apmf_sbios_req *req)
>>  {
>>  	return apmf_if_call_store_buffer(pdev, APMF_FUNC_SBIOS_REQUESTS,
>> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
>> index 4364af72a7a3..f11d2a348696 100644
>> --- a/drivers/platform/x86/amd/pmf/pmf.h
>> +++ b/drivers/platform/x86/amd/pmf/pmf.h
>> @@ -116,6 +116,18 @@ struct apmf_sbios_req {
>>  	u8 skin_temp_hs2;
>>  } __packed;
>>  
>> +struct apmf_sbios_req_v2 {
>> +	u16 size;
>> +	u32 pending_req;
>> +	u8 rsvd;
>> +	u32 update_ppt_pmf;
>> +	u32 update_ppt_pmf_apu_only;
>> +	u32 update_stt_min;
>> +	u8 update_stt_apu;
>> +	u8 update_stt_hs2;
> 
> Is it intentional that these do not match with the names in struct 
> apmf_sbios_req? I mean some of the fields look suspiciously close in name 
> so is the purpose still the same and somebody just invented new names for the 
> same field?

The idea is to optimize certain fields in the BIOS menu that OEMs have
to feed in while making the right choices for the power settings for
different features.

The entire series is targeted towards that where the interface between
the driver and the BIOS is improvised so that:

- Multiple features can link to one state, so OEMs doesn’t need to
program same parameters in multiple locations.
- If we need to add new power controller limits , we don’t have to
touch APMF functions, its more expandable adding new fields in APS
methods.

To answers to your question of apmf_sbios_req vs apmf_sbios_req_v2:

It calls for a new struct _v2, because:
- AMT support has been dropped so there shall be no pending events
from DYTC (like the CQL and AMT)
- As per the new design, the PMFW has given control to set PPT and STT
limits and no pending requests on updating SPL limits.

But as per names, I don't think there is no new invention :-)

FYI, here PPT means Power Packaging Tracking, so it could be SPPT
(Slow PPT) or FPPT (Fast PPT) and SST means Skin Temperature Tracking.

I will address your other remarks.

Thanks,
Shyam

Re: [PATCH 5/7] platform/x86/amd/pmf: Add support to get APTS index numbers for static slider

[Shyam Sundar S K]

On 2/27/2024 19:43, Ilpo Järvinen wrote:
> On Tue, 27 Feb 2024, Shyam Sundar S K wrote:
> 
>> APMF spec has a newer section called the APTS (AMD Performance and
>> Thermal State) information, where each slider/power mode is associated
>> with an index number.
>>
>> Add support to get these indices for the Static Slider.
>>
>> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>> ---
>>  drivers/platform/x86/amd/pmf/acpi.c | 10 +++++++
>>  drivers/platform/x86/amd/pmf/pmf.h  | 24 +++++++++++++++++
>>  drivers/platform/x86/amd/pmf/sps.c  | 42 ++++++++++++++++++++++++++++-
>>  3 files changed, 75 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
>> index 0fc8ad0ac3e9..28df45c058db 100644
>> --- a/drivers/platform/x86/amd/pmf/acpi.c
>> +++ b/drivers/platform/x86/amd/pmf/acpi.c
>> @@ -96,6 +96,16 @@ int is_apmf_func_supported(struct amd_pmf_dev *pdev, unsigned long index)
>>  	return !!(pdev->supported_func & BIT(index - 1));
>>  }
>>  
>> +int apmf_get_static_slider_granular_v2(struct amd_pmf_dev *pdev,
>> +				       struct apmf_static_slider_granular_output_v2 *data)
>> +{
>> +	if (!is_apmf_func_supported(pdev, APMF_FUNC_STATIC_SLIDER_GRANULAR))
>> +		return -EINVAL;
>> +
>> +	return apmf_if_call_store_buffer(pdev, APMF_FUNC_STATIC_SLIDER_GRANULAR,
>> +								data, sizeof(*data));
>> +}
>> +
>>  int apmf_get_static_slider_granular(struct amd_pmf_dev *pdev,
>>  				    struct apmf_static_slider_granular_output *data)
>>  {
>> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
>> index 5cad11369697..b27e96aeac23 100644
>> --- a/drivers/platform/x86/amd/pmf/pmf.h
>> +++ b/drivers/platform/x86/amd/pmf/pmf.h
>> @@ -85,6 +85,7 @@
>>  #define MAX_OPERATION_PARAMS					4
>>  
>>  #define PMF_IF_V1		1
>> +#define PMF_IF_V2		2
>>  
>>  struct sbios_hb_event_v2 {
>>  	u16 size;
>> @@ -264,6 +265,17 @@ struct amd_pmf_dev {
>>  	u16 pmf_if_version;
>>  };
>>  
>> +struct apmf_sps_prop_granular_v2 {
>> +	u8 ac_best_perf;
>> +	u8 ac_balanced;
>> +	u8 ac_best_pwr_efficiency;
>> +	u8 ac_energy_saver;
>> +	u8 dc_best_perf;
>> +	u8 dc_balanced;
>> +	u8 dc_best_pwr_efficiency;
>> +	u8 dc_battery_saver;
> 
> I started to wonder if these could be made into an two element array with 
> 4xu8 in each, one for AC and DC because it would simplify some other code 
> in the subsequent patches (and perhaps even in this patch)?
> 

OK I understand your point. Let me give it a try. (I tried to retain
the same struct fields across Linux and Windows so that its easy to
maintain)

Will address your remarks on other patches in the series in the new
revision.

Thanks,
Shyam

Re: [PATCH 2/7] platform/x86/amd/pmf: Disable debugfs support for querying power thermals

[Shyam Sundar S K]

On 2/27/2024 21:25, Kuppuswamy Sathyanarayanan wrote:
> 
> On 2/27/24 4:55 AM, Shyam Sundar S K wrote:
>> The GET interface to receive the active power thermal information from
>> the PMFW has been depricated. Hence drop the debugfs support for the
>> newer platforms.
> 
> Instead of "newer platforms", I recommend to use version numbers.
> 
> IIUC, you don't want to expose it for ver > 1.

OK. I will change it.

Thanks,
Shyam

> 
>>
>> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>> ---
>>  drivers/platform/x86/amd/pmf/core.c | 5 +++--
>>  drivers/platform/x86/amd/pmf/pmf.h  | 2 ++
>>  2 files changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
>> index 4f734e049f4a..2230bc2b1f59 100644
>> --- a/drivers/platform/x86/amd/pmf/core.c
>> +++ b/drivers/platform/x86/amd/pmf/core.c
>> @@ -113,8 +113,9 @@ static void amd_pmf_dbgfs_unregister(struct amd_pmf_dev *dev)
>>  static void amd_pmf_dbgfs_register(struct amd_pmf_dev *dev)
>>  {
>>  	dev->dbgfs_dir = debugfs_create_dir("amd_pmf", NULL);
>> -	debugfs_create_file("current_power_limits", 0644, dev->dbgfs_dir, dev,
>> -			    &current_power_limits_fops);
>> +	if (dev->pmf_if_version == PMF_IF_V1)
>> +		debugfs_create_file("current_power_limits", 0644, dev->dbgfs_dir, dev,
>> +				    &current_power_limits_fops);
>>  }
>>  
>>  int amd_pmf_get_power_source(void)
>> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
>> index e51ac981af73..4364af72a7a3 100644
>> --- a/drivers/platform/x86/amd/pmf/pmf.h
>> +++ b/drivers/platform/x86/amd/pmf/pmf.h
>> @@ -83,6 +83,8 @@
>>  #define TA_OUTPUT_RESERVED_MEM				906
>>  #define MAX_OPERATION_PARAMS					4
>>  
>> +#define PMF_IF_V1		1
>> +
>>  /* AMD PMF BIOS interfaces */
>>  struct apmf_verify_interface {
>>  	u16 size;
> 

Re: [PATCH 3/7] platform/x86/amd/pmf: Add support to get sbios requests in PMF driver

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 4589 bytes --]

On Wed, 28 Feb 2024, Shyam Sundar S K wrote:

> 
> 
> On 2/27/2024 18:58, Ilpo Järvinen wrote:
> > On Tue, 27 Feb 2024, Shyam Sundar S K wrote:
> > 
> >> Update the APMF function index 2 for family 1Ah, that gets the
> >> information of  SBIOS requests (like the pending requests from BIOS,
> > 
> > extra space.
> > 
> >> custom notifications, updation of power limits etc).
> >>
> >> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> >> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> >> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> >> ---
> >>  drivers/platform/x86/amd/pmf/acpi.c |  6 ++++++
> >>  drivers/platform/x86/amd/pmf/pmf.h  | 13 +++++++++++++
> >>  2 files changed, 19 insertions(+)
> >>
> >> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
> >> index 1f287a147c57..1b2a099c0cef 100644
> >> --- a/drivers/platform/x86/amd/pmf/acpi.c
> >> +++ b/drivers/platform/x86/amd/pmf/acpi.c
> >> @@ -166,6 +166,12 @@ int apmf_get_auto_mode_def(struct amd_pmf_dev *pdev, struct apmf_auto_mode *data
> >>  	return apmf_if_call_store_buffer(pdev, APMF_FUNC_AUTO_MODE, data, sizeof(*data));
> >>  }
> >>  
> >> +int apmf_get_sbios_requests_v2(struct amd_pmf_dev *pdev, struct apmf_sbios_req_v2 *req)
> >> +{
> >> +	return apmf_if_call_store_buffer(pdev, APMF_FUNC_SBIOS_REQUESTS,
> >> +									req, sizeof(*req));
> > 
> > Fix the alignment please.
> > 
> >> +}
> >> +
> >>  int apmf_get_sbios_requests(struct amd_pmf_dev *pdev, struct apmf_sbios_req *req)
> >>  {
> >>  	return apmf_if_call_store_buffer(pdev, APMF_FUNC_SBIOS_REQUESTS,
> >> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> >> index 4364af72a7a3..f11d2a348696 100644
> >> --- a/drivers/platform/x86/amd/pmf/pmf.h
> >> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> >> @@ -116,6 +116,18 @@ struct apmf_sbios_req {
> >>  	u8 skin_temp_hs2;
> >>  } __packed;
> >>  
> >> +struct apmf_sbios_req_v2 {
> >> +	u16 size;
> >> +	u32 pending_req;
> >> +	u8 rsvd;
> >> +	u32 update_ppt_pmf;
> >> +	u32 update_ppt_pmf_apu_only;
> >> +	u32 update_stt_min;
> >> +	u8 update_stt_apu;
> >> +	u8 update_stt_hs2;
> > 
> > Is it intentional that these do not match with the names in struct 
> > apmf_sbios_req? I mean some of the fields look suspiciously close in name 
> > so is the purpose still the same and somebody just invented new names for the 
> > same field?
> 
> The idea is to optimize certain fields in the BIOS menu that OEMs have
> to feed in while making the right choices for the power settings for
> different features.
> 
> The entire series is targeted towards that where the interface between
> the driver and the BIOS is improvised so that:
> 
> - Multiple features can link to one state, so OEMs doesn’t need to
> program same parameters in multiple locations.
> - If we need to add new power controller limits , we don’t have to
> touch APMF functions, its more expandable adding new fields in APS
> methods.
> 
> To answers to your question of apmf_sbios_req vs apmf_sbios_req_v2:
> 
> It calls for a new struct _v2, because:
> - AMT support has been dropped so there shall be no pending events
> from DYTC (like the CQL and AMT)
> - As per the new design, the PMFW has given control to set PPT and STT
> limits and no pending requests on updating SPL limits.
> 
> But as per names, I don't think there is no new invention :-)

Hi again,

I'm fine with adding _v2 struct (I could see they're not identical).

What I'm still left unsure if we spoke past each other so let me state 
the question in more concrete terms:

- apmf_sbios_req has a field called stt_min_limit
- apmf_sbios_req_v2 has a field called update_stt_min

My question is, are those just the same but only named differently for 
some reason, or does the "limit" and/or "update" difference actually imply 
there's change in how that field is used?

Similar question applies to the other fields which look close but not
identical. There's no need for you to itemize and explain each field for 
me specifically in the reply, I just prefer the same thing called the same
in both structs if that's the case.

It could be you tried to answer this with your second bullet but I just 
don't understand its meaning deeply enough, thus I'm asking again, please 
bear with me.

> FYI, here PPT means Power Packaging Tracking, so it could be SPPT
> (Slow PPT) or FPPT (Fast PPT) and SST means Skin Temperature Tracking.


-- 
 i.

Re: [PATCH 3/7] platform/x86/amd/pmf: Add support to get sbios requests in PMF driver

[Shyam Sundar S K]

On 2/29/2024 18:49, Ilpo Järvinen wrote:
> On Wed, 28 Feb 2024, Shyam Sundar S K wrote:
> 
>>
>>
>> On 2/27/2024 18:58, Ilpo Järvinen wrote:
>>> On Tue, 27 Feb 2024, Shyam Sundar S K wrote:
>>>
>>>> Update the APMF function index 2 for family 1Ah, that gets the
>>>> information of  SBIOS requests (like the pending requests from BIOS,
>>>
>>> extra space.
>>>
>>>> custom notifications, updation of power limits etc).
>>>>
>>>> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>>>> ---
>>>>  drivers/platform/x86/amd/pmf/acpi.c |  6 ++++++
>>>>  drivers/platform/x86/amd/pmf/pmf.h  | 13 +++++++++++++
>>>>  2 files changed, 19 insertions(+)
>>>>
>>>> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
>>>> index 1f287a147c57..1b2a099c0cef 100644
>>>> --- a/drivers/platform/x86/amd/pmf/acpi.c
>>>> +++ b/drivers/platform/x86/amd/pmf/acpi.c
>>>> @@ -166,6 +166,12 @@ int apmf_get_auto_mode_def(struct amd_pmf_dev *pdev, struct apmf_auto_mode *data
>>>>  	return apmf_if_call_store_buffer(pdev, APMF_FUNC_AUTO_MODE, data, sizeof(*data));
>>>>  }
>>>>  
>>>> +int apmf_get_sbios_requests_v2(struct amd_pmf_dev *pdev, struct apmf_sbios_req_v2 *req)
>>>> +{
>>>> +	return apmf_if_call_store_buffer(pdev, APMF_FUNC_SBIOS_REQUESTS,
>>>> +									req, sizeof(*req));
>>>
>>> Fix the alignment please.
>>>
>>>> +}
>>>> +
>>>>  int apmf_get_sbios_requests(struct amd_pmf_dev *pdev, struct apmf_sbios_req *req)
>>>>  {
>>>>  	return apmf_if_call_store_buffer(pdev, APMF_FUNC_SBIOS_REQUESTS,
>>>> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
>>>> index 4364af72a7a3..f11d2a348696 100644
>>>> --- a/drivers/platform/x86/amd/pmf/pmf.h
>>>> +++ b/drivers/platform/x86/amd/pmf/pmf.h
>>>> @@ -116,6 +116,18 @@ struct apmf_sbios_req {
>>>>  	u8 skin_temp_hs2;
>>>>  } __packed;
>>>>  
>>>> +struct apmf_sbios_req_v2 {
>>>> +	u16 size;
>>>> +	u32 pending_req;
>>>> +	u8 rsvd;
>>>> +	u32 update_ppt_pmf;
>>>> +	u32 update_ppt_pmf_apu_only;
>>>> +	u32 update_stt_min;
>>>> +	u8 update_stt_apu;
>>>> +	u8 update_stt_hs2;
>>>
>>> Is it intentional that these do not match with the names in struct 
>>> apmf_sbios_req? I mean some of the fields look suspiciously close in name 
>>> so is the purpose still the same and somebody just invented new names for the 
>>> same field?
>>
>> The idea is to optimize certain fields in the BIOS menu that OEMs have
>> to feed in while making the right choices for the power settings for
>> different features.
>>
>> The entire series is targeted towards that where the interface between
>> the driver and the BIOS is improvised so that:
>>
>> - Multiple features can link to one state, so OEMs doesn’t need to
>> program same parameters in multiple locations.
>> - If we need to add new power controller limits , we don’t have to
>> touch APMF functions, its more expandable adding new fields in APS
>> methods.
>>
>> To answers to your question of apmf_sbios_req vs apmf_sbios_req_v2:
>>
>> It calls for a new struct _v2, because:
>> - AMT support has been dropped so there shall be no pending events
>> from DYTC (like the CQL and AMT)
>> - As per the new design, the PMFW has given control to set PPT and STT
>> limits and no pending requests on updating SPL limits.
>>
>> But as per names, I don't think there is no new invention :-)
> 
> Hi again,
> 
> I'm fine with adding _v2 struct (I could see they're not identical).
> 
> What I'm still left unsure if we spoke past each other so let me state 
> the question in more concrete terms:
> 
> - apmf_sbios_req has a field called stt_min_limit
> - apmf_sbios_req_v2 has a field called update_stt_min
> 
> My question is, are those just the same but only named differently for 
> some reason, or does the "limit" and/or "update" difference actually imply 
> there's change in how that field is used?

Hi Ilpo,

Apologies for the long delay.

Your question is valid and hence I had to go back to my FW
counterparts to get my basics right before responding back.

So the crux is, for each of the power controller within the CPU
infrastructure, like the Slow PPT, Fast PPT, STAPM, TDC SOC, EDC VDD
etc., all of them are guarded by two parameters:

- one, "limit", a max threshold a software can set
- two, "value", that can be updated to based on the changing system
dynamics.

So, atleast in the PMF driver context the field names can remain
constant. The field names in apmf_sbios_req looks apt here, so in the
next revision I will make fields in apmf_sbios_req and
apmf_sbios_req_v2 look the same (w.r.t the naming).

Before respin, can you have a look at the other patches and see if you
have remarks?

Thanks,
Shyam

> 
> Similar question applies to the other fields which look close but not
> identical. There's no need for you to itemize and explain each field for 
> me specifically in the reply, I just prefer the same thing called the same
> in both structs if that's the case.
> 
> It could be you tried to answer this with your second bullet but I just 
> don't understand its meaning deeply enough, thus I'm asking again, please 
> bear with me.
> 
>> FYI, here PPT means Power Packaging Tracking, so it could be SPPT
>> (Slow PPT) or FPPT (Fast PPT) and SST means Skin Temperature Tracking.
> 
> 

Re: [PATCH 3/7] platform/x86/amd/pmf: Add support to get sbios requests in PMF driver

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 5369 bytes --]

On Wed, 6 Mar 2024, Shyam Sundar S K wrote:
> On 2/29/2024 18:49, Ilpo Järvinen wrote:
> > On Wed, 28 Feb 2024, Shyam Sundar S K wrote:
> >> On 2/27/2024 18:58, Ilpo Järvinen wrote:
> >>> On Tue, 27 Feb 2024, Shyam Sundar S K wrote:
> >>>
> >>>> Update the APMF function index 2 for family 1Ah, that gets the
> >>>> information of  SBIOS requests (like the pending requests from BIOS,
> >>>
> >>> extra space.
> >>>
> >>>> custom notifications, updation of power limits etc).
> >>>>
> >>>> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> >>>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> >>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> >>>> ---
> >>>>  drivers/platform/x86/amd/pmf/acpi.c |  6 ++++++
> >>>>  drivers/platform/x86/amd/pmf/pmf.h  | 13 +++++++++++++
> >>>>  2 files changed, 19 insertions(+)
> >>>>
> >>>> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
> >>>> index 1f287a147c57..1b2a099c0cef 100644
> >>>> --- a/drivers/platform/x86/amd/pmf/acpi.c
> >>>> +++ b/drivers/platform/x86/amd/pmf/acpi.c
> >>>> @@ -166,6 +166,12 @@ int apmf_get_auto_mode_def(struct amd_pmf_dev *pdev, struct apmf_auto_mode *data
> >>>>  	return apmf_if_call_store_buffer(pdev, APMF_FUNC_AUTO_MODE, data, sizeof(*data));
> >>>>  }
> >>>>  
> >>>> +int apmf_get_sbios_requests_v2(struct amd_pmf_dev *pdev, struct apmf_sbios_req_v2 *req)
> >>>> +{
> >>>> +	return apmf_if_call_store_buffer(pdev, APMF_FUNC_SBIOS_REQUESTS,
> >>>> +									req, sizeof(*req));
> >>>
> >>> Fix the alignment please.
> >>>
> >>>> +}
> >>>> +
> >>>>  int apmf_get_sbios_requests(struct amd_pmf_dev *pdev, struct apmf_sbios_req *req)
> >>>>  {
> >>>>  	return apmf_if_call_store_buffer(pdev, APMF_FUNC_SBIOS_REQUESTS,
> >>>> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> >>>> index 4364af72a7a3..f11d2a348696 100644
> >>>> --- a/drivers/platform/x86/amd/pmf/pmf.h
> >>>> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> >>>> @@ -116,6 +116,18 @@ struct apmf_sbios_req {
> >>>>  	u8 skin_temp_hs2;
> >>>>  } __packed;
> >>>>  
> >>>> +struct apmf_sbios_req_v2 {
> >>>> +	u16 size;
> >>>> +	u32 pending_req;
> >>>> +	u8 rsvd;
> >>>> +	u32 update_ppt_pmf;
> >>>> +	u32 update_ppt_pmf_apu_only;
> >>>> +	u32 update_stt_min;
> >>>> +	u8 update_stt_apu;
> >>>> +	u8 update_stt_hs2;
> >>>
> >>> Is it intentional that these do not match with the names in struct 
> >>> apmf_sbios_req? I mean some of the fields look suspiciously close in name 
> >>> so is the purpose still the same and somebody just invented new names for the 
> >>> same field?
> >>
> >> The idea is to optimize certain fields in the BIOS menu that OEMs have
> >> to feed in while making the right choices for the power settings for
> >> different features.
> >>
> >> The entire series is targeted towards that where the interface between
> >> the driver and the BIOS is improvised so that:
> >>
> >> - Multiple features can link to one state, so OEMs doesn’t need to
> >> program same parameters in multiple locations.
> >> - If we need to add new power controller limits , we don’t have to
> >> touch APMF functions, its more expandable adding new fields in APS
> >> methods.
> >>
> >> To answers to your question of apmf_sbios_req vs apmf_sbios_req_v2:
> >>
> >> It calls for a new struct _v2, because:
> >> - AMT support has been dropped so there shall be no pending events
> >> from DYTC (like the CQL and AMT)
> >> - As per the new design, the PMFW has given control to set PPT and STT
> >> limits and no pending requests on updating SPL limits.
> >>
> >> But as per names, I don't think there is no new invention :-)
> > 
> > Hi again,
> > 
> > I'm fine with adding _v2 struct (I could see they're not identical).
> > 
> > What I'm still left unsure if we spoke past each other so let me state 
> > the question in more concrete terms:
> > 
> > - apmf_sbios_req has a field called stt_min_limit
> > - apmf_sbios_req_v2 has a field called update_stt_min
> > 
> > My question is, are those just the same but only named differently for 
> > some reason, or does the "limit" and/or "update" difference actually imply 
> > there's change in how that field is used?
> 
> Hi Ilpo,
> 
> Apologies for the long delay.
> 
> Your question is valid and hence I had to go back to my FW
> counterparts to get my basics right before responding back.
> 
> So the crux is, for each of the power controller within the CPU
> infrastructure, like the Slow PPT, Fast PPT, STAPM, TDC SOC, EDC VDD
> etc., all of them are guarded by two parameters:
> 
> - one, "limit", a max threshold a software can set
> - two, "value", that can be updated to based on the changing system
> dynamics.
> 
> So, atleast in the PMF driver context the field names can remain
> constant. The field names in apmf_sbios_req looks apt here, so in the
> next revision I will make fields in apmf_sbios_req and
> apmf_sbios_req_v2 look the same (w.r.t the naming).
> 
> Before respin, can you have a look at the other patches and see if you
> have remarks?

Hi,

I already looked at them briefly and didn't come across other things to 
say except that the use arrays made things cleaner. :-) So please just 
respin.

-- 
 i.

Re: [PATCH 3/7] platform/x86/amd/pmf: Add support to get sbios requests in PMF driver

[Shyam Sundar S K]

On 3/6/2024 16:04, Ilpo Järvinen wrote:
> On Wed, 6 Mar 2024, Shyam Sundar S K wrote:
>> On 2/29/2024 18:49, Ilpo Järvinen wrote:
>>> On Wed, 28 Feb 2024, Shyam Sundar S K wrote:
>>>> On 2/27/2024 18:58, Ilpo Järvinen wrote:
>>>>> On Tue, 27 Feb 2024, Shyam Sundar S K wrote:
>>>>>
>>>>>> Update the APMF function index 2 for family 1Ah, that gets the
>>>>>> information of  SBIOS requests (like the pending requests from BIOS,
>>>>>
>>>>> extra space.
>>>>>
>>>>>> custom notifications, updation of power limits etc).
>>>>>>
>>>>>> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>>>>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
>>>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>>>>>> ---
>>>>>>  drivers/platform/x86/amd/pmf/acpi.c |  6 ++++++
>>>>>>  drivers/platform/x86/amd/pmf/pmf.h  | 13 +++++++++++++
>>>>>>  2 files changed, 19 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
>>>>>> index 1f287a147c57..1b2a099c0cef 100644
>>>>>> --- a/drivers/platform/x86/amd/pmf/acpi.c
>>>>>> +++ b/drivers/platform/x86/amd/pmf/acpi.c
>>>>>> @@ -166,6 +166,12 @@ int apmf_get_auto_mode_def(struct amd_pmf_dev *pdev, struct apmf_auto_mode *data
>>>>>>  	return apmf_if_call_store_buffer(pdev, APMF_FUNC_AUTO_MODE, data, sizeof(*data));
>>>>>>  }
>>>>>>  
>>>>>> +int apmf_get_sbios_requests_v2(struct amd_pmf_dev *pdev, struct apmf_sbios_req_v2 *req)
>>>>>> +{
>>>>>> +	return apmf_if_call_store_buffer(pdev, APMF_FUNC_SBIOS_REQUESTS,
>>>>>> +									req, sizeof(*req));
>>>>>
>>>>> Fix the alignment please.
>>>>>
>>>>>> +}
>>>>>> +
>>>>>>  int apmf_get_sbios_requests(struct amd_pmf_dev *pdev, struct apmf_sbios_req *req)
>>>>>>  {
>>>>>>  	return apmf_if_call_store_buffer(pdev, APMF_FUNC_SBIOS_REQUESTS,
>>>>>> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
>>>>>> index 4364af72a7a3..f11d2a348696 100644
>>>>>> --- a/drivers/platform/x86/amd/pmf/pmf.h
>>>>>> +++ b/drivers/platform/x86/amd/pmf/pmf.h
>>>>>> @@ -116,6 +116,18 @@ struct apmf_sbios_req {
>>>>>>  	u8 skin_temp_hs2;
>>>>>>  } __packed;
>>>>>>  
>>>>>> +struct apmf_sbios_req_v2 {
>>>>>> +	u16 size;
>>>>>> +	u32 pending_req;
>>>>>> +	u8 rsvd;
>>>>>> +	u32 update_ppt_pmf;
>>>>>> +	u32 update_ppt_pmf_apu_only;
>>>>>> +	u32 update_stt_min;
>>>>>> +	u8 update_stt_apu;
>>>>>> +	u8 update_stt_hs2;
>>>>>
>>>>> Is it intentional that these do not match with the names in struct 
>>>>> apmf_sbios_req? I mean some of the fields look suspiciously close in name 
>>>>> so is the purpose still the same and somebody just invented new names for the 
>>>>> same field?
>>>>
>>>> The idea is to optimize certain fields in the BIOS menu that OEMs have
>>>> to feed in while making the right choices for the power settings for
>>>> different features.
>>>>
>>>> The entire series is targeted towards that where the interface between
>>>> the driver and the BIOS is improvised so that:
>>>>
>>>> - Multiple features can link to one state, so OEMs doesn’t need to
>>>> program same parameters in multiple locations.
>>>> - If we need to add new power controller limits , we don’t have to
>>>> touch APMF functions, its more expandable adding new fields in APS
>>>> methods.
>>>>
>>>> To answers to your question of apmf_sbios_req vs apmf_sbios_req_v2:
>>>>
>>>> It calls for a new struct _v2, because:
>>>> - AMT support has been dropped so there shall be no pending events
>>>> from DYTC (like the CQL and AMT)
>>>> - As per the new design, the PMFW has given control to set PPT and STT
>>>> limits and no pending requests on updating SPL limits.
>>>>
>>>> But as per names, I don't think there is no new invention :-)
>>>
>>> Hi again,
>>>
>>> I'm fine with adding _v2 struct (I could see they're not identical).
>>>
>>> What I'm still left unsure if we spoke past each other so let me state 
>>> the question in more concrete terms:
>>>
>>> - apmf_sbios_req has a field called stt_min_limit
>>> - apmf_sbios_req_v2 has a field called update_stt_min
>>>
>>> My question is, are those just the same but only named differently for 
>>> some reason, or does the "limit" and/or "update" difference actually imply 
>>> there's change in how that field is used?
>>
>> Hi Ilpo,
>>
>> Apologies for the long delay.
>>
>> Your question is valid and hence I had to go back to my FW
>> counterparts to get my basics right before responding back.
>>
>> So the crux is, for each of the power controller within the CPU
>> infrastructure, like the Slow PPT, Fast PPT, STAPM, TDC SOC, EDC VDD
>> etc., all of them are guarded by two parameters:
>>
>> - one, "limit", a max threshold a software can set
>> - two, "value", that can be updated to based on the changing system
>> dynamics.
>>
>> So, atleast in the PMF driver context the field names can remain
>> constant. The field names in apmf_sbios_req looks apt here, so in the
>> next revision I will make fields in apmf_sbios_req and
>> apmf_sbios_req_v2 look the same (w.r.t the naming).
>>
>> Before respin, can you have a look at the other patches and see if you
>> have remarks?
> 
> Hi,
> 
> I already looked at them briefly and didn't come across other things to 
> say except that the use arrays made things cleaner. :-) So please just 
> respin.
> 

Sure. You want this to be rebased to review-hans or review-ilpo tree?

Thanks,
Shyam

Re: [PATCH 3/7] platform/x86/amd/pmf: Add support to get sbios requests in PMF driver

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 5875 bytes --]

On Wed, 6 Mar 2024, Shyam Sundar S K wrote:
> On 3/6/2024 16:04, Ilpo Järvinen wrote:
> > On Wed, 6 Mar 2024, Shyam Sundar S K wrote:
> >> On 2/29/2024 18:49, Ilpo Järvinen wrote:
> >>> On Wed, 28 Feb 2024, Shyam Sundar S K wrote:
> >>>> On 2/27/2024 18:58, Ilpo Järvinen wrote:
> >>>>> On Tue, 27 Feb 2024, Shyam Sundar S K wrote:
> >>>>>
> >>>>>> Update the APMF function index 2 for family 1Ah, that gets the
> >>>>>> information of  SBIOS requests (like the pending requests from BIOS,
> >>>>>
> >>>>> extra space.
> >>>>>
> >>>>>> custom notifications, updation of power limits etc).
> >>>>>>
> >>>>>> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> >>>>>> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com>
> >>>>>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> >>>>>> ---
> >>>>>>  drivers/platform/x86/amd/pmf/acpi.c |  6 ++++++
> >>>>>>  drivers/platform/x86/amd/pmf/pmf.h  | 13 +++++++++++++
> >>>>>>  2 files changed, 19 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c
> >>>>>> index 1f287a147c57..1b2a099c0cef 100644
> >>>>>> --- a/drivers/platform/x86/amd/pmf/acpi.c
> >>>>>> +++ b/drivers/platform/x86/amd/pmf/acpi.c
> >>>>>> @@ -166,6 +166,12 @@ int apmf_get_auto_mode_def(struct amd_pmf_dev *pdev, struct apmf_auto_mode *data
> >>>>>>  	return apmf_if_call_store_buffer(pdev, APMF_FUNC_AUTO_MODE, data, sizeof(*data));
> >>>>>>  }
> >>>>>>  
> >>>>>> +int apmf_get_sbios_requests_v2(struct amd_pmf_dev *pdev, struct apmf_sbios_req_v2 *req)
> >>>>>> +{
> >>>>>> +	return apmf_if_call_store_buffer(pdev, APMF_FUNC_SBIOS_REQUESTS,
> >>>>>> +									req, sizeof(*req));
> >>>>>
> >>>>> Fix the alignment please.
> >>>>>
> >>>>>> +}
> >>>>>> +
> >>>>>>  int apmf_get_sbios_requests(struct amd_pmf_dev *pdev, struct apmf_sbios_req *req)
> >>>>>>  {
> >>>>>>  	return apmf_if_call_store_buffer(pdev, APMF_FUNC_SBIOS_REQUESTS,
> >>>>>> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h
> >>>>>> index 4364af72a7a3..f11d2a348696 100644
> >>>>>> --- a/drivers/platform/x86/amd/pmf/pmf.h
> >>>>>> +++ b/drivers/platform/x86/amd/pmf/pmf.h
> >>>>>> @@ -116,6 +116,18 @@ struct apmf_sbios_req {
> >>>>>>  	u8 skin_temp_hs2;
> >>>>>>  } __packed;
> >>>>>>  
> >>>>>> +struct apmf_sbios_req_v2 {
> >>>>>> +	u16 size;
> >>>>>> +	u32 pending_req;
> >>>>>> +	u8 rsvd;
> >>>>>> +	u32 update_ppt_pmf;
> >>>>>> +	u32 update_ppt_pmf_apu_only;
> >>>>>> +	u32 update_stt_min;
> >>>>>> +	u8 update_stt_apu;
> >>>>>> +	u8 update_stt_hs2;
> >>>>>
> >>>>> Is it intentional that these do not match with the names in struct 
> >>>>> apmf_sbios_req? I mean some of the fields look suspiciously close in name 
> >>>>> so is the purpose still the same and somebody just invented new names for the 
> >>>>> same field?
> >>>>
> >>>> The idea is to optimize certain fields in the BIOS menu that OEMs have
> >>>> to feed in while making the right choices for the power settings for
> >>>> different features.
> >>>>
> >>>> The entire series is targeted towards that where the interface between
> >>>> the driver and the BIOS is improvised so that:
> >>>>
> >>>> - Multiple features can link to one state, so OEMs doesn’t need to
> >>>> program same parameters in multiple locations.
> >>>> - If we need to add new power controller limits , we don’t have to
> >>>> touch APMF functions, its more expandable adding new fields in APS
> >>>> methods.
> >>>>
> >>>> To answers to your question of apmf_sbios_req vs apmf_sbios_req_v2:
> >>>>
> >>>> It calls for a new struct _v2, because:
> >>>> - AMT support has been dropped so there shall be no pending events
> >>>> from DYTC (like the CQL and AMT)
> >>>> - As per the new design, the PMFW has given control to set PPT and STT
> >>>> limits and no pending requests on updating SPL limits.
> >>>>
> >>>> But as per names, I don't think there is no new invention :-)
> >>>
> >>> Hi again,
> >>>
> >>> I'm fine with adding _v2 struct (I could see they're not identical).
> >>>
> >>> What I'm still left unsure if we spoke past each other so let me state 
> >>> the question in more concrete terms:
> >>>
> >>> - apmf_sbios_req has a field called stt_min_limit
> >>> - apmf_sbios_req_v2 has a field called update_stt_min
> >>>
> >>> My question is, are those just the same but only named differently for 
> >>> some reason, or does the "limit" and/or "update" difference actually imply 
> >>> there's change in how that field is used?
> >>
> >> Hi Ilpo,
> >>
> >> Apologies for the long delay.
> >>
> >> Your question is valid and hence I had to go back to my FW
> >> counterparts to get my basics right before responding back.
> >>
> >> So the crux is, for each of the power controller within the CPU
> >> infrastructure, like the Slow PPT, Fast PPT, STAPM, TDC SOC, EDC VDD
> >> etc., all of them are guarded by two parameters:
> >>
> >> - one, "limit", a max threshold a software can set
> >> - two, "value", that can be updated to based on the changing system
> >> dynamics.
> >>
> >> So, atleast in the PMF driver context the field names can remain
> >> constant. The field names in apmf_sbios_req looks apt here, so in the
> >> next revision I will make fields in apmf_sbios_req and
> >> apmf_sbios_req_v2 look the same (w.r.t the naming).
> >>
> >> Before respin, can you have a look at the other patches and see if you
> >> have remarks?
> > 
> > Hi,
> > 
> > I already looked at them briefly and didn't come across other things to 
> > say except that the use arrays made things cleaner. :-) So please just 
> > respin.
> 
> Sure. You want this to be rebased to review-hans or review-ilpo tree?

I'm handling for-next for this cycle so review-ilpo.

-- 
 i.