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; }