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

[Mario Limonciello <superm1@kernel.org>]

On 4/7/2025 10:19 AM, Ilpo Järvinen wrote:
> 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.
> 

Do I follow right that you mean something like this?

static inline u32 amd_pmf_convert_q88 (u32 val)
{
	return val << 8;
}