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@xxxxxxx>
>> ---
>> 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) {
>
