[PATCH 0/3] platform/x86/amd/pmf: Updates to AMD PMF driver

[PATCH 0/3] platform/x86/amd/pmf: Updates to AMD PMF driver

[Shyam Sundar S K]

This patch series includes:

1.Enable more debug information in the PMF driver
  - Add ACPI debug logs to get default thermals values for SPS, Auto
    Mode and CnQF during PMF driver initialization.
  - Add the PMF debug facility to enable more debug spew.

2. Change the SMN pair index for driver probing.

Shyam Sundar S K (3):
  platform/x86/amd/pmf: Add PMF acpi debug support
  platform/x86/amd/pmf: Add PMF debug facilities
  platform/x86/amd/pmf: Move out of BIOS SMN pair for driver probe

 drivers/platform/x86/amd/pmf/Kconfig     |  22 ++++
 drivers/platform/x86/amd/pmf/auto-mode.c | 142 +++++++++++++++++++++++
 drivers/platform/x86/amd/pmf/cnqf.c      |  85 +++++++++++++-
 drivers/platform/x86/amd/pmf/core.c      |  22 +---
 drivers/platform/x86/amd/pmf/sps.c       |  55 +++++++++
 5 files changed, 307 insertions(+), 19 deletions(-)

-- 
2.25.1

[PATCH 1/3] platform/x86/amd/pmf: Add PMF acpi debug support

[Shyam Sundar S K]

PMF driver maintains an internal config store for each PMF feature
after the feature init happens. Having a debug mechanism to triage
in-field issues w.r.t to mode switch not happening based on the OEM
fed values via the ACPI method to PMF driver is becoming the need of
the hour. Add support to get more ACPI debug spew guarded by a CONFIG.

Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/platform/x86/amd/pmf/Kconfig     |  11 +++
 drivers/platform/x86/amd/pmf/auto-mode.c | 120 +++++++++++++++++++++++
 drivers/platform/x86/amd/pmf/cnqf.c      |  66 ++++++++++++-
 drivers/platform/x86/amd/pmf/sps.c       |  55 +++++++++++
 4 files changed, 250 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/amd/pmf/Kconfig b/drivers/platform/x86/amd/pmf/Kconfig
index 6d89528c3177..f4fd764e55a6 100644
--- a/drivers/platform/x86/amd/pmf/Kconfig
+++ b/drivers/platform/x86/amd/pmf/Kconfig
@@ -15,3 +15,14 @@ config AMD_PMF
 
 	  To compile this driver as a module, choose M here: the module will
 	  be called amd_pmf.
+
+config AMD_PMF_ACPI_DEBUG
+	bool "AMD PMF acpi debug"
+	depends on AMD_PMF
+	help
+	 Enabling this option would give more debug information on the OEM fed
+	 power setting values for each of the PMF feature. PMF driver gets this
+	 information after evaluating a ACPI method and the information is stored
+	 in the PMF config store.
+
+	 Say Y here to enable more debug logs and Say N here if you are not sure.
diff --git a/drivers/platform/x86/amd/pmf/auto-mode.c b/drivers/platform/x86/amd/pmf/auto-mode.c
index 96a8e1832c05..777490fcf8b9 100644
--- a/drivers/platform/x86/amd/pmf/auto-mode.c
+++ b/drivers/platform/x86/amd/pmf/auto-mode.c
@@ -15,6 +15,98 @@
 static struct auto_mode_mode_config config_store;
 static const char *state_as_str(unsigned int state);
 
+#ifdef CONFIG_AMD_PMF_ACPI_DEBUG
+static void amd_pmf_dump_auto_mode_defaults(struct auto_mode_mode_config data)
+{
+	struct auto_mode_mode_settings *auto_mode;
+
+	pr_debug("Auto Mode Data - BEGIN\n");
+
+	/* time constant */
+	pr_debug("balanced_to_perf: %u\n",
+		 config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE]
+		 .time_constant);
+	pr_debug("perf_to_balanced: %u\n",
+		 config_store.transition[AUTO_TRANSITION_FROM_PERFORMANCE_TO_BALANCE]
+		 .time_constant);
+	pr_debug("quiet_to_balanced: %u\n",
+		 config_store.transition[AUTO_TRANSITION_FROM_QUIET_TO_BALANCE].time_constant);
+	pr_debug("balanced_to_quiet: %u\n",
+		 config_store.transition[AUTO_TRANSITION_TO_QUIET].time_constant);
+
+	/* power floor */
+	pr_debug("pfloor_perf: %u\n", config_store.mode_set[AUTO_PERFORMANCE].power_floor);
+	pr_debug("pfloor_balanced: %u\n", config_store.mode_set[AUTO_BALANCE].power_floor);
+	pr_debug("pfloor_quiet: %u\n", config_store.mode_set[AUTO_QUIET].power_floor);
+
+	/* Power delta for mode change */
+	pr_debug("pd_balanced_to_perf: %u\n",
+		 config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].power_delta);
+	pr_debug("pd_perf_to_balanced: %u\n",
+		 config_store.transition[AUTO_TRANSITION_FROM_PERFORMANCE_TO_BALANCE].power_delta);
+	pr_debug("pd_quiet_to_balanced: %u\n",
+		 config_store.transition[AUTO_TRANSITION_FROM_QUIET_TO_BALANCE].power_delta);
+	pr_debug("pd_balanced_to_quiet: %u\n",
+		 config_store.transition[AUTO_TRANSITION_TO_QUIET].power_delta);
+
+	/* skin temperature limits */
+	auto_mode = &config_store.mode_set[AUTO_PERFORMANCE_ON_LAP];
+	pr_debug("stt_apu_perf_on_lap: %u\n", auto_mode->power_control.stt_skin_temp[STT_TEMP_APU]);
+	pr_debug("stt_hs2_perf_on_lap: %u\n", auto_mode->power_control.stt_skin_temp[STT_TEMP_HS2]);
+	pr_debug("stt_min_limit_perf_on_lap: %u\n", auto_mode->power_control.stt_min);
+
+	auto_mode = &config_store.mode_set[AUTO_PERFORMANCE];
+	pr_debug("stt_apu_perf: %u\n", auto_mode->power_control.stt_skin_temp[STT_TEMP_APU]);
+	pr_debug("stt_hs2_perf: %u\n", auto_mode->power_control.stt_skin_temp[STT_TEMP_HS2]);
+	pr_debug("stt_min_limit_perf: %u\n", auto_mode->power_control.stt_min);
+
+	auto_mode = &config_store.mode_set[AUTO_BALANCE];
+	pr_debug("stt_apu_balanced: %u\n", auto_mode->power_control.stt_skin_temp[STT_TEMP_APU]);
+	pr_debug("stt_hs2_balanced: %u\n", auto_mode->power_control.stt_skin_temp[STT_TEMP_HS2]);
+	pr_debug("stt_min_limit_balanced: %u\n", auto_mode->power_control.stt_min);
+
+	auto_mode = &config_store.mode_set[AUTO_QUIET];
+	pr_debug("stt_apu_quiet: %u\n", auto_mode->power_control.stt_skin_temp[STT_TEMP_APU]);
+	pr_debug("stt_hs2_quiet: %u\n", auto_mode->power_control.stt_skin_temp[STT_TEMP_HS2]);
+	pr_debug("stt_min_limit_quiet: %u\n", auto_mode->power_control.stt_min);
+
+	/* SPL based power limits */
+	auto_mode = &config_store.mode_set[AUTO_PERFORMANCE_ON_LAP];
+	pr_debug("fppt_perf_on_lap: %u\n", auto_mode->power_control.fppt);
+	pr_debug("sppt_perf_on_lap: %u\n", auto_mode->power_control.sppt);
+	pr_debug("spl_perf_on_lap: %u\n", auto_mode->power_control.spl);
+	pr_debug("sppt_apu_only_perf_on_lap: %u\n", auto_mode->power_control.sppt_apu_only);
+
+	auto_mode = &config_store.mode_set[AUTO_PERFORMANCE];
+	pr_debug("fppt_perf: %u\n", auto_mode->power_control.fppt);
+	pr_debug("sppt_perf: %u\n", auto_mode->power_control.sppt);
+	pr_debug("spl_perf: %u\n", auto_mode->power_control.spl);
+	pr_debug("sppt_apu_only_perf: %u\n", auto_mode->power_control.sppt_apu_only);
+
+	auto_mode = &config_store.mode_set[AUTO_BALANCE];
+	pr_debug("fppt_balanced: %u\n", auto_mode->power_control.fppt);
+	pr_debug("sppt_balanced: %u\n", auto_mode->power_control.sppt);
+	pr_debug("spl_balanced: %u\n", auto_mode->power_control.spl);
+	pr_debug("sppt_apu_only_balanced: %u\n", auto_mode->power_control.sppt_apu_only);
+
+	auto_mode = &config_store.mode_set[AUTO_QUIET];
+	pr_debug("fppt_quiet: %u\n", auto_mode->power_control.fppt);
+	pr_debug("sppt_quiet: %u\n", auto_mode->power_control.sppt);
+	pr_debug("spl_quiet: %u\n", auto_mode->power_control.spl);
+	pr_debug("sppt_apu_only_quiet: %u\n", auto_mode->power_control.sppt_apu_only);
+
+	/* Fan ID */
+	pr_debug("fan_id_perf: %lu\n",
+		 config_store.mode_set[AUTO_PERFORMANCE].fan_control.fan_id);
+	pr_debug("fan_id_balanced: %lu\n",
+		 config_store.mode_set[AUTO_BALANCE].fan_control.fan_id);
+	pr_debug("fan_id_quiet: %lu\n",
+		 config_store.mode_set[AUTO_QUIET].fan_control.fan_id);
+
+	pr_debug("Auto Mode Data - END\n");
+}
+#endif
+
 static void amd_pmf_set_automode(struct amd_pmf_dev *dev, int idx,
 				 struct auto_mode_mode_config *table)
 {
@@ -140,6 +232,30 @@ static void amd_pmf_get_power_threshold(void)
 	config_store.transition[AUTO_TRANSITION_FROM_PERFORMANCE_TO_BALANCE].power_threshold =
 		config_store.mode_set[AUTO_PERFORMANCE].power_floor -
 		config_store.transition[AUTO_TRANSITION_FROM_PERFORMANCE_TO_BALANCE].power_delta;
+
+#ifdef CONFIG_AMD_PMF_ACPI_DEBUG
+	pr_debug("[AUTO MODE TO_QUIET] pt:%d pf:%d pd: %u",
+		 config_store.transition[AUTO_TRANSITION_TO_QUIET].power_threshold,
+		 config_store.mode_set[AUTO_BALANCE].power_floor,
+		 config_store.transition[AUTO_TRANSITION_TO_QUIET].power_delta);
+
+	pr_debug("[AUTO MODE TO_PERFORMANCE] pt:%d pf:%d pd: %u",
+		 config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].power_threshold,
+		 config_store.mode_set[AUTO_BALANCE].power_floor,
+		 config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].power_delta);
+
+	pr_debug("[AUTO MODE QUIET_TO_BALANCE] pt:%d pf:%d pd: %u",
+		 config_store.transition[AUTO_TRANSITION_FROM_QUIET_TO_BALANCE]
+		 .power_threshold,
+		 config_store.mode_set[AUTO_QUIET].power_floor,
+		 config_store.transition[AUTO_TRANSITION_FROM_QUIET_TO_BALANCE].power_delta);
+
+	pr_debug("[AUTO MODE PERFORMANCE_TO_BALANCE] pt:%d pf:%d pd: %u",
+		 config_store.transition[AUTO_TRANSITION_FROM_PERFORMANCE_TO_BALANCE]
+		 .power_threshold,
+		 config_store.mode_set[AUTO_PERFORMANCE].power_floor,
+		 config_store.transition[AUTO_TRANSITION_FROM_PERFORMANCE_TO_BALANCE].power_delta);
+#endif
 }
 
 static const char *state_as_str(unsigned int state)
@@ -262,6 +378,10 @@ static void amd_pmf_load_defaults_auto_mode(struct amd_pmf_dev *dev)
 	/* set to initial default values */
 	config_store.current_mode = AUTO_BALANCE;
 	dev->socket_power_history_idx = -1;
+
+#ifdef CONFIG_AMD_PMF_ACPI_DEBUG
+	amd_pmf_dump_auto_mode_defaults(config_store);
+#endif
 }
 
 int amd_pmf_reset_amt(struct amd_pmf_dev *dev)
diff --git a/drivers/platform/x86/amd/pmf/cnqf.c b/drivers/platform/x86/amd/pmf/cnqf.c
index 4beb22a19466..4b9691cd592a 100644
--- a/drivers/platform/x86/amd/pmf/cnqf.c
+++ b/drivers/platform/x86/amd/pmf/cnqf.c
@@ -13,6 +13,60 @@
 
 static struct cnqf_config config_store;
 
+#ifdef CONFIG_AMD_PMF_ACPI_DEBUG
+static const char *amd_pmf_cnqf_state_as_str(unsigned int state)
+{
+	switch (state) {
+	case APMF_CNQF_TURBO:
+		return "turbo";
+	case APMF_CNQF_PERFORMANCE:
+		return "performance";
+	case APMF_CNQF_BALANCE:
+		return "balance";
+	case APMF_CNQF_QUIET:
+		return "quiet";
+	default:
+		return "Unknown CnQF State";
+	}
+}
+
+static void amd_pmf_cnqf_dump_defaults(struct apmf_dyn_slider_output data, int idx)
+{
+	int i;
+
+	pr_debug("CnQF %s Defaults - BEGIN\n", idx ? "DC" : "AC");
+	pr_debug("size: %u\n", data.size);
+	pr_debug("flags: %u\n", data.flags);
+
+	/* Time constants */
+	pr_debug("t_perf_to_turbo: %u\n", data.t_perf_to_turbo);
+	pr_debug("t_balanced_to_perf: %u\n", data.t_balanced_to_perf);
+	pr_debug("t_quiet_to_balanced: %u\n", data.t_quiet_to_balanced);
+	pr_debug("t_balanced_to_quiet: %u\n", data.t_balanced_to_quiet);
+	pr_debug("t_perf_to_balanced: %u\n", data.t_perf_to_balanced);
+	pr_debug("t_turbo_to_perf: %u\n", data.t_turbo_to_perf);
+
+	for (i = 0 ; i < CNQF_MODE_MAX ; i++) {
+		pr_debug("pfloor_%s: %u\n", amd_pmf_cnqf_state_as_str(i), data.ps[i].pfloor);
+		pr_debug("fppt_%s: %u\n", amd_pmf_cnqf_state_as_str(i), data.ps[i].fppt);
+		pr_debug("sppt_%s: %u\n", amd_pmf_cnqf_state_as_str(i), data.ps[i].sppt);
+		pr_debug("sppt_apuonly_%s: %u\n", amd_pmf_cnqf_state_as_str(i),
+			 data.ps[i].sppt_apu_only);
+		pr_debug("spl_%s: %u\n", amd_pmf_cnqf_state_as_str(i),
+			 data.ps[i].spl);
+		pr_debug("stt_minlimit_%s: %u\n", amd_pmf_cnqf_state_as_str(i),
+			 data.ps[i].stt_min_limit);
+		pr_debug("stt_skintemp_apu_%s: %u\n", amd_pmf_cnqf_state_as_str(i),
+			 data.ps[i].stt_skintemp[STT_TEMP_APU]);
+		pr_debug("stt_skintemp_hs2_%s: %u\n", amd_pmf_cnqf_state_as_str(i),
+			 data.ps[i].stt_skintemp[STT_TEMP_HS2]);
+		pr_debug("fan_id_%s: %d\n", amd_pmf_cnqf_state_as_str(i), data.ps[i].fan_id);
+	}
+
+	pr_debug("CnQF %s Defaults - END\n", idx ? "DC" : "AC");
+}
+#endif
+
 static int amd_pmf_set_cnqf(struct amd_pmf_dev *dev, int src, int idx,
 			    struct cnqf_config *table)
 {
@@ -275,10 +329,18 @@ static int amd_pmf_load_defaults_cnqf(struct amd_pmf_dev *dev)
 		if (!is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_AC + i))
 			continue;
 
-		if (i == POWER_SOURCE_AC)
+		if (i == POWER_SOURCE_AC) {
 			ret = apmf_get_dyn_slider_def_ac(dev, &out);
-		else
+#ifdef CONFIG_AMD_PMF_ACPI_DEBUG
+			amd_pmf_cnqf_dump_defaults(out, i);
+#endif
+		} else {
 			ret = apmf_get_dyn_slider_def_dc(dev, &out);
+#ifdef CONFIG_AMD_PMF_ACPI_DEBUG
+			amd_pmf_cnqf_dump_defaults(out, i);
+#endif
+		}
+
 		if (ret) {
 			dev_err(dev->dev, "APMF apmf_get_dyn_slider_def_dc failed :%d\n", ret);
 			return ret;
diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c
index bed762d47a14..affb8ef4f203 100644
--- a/drivers/platform/x86/amd/pmf/sps.c
+++ b/drivers/platform/x86/amd/pmf/sps.c
@@ -12,6 +12,58 @@
 
 static struct amd_pmf_static_slider_granular config_store;
 
+#ifdef CONFIG_AMD_PMF_ACPI_DEBUG
+static const char *slider_as_str(unsigned int state)
+{
+	switch (state) {
+	case POWER_MODE_PERFORMANCE:
+		return "PERFORMANCE";
+	case POWER_MODE_BALANCED_POWER:
+		return "BALANCED_POWER";
+	case POWER_MODE_POWER_SAVER:
+		return "POWER_SAVER";
+	default:
+		return "Unknown Slider State";
+	}
+}
+
+static const char *source_as_str(unsigned int state)
+{
+	switch (state) {
+	case POWER_SOURCE_AC:
+		return "AC";
+	case POWER_SOURCE_DC:
+		return "DC";
+	default:
+		return "Unknown Power State";
+	}
+}
+
+static void amd_pmf_dump_sps_defaults(struct amd_pmf_static_slider_granular data)
+{
+	int i, j;
+
+	pr_debug("Static Slider Data - BEGIN\n");
+
+	for (i = 0; i < POWER_SOURCE_MAX; i++) {
+		for (j = 0; j < POWER_MODE_MAX; j++) {
+			pr_debug("--- Source:%s Mode:%s ---\n", source_as_str(i), slider_as_str(j));
+			pr_debug("SPL: %u mW\n", data.prop[i][j].spl);
+			pr_debug("SPPT: %u mW\n", data.prop[i][j].sppt);
+			pr_debug("SPPT_ApuOnly: %u mW\n", data.prop[i][j].sppt_apu_only);
+			pr_debug("FPPT: %u mW\n", data.prop[i][j].fppt);
+			pr_debug("STTMinLimit: %u mW\n", data.prop[i][j].stt_min);
+			pr_debug("STT_SkinTempLimit_APU: %u C\n",
+				 data.prop[i][j].stt_skin_temp[STT_TEMP_APU]);
+			pr_debug("STT_SkinTempLimit_HS2: %u C\n",
+				 data.prop[i][j].stt_skin_temp[STT_TEMP_HS2]);
+		}
+	}
+
+	pr_debug("Static Slider Data - END\n");
+}
+#endif
+
 static void amd_pmf_load_defaults_sps(struct amd_pmf_dev *dev)
 {
 	struct apmf_static_slider_granular_output output;
@@ -36,6 +88,9 @@ static void amd_pmf_load_defaults_sps(struct amd_pmf_dev *dev)
 			idx++;
 		}
 	}
+#ifdef CONFIG_AMD_PMF_ACPI_DEBUG
+	amd_pmf_dump_sps_defaults(config_store);
+#endif
 }
 
 void amd_pmf_update_slider(struct amd_pmf_dev *dev, bool op, int idx,
-- 
2.25.1

Re: [PATCH 1/3] platform/x86/amd/pmf: Add PMF acpi debug support

[Hans de Goede]

Hi Shyam,

On 4/6/23 18:48, Shyam Sundar S K wrote:
> PMF driver maintains an internal config store for each PMF feature
> after the feature init happens. Having a debug mechanism to triage
> in-field issues w.r.t to mode switch not happening based on the OEM
> fed values via the ACPI method to PMF driver is becoming the need of
> the hour. Add support to get more ACPI debug spew guarded by a CONFIG.
> 
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>  drivers/platform/x86/amd/pmf/Kconfig     |  11 +++
>  drivers/platform/x86/amd/pmf/auto-mode.c | 120 +++++++++++++++++++++++
>  drivers/platform/x86/amd/pmf/cnqf.c      |  66 ++++++++++++-
>  drivers/platform/x86/amd/pmf/sps.c       |  55 +++++++++++
>  4 files changed, 250 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmf/Kconfig b/drivers/platform/x86/amd/pmf/Kconfig
> index 6d89528c3177..f4fd764e55a6 100644
> --- a/drivers/platform/x86/amd/pmf/Kconfig
> +++ b/drivers/platform/x86/amd/pmf/Kconfig
> @@ -15,3 +15,14 @@ config AMD_PMF
>  
>  	  To compile this driver as a module, choose M here: the module will
>  	  be called amd_pmf.
> +
> +config AMD_PMF_ACPI_DEBUG
> +	bool "AMD PMF acpi debug"
> +	depends on AMD_PMF
> +	help
> +	 Enabling this option would give more debug information on the OEM fed
> +	 power setting values for each of the PMF feature. PMF driver gets this
> +	 information after evaluating a ACPI method and the information is stored
> +	 in the PMF config store.
> +
> +	 Say Y here to enable more debug logs and Say N here if you are not sure.
> diff --git a/drivers/platform/x86/amd/pmf/auto-mode.c b/drivers/platform/x86/amd/pmf/auto-mode.c
> index 96a8e1832c05..777490fcf8b9 100644
> --- a/drivers/platform/x86/amd/pmf/auto-mode.c
> +++ b/drivers/platform/x86/amd/pmf/auto-mode.c
> @@ -15,6 +15,98 @@
>  static struct auto_mode_mode_config config_store;
>  static const char *state_as_str(unsigned int state);
>  
> +#ifdef CONFIG_AMD_PMF_ACPI_DEBUG
> +static void amd_pmf_dump_auto_mode_defaults(struct auto_mode_mode_config data)

Please use a pointer here. Right now you are making a copy of the entire
struct on the stack and stack space is limited in the kernel.

> +{
> +	struct auto_mode_mode_settings *auto_mode;
> +
> +	pr_debug("Auto Mode Data - BEGIN\n");
> +
> +	/* time constant */
> +	pr_debug("balanced_to_perf: %u\n",
> +		 config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE]
> +		 .time_constant);
> +	pr_debug("perf_to_balanced: %u\n",
> +		 config_store.transition[AUTO_TRANSITION_FROM_PERFORMANCE_TO_BALANCE]
> +		 .time_constant);
> +	pr_debug("quiet_to_balanced: %u\n",
> +		 config_store.transition[AUTO_TRANSITION_FROM_QUIET_TO_BALANCE].time_constant);
> +	pr_debug("balanced_to_quiet: %u\n",
> +		 config_store.transition[AUTO_TRANSITION_TO_QUIET].time_constant);
> +
> +	/* power floor */
> +	pr_debug("pfloor_perf: %u\n", config_store.mode_set[AUTO_PERFORMANCE].power_floor);
> +	pr_debug("pfloor_balanced: %u\n", config_store.mode_set[AUTO_BALANCE].power_floor);
> +	pr_debug("pfloor_quiet: %u\n", config_store.mode_set[AUTO_QUIET].power_floor);
> +
> +	/* Power delta for mode change */
> +	pr_debug("pd_balanced_to_perf: %u\n",
> +		 config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].power_delta);
> +	pr_debug("pd_perf_to_balanced: %u\n",
> +		 config_store.transition[AUTO_TRANSITION_FROM_PERFORMANCE_TO_BALANCE].power_delta);
> +	pr_debug("pd_quiet_to_balanced: %u\n",
> +		 config_store.transition[AUTO_TRANSITION_FROM_QUIET_TO_BALANCE].power_delta);
> +	pr_debug("pd_balanced_to_quiet: %u\n",
> +		 config_store.transition[AUTO_TRANSITION_TO_QUIET].power_delta);
> +
> +	/* skin temperature limits */
> +	auto_mode = &config_store.mode_set[AUTO_PERFORMANCE_ON_LAP];
> +	pr_debug("stt_apu_perf_on_lap: %u\n", auto_mode->power_control.stt_skin_temp[STT_TEMP_APU]);
> +	pr_debug("stt_hs2_perf_on_lap: %u\n", auto_mode->power_control.stt_skin_temp[STT_TEMP_HS2]);
> +	pr_debug("stt_min_limit_perf_on_lap: %u\n", auto_mode->power_control.stt_min);
> +
> +	auto_mode = &config_store.mode_set[AUTO_PERFORMANCE];
> +	pr_debug("stt_apu_perf: %u\n", auto_mode->power_control.stt_skin_temp[STT_TEMP_APU]);
> +	pr_debug("stt_hs2_perf: %u\n", auto_mode->power_control.stt_skin_temp[STT_TEMP_HS2]);
> +	pr_debug("stt_min_limit_perf: %u\n", auto_mode->power_control.stt_min);
> +
> +	auto_mode = &config_store.mode_set[AUTO_BALANCE];
> +	pr_debug("stt_apu_balanced: %u\n", auto_mode->power_control.stt_skin_temp[STT_TEMP_APU]);
> +	pr_debug("stt_hs2_balanced: %u\n", auto_mode->power_control.stt_skin_temp[STT_TEMP_HS2]);
> +	pr_debug("stt_min_limit_balanced: %u\n", auto_mode->power_control.stt_min);
> +
> +	auto_mode = &config_store.mode_set[AUTO_QUIET];
> +	pr_debug("stt_apu_quiet: %u\n", auto_mode->power_control.stt_skin_temp[STT_TEMP_APU]);
> +	pr_debug("stt_hs2_quiet: %u\n", auto_mode->power_control.stt_skin_temp[STT_TEMP_HS2]);
> +	pr_debug("stt_min_limit_quiet: %u\n", auto_mode->power_control.stt_min);
> +
> +	/* SPL based power limits */
> +	auto_mode = &config_store.mode_set[AUTO_PERFORMANCE_ON_LAP];
> +	pr_debug("fppt_perf_on_lap: %u\n", auto_mode->power_control.fppt);
> +	pr_debug("sppt_perf_on_lap: %u\n", auto_mode->power_control.sppt);
> +	pr_debug("spl_perf_on_lap: %u\n", auto_mode->power_control.spl);
> +	pr_debug("sppt_apu_only_perf_on_lap: %u\n", auto_mode->power_control.sppt_apu_only);
> +
> +	auto_mode = &config_store.mode_set[AUTO_PERFORMANCE];
> +	pr_debug("fppt_perf: %u\n", auto_mode->power_control.fppt);
> +	pr_debug("sppt_perf: %u\n", auto_mode->power_control.sppt);
> +	pr_debug("spl_perf: %u\n", auto_mode->power_control.spl);
> +	pr_debug("sppt_apu_only_perf: %u\n", auto_mode->power_control.sppt_apu_only);
> +
> +	auto_mode = &config_store.mode_set[AUTO_BALANCE];
> +	pr_debug("fppt_balanced: %u\n", auto_mode->power_control.fppt);
> +	pr_debug("sppt_balanced: %u\n", auto_mode->power_control.sppt);
> +	pr_debug("spl_balanced: %u\n", auto_mode->power_control.spl);
> +	pr_debug("sppt_apu_only_balanced: %u\n", auto_mode->power_control.sppt_apu_only);
> +
> +	auto_mode = &config_store.mode_set[AUTO_QUIET];
> +	pr_debug("fppt_quiet: %u\n", auto_mode->power_control.fppt);
> +	pr_debug("sppt_quiet: %u\n", auto_mode->power_control.sppt);
> +	pr_debug("spl_quiet: %u\n", auto_mode->power_control.spl);
> +	pr_debug("sppt_apu_only_quiet: %u\n", auto_mode->power_control.sppt_apu_only);
> +
> +	/* Fan ID */
> +	pr_debug("fan_id_perf: %lu\n",
> +		 config_store.mode_set[AUTO_PERFORMANCE].fan_control.fan_id);
> +	pr_debug("fan_id_balanced: %lu\n",
> +		 config_store.mode_set[AUTO_BALANCE].fan_control.fan_id);
> +	pr_debug("fan_id_quiet: %lu\n",
> +		 config_store.mode_set[AUTO_QUIET].fan_control.fan_id);
> +
> +	pr_debug("Auto Mode Data - END\n");
> +}

Please add:

#else
static void amd_pmf_dump_auto_mode_defaults(struct auto_mode_mode_config *data) {}

here.

> +#endif
> +
>  static void amd_pmf_set_automode(struct amd_pmf_dev *dev, int idx,
>  				 struct auto_mode_mode_config *table)
>  {
> @@ -140,6 +232,30 @@ static void amd_pmf_get_power_threshold(void)
>  	config_store.transition[AUTO_TRANSITION_FROM_PERFORMANCE_TO_BALANCE].power_threshold =
>  		config_store.mode_set[AUTO_PERFORMANCE].power_floor -
>  		config_store.transition[AUTO_TRANSITION_FROM_PERFORMANCE_TO_BALANCE].power_delta;
> +
> +#ifdef CONFIG_AMD_PMF_ACPI_DEBUG
> +	pr_debug("[AUTO MODE TO_QUIET] pt:%d pf:%d pd: %u",
> +		 config_store.transition[AUTO_TRANSITION_TO_QUIET].power_threshold,
> +		 config_store.mode_set[AUTO_BALANCE].power_floor,
> +		 config_store.transition[AUTO_TRANSITION_TO_QUIET].power_delta);
> +
> +	pr_debug("[AUTO MODE TO_PERFORMANCE] pt:%d pf:%d pd: %u",
> +		 config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].power_threshold,
> +		 config_store.mode_set[AUTO_BALANCE].power_floor,
> +		 config_store.transition[AUTO_TRANSITION_TO_PERFORMANCE].power_delta);
> +
> +	pr_debug("[AUTO MODE QUIET_TO_BALANCE] pt:%d pf:%d pd: %u",
> +		 config_store.transition[AUTO_TRANSITION_FROM_QUIET_TO_BALANCE]
> +		 .power_threshold,
> +		 config_store.mode_set[AUTO_QUIET].power_floor,
> +		 config_store.transition[AUTO_TRANSITION_FROM_QUIET_TO_BALANCE].power_delta);
> +
> +	pr_debug("[AUTO MODE PERFORMANCE_TO_BALANCE] pt:%d pf:%d pd: %u",
> +		 config_store.transition[AUTO_TRANSITION_FROM_PERFORMANCE_TO_BALANCE]
> +		 .power_threshold,
> +		 config_store.mode_set[AUTO_PERFORMANCE].power_floor,
> +		 config_store.transition[AUTO_TRANSITION_FROM_PERFORMANCE_TO_BALANCE].power_delta);
> +#endif
>  }
>  
>  static const char *state_as_str(unsigned int state)
> @@ -262,6 +378,10 @@ static void amd_pmf_load_defaults_auto_mode(struct amd_pmf_dev *dev)
>  	/* set to initial default values */
>  	config_store.current_mode = AUTO_BALANCE;
>  	dev->socket_power_history_idx = -1;
> +
> +#ifdef CONFIG_AMD_PMF_ACPI_DEBUG
> +	amd_pmf_dump_auto_mode_defaults(config_store);
> +#endif

And drop the #ifdef and #endif lines here.

>  }
>  
>  int amd_pmf_reset_amt(struct amd_pmf_dev *dev)
> diff --git a/drivers/platform/x86/amd/pmf/cnqf.c b/drivers/platform/x86/amd/pmf/cnqf.c
> index 4beb22a19466..4b9691cd592a 100644
> --- a/drivers/platform/x86/amd/pmf/cnqf.c
> +++ b/drivers/platform/x86/amd/pmf/cnqf.c
> @@ -13,6 +13,60 @@
>  
>  static struct cnqf_config config_store;
>  
> +#ifdef CONFIG_AMD_PMF_ACPI_DEBUG
> +static const char *amd_pmf_cnqf_state_as_str(unsigned int state)
> +{
> +	switch (state) {
> +	case APMF_CNQF_TURBO:
> +		return "turbo";
> +	case APMF_CNQF_PERFORMANCE:
> +		return "performance";
> +	case APMF_CNQF_BALANCE:
> +		return "balance";
> +	case APMF_CNQF_QUIET:
> +		return "quiet";
> +	default:
> +		return "Unknown CnQF State";
> +	}
> +}
> +
> +static void amd_pmf_cnqf_dump_defaults(struct apmf_dyn_slider_output data, int idx)

Please use a pointer here. Right now you are making a copy of the entire
struct on the stack and stack space is limited in the kernel.

> +{
> +	int i;
> +
> +	pr_debug("CnQF %s Defaults - BEGIN\n", idx ? "DC" : "AC");
> +	pr_debug("size: %u\n", data.size);
> +	pr_debug("flags: %u\n", data.flags);
> +
> +	/* Time constants */
> +	pr_debug("t_perf_to_turbo: %u\n", data.t_perf_to_turbo);
> +	pr_debug("t_balanced_to_perf: %u\n", data.t_balanced_to_perf);
> +	pr_debug("t_quiet_to_balanced: %u\n", data.t_quiet_to_balanced);
> +	pr_debug("t_balanced_to_quiet: %u\n", data.t_balanced_to_quiet);
> +	pr_debug("t_perf_to_balanced: %u\n", data.t_perf_to_balanced);
> +	pr_debug("t_turbo_to_perf: %u\n", data.t_turbo_to_perf);
> +
> +	for (i = 0 ; i < CNQF_MODE_MAX ; i++) {
> +		pr_debug("pfloor_%s: %u\n", amd_pmf_cnqf_state_as_str(i), data.ps[i].pfloor);
> +		pr_debug("fppt_%s: %u\n", amd_pmf_cnqf_state_as_str(i), data.ps[i].fppt);
> +		pr_debug("sppt_%s: %u\n", amd_pmf_cnqf_state_as_str(i), data.ps[i].sppt);
> +		pr_debug("sppt_apuonly_%s: %u\n", amd_pmf_cnqf_state_as_str(i),
> +			 data.ps[i].sppt_apu_only);
> +		pr_debug("spl_%s: %u\n", amd_pmf_cnqf_state_as_str(i),
> +			 data.ps[i].spl);
> +		pr_debug("stt_minlimit_%s: %u\n", amd_pmf_cnqf_state_as_str(i),
> +			 data.ps[i].stt_min_limit);
> +		pr_debug("stt_skintemp_apu_%s: %u\n", amd_pmf_cnqf_state_as_str(i),
> +			 data.ps[i].stt_skintemp[STT_TEMP_APU]);
> +		pr_debug("stt_skintemp_hs2_%s: %u\n", amd_pmf_cnqf_state_as_str(i),
> +			 data.ps[i].stt_skintemp[STT_TEMP_HS2]);
> +		pr_debug("fan_id_%s: %d\n", amd_pmf_cnqf_state_as_str(i), data.ps[i].fan_id);
> +	}
> +
> +	pr_debug("CnQF %s Defaults - END\n", idx ? "DC" : "AC");
> +}

Please add:

#else
static void amd_pmf_cnqf_dump_defaults(struct apmf_dyn_slider_output *data, int idx) {}

here.

> +#endif
> +
>  static int amd_pmf_set_cnqf(struct amd_pmf_dev *dev, int src, int idx,
>  			    struct cnqf_config *table)
>  {
> @@ -275,10 +329,18 @@ static int amd_pmf_load_defaults_cnqf(struct amd_pmf_dev *dev)
>  		if (!is_apmf_func_supported(dev, APMF_FUNC_DYN_SLIDER_AC + i))
>  			continue;
>  
> -		if (i == POWER_SOURCE_AC)
> +		if (i == POWER_SOURCE_AC) {
>  			ret = apmf_get_dyn_slider_def_ac(dev, &out);
> -		else
> +#ifdef CONFIG_AMD_PMF_ACPI_DEBUG
> +			amd_pmf_cnqf_dump_defaults(out, i);
> +#endif

And drop the #ifdef and #endif lines here.

> +		} else {
>  			ret = apmf_get_dyn_slider_def_dc(dev, &out);
> +#ifdef CONFIG_AMD_PMF_ACPI_DEBUG
> +			amd_pmf_cnqf_dump_defaults(out, i);
> +#endif

And here.

> +		}
> +
>  		if (ret) {
>  			dev_err(dev->dev, "APMF apmf_get_dyn_slider_def_dc failed :%d\n", ret);
>  			return ret;
> diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c
> index bed762d47a14..affb8ef4f203 100644
> --- a/drivers/platform/x86/amd/pmf/sps.c
> +++ b/drivers/platform/x86/amd/pmf/sps.c
> @@ -12,6 +12,58 @@
>  
>  static struct amd_pmf_static_slider_granular config_store;
>  
> +#ifdef CONFIG_AMD_PMF_ACPI_DEBUG
> +static const char *slider_as_str(unsigned int state)
> +{
> +	switch (state) {
> +	case POWER_MODE_PERFORMANCE:
> +		return "PERFORMANCE";
> +	case POWER_MODE_BALANCED_POWER:
> +		return "BALANCED_POWER";
> +	case POWER_MODE_POWER_SAVER:
> +		return "POWER_SAVER";
> +	default:
> +		return "Unknown Slider State";
> +	}
> +}
> +
> +static const char *source_as_str(unsigned int state)
> +{
> +	switch (state) {
> +	case POWER_SOURCE_AC:
> +		return "AC";
> +	case POWER_SOURCE_DC:
> +		return "DC";
> +	default:
> +		return "Unknown Power State";
> +	}
> +}
> +
> +static void amd_pmf_dump_sps_defaults(struct amd_pmf_static_slider_granular data)

Please use a pointer here. Right now you are making a copy of the entire
struct on the stack and stack space is limited in the kernel.

> +{
> +	int i, j;
> +
> +	pr_debug("Static Slider Data - BEGIN\n");
> +
> +	for (i = 0; i < POWER_SOURCE_MAX; i++) {
> +		for (j = 0; j < POWER_MODE_MAX; j++) {
> +			pr_debug("--- Source:%s Mode:%s ---\n", source_as_str(i), slider_as_str(j));
> +			pr_debug("SPL: %u mW\n", data.prop[i][j].spl);
> +			pr_debug("SPPT: %u mW\n", data.prop[i][j].sppt);
> +			pr_debug("SPPT_ApuOnly: %u mW\n", data.prop[i][j].sppt_apu_only);
> +			pr_debug("FPPT: %u mW\n", data.prop[i][j].fppt);
> +			pr_debug("STTMinLimit: %u mW\n", data.prop[i][j].stt_min);
> +			pr_debug("STT_SkinTempLimit_APU: %u C\n",
> +				 data.prop[i][j].stt_skin_temp[STT_TEMP_APU]);
> +			pr_debug("STT_SkinTempLimit_HS2: %u C\n",
> +				 data.prop[i][j].stt_skin_temp[STT_TEMP_HS2]);
> +		}
> +	}
> +
> +	pr_debug("Static Slider Data - END\n");
> +}

Please add:

#else
static void amd_pmf_dump_sps_defaults(struct amd_pmf_static_slider_granular *data) {}

here.

> +#endif
> +
>  static void amd_pmf_load_defaults_sps(struct amd_pmf_dev *dev)
>  {
>  	struct apmf_static_slider_granular_output output;
> @@ -36,6 +88,9 @@ static void amd_pmf_load_defaults_sps(struct amd_pmf_dev *dev)
>  			idx++;
>  		}
>  	}
> +#ifdef CONFIG_AMD_PMF_ACPI_DEBUG
> +	amd_pmf_dump_sps_defaults(config_store);
> +#endif

And drop the #ifdef and #endif lines here.

>  }
>  
>  void amd_pmf_update_slider(struct amd_pmf_dev *dev, bool op, int idx,

Regards,

Hans

[PATCH 2/3] platform/x86/amd/pmf: Add PMF debug facilities

[Shyam Sundar S K]

At times, when the mode transitions fail to happen, the current
driver does not give enough debug information on why the transition
failed or the default preset values did not load. Having an on-demand
logs guarded by CONFIG would be helpful in such cases.

Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
---
 drivers/platform/x86/amd/pmf/Kconfig     | 10 ++++++++++
 drivers/platform/x86/amd/pmf/auto-mode.c | 22 ++++++++++++++++++++++
 drivers/platform/x86/amd/pmf/cnqf.c      | 19 +++++++++++++++++++
 3 files changed, 51 insertions(+)

diff --git a/drivers/platform/x86/amd/pmf/Kconfig b/drivers/platform/x86/amd/pmf/Kconfig
index f4fd764e55a6..7129de0fb9fb 100644
--- a/drivers/platform/x86/amd/pmf/Kconfig
+++ b/drivers/platform/x86/amd/pmf/Kconfig
@@ -26,3 +26,13 @@ config AMD_PMF_ACPI_DEBUG
 	 in the PMF config store.
 
 	 Say Y here to enable more debug logs and Say N here if you are not sure.
+
+config AMD_PMF_DEBUG_FACILITIES
+	bool "PMF debug facilities"
+	depends on AMD_PMF
+	help
+	 Enabling this option would give more debug information on the PMF interna
+	 counters such as time constants, power thresholds, target modes and
+	 power mode transitions of auto mode and CnQF features.
+
+	 Say Y here to enable logs and Say N here if you are not sure.
diff --git a/drivers/platform/x86/amd/pmf/auto-mode.c b/drivers/platform/x86/amd/pmf/auto-mode.c
index 777490fcf8b9..560379b5cda7 100644
--- a/drivers/platform/x86/amd/pmf/auto-mode.c
+++ b/drivers/platform/x86/amd/pmf/auto-mode.c
@@ -177,11 +177,33 @@ void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t t
 			config_store.transition[i].applied = false;
 			update = true;
 		}
+
+#ifdef CONFIG_AMD_PMF_DEBUG_FACILITIES
+		dev_dbg(dev->dev, "[AUTO MODE] time_ms:%lld avg_power:%d mode:%s timer:%u tc:%d\n",
+			time_elapsed_ms, avg_power,
+			state_as_str(config_store.current_mode),
+			config_store.transition[i].timer,
+			config_store.transition[i].time_constant);
+
+		dev_dbg(dev->dev, "[AUTO MODE] shiftup:%d pt:%d pf:%d pd:%u\n",
+			config_store.transition[i].shifting_up,
+			config_store.transition[i].power_threshold,
+			config_store.mode_set[i].power_floor,
+			config_store.transition[i].power_delta);
+#endif
 	}
 
 	dev_dbg(dev->dev, "[AUTO_MODE] avg power: %u mW mode: %s\n", avg_power,
 		state_as_str(config_store.current_mode));
 
+#ifdef CONFIG_AMD_PMF_DEBUG_FACILITIES
+	dev_dbg(dev->dev, "[AUTO MODE] priority1: %u, priority2: %u, priority3: %u, priority4: %u",
+		config_store.transition[0].applied,
+		config_store.transition[1].applied,
+		config_store.transition[2].applied,
+		config_store. transition[3].applied);
+#endif
+
 	if (update) {
 		for (j = 0; j < AUTO_TRANSITION_MAX; j++) {
 			/* Apply the mode with highest priority indentified */
diff --git a/drivers/platform/x86/amd/pmf/cnqf.c b/drivers/platform/x86/amd/pmf/cnqf.c
index 4b9691cd592a..1f25016b3865 100644
--- a/drivers/platform/x86/amd/pmf/cnqf.c
+++ b/drivers/platform/x86/amd/pmf/cnqf.c
@@ -174,6 +174,13 @@ int amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_l
 		config_store.trans_param[src][i].count++;
 
 		tp = &config_store.trans_param[src][i];
+
+#ifdef CONFIG_AMD_PMF_DEBUG_FACILITIES
+		dev_dbg(dev->dev, "avg_power:%d total_power:%d count:%d timer:%d\n", avg_power,
+			config_store.trans_param[src][i].total_power,
+			config_store.trans_param[src][i].count,
+			config_store.trans_param[src][i].timer);
+#endif
 		if (tp->timer >= tp->time_constant && tp->count) {
 			avg_power = tp->total_power / tp->count;
 
@@ -194,6 +201,18 @@ int amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_l
 	dev_dbg(dev->dev, "[CNQF] Avg power: %u mW socket power: %u mW mode:%s\n",
 		avg_power, socket_power, state_as_str(config_store.current_mode));
 
+#ifdef AMD_PMF_DEBUG_FACILITIES
+	dev_dbg(dev->dev, "[CNQF] priority 1: %u, priority 2: %u, priority 3: %u",
+		config_store.trans_param[src][0].priority,
+		config_store.trans_param[src][1].priority,
+		config_store.trans_param[src][2].priority);
+
+	dev_dbg(dev->dev, "[CNQF] priority 4: %u, priority 5: %u, priority 6: %u",
+		config_store.trans_param[src][3].priority,
+		config_store.trans_param[src][4].priority,
+		config_store.trans_param[src][5].priority);
+#endif
+
 	for (j = 0; j < CNQF_TRANSITION_MAX; j++) {
 		/* apply the highest priority */
 		if (config_store.trans_param[src][j].priority) {
-- 
2.25.1

Re: [PATCH 2/3] platform/x86/amd/pmf: Add PMF debug facilities

[Limonciello, Mario]

On 4/6/2023 11:48, Shyam Sundar S K wrote:
> At times, when the mode transitions fail to happen, the current
> driver does not give enough debug information on why the transition
> failed or the default preset values did not load. Having an on-demand
> logs guarded by CONFIG would be helpful in such cases.
> 
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>   drivers/platform/x86/amd/pmf/Kconfig     | 10 ++++++++++
>   drivers/platform/x86/amd/pmf/auto-mode.c | 22 ++++++++++++++++++++++
>   drivers/platform/x86/amd/pmf/cnqf.c      | 19 +++++++++++++++++++
>   3 files changed, 51 insertions(+)
> 
> diff --git a/drivers/platform/x86/amd/pmf/Kconfig b/drivers/platform/x86/amd/pmf/Kconfig
> index f4fd764e55a6..7129de0fb9fb 100644
> --- a/drivers/platform/x86/amd/pmf/Kconfig
> +++ b/drivers/platform/x86/amd/pmf/Kconfig
> @@ -26,3 +26,13 @@ config AMD_PMF_ACPI_DEBUG
>   	 in the PMF config store.
>   
>   	 Say Y here to enable more debug logs and Say N here if you are not sure.
> +
> +config AMD_PMF_DEBUG_FACILITIES
> +	bool "PMF debug facilities"
> +	depends on AMD_PMF
> +	help
> +	 Enabling this option would give more debug information on the PMF interna
> +	 counters such as time constants, power thresholds, target modes and
> +	 power mode transitions of auto mode and CnQF features.

With the availability of dynamic debugging is there a lot of benefit to 
guarding all the new dev_dbg statements behind a config option?

Is it because of performance impact?

> +
> +	 Say Y here to enable logs and Say N here if you are not sure.
> diff --git a/drivers/platform/x86/amd/pmf/auto-mode.c b/drivers/platform/x86/amd/pmf/auto-mode.c
> index 777490fcf8b9..560379b5cda7 100644
> --- a/drivers/platform/x86/amd/pmf/auto-mode.c
> +++ b/drivers/platform/x86/amd/pmf/auto-mode.c
> @@ -177,11 +177,33 @@ void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t t
>   			config_store.transition[i].applied = false;
>   			update = true;
>   		}
> +
> +#ifdef CONFIG_AMD_PMF_DEBUG_FACILITIES
> +		dev_dbg(dev->dev, "[AUTO MODE] time_ms:%lld avg_power:%d mode:%s timer:%u tc:%d\n",
> +			time_elapsed_ms, avg_power,
> +			state_as_str(config_store.current_mode),
> +			config_store.transition[i].timer,
> +			config_store.transition[i].time_constant);
> +
> +		dev_dbg(dev->dev, "[AUTO MODE] shiftup:%d pt:%d pf:%d pd:%u\n",
> +			config_store.transition[i].shifting_up,
> +			config_store.transition[i].power_threshold,
> +			config_store.mode_set[i].power_floor,
> +			config_store.transition[i].power_delta);
> +#endif
>   	}
>   
>   	dev_dbg(dev->dev, "[AUTO_MODE] avg power: %u mW mode: %s\n", avg_power,
>   		state_as_str(config_store.current_mode));
>   
> +#ifdef CONFIG_AMD_PMF_DEBUG_FACILITIES
> +	dev_dbg(dev->dev, "[AUTO MODE] priority1: %u, priority2: %u, priority3: %u, priority4: %u",
> +		config_store.transition[0].applied,
> +		config_store.transition[1].applied,
> +		config_store.transition[2].applied,
> +		config_store. transition[3].applied);
> +#endif
> +
>   	if (update) {
>   		for (j = 0; j < AUTO_TRANSITION_MAX; j++) {
>   			/* Apply the mode with highest priority indentified */
> diff --git a/drivers/platform/x86/amd/pmf/cnqf.c b/drivers/platform/x86/amd/pmf/cnqf.c
> index 4b9691cd592a..1f25016b3865 100644
> --- a/drivers/platform/x86/amd/pmf/cnqf.c
> +++ b/drivers/platform/x86/amd/pmf/cnqf.c
> @@ -174,6 +174,13 @@ int amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_l
>   		config_store.trans_param[src][i].count++;
>   
>   		tp = &config_store.trans_param[src][i];
> +
> +#ifdef CONFIG_AMD_PMF_DEBUG_FACILITIES
> +		dev_dbg(dev->dev, "avg_power:%d total_power:%d count:%d timer:%d\n", avg_power,
> +			config_store.trans_param[src][i].total_power,
> +			config_store.trans_param[src][i].count,
> +			config_store.trans_param[src][i].timer);
> +#endif
>   		if (tp->timer >= tp->time_constant && tp->count) {
>   			avg_power = tp->total_power / tp->count;
>   
> @@ -194,6 +201,18 @@ int amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_l
>   	dev_dbg(dev->dev, "[CNQF] Avg power: %u mW socket power: %u mW mode:%s\n",
>   		avg_power, socket_power, state_as_str(config_store.current_mode));
>   
> +#ifdef AMD_PMF_DEBUG_FACILITIES
> +	dev_dbg(dev->dev, "[CNQF] priority 1: %u, priority 2: %u, priority 3: %u",
> +		config_store.trans_param[src][0].priority,
> +		config_store.trans_param[src][1].priority,
> +		config_store.trans_param[src][2].priority);
> +
> +	dev_dbg(dev->dev, "[CNQF] priority 4: %u, priority 5: %u, priority 6: %u",
> +		config_store.trans_param[src][3].priority,
> +		config_store.trans_param[src][4].priority,
> +		config_store.trans_param[src][5].priority);
> +#endif
> +
>   	for (j = 0; j < CNQF_TRANSITION_MAX; j++) {
>   		/* apply the highest priority */
>   		if (config_store.trans_param[src][j].priority) {

Re: [PATCH 2/3] platform/x86/amd/pmf: Add PMF debug facilities

[Shyam Sundar S K]

Hi Mario

On 4/6/2023 10:21 PM, Limonciello, Mario wrote:
> On 4/6/2023 11:48, Shyam Sundar S K wrote:
>> At times, when the mode transitions fail to happen, the current
>> driver does not give enough debug information on why the transition
>> failed or the default preset values did not load. Having an on-demand
>> logs guarded by CONFIG would be helpful in such cases.
>>
>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>> ---
>>   drivers/platform/x86/amd/pmf/Kconfig     | 10 ++++++++++
>>   drivers/platform/x86/amd/pmf/auto-mode.c | 22 ++++++++++++++++++++++
>>   drivers/platform/x86/amd/pmf/cnqf.c      | 19 +++++++++++++++++++
>>   3 files changed, 51 insertions(+)
>>
>> diff --git a/drivers/platform/x86/amd/pmf/Kconfig
>> b/drivers/platform/x86/amd/pmf/Kconfig
>> index f4fd764e55a6..7129de0fb9fb 100644
>> --- a/drivers/platform/x86/amd/pmf/Kconfig
>> +++ b/drivers/platform/x86/amd/pmf/Kconfig
>> @@ -26,3 +26,13 @@ config AMD_PMF_ACPI_DEBUG
>>        in the PMF config store.
>>          Say Y here to enable more debug logs and Say N here if you
>> are not sure.
>> +
>> +config AMD_PMF_DEBUG_FACILITIES
>> +    bool "PMF debug facilities"
>> +    depends on AMD_PMF
>> +    help
>> +     Enabling this option would give more debug information on the
>> PMF interna
>> +     counters such as time constants, power thresholds, target modes and
>> +     power mode transitions of auto mode and CnQF features.
> 
> With the availability of dynamic debugging is there a lot of benefit to
> guarding all the new dev_dbg statements behind a config option?
> 
> Is it because of performance impact?

Yes right.

But not all these would be useful all the times when we turn on dyndbg.
I borrowed some of these stuff actually from thinkpad_acpi driver.

Thanks,
Shyam

> 
>> +
>> +     Say Y here to enable logs and Say N here if you are not sure.
>> diff --git a/drivers/platform/x86/amd/pmf/auto-mode.c
>> b/drivers/platform/x86/amd/pmf/auto-mode.c
>> index 777490fcf8b9..560379b5cda7 100644
>> --- a/drivers/platform/x86/amd/pmf/auto-mode.c
>> +++ b/drivers/platform/x86/amd/pmf/auto-mode.c
>> @@ -177,11 +177,33 @@ void amd_pmf_trans_automode(struct amd_pmf_dev
>> *dev, int socket_power, ktime_t t
>>               config_store.transition[i].applied = false;
>>               update = true;
>>           }
>> +
>> +#ifdef CONFIG_AMD_PMF_DEBUG_FACILITIES
>> +        dev_dbg(dev->dev, "[AUTO MODE] time_ms:%lld avg_power:%d
>> mode:%s timer:%u tc:%d\n",
>> +            time_elapsed_ms, avg_power,
>> +            state_as_str(config_store.current_mode),
>> +            config_store.transition[i].timer,
>> +            config_store.transition[i].time_constant);
>> +
>> +        dev_dbg(dev->dev, "[AUTO MODE] shiftup:%d pt:%d pf:%d pd:%u\n",
>> +            config_store.transition[i].shifting_up,
>> +            config_store.transition[i].power_threshold,
>> +            config_store.mode_set[i].power_floor,
>> +            config_store.transition[i].power_delta);
>> +#endif
>>       }
>>         dev_dbg(dev->dev, "[AUTO_MODE] avg power: %u mW mode: %s\n",
>> avg_power,
>>           state_as_str(config_store.current_mode));
>>   +#ifdef CONFIG_AMD_PMF_DEBUG_FACILITIES
>> +    dev_dbg(dev->dev, "[AUTO MODE] priority1: %u, priority2: %u,
>> priority3: %u, priority4: %u",
>> +        config_store.transition[0].applied,
>> +        config_store.transition[1].applied,
>> +        config_store.transition[2].applied,
>> +        config_store. transition[3].applied);
>> +#endif
>> +
>>       if (update) {
>>           for (j = 0; j < AUTO_TRANSITION_MAX; j++) {
>>               /* Apply the mode with highest priority indentified */
>> diff --git a/drivers/platform/x86/amd/pmf/cnqf.c
>> b/drivers/platform/x86/amd/pmf/cnqf.c
>> index 4b9691cd592a..1f25016b3865 100644
>> --- a/drivers/platform/x86/amd/pmf/cnqf.c
>> +++ b/drivers/platform/x86/amd/pmf/cnqf.c
>> @@ -174,6 +174,13 @@ int amd_pmf_trans_cnqf(struct amd_pmf_dev *dev,
>> int socket_power, ktime_t time_l
>>           config_store.trans_param[src][i].count++;
>>             tp = &config_store.trans_param[src][i];
>> +
>> +#ifdef CONFIG_AMD_PMF_DEBUG_FACILITIES
>> +        dev_dbg(dev->dev, "avg_power:%d total_power:%d count:%d
>> timer:%d\n", avg_power,
>> +            config_store.trans_param[src][i].total_power,
>> +            config_store.trans_param[src][i].count,
>> +            config_store.trans_param[src][i].timer);
>> +#endif
>>           if (tp->timer >= tp->time_constant && tp->count) {
>>               avg_power = tp->total_power / tp->count;
>>   @@ -194,6 +201,18 @@ int amd_pmf_trans_cnqf(struct amd_pmf_dev *dev,
>> int socket_power, ktime_t time_l
>>       dev_dbg(dev->dev, "[CNQF] Avg power: %u mW socket power: %u mW
>> mode:%s\n",
>>           avg_power, socket_power,
>> state_as_str(config_store.current_mode));
>>   +#ifdef AMD_PMF_DEBUG_FACILITIES
>> +    dev_dbg(dev->dev, "[CNQF] priority 1: %u, priority 2: %u,
>> priority 3: %u",
>> +        config_store.trans_param[src][0].priority,
>> +        config_store.trans_param[src][1].priority,
>> +        config_store.trans_param[src][2].priority);
>> +
>> +    dev_dbg(dev->dev, "[CNQF] priority 4: %u, priority 5: %u,
>> priority 6: %u",
>> +        config_store.trans_param[src][3].priority,
>> +        config_store.trans_param[src][4].priority,
>> +        config_store.trans_param[src][5].priority);
>> +#endif
>> +
>>       for (j = 0; j < CNQF_TRANSITION_MAX; j++) {
>>           /* apply the highest priority */
>>           if (config_store.trans_param[src][j].priority) {
> 

Re: [PATCH 2/3] platform/x86/amd/pmf: Add PMF debug facilities

[Hans de Goede]

Hi,

On 4/6/23 18:48, Shyam Sundar S K wrote:
> At times, when the mode transitions fail to happen, the current
> driver does not give enough debug information on why the transition
> failed or the default preset values did not load. Having an on-demand
> logs guarded by CONFIG would be helpful in such cases.
> 
> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
> ---
>  drivers/platform/x86/amd/pmf/Kconfig     | 10 ++++++++++
>  drivers/platform/x86/amd/pmf/auto-mode.c | 22 ++++++++++++++++++++++
>  drivers/platform/x86/amd/pmf/cnqf.c      | 19 +++++++++++++++++++
>  3 files changed, 51 insertions(+)
> 
> diff --git a/drivers/platform/x86/amd/pmf/Kconfig b/drivers/platform/x86/amd/pmf/Kconfig
> index f4fd764e55a6..7129de0fb9fb 100644
> --- a/drivers/platform/x86/amd/pmf/Kconfig
> +++ b/drivers/platform/x86/amd/pmf/Kconfig
> @@ -26,3 +26,13 @@ config AMD_PMF_ACPI_DEBUG
>  	 in the PMF config store.
>  
>  	 Say Y here to enable more debug logs and Say N here if you are not sure.
> +
> +config AMD_PMF_DEBUG_FACILITIES
> +	bool "PMF debug facilities"
> +	depends on AMD_PMF
> +	help
> +	 Enabling this option would give more debug information on the PMF interna
> +	 counters such as time constants, power thresholds, target modes and
> +	 power mode transitions of auto mode and CnQF features.
> +
> +	 Say Y here to enable logs and Say N here if you are not sure.

Do we really need *both* CONFIG_AMD_PMF_ACPI_DEBUG and CONFIG_AMD_PMF_DEBUG_FACILITIES ?

To me it seems that both could be covered by a generic CONFIG_AMD_PMF_DEBUG option ?
especially since this patch only adds a few extra dev_dbg-s.

> diff --git a/drivers/platform/x86/amd/pmf/auto-mode.c b/drivers/platform/x86/amd/pmf/auto-mode.c
> index 777490fcf8b9..560379b5cda7 100644
> --- a/drivers/platform/x86/amd/pmf/auto-mode.c
> +++ b/drivers/platform/x86/amd/pmf/auto-mode.c
> @@ -177,11 +177,33 @@ void amd_pmf_trans_automode(struct amd_pmf_dev *dev, int socket_power, ktime_t t
>  			config_store.transition[i].applied = false;
>  			update = true;
>  		}
> +
> +#ifdef CONFIG_AMD_PMF_DEBUG_FACILITIES
> +		dev_dbg(dev->dev, "[AUTO MODE] time_ms:%lld avg_power:%d mode:%s timer:%u tc:%d\n",
> +			time_elapsed_ms, avg_power,
> +			state_as_str(config_store.current_mode),
> +			config_store.transition[i].timer,
> +			config_store.transition[i].time_constant);
> +
> +		dev_dbg(dev->dev, "[AUTO MODE] shiftup:%d pt:%d pf:%d pd:%u\n",
> +			config_store.transition[i].shifting_up,
> +			config_store.transition[i].power_threshold,
> +			config_store.mode_set[i].power_floor,
> +			config_store.transition[i].power_delta);
> +#endif
>  	}
>  
>  	dev_dbg(dev->dev, "[AUTO_MODE] avg power: %u mW mode: %s\n", avg_power,
>  		state_as_str(config_store.current_mode));
>  
> +#ifdef CONFIG_AMD_PMF_DEBUG_FACILITIES
> +	dev_dbg(dev->dev, "[AUTO MODE] priority1: %u, priority2: %u, priority3: %u, priority4: %u",
> +		config_store.transition[0].applied,
> +		config_store.transition[1].applied,
> +		config_store.transition[2].applied,
> +		config_store. transition[3].applied);
> +#endif
> +
>  	if (update) {
>  		for (j = 0; j < AUTO_TRANSITION_MAX; j++) {
>  			/* Apply the mode with highest priority indentified */
> diff --git a/drivers/platform/x86/amd/pmf/cnqf.c b/drivers/platform/x86/amd/pmf/cnqf.c
> index 4b9691cd592a..1f25016b3865 100644
> --- a/drivers/platform/x86/amd/pmf/cnqf.c
> +++ b/drivers/platform/x86/amd/pmf/cnqf.c
> @@ -174,6 +174,13 @@ int amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_l
>  		config_store.trans_param[src][i].count++;
>  
>  		tp = &config_store.trans_param[src][i];
> +
> +#ifdef CONFIG_AMD_PMF_DEBUG_FACILITIES
> +		dev_dbg(dev->dev, "avg_power:%d total_power:%d count:%d timer:%d\n", avg_power,
> +			config_store.trans_param[src][i].total_power,
> +			config_store.trans_param[src][i].count,
> +			config_store.trans_param[src][i].timer);
> +#endif
>  		if (tp->timer >= tp->time_constant && tp->count) {
>  			avg_power = tp->total_power / tp->count;
>  
> @@ -194,6 +201,18 @@ int amd_pmf_trans_cnqf(struct amd_pmf_dev *dev, int socket_power, ktime_t time_l
>  	dev_dbg(dev->dev, "[CNQF] Avg power: %u mW socket power: %u mW mode:%s\n",
>  		avg_power, socket_power, state_as_str(config_store.current_mode));
>  
> +#ifdef AMD_PMF_DEBUG_FACILITIES
> +	dev_dbg(dev->dev, "[CNQF] priority 1: %u, priority 2: %u, priority 3: %u",
> +		config_store.trans_param[src][0].priority,
> +		config_store.trans_param[src][1].priority,
> +		config_store.trans_param[src][2].priority);
> +
> +	dev_dbg(dev->dev, "[CNQF] priority 4: %u, priority 5: %u, priority 6: %u",
> +		config_store.trans_param[src][3].priority,
> +		config_store.trans_param[src][4].priority,
> +		config_store.trans_param[src][5].priority);
> +#endif
> +
>  	for (j = 0; j < CNQF_TRANSITION_MAX; j++) {
>  		/* apply the highest priority */
>  		if (config_store.trans_param[src][j].priority) {


Regards,

Hans

[PATCH 3/3] platform/x86/amd/pmf: Move out of BIOS SMN pair for driver probe

[Shyam Sundar S K]

The current SMN index used for the driver probe seems to be meant
for the BIOS pair and there are potential concurrency problems that can
occur with an inopportune SMI.

It is been advised to use SMN_INDEX_0 instead of SMN_INDEX_2, which is
what amd_nb.c provides and this function has protections to ensure that
only one caller can use it at a time.

Fixes: da5ce22df5fe ("platform/x86/amd/pmf: Add support for PMF core layer")
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/Kconfig |  1 +
 drivers/platform/x86/amd/pmf/core.c  | 22 +++++-----------------
 2 files changed, 6 insertions(+), 17 deletions(-)

diff --git a/drivers/platform/x86/amd/pmf/Kconfig b/drivers/platform/x86/amd/pmf/Kconfig
index 7129de0fb9fb..c7cda8bd478c 100644
--- a/drivers/platform/x86/amd/pmf/Kconfig
+++ b/drivers/platform/x86/amd/pmf/Kconfig
@@ -7,6 +7,7 @@ config AMD_PMF
 	tristate "AMD Platform Management Framework"
 	depends on ACPI && PCI
 	depends on POWER_SUPPLY
+	depends on AMD_NB
 	select ACPI_PLATFORM_PROFILE
 	help
 	  This driver provides support for the AMD Platform Management Framework.
diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
index da23639071d7..0acc0b622129 100644
--- a/drivers/platform/x86/amd/pmf/core.c
+++ b/drivers/platform/x86/amd/pmf/core.c
@@ -8,6 +8,7 @@
  * Author: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
  */
 
+#include <asm/amd_nb.h>
 #include <linux/debugfs.h>
 #include <linux/iopoll.h>
 #include <linux/module.h>
@@ -22,8 +23,6 @@
 #define AMD_PMF_REGISTER_ARGUMENT	0xA58
 
 /* Base address of SMU for mapping physical address to virtual address */
-#define AMD_PMF_SMU_INDEX_ADDRESS	0xB8
-#define AMD_PMF_SMU_INDEX_DATA		0xBC
 #define AMD_PMF_MAPPING_SIZE		0x01000
 #define AMD_PMF_BASE_ADDR_OFFSET	0x10000
 #define AMD_PMF_BASE_ADDR_LO		0x13B102E8
@@ -348,30 +347,19 @@ static int amd_pmf_probe(struct platform_device *pdev)
 	}
 
 	dev->cpu_id = rdev->device;
-	err = pci_write_config_dword(rdev, AMD_PMF_SMU_INDEX_ADDRESS, AMD_PMF_BASE_ADDR_LO);
-	if (err) {
-		dev_err(dev->dev, "error writing to 0x%x\n", AMD_PMF_SMU_INDEX_ADDRESS);
-		pci_dev_put(rdev);
-		return pcibios_err_to_errno(err);
-	}
 
-	err = pci_read_config_dword(rdev, AMD_PMF_SMU_INDEX_DATA, &val);
+	err = amd_smn_read(0, AMD_PMF_BASE_ADDR_LO, &val);
 	if (err) {
+		dev_err(dev->dev, "error in reading from 0x%x\n", AMD_PMF_BASE_ADDR_LO);
 		pci_dev_put(rdev);
 		return pcibios_err_to_errno(err);
 	}
 
 	base_addr_lo = val & AMD_PMF_BASE_ADDR_HI_MASK;
 
-	err = pci_write_config_dword(rdev, AMD_PMF_SMU_INDEX_ADDRESS, AMD_PMF_BASE_ADDR_HI);
-	if (err) {
-		dev_err(dev->dev, "error writing to 0x%x\n", AMD_PMF_SMU_INDEX_ADDRESS);
-		pci_dev_put(rdev);
-		return pcibios_err_to_errno(err);
-	}
-
-	err = pci_read_config_dword(rdev, AMD_PMF_SMU_INDEX_DATA, &val);
+	err = amd_smn_read(0, AMD_PMF_BASE_ADDR_HI, &val);
 	if (err) {
+		dev_err(dev->dev, "error in reading from 0x%x\n", AMD_PMF_BASE_ADDR_HI);
 		pci_dev_put(rdev);
 		return pcibios_err_to_errno(err);
 	}
-- 
2.25.1

Re: [PATCH 3/3] platform/x86/amd/pmf: Move out of BIOS SMN pair for driver probe

[Limonciello, Mario]

On 4/6/2023 11:48, Shyam Sundar S K wrote:
> The current SMN index used for the driver probe seems to be meant
> for the BIOS pair and there are potential concurrency problems that can
> occur with an inopportune SMI.
> 
> It is been advised to use SMN_INDEX_0 instead of SMN_INDEX_2, which is
> what amd_nb.c provides and this function has protections to ensure that
> only one caller can use it at a time.
> 
> Fixes: da5ce22df5fe ("platform/x86/amd/pmf: Add support for PMF core layer")
> 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>

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

> ---
>   drivers/platform/x86/amd/pmf/Kconfig |  1 +
>   drivers/platform/x86/amd/pmf/core.c  | 22 +++++-----------------
>   2 files changed, 6 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmf/Kconfig b/drivers/platform/x86/amd/pmf/Kconfig
> index 7129de0fb9fb..c7cda8bd478c 100644
> --- a/drivers/platform/x86/amd/pmf/Kconfig
> +++ b/drivers/platform/x86/amd/pmf/Kconfig
> @@ -7,6 +7,7 @@ config AMD_PMF
>   	tristate "AMD Platform Management Framework"
>   	depends on ACPI && PCI
>   	depends on POWER_SUPPLY
> +	depends on AMD_NB
>   	select ACPI_PLATFORM_PROFILE
>   	help
>   	  This driver provides support for the AMD Platform Management Framework.
> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
> index da23639071d7..0acc0b622129 100644
> --- a/drivers/platform/x86/amd/pmf/core.c
> +++ b/drivers/platform/x86/amd/pmf/core.c
> @@ -8,6 +8,7 @@
>    * Author: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>    */
>   
> +#include <asm/amd_nb.h>
>   #include <linux/debugfs.h>
>   #include <linux/iopoll.h>
>   #include <linux/module.h>
> @@ -22,8 +23,6 @@
>   #define AMD_PMF_REGISTER_ARGUMENT	0xA58
>   
>   /* Base address of SMU for mapping physical address to virtual address */
> -#define AMD_PMF_SMU_INDEX_ADDRESS	0xB8
> -#define AMD_PMF_SMU_INDEX_DATA		0xBC
>   #define AMD_PMF_MAPPING_SIZE		0x01000
>   #define AMD_PMF_BASE_ADDR_OFFSET	0x10000
>   #define AMD_PMF_BASE_ADDR_LO		0x13B102E8
> @@ -348,30 +347,19 @@ static int amd_pmf_probe(struct platform_device *pdev)
>   	}
>   
>   	dev->cpu_id = rdev->device;
> -	err = pci_write_config_dword(rdev, AMD_PMF_SMU_INDEX_ADDRESS, AMD_PMF_BASE_ADDR_LO);
> -	if (err) {
> -		dev_err(dev->dev, "error writing to 0x%x\n", AMD_PMF_SMU_INDEX_ADDRESS);
> -		pci_dev_put(rdev);
> -		return pcibios_err_to_errno(err);
> -	}
>   
> -	err = pci_read_config_dword(rdev, AMD_PMF_SMU_INDEX_DATA, &val);
> +	err = amd_smn_read(0, AMD_PMF_BASE_ADDR_LO, &val);
>   	if (err) {
> +		dev_err(dev->dev, "error in reading from 0x%x\n", AMD_PMF_BASE_ADDR_LO);
>   		pci_dev_put(rdev);
>   		return pcibios_err_to_errno(err);
>   	}
>   
>   	base_addr_lo = val & AMD_PMF_BASE_ADDR_HI_MASK;
>   
> -	err = pci_write_config_dword(rdev, AMD_PMF_SMU_INDEX_ADDRESS, AMD_PMF_BASE_ADDR_HI);
> -	if (err) {
> -		dev_err(dev->dev, "error writing to 0x%x\n", AMD_PMF_SMU_INDEX_ADDRESS);
> -		pci_dev_put(rdev);
> -		return pcibios_err_to_errno(err);
> -	}
> -
> -	err = pci_read_config_dword(rdev, AMD_PMF_SMU_INDEX_DATA, &val);
> +	err = amd_smn_read(0, AMD_PMF_BASE_ADDR_HI, &val);
>   	if (err) {
> +		dev_err(dev->dev, "error in reading from 0x%x\n", AMD_PMF_BASE_ADDR_HI);
>   		pci_dev_put(rdev);
>   		return pcibios_err_to_errno(err);
>   	}

Re: [PATCH 3/3] platform/x86/amd/pmf: Move out of BIOS SMN pair for driver probe

[Hans de Goede]

Hi,

On 4/6/23 18:48, Shyam Sundar S K wrote:
> The current SMN index used for the driver probe seems to be meant
> for the BIOS pair and there are potential concurrency problems that can
> occur with an inopportune SMI.
> 
> It is been advised to use SMN_INDEX_0 instead of SMN_INDEX_2, which is
> what amd_nb.c provides and this function has protections to ensure that
> only one caller can use it at a time.
> 
> Fixes: da5ce22df5fe ("platform/x86/amd/pmf: Add support for PMF core layer")
> 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>

Since this is a fix and since this applies cleanly without
the other 2 debug patches I have merged this into my
review-hans branch now, so that it can be merged by Linus for
6.4-rc1 :

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans




> ---
>  drivers/platform/x86/amd/pmf/Kconfig |  1 +
>  drivers/platform/x86/amd/pmf/core.c  | 22 +++++-----------------
>  2 files changed, 6 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmf/Kconfig b/drivers/platform/x86/amd/pmf/Kconfig
> index 7129de0fb9fb..c7cda8bd478c 100644
> --- a/drivers/platform/x86/amd/pmf/Kconfig
> +++ b/drivers/platform/x86/amd/pmf/Kconfig
> @@ -7,6 +7,7 @@ config AMD_PMF
>  	tristate "AMD Platform Management Framework"
>  	depends on ACPI && PCI
>  	depends on POWER_SUPPLY
> +	depends on AMD_NB
>  	select ACPI_PLATFORM_PROFILE
>  	help
>  	  This driver provides support for the AMD Platform Management Framework.
> diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c
> index da23639071d7..0acc0b622129 100644
> --- a/drivers/platform/x86/amd/pmf/core.c
> +++ b/drivers/platform/x86/amd/pmf/core.c
> @@ -8,6 +8,7 @@
>   * Author: Shyam Sundar S K <Shyam-sundar.S-k@amd.com>
>   */
>  
> +#include <asm/amd_nb.h>
>  #include <linux/debugfs.h>
>  #include <linux/iopoll.h>
>  #include <linux/module.h>
> @@ -22,8 +23,6 @@
>  #define AMD_PMF_REGISTER_ARGUMENT	0xA58
>  
>  /* Base address of SMU for mapping physical address to virtual address */
> -#define AMD_PMF_SMU_INDEX_ADDRESS	0xB8
> -#define AMD_PMF_SMU_INDEX_DATA		0xBC
>  #define AMD_PMF_MAPPING_SIZE		0x01000
>  #define AMD_PMF_BASE_ADDR_OFFSET	0x10000
>  #define AMD_PMF_BASE_ADDR_LO		0x13B102E8
> @@ -348,30 +347,19 @@ static int amd_pmf_probe(struct platform_device *pdev)
>  	}
>  
>  	dev->cpu_id = rdev->device;
> -	err = pci_write_config_dword(rdev, AMD_PMF_SMU_INDEX_ADDRESS, AMD_PMF_BASE_ADDR_LO);
> -	if (err) {
> -		dev_err(dev->dev, "error writing to 0x%x\n", AMD_PMF_SMU_INDEX_ADDRESS);
> -		pci_dev_put(rdev);
> -		return pcibios_err_to_errno(err);
> -	}
>  
> -	err = pci_read_config_dword(rdev, AMD_PMF_SMU_INDEX_DATA, &val);
> +	err = amd_smn_read(0, AMD_PMF_BASE_ADDR_LO, &val);
>  	if (err) {
> +		dev_err(dev->dev, "error in reading from 0x%x\n", AMD_PMF_BASE_ADDR_LO);
>  		pci_dev_put(rdev);
>  		return pcibios_err_to_errno(err);
>  	}
>  
>  	base_addr_lo = val & AMD_PMF_BASE_ADDR_HI_MASK;
>  
> -	err = pci_write_config_dword(rdev, AMD_PMF_SMU_INDEX_ADDRESS, AMD_PMF_BASE_ADDR_HI);
> -	if (err) {
> -		dev_err(dev->dev, "error writing to 0x%x\n", AMD_PMF_SMU_INDEX_ADDRESS);
> -		pci_dev_put(rdev);
> -		return pcibios_err_to_errno(err);
> -	}
> -
> -	err = pci_read_config_dword(rdev, AMD_PMF_SMU_INDEX_DATA, &val);
> +	err = amd_smn_read(0, AMD_PMF_BASE_ADDR_HI, &val);
>  	if (err) {
> +		dev_err(dev->dev, "error in reading from 0x%x\n", AMD_PMF_BASE_ADDR_HI);
>  		pci_dev_put(rdev);
>  		return pcibios_err_to_errno(err);
>  	}