Re: [PATCH v2] platform/x86: amd: pmf: Fix STT limits

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Mon, 7 Apr 2025, Mario Limonciello wrote:

> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> On some platforms it has been observed that STT limits are not being applied
> properly causing poor performance as power limits are set too low.
> 
> STT limits that are sent to the platform are supposed to be in Q8.8
> format.  Convert them before sending.
> 
> Reported-by: Yijun Shen <Yijun.Shen@dell.com>
> Fixes: 7c45534afa443 ("platform/x86/amd/pmf: Add support for PMF Policy Binary")
> Cc: stable@vger.kernel.org
> Tested-By: Yijun Shen <Yijun_Shen@Dell.com>
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> v2:
>  * Handle cases for auto-mode, cnqf, and sps as well
> ---
>  drivers/platform/x86/amd/pmf/auto-mode.c | 4 ++--
>  drivers/platform/x86/amd/pmf/cnqf.c      | 4 ++--
>  drivers/platform/x86/amd/pmf/sps.c       | 8 ++++----
>  drivers/platform/x86/amd/pmf/tee-if.c    | 4 ++--
>  4 files changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/pmf/auto-mode.c b/drivers/platform/x86/amd/pmf/auto-mode.c
> index 02ff68be10d01..df37f8a84a007 100644
> --- a/drivers/platform/x86/amd/pmf/auto-mode.c
> +++ b/drivers/platform/x86/amd/pmf/auto-mode.c
> @@ -120,9 +120,9 @@ static void amd_pmf_set_automode(struct amd_pmf_dev *dev, int idx,
>  	amd_pmf_send_cmd(dev, SET_SPPT_APU_ONLY, false, pwr_ctrl->sppt_apu_only, NULL);
>  	amd_pmf_send_cmd(dev, SET_STT_MIN_LIMIT, false, pwr_ctrl->stt_min, NULL);
>  	amd_pmf_send_cmd(dev, SET_STT_LIMIT_APU, false,
> -			 pwr_ctrl->stt_skin_temp[STT_TEMP_APU], NULL);
> +			 pwr_ctrl->stt_skin_temp[STT_TEMP_APU] << 8, NULL);

Hi Mario,

Could we add some helper on constructing the fixed-point number from the 
integer part as this magic shifting makes the intent somewhat harder to 
follow just by reading the code itself?

I hoped that include/linux/ would have had something for this but it seems 
generic fixed-point helpers are almost non-existing except for very 
specific use cases such as averages so maybe add a helper only for this 
driver for now as this will be routed through fixes branch so doing random 
things i include/linux/ might not be preferrable and would require larger 
review audience.

What I mean for general helpers is that it would be nice to have something 
like DECLARE_FIXEDPOINT() similar to DECLARE_EWMA() macro (and maybe a 
signed variant too) which creates a few helper functions for the given 
name prefix. It seems there's plenty of code which would benefit from such 
helpers and would avoid the need to comment the fixed-point operations 
(not to speak of how many of such ops likely lack the comment). So at 
least keep that in mind for naming the helpers so the conversion to
a generic helper could be done smoothly.

-- 
 i.

>  	amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false,
> -			 pwr_ctrl->stt_skin_temp[STT_TEMP_HS2], NULL);
> +			 pwr_ctrl->stt_skin_temp[STT_TEMP_HS2] << 8, NULL);
>  
>  	if (is_apmf_func_supported(dev, APMF_FUNC_SET_FAN_IDX))
>  		apmf_update_fan_idx(dev, config_store.mode_set[idx].fan_control.manual,
> diff --git a/drivers/platform/x86/amd/pmf/cnqf.c b/drivers/platform/x86/amd/pmf/cnqf.c
> index bc8899e15c914..6a5ecc05961d9 100644
> --- a/drivers/platform/x86/amd/pmf/cnqf.c
> +++ b/drivers/platform/x86/amd/pmf/cnqf.c
> @@ -81,9 +81,9 @@ static int amd_pmf_set_cnqf(struct amd_pmf_dev *dev, int src, int idx,
>  	amd_pmf_send_cmd(dev, SET_SPPT, false, pc->sppt, NULL);
>  	amd_pmf_send_cmd(dev, SET_SPPT_APU_ONLY, false, pc->sppt_apu_only, NULL);
>  	amd_pmf_send_cmd(dev, SET_STT_MIN_LIMIT, false, pc->stt_min, NULL);
> -	amd_pmf_send_cmd(dev, SET_STT_LIMIT_APU, false, pc->stt_skin_temp[STT_TEMP_APU],
> +	amd_pmf_send_cmd(dev, SET_STT_LIMIT_APU, false, pc->stt_skin_temp[STT_TEMP_APU] << 8,
>  			 NULL);
> -	amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false, pc->stt_skin_temp[STT_TEMP_HS2],
> +	amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false, pc->stt_skin_temp[STT_TEMP_HS2] << 8,
>  			 NULL);
>  
>  	if (is_apmf_func_supported(dev, APMF_FUNC_SET_FAN_IDX))
> diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c
> index d3083383f11fb..ec10db1bfa5ec 100644
> --- a/drivers/platform/x86/amd/pmf/sps.c
> +++ b/drivers/platform/x86/amd/pmf/sps.c
> @@ -198,9 +198,9 @@ static void amd_pmf_update_slider_v2(struct amd_pmf_dev *dev, int idx)
>  	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);
> +			 apts_config_store.val[idx].stt_skin_temp_limit_apu << 8, NULL);
>  	amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false,
> -			 apts_config_store.val[idx].stt_skin_temp_limit_hs2, NULL);
> +			 apts_config_store.val[idx].stt_skin_temp_limit_hs2 << 8, NULL);
>  }
>  
>  void amd_pmf_update_slider(struct amd_pmf_dev *dev, bool op, int idx,
> @@ -217,9 +217,9 @@ void amd_pmf_update_slider(struct amd_pmf_dev *dev, bool op, int idx,
>  		amd_pmf_send_cmd(dev, SET_STT_MIN_LIMIT, false,
>  				 config_store.prop[src][idx].stt_min, NULL);
>  		amd_pmf_send_cmd(dev, SET_STT_LIMIT_APU, false,
> -				 config_store.prop[src][idx].stt_skin_temp[STT_TEMP_APU], NULL);
> +				 config_store.prop[src][idx].stt_skin_temp[STT_TEMP_APU] << 8, NULL);
>  		amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false,
> -				 config_store.prop[src][idx].stt_skin_temp[STT_TEMP_HS2], NULL);
> +				 config_store.prop[src][idx].stt_skin_temp[STT_TEMP_HS2] << 8, NULL);
>  	} else if (op == SLIDER_OP_GET) {
>  		amd_pmf_send_cmd(dev, GET_SPL, true, ARG_NONE, &table->prop[src][idx].spl);
>  		amd_pmf_send_cmd(dev, GET_FPPT, true, ARG_NONE, &table->prop[src][idx].fppt);
> diff --git a/drivers/platform/x86/amd/pmf/tee-if.c b/drivers/platform/x86/amd/pmf/tee-if.c
> index d6a871f0d8ff2..7096923107929 100644
> --- a/drivers/platform/x86/amd/pmf/tee-if.c
> +++ b/drivers/platform/x86/amd/pmf/tee-if.c
> @@ -142,7 +142,7 @@ static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_
>  
>  		case PMF_POLICY_STT_SKINTEMP_APU:
>  			if (dev->prev_data->stt_skintemp_apu != val) {
> -				amd_pmf_send_cmd(dev, SET_STT_LIMIT_APU, false, val, NULL);
> +				amd_pmf_send_cmd(dev, SET_STT_LIMIT_APU, false, val << 8, NULL);
>  				dev_dbg(dev->dev, "update STT_SKINTEMP_APU: %u\n", val);
>  				dev->prev_data->stt_skintemp_apu = val;
>  			}
> @@ -150,7 +150,7 @@ static void amd_pmf_apply_policies(struct amd_pmf_dev *dev, struct ta_pmf_enact_
>  
>  		case PMF_POLICY_STT_SKINTEMP_HS2:
>  			if (dev->prev_data->stt_skintemp_hs2 != val) {
> -				amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false, val, NULL);
> +				amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false, val << 8, NULL);
>  				dev_dbg(dev->dev, "update STT_SKINTEMP_HS2: %u\n", val);
>  				dev->prev_data->stt_skintemp_hs2 = val;
>  			}
>