| Message ID | 20220802151149.2123699-4-Shyam-sundar.S-k@amd.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | platform/x86/amd/pmf: Introduce AMD PMF Driver | expand |
Hi, On 8/2/22 17:11, Shyam Sundar S K wrote: > SPS (a.k.a. Static Power Slider) gives a feel of Windows performance > power slider for the Linux users, where the user selects a certain > mode (like "balanced", "low-power" or "performance") and the thermals > associated with each selected mode gets applied from the silicon > side via the mailboxes defined through PMFW. > > PMF driver hooks to platform_profile by reading the PMF ACPI fn9 to > see if the support is being advertised by ACPI interface. > > If supported, the PMF driver reacts to platform_profile selection choices > made by the user and adjust the system thermal behavior. > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> > --- > drivers/platform/x86/amd/pmf/Makefile | 2 +- > drivers/platform/x86/amd/pmf/acpi.c | 10 ++ > drivers/platform/x86/amd/pmf/core.c | 26 +++++ > drivers/platform/x86/amd/pmf/pmf.h | 64 +++++++++++ > drivers/platform/x86/amd/pmf/sps.c | 149 ++++++++++++++++++++++++++ > 5 files changed, 250 insertions(+), 1 deletion(-) > create mode 100644 drivers/platform/x86/amd/pmf/sps.c > > diff --git a/drivers/platform/x86/amd/pmf/Makefile b/drivers/platform/x86/amd/pmf/Makefile > index 2617eba773ce..557521a80427 100644 > --- a/drivers/platform/x86/amd/pmf/Makefile > +++ b/drivers/platform/x86/amd/pmf/Makefile > @@ -5,4 +5,4 @@ > # > > obj-$(CONFIG_AMD_PMF) += amd-pmf.o > -amd-pmf-objs := core.o acpi.o > +amd-pmf-objs := core.o acpi.o sps.o > diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c > index b378f9e31194..5997ab724f3a 100644 > --- a/drivers/platform/x86/amd/pmf/acpi.c > +++ b/drivers/platform/x86/amd/pmf/acpi.c > @@ -93,6 +93,16 @@ int is_apmf_func_supported(struct amd_pmf_dev *pdev, unsigned long index) > return !!(pdev->supported_func & BIT(index - 1)); > } > > +int apmf_get_static_slider_granular(struct amd_pmf_dev *pdev, > + struct apmf_static_slider_granular_output *data) > +{ > + if (!is_apmf_func_supported(pdev, APMF_FUNC_STATIC_SLIDER_GRANULAR)) > + return -EINVAL; > + > + return apmf_if_call_store_buffer(pdev, APMF_FUNC_STATIC_SLIDER_GRANULAR, > + data, sizeof(*data)); > +} > + > static int apmf_if_verify_interface(struct amd_pmf_dev *pdev) > { > struct apmf_verify_interface output; > diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c > index c5002b7bb904..a70ab6c9608a 100644 > --- a/drivers/platform/x86/amd/pmf/core.c > +++ b/drivers/platform/x86/amd/pmf/core.c > @@ -12,6 +12,7 @@ > #include <linux/module.h> > #include <linux/pci.h> > #include <linux/platform_device.h> > +#include <linux/power_supply.h> > #include "pmf.h" > > /* PMF-SMU communication registers */ > @@ -45,6 +46,14 @@ > #define DELAY_MIN_US 2000 > #define DELAY_MAX_US 3000 > > +int amd_pmf_get_power_source(void) > +{ > + if (power_supply_is_system_supplied() > 0) > + return POWER_SOURCE_AC; > + else > + return POWER_SOURCE_DC; > +} > + > static inline u32 amd_pmf_reg_read(struct amd_pmf_dev *dev, int reg_offset) > { > return ioread32(dev->regbase + reg_offset); > @@ -138,6 +147,21 @@ static const struct pci_device_id pmf_pci_ids[] = { > { } > }; > > +static void amd_pmf_init_features(struct amd_pmf_dev *dev) > +{ > + /* Enable Static Slider */ > + if (is_apmf_func_supported(dev, APMF_FUNC_STATIC_SLIDER_GRANULAR)) { > + amd_pmf_init_sps(dev); > + dev_dbg(dev->dev, "SPS enabled and Platform Profiles registered\n"); > + } > +} > + > +static void amd_pmf_deinit_features(struct amd_pmf_dev *dev) > +{ > + if (is_apmf_func_supported(dev, APMF_FUNC_STATIC_SLIDER_GRANULAR)) > + amd_pmf_deinit_sps(dev); > +} > + > static const struct acpi_device_id amd_pmf_acpi_ids[] = { > {"AMDI0102", 0}, > { } > @@ -206,6 +230,7 @@ static int amd_pmf_probe(struct platform_device *pdev) > > apmf_acpi_init(dev); > platform_set_drvdata(pdev, dev); > + amd_pmf_init_features(dev); > > mutex_init(&dev->lock); > dev_info(dev->dev, "registered PMF device successfully\n"); > @@ -218,6 +243,7 @@ static int amd_pmf_remove(struct platform_device *pdev) > struct amd_pmf_dev *dev = platform_get_drvdata(pdev); > > mutex_destroy(&dev->lock); > + amd_pmf_deinit_features(dev); > kfree(dev->buf); > return 0; > } > diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h > index bdadbff168ee..5c867dac7d44 100644 > --- a/drivers/platform/x86/amd/pmf/pmf.h > +++ b/drivers/platform/x86/amd/pmf/pmf.h > @@ -12,10 +12,12 @@ > #define PMF_H > > #include <linux/acpi.h> > +#include <linux/platform_profile.h> > > /* APMF Functions */ > #define APMF_FUNC_VERIFY_INTERFACE 0 > #define APMF_FUNC_GET_SYS_PARAMS 1 > +#define APMF_FUNC_STATIC_SLIDER_GRANULAR 9 > > /* Message Definitions */ > #define SET_SPL 0x03 /* SPL: Sustained Power Limit */ > @@ -36,6 +38,8 @@ > #define GET_STT_LIMIT_APU 0x20 > #define GET_STT_LIMIT_HS2 0x21 > > +#define ARG_NONE 0 > + > /* AMD PMF BIOS interfaces */ > struct apmf_verify_interface { > u16 size; > @@ -51,6 +55,30 @@ struct apmf_system_params { > u8 command_code; > } __packed; > > +enum amd_stt_skin_temp { > + STT_TEMP_APU, > + STT_TEMP_HS2, > + STT_TEMP_COUNT, > +}; > + > +enum amd_slider_op { > + SLIDER_OP_GET, > + SLIDER_OP_SET, > +}; > + > +enum power_source { > + POWER_SOURCE_AC, > + POWER_SOURCE_DC, > + POWER_SOURCE_MAX, > +}; > + > +enum power_modes { > + POWER_MODE_PERFORMANCE, > + POWER_MODE_BALANCED_POWER, > + POWER_MODE_POWER_SAVER, > + POWER_MODE_MAX, > +}; > + > struct amd_pmf_dev { > void __iomem *regbase; > void __iomem *smu_virt_addr; > @@ -60,10 +88,46 @@ struct amd_pmf_dev { > struct device *dev; > struct mutex lock; /* protects the PMF interface */ > u32 supported_func; > + enum platform_profile_option current_profile; > + struct platform_profile_handler pprof; > +}; > + > +struct apmf_sps_prop_granular { > + u32 fppt; > + u32 sppt; > + u32 sppt_apu_only; > + u32 spl; > + u32 stt_min; > + u8 stt_skin_temp[STT_TEMP_COUNT]; > + u32 fan_id; > +} __packed; > + > +/* Static Slider */ > +struct apmf_static_slider_granular_output { > + u16 size; > + struct apmf_sps_prop_granular prop[POWER_SOURCE_MAX * POWER_MODE_MAX]; > +} __packed; > + > +struct amd_pmf_static_slider_granular { > + u16 size; > + struct apmf_sps_prop_granular prop[POWER_SOURCE_MAX][POWER_MODE_MAX]; > }; > > /* Core Layer */ > int apmf_acpi_init(struct amd_pmf_dev *pmf_dev); > +int is_apmf_func_supported(struct amd_pmf_dev *pdev, unsigned long index); > int amd_pmf_send_cmd(struct amd_pmf_dev *dev, u8 message, bool get, u32 arg, u32 *data); > +int amd_pmf_get_power_source(void); > + > +/* SPS Layer */ > +u8 amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf); > +void amd_pmf_update_slider(struct amd_pmf_dev *dev, bool op, int idx, > + struct amd_pmf_static_slider_granular *table); > +int amd_pmf_init_sps(struct amd_pmf_dev *dev); > +void amd_pmf_deinit_sps(struct amd_pmf_dev *dev); > +int apmf_get_static_slider_granular(struct amd_pmf_dev *pdev, > + struct apmf_static_slider_granular_output *output); > +void amd_pmf_load_defaults_sps(struct amd_pmf_dev *dev); > + > > #endif /* PMF_H */ > diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c > new file mode 100644 > index 000000000000..ef4df3fd774b > --- /dev/null > +++ b/drivers/platform/x86/amd/pmf/sps.c > @@ -0,0 +1,149 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * AMD Platform Management Framework (PMF) Driver > + * > + * Copyright (c) 2022, Advanced Micro Devices, Inc. > + * All Rights Reserved. > + * > + * Author: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> > + */ > + > +#include "pmf.h" > + > +static int amd_pmf_profile_get(struct platform_profile_handler *pprof, > + enum platform_profile_option *profile); > +static int amd_pmf_profile_set(struct platform_profile_handler *pprof, > + enum platform_profile_option profile); > +static struct amd_pmf_static_slider_granular config_store; > + > +void amd_pmf_load_defaults_sps(struct amd_pmf_dev *dev) This function is only used in this file, so it can be static, I will fix this up while merging this. > +{ > + struct apmf_static_slider_granular_output output; > + int i, j, idx = 0; > + > + memset(&config_store, 0, sizeof(config_store)); > + apmf_get_static_slider_granular(dev, &output); > + > + for (i = 0; i < POWER_SOURCE_MAX; i++) { > + for (j = 0; j < POWER_MODE_MAX; j++) { > + config_store.prop[i][j].spl = output.prop[idx].spl; > + config_store.prop[i][j].sppt = output.prop[idx].sppt; > + config_store.prop[i][j].sppt_apu_only = > + output.prop[idx].sppt_apu_only; > + config_store.prop[i][j].fppt = output.prop[idx].fppt; > + config_store.prop[i][j].stt_min = output.prop[idx].stt_min; > + config_store.prop[i][j].stt_skin_temp[STT_TEMP_APU] = > + output.prop[idx].stt_skin_temp[STT_TEMP_APU]; > + config_store.prop[i][j].stt_skin_temp[STT_TEMP_HS2] = > + output.prop[idx].stt_skin_temp[STT_TEMP_HS2]; > + config_store.prop[i][j].fan_id = output.prop[idx].fan_id; > + idx++; > + } > + } > +} > + > +void amd_pmf_update_slider(struct amd_pmf_dev *dev, bool op, int idx, > + struct amd_pmf_static_slider_granular *table) > +{ > + int src = amd_pmf_get_power_source(); > + > + if (op == SLIDER_OP_SET) { > + amd_pmf_send_cmd(dev, SET_SPL, false, config_store.prop[src][idx].spl, NULL); > + amd_pmf_send_cmd(dev, SET_FPPT, false, config_store.prop[src][idx].fppt, NULL); > + amd_pmf_send_cmd(dev, SET_SPPT, false, config_store.prop[src][idx].sppt, NULL); > + amd_pmf_send_cmd(dev, SET_SPPT_APU_ONLY, false, > + config_store.prop[src][idx].sppt_apu_only, NULL); > + 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); > + amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false, > + config_store.prop[src][idx].stt_skin_temp[STT_TEMP_HS2], 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); > + amd_pmf_send_cmd(dev, GET_SPPT, true, ARG_NONE, &table->prop[src][idx].sppt); > + amd_pmf_send_cmd(dev, GET_SPPT_APU_ONLY, true, ARG_NONE, > + &table->prop[src][idx].sppt_apu_only); > + amd_pmf_send_cmd(dev, GET_STT_MIN_LIMIT, true, ARG_NONE, > + &table->prop[src][idx].stt_min); > + amd_pmf_send_cmd(dev, GET_STT_LIMIT_APU, true, ARG_NONE, > + (u32 *)&table->prop[src][idx].stt_skin_temp[STT_TEMP_APU]); > + amd_pmf_send_cmd(dev, GET_STT_LIMIT_HS2, true, ARG_NONE, > + (u32 *)&table->prop[src][idx].stt_skin_temp[STT_TEMP_HS2]); > + } > +} > + > +static int amd_pmf_profile_get(struct platform_profile_handler *pprof, > + enum platform_profile_option *profile) > +{ > + struct amd_pmf_dev *pmf = container_of(pprof, struct amd_pmf_dev, pprof); > + > + *profile = pmf->current_profile; > + return 0; > +} > + > +u8 amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf) > +{ > + u8 mode; > + > + switch (pmf->current_profile) { > + case PLATFORM_PROFILE_PERFORMANCE: > + mode = POWER_MODE_PERFORMANCE; > + break; > + case PLATFORM_PROFILE_BALANCED: > + mode = POWER_MODE_BALANCED_POWER; > + break; > + case PLATFORM_PROFILE_LOW_POWER: > + mode = POWER_MODE_POWER_SAVER; > + break; > + default: > + dev_err(pmf->dev, "Unknown Platform Profile.\n"); > + break; > + } > + > + return mode; > +} > + > +int amd_pmf_profile_set(struct platform_profile_handler *pprof, > + enum platform_profile_option profile) This function is only used in this file, so it can be static (it actually is declared static at the top of the file) I will fix this up while merging this. > +{ > + struct amd_pmf_dev *pmf = container_of(pprof, struct amd_pmf_dev, pprof); > + u8 mode; > + > + pmf->current_profile = profile; > + mode = amd_pmf_get_pprof_modes(pmf); > + amd_pmf_update_slider(pmf, SLIDER_OP_SET, mode, NULL); > + return 0; > +} > + > +int amd_pmf_init_sps(struct amd_pmf_dev *dev) > +{ > + int err = 0; > + > + dev->pprof.profile_get = amd_pmf_profile_get; > + dev->pprof.profile_set = amd_pmf_profile_set; > + > + /* Setup supported modes */ > + set_bit(PLATFORM_PROFILE_LOW_POWER, dev->pprof.choices); > + set_bit(PLATFORM_PROFILE_BALANCED, dev->pprof.choices); > + set_bit(PLATFORM_PROFILE_PERFORMANCE, dev->pprof.choices); > + > + /* Create platform_profile structure and register */ > + err = platform_profile_register(&dev->pprof); > + if (err) { > + dev_err(dev->dev, "Failed to register SPS support, this is most likely an SBIOS bug: %d\n", > + err); > + return -EEXIST; > + } > + > + dev->current_profile = PLATFORM_PROFILE_BALANCED; > + amd_pmf_load_defaults_sps(dev); I just realized that there is a theoretical race here where userspace can call amd_pmf_profile_set before amd_pmf_load_defaults_sps() is finshed, so the platform_profile_register() must be moved to the end of the function. I will fix this up while merging this. Regards, Hans > + > + return err; > +} > + > +void amd_pmf_deinit_sps(struct amd_pmf_dev *dev) > +{ > + platform_profile_remove(); > +}
Hi Shyam, On Tue, Aug 02, 2022 at 08:41:41PM +0530, Shyam Sundar S K wrote: > SPS (a.k.a. Static Power Slider) gives a feel of Windows performance > power slider for the Linux users, where the user selects a certain > mode (like "balanced", "low-power" or "performance") and the thermals > associated with each selected mode gets applied from the silicon > side via the mailboxes defined through PMFW. > > PMF driver hooks to platform_profile by reading the PMF ACPI fn9 to > see if the support is being advertised by ACPI interface. > > If supported, the PMF driver reacts to platform_profile selection choices > made by the user and adjust the system thermal behavior. > > Reviewed-by: Hans de Goede <hdegoede@redhat.com> > Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> <snip> > diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c > new file mode 100644 > index 000000000000..ef4df3fd774b > --- /dev/null > +++ b/drivers/platform/x86/amd/pmf/sps.c <snip> > +u8 amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf) > +{ > + u8 mode; > + > + switch (pmf->current_profile) { > + case PLATFORM_PROFILE_PERFORMANCE: > + mode = POWER_MODE_PERFORMANCE; > + break; > + case PLATFORM_PROFILE_BALANCED: > + mode = POWER_MODE_BALANCED_POWER; > + break; > + case PLATFORM_PROFILE_LOW_POWER: > + mode = POWER_MODE_POWER_SAVER; > + break; > + default: > + dev_err(pmf->dev, "Unknown Platform Profile.\n"); > + break; > + } > + > + return mode; > +} This patch is now in -next as commit 4c71ae414474 ("platform/x86/amd/pmf: Add support SPS PMF feature"), where it causes the following clang warning: drivers/platform/x86/amd/pmf/sps.c:96:2: error: variable 'mode' is used uninitialized whenever switch default is taken [-Werror,-Wsometimes-uninitialized] default: ^~~~~~~ drivers/platform/x86/amd/pmf/sps.c:101:9: note: uninitialized use occurs here return mode; ^~~~ drivers/platform/x86/amd/pmf/sps.c:84:9: note: initialize the variable 'mode' to silence this warning u8 mode; ^ = '\0' 1 error generated. As far as I can tell, the default case cannot actually happen due to the advertising of choices in amd_pmf_init_sps() and the check against those choices in platform_profile_store() but it would be good to avoid this warning, especially given that it is fatal with CONFIG_WERROR. I do not mind sending a patch for this but I am a little unclear what the best fix would be. Removing the default case would cause -Wswitch warnings because current_profile is an enum (plus it would make finding invalid profiles harder if there was ever a change in the core). Perhaps changing the return type to be an int, returning an error code in the default case, then updating the call sites to check for an error? I am open to other suggestions (or if you want to sent a fix yourself, just consider this a report). Cheers, Nathan
Hi Nathan, Thanks for bringing this up. On 8/19/2022 4:17 AM, Nathan Chancellor wrote: > Hi Shyam, > > On Tue, Aug 02, 2022 at 08:41:41PM +0530, Shyam Sundar S K wrote: >> SPS (a.k.a. Static Power Slider) gives a feel of Windows performance >> power slider for the Linux users, where the user selects a certain >> mode (like "balanced", "low-power" or "performance") and the thermals >> associated with each selected mode gets applied from the silicon >> side via the mailboxes defined through PMFW. >> >> PMF driver hooks to platform_profile by reading the PMF ACPI fn9 to >> see if the support is being advertised by ACPI interface. >> >> If supported, the PMF driver reacts to platform_profile selection choices >> made by the user and adjust the system thermal behavior. >> >> Reviewed-by: Hans de Goede <hdegoede@redhat.com> >> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> > > <snip> > >> diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c >> new file mode 100644 >> index 000000000000..ef4df3fd774b >> --- /dev/null >> +++ b/drivers/platform/x86/amd/pmf/sps.c > > <snip> > >> +u8 amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf) >> +{ >> + u8 mode; >> + >> + switch (pmf->current_profile) { >> + case PLATFORM_PROFILE_PERFORMANCE: >> + mode = POWER_MODE_PERFORMANCE; >> + break; >> + case PLATFORM_PROFILE_BALANCED: >> + mode = POWER_MODE_BALANCED_POWER; >> + break; >> + case PLATFORM_PROFILE_LOW_POWER: >> + mode = POWER_MODE_POWER_SAVER; >> + break; >> + default: >> + dev_err(pmf->dev, "Unknown Platform Profile.\n"); >> + break; >> + } >> + >> + return mode; >> +} > > This patch is now in -next as commit 4c71ae414474 > ("platform/x86/amd/pmf: Add support SPS PMF feature"), where it causes > the following clang warning: > > drivers/platform/x86/amd/pmf/sps.c:96:2: error: variable 'mode' is used uninitialized whenever switch default is taken [-Werror,-Wsometimes-uninitialized] > default: > ^~~~~~~ > drivers/platform/x86/amd/pmf/sps.c:101:9: note: uninitialized use occurs here > return mode; > ^~~~ > drivers/platform/x86/amd/pmf/sps.c:84:9: note: initialize the variable 'mode' to silence this warning > u8 mode; > ^ > = '\0' > 1 error generated. > > As far as I can tell, the default case cannot actually happen due to the > advertising of choices in amd_pmf_init_sps() and the check against those > choices in platform_profile_store() but it would be good to avoid this > warning, especially given that it is fatal with CONFIG_WERROR. > > I do not mind sending a patch for this but I am a little unclear what > the best fix would be. Removing the default case would cause -Wswitch > warnings because current_profile is an enum (plus it would make finding > invalid profiles harder if there was ever a change in the core). Perhaps > changing the return type to be an int, returning an error code in the > default case, then updating the call sites to check for an error? I am > open to other suggestions (or if you want to sent a fix yourself, just > consider this a report). yes, you are right. We can just change the return an error code like the other driver implementing the platform_profile. Like below. diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h index 7613ed2ef6e3..172610f93bd1 100644 --- a/drivers/platform/x86/amd/pmf/pmf.h +++ b/drivers/platform/x86/amd/pmf/pmf.h @@ -303,7 +303,7 @@ int amd_pmf_init_metrics_table(struct amd_pmf_dev *dev); int amd_pmf_get_power_source(void); /* SPS Layer */ -u8 amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf); +int amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf); void amd_pmf_update_slider(struct amd_pmf_dev *dev, bool op, int idx, struct amd_pmf_static_slider_granular *table); int amd_pmf_init_sps(struct amd_pmf_dev *dev); diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c index 8923e29cc6ca..dba7e36962dc 100644 --- a/drivers/platform/x86/amd/pmf/sps.c +++ b/drivers/platform/x86/amd/pmf/sps.c @@ -79,9 +79,9 @@ static int amd_pmf_profile_get(struct platform_profile_handler *pprof, return 0; } -u8 amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf) +int amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf) { - u8 mode; + int mode; switch (pmf->current_profile) { case PLATFORM_PROFILE_PERFORMANCE: @@ -95,7 +95,7 @@ u8 amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf) break; default: dev_err(pmf->dev, "Unknown Platform Profile.\n"); - break; + return -EOPNOTSUPP; } return mode; @@ -105,10 +105,13 @@ static int amd_pmf_profile_set(struct platform_profile_handler *pprof, enum platform_profile_option profile) { struct amd_pmf_dev *pmf = container_of(pprof, struct amd_pmf_dev, pprof); - u8 mode; + int mode; pmf->current_profile = profile; mode = amd_pmf_get_pprof_modes(pmf); + if (mode < 0) + return mode; + amd_pmf_update_slider(pmf, SLIDER_OP_SET, mode, NULL); return 0; } Thanks, Shyam > > Cheers, > Nathan >
Hi Shyam, On 8/19/22 10:58, Shyam Sundar S K wrote: > Hi Nathan, > > Thanks for bringing this up. > > On 8/19/2022 4:17 AM, Nathan Chancellor wrote: >> Hi Shyam, >> >> On Tue, Aug 02, 2022 at 08:41:41PM +0530, Shyam Sundar S K wrote: >>> SPS (a.k.a. Static Power Slider) gives a feel of Windows performance >>> power slider for the Linux users, where the user selects a certain >>> mode (like "balanced", "low-power" or "performance") and the thermals >>> associated with each selected mode gets applied from the silicon >>> side via the mailboxes defined through PMFW. >>> >>> PMF driver hooks to platform_profile by reading the PMF ACPI fn9 to >>> see if the support is being advertised by ACPI interface. >>> >>> If supported, the PMF driver reacts to platform_profile selection choices >>> made by the user and adjust the system thermal behavior. >>> >>> Reviewed-by: Hans de Goede <hdegoede@redhat.com> >>> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> >> >> <snip> >> >>> diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c >>> new file mode 100644 >>> index 000000000000..ef4df3fd774b >>> --- /dev/null >>> +++ b/drivers/platform/x86/amd/pmf/sps.c >> >> <snip> >> >>> +u8 amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf) >>> +{ >>> + u8 mode; >>> + >>> + switch (pmf->current_profile) { >>> + case PLATFORM_PROFILE_PERFORMANCE: >>> + mode = POWER_MODE_PERFORMANCE; >>> + break; >>> + case PLATFORM_PROFILE_BALANCED: >>> + mode = POWER_MODE_BALANCED_POWER; >>> + break; >>> + case PLATFORM_PROFILE_LOW_POWER: >>> + mode = POWER_MODE_POWER_SAVER; >>> + break; >>> + default: >>> + dev_err(pmf->dev, "Unknown Platform Profile.\n"); >>> + break; >>> + } >>> + >>> + return mode; >>> +} >> >> This patch is now in -next as commit 4c71ae414474 >> ("platform/x86/amd/pmf: Add support SPS PMF feature"), where it causes >> the following clang warning: >> >> drivers/platform/x86/amd/pmf/sps.c:96:2: error: variable 'mode' is used uninitialized whenever switch default is taken [-Werror,-Wsometimes-uninitialized] >> default: >> ^~~~~~~ >> drivers/platform/x86/amd/pmf/sps.c:101:9: note: uninitialized use occurs here >> return mode; >> ^~~~ >> drivers/platform/x86/amd/pmf/sps.c:84:9: note: initialize the variable 'mode' to silence this warning >> u8 mode; >> ^ >> = '\0' >> 1 error generated. >> >> As far as I can tell, the default case cannot actually happen due to the >> advertising of choices in amd_pmf_init_sps() and the check against those >> choices in platform_profile_store() but it would be good to avoid this >> warning, especially given that it is fatal with CONFIG_WERROR. >> >> I do not mind sending a patch for this but I am a little unclear what >> the best fix would be. Removing the default case would cause -Wswitch >> warnings because current_profile is an enum (plus it would make finding >> invalid profiles harder if there was ever a change in the core). Perhaps >> changing the return type to be an int, returning an error code in the >> default case, then updating the call sites to check for an error? I am >> open to other suggestions (or if you want to sent a fix yourself, just >> consider this a report). > > yes, you are right. We can just change the return an error code like the > other driver implementing the platform_profile. Like below. Yes returning -EOPNOTSUPP is the right thing to do in the default case. Can you turn this into a proper patch/commit and submit it please? Regards, Hans > > diff --git a/drivers/platform/x86/amd/pmf/pmf.h > b/drivers/platform/x86/amd/pmf/pmf.h > index 7613ed2ef6e3..172610f93bd1 100644 > --- a/drivers/platform/x86/amd/pmf/pmf.h > +++ b/drivers/platform/x86/amd/pmf/pmf.h > @@ -303,7 +303,7 @@ int amd_pmf_init_metrics_table(struct amd_pmf_dev *dev); > int amd_pmf_get_power_source(void); > > /* SPS Layer */ > -u8 amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf); > +int amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf); > void amd_pmf_update_slider(struct amd_pmf_dev *dev, bool op, int idx, > struct amd_pmf_static_slider_granular *table); > int amd_pmf_init_sps(struct amd_pmf_dev *dev); > diff --git a/drivers/platform/x86/amd/pmf/sps.c > b/drivers/platform/x86/amd/pmf/sps.c > index 8923e29cc6ca..dba7e36962dc 100644 > --- a/drivers/platform/x86/amd/pmf/sps.c > +++ b/drivers/platform/x86/amd/pmf/sps.c > @@ -79,9 +79,9 @@ static int amd_pmf_profile_get(struct > platform_profile_handler *pprof, > return 0; > } > > -u8 amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf) > +int amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf) > { > - u8 mode; > + int mode; > > switch (pmf->current_profile) { > case PLATFORM_PROFILE_PERFORMANCE: > @@ -95,7 +95,7 @@ u8 amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf) > break; > default: > dev_err(pmf->dev, "Unknown Platform Profile.\n"); > - break; > + return -EOPNOTSUPP; > } > > return mode; > @@ -105,10 +105,13 @@ static int amd_pmf_profile_set(struct > platform_profile_handler *pprof, > enum platform_profile_option profile) > { > struct amd_pmf_dev *pmf = container_of(pprof, struct > amd_pmf_dev, pprof); > - u8 mode; > + int mode; > > pmf->current_profile = profile; > mode = amd_pmf_get_pprof_modes(pmf); > + if (mode < 0) > + return mode; > + > amd_pmf_update_slider(pmf, SLIDER_OP_SET, mode, NULL); > return 0; > } > > > Thanks, > Shyam > >> >> Cheers, >> Nathan >> >
diff --git a/drivers/platform/x86/amd/pmf/Makefile b/drivers/platform/x86/amd/pmf/Makefile index 2617eba773ce..557521a80427 100644 --- a/drivers/platform/x86/amd/pmf/Makefile +++ b/drivers/platform/x86/amd/pmf/Makefile @@ -5,4 +5,4 @@ # obj-$(CONFIG_AMD_PMF) += amd-pmf.o -amd-pmf-objs := core.o acpi.o +amd-pmf-objs := core.o acpi.o sps.o diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c index b378f9e31194..5997ab724f3a 100644 --- a/drivers/platform/x86/amd/pmf/acpi.c +++ b/drivers/platform/x86/amd/pmf/acpi.c @@ -93,6 +93,16 @@ int is_apmf_func_supported(struct amd_pmf_dev *pdev, unsigned long index) return !!(pdev->supported_func & BIT(index - 1)); } +int apmf_get_static_slider_granular(struct amd_pmf_dev *pdev, + struct apmf_static_slider_granular_output *data) +{ + if (!is_apmf_func_supported(pdev, APMF_FUNC_STATIC_SLIDER_GRANULAR)) + return -EINVAL; + + return apmf_if_call_store_buffer(pdev, APMF_FUNC_STATIC_SLIDER_GRANULAR, + data, sizeof(*data)); +} + static int apmf_if_verify_interface(struct amd_pmf_dev *pdev) { struct apmf_verify_interface output; diff --git a/drivers/platform/x86/amd/pmf/core.c b/drivers/platform/x86/amd/pmf/core.c index c5002b7bb904..a70ab6c9608a 100644 --- a/drivers/platform/x86/amd/pmf/core.c +++ b/drivers/platform/x86/amd/pmf/core.c @@ -12,6 +12,7 @@ #include <linux/module.h> #include <linux/pci.h> #include <linux/platform_device.h> +#include <linux/power_supply.h> #include "pmf.h" /* PMF-SMU communication registers */ @@ -45,6 +46,14 @@ #define DELAY_MIN_US 2000 #define DELAY_MAX_US 3000 +int amd_pmf_get_power_source(void) +{ + if (power_supply_is_system_supplied() > 0) + return POWER_SOURCE_AC; + else + return POWER_SOURCE_DC; +} + static inline u32 amd_pmf_reg_read(struct amd_pmf_dev *dev, int reg_offset) { return ioread32(dev->regbase + reg_offset); @@ -138,6 +147,21 @@ static const struct pci_device_id pmf_pci_ids[] = { { } }; +static void amd_pmf_init_features(struct amd_pmf_dev *dev) +{ + /* Enable Static Slider */ + if (is_apmf_func_supported(dev, APMF_FUNC_STATIC_SLIDER_GRANULAR)) { + amd_pmf_init_sps(dev); + dev_dbg(dev->dev, "SPS enabled and Platform Profiles registered\n"); + } +} + +static void amd_pmf_deinit_features(struct amd_pmf_dev *dev) +{ + if (is_apmf_func_supported(dev, APMF_FUNC_STATIC_SLIDER_GRANULAR)) + amd_pmf_deinit_sps(dev); +} + static const struct acpi_device_id amd_pmf_acpi_ids[] = { {"AMDI0102", 0}, { } @@ -206,6 +230,7 @@ static int amd_pmf_probe(struct platform_device *pdev) apmf_acpi_init(dev); platform_set_drvdata(pdev, dev); + amd_pmf_init_features(dev); mutex_init(&dev->lock); dev_info(dev->dev, "registered PMF device successfully\n"); @@ -218,6 +243,7 @@ static int amd_pmf_remove(struct platform_device *pdev) struct amd_pmf_dev *dev = platform_get_drvdata(pdev); mutex_destroy(&dev->lock); + amd_pmf_deinit_features(dev); kfree(dev->buf); return 0; } diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h index bdadbff168ee..5c867dac7d44 100644 --- a/drivers/platform/x86/amd/pmf/pmf.h +++ b/drivers/platform/x86/amd/pmf/pmf.h @@ -12,10 +12,12 @@ #define PMF_H #include <linux/acpi.h> +#include <linux/platform_profile.h> /* APMF Functions */ #define APMF_FUNC_VERIFY_INTERFACE 0 #define APMF_FUNC_GET_SYS_PARAMS 1 +#define APMF_FUNC_STATIC_SLIDER_GRANULAR 9 /* Message Definitions */ #define SET_SPL 0x03 /* SPL: Sustained Power Limit */ @@ -36,6 +38,8 @@ #define GET_STT_LIMIT_APU 0x20 #define GET_STT_LIMIT_HS2 0x21 +#define ARG_NONE 0 + /* AMD PMF BIOS interfaces */ struct apmf_verify_interface { u16 size; @@ -51,6 +55,30 @@ struct apmf_system_params { u8 command_code; } __packed; +enum amd_stt_skin_temp { + STT_TEMP_APU, + STT_TEMP_HS2, + STT_TEMP_COUNT, +}; + +enum amd_slider_op { + SLIDER_OP_GET, + SLIDER_OP_SET, +}; + +enum power_source { + POWER_SOURCE_AC, + POWER_SOURCE_DC, + POWER_SOURCE_MAX, +}; + +enum power_modes { + POWER_MODE_PERFORMANCE, + POWER_MODE_BALANCED_POWER, + POWER_MODE_POWER_SAVER, + POWER_MODE_MAX, +}; + struct amd_pmf_dev { void __iomem *regbase; void __iomem *smu_virt_addr; @@ -60,10 +88,46 @@ struct amd_pmf_dev { struct device *dev; struct mutex lock; /* protects the PMF interface */ u32 supported_func; + enum platform_profile_option current_profile; + struct platform_profile_handler pprof; +}; + +struct apmf_sps_prop_granular { + u32 fppt; + u32 sppt; + u32 sppt_apu_only; + u32 spl; + u32 stt_min; + u8 stt_skin_temp[STT_TEMP_COUNT]; + u32 fan_id; +} __packed; + +/* Static Slider */ +struct apmf_static_slider_granular_output { + u16 size; + struct apmf_sps_prop_granular prop[POWER_SOURCE_MAX * POWER_MODE_MAX]; +} __packed; + +struct amd_pmf_static_slider_granular { + u16 size; + struct apmf_sps_prop_granular prop[POWER_SOURCE_MAX][POWER_MODE_MAX]; }; /* Core Layer */ int apmf_acpi_init(struct amd_pmf_dev *pmf_dev); +int is_apmf_func_supported(struct amd_pmf_dev *pdev, unsigned long index); int amd_pmf_send_cmd(struct amd_pmf_dev *dev, u8 message, bool get, u32 arg, u32 *data); +int amd_pmf_get_power_source(void); + +/* SPS Layer */ +u8 amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf); +void amd_pmf_update_slider(struct amd_pmf_dev *dev, bool op, int idx, + struct amd_pmf_static_slider_granular *table); +int amd_pmf_init_sps(struct amd_pmf_dev *dev); +void amd_pmf_deinit_sps(struct amd_pmf_dev *dev); +int apmf_get_static_slider_granular(struct amd_pmf_dev *pdev, + struct apmf_static_slider_granular_output *output); +void amd_pmf_load_defaults_sps(struct amd_pmf_dev *dev); + #endif /* PMF_H */ diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c new file mode 100644 index 000000000000..ef4df3fd774b --- /dev/null +++ b/drivers/platform/x86/amd/pmf/sps.c @@ -0,0 +1,149 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * AMD Platform Management Framework (PMF) Driver + * + * Copyright (c) 2022, Advanced Micro Devices, Inc. + * All Rights Reserved. + * + * Author: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> + */ + +#include "pmf.h" + +static int amd_pmf_profile_get(struct platform_profile_handler *pprof, + enum platform_profile_option *profile); +static int amd_pmf_profile_set(struct platform_profile_handler *pprof, + enum platform_profile_option profile); +static struct amd_pmf_static_slider_granular config_store; + +void amd_pmf_load_defaults_sps(struct amd_pmf_dev *dev) +{ + struct apmf_static_slider_granular_output output; + int i, j, idx = 0; + + memset(&config_store, 0, sizeof(config_store)); + apmf_get_static_slider_granular(dev, &output); + + for (i = 0; i < POWER_SOURCE_MAX; i++) { + for (j = 0; j < POWER_MODE_MAX; j++) { + config_store.prop[i][j].spl = output.prop[idx].spl; + config_store.prop[i][j].sppt = output.prop[idx].sppt; + config_store.prop[i][j].sppt_apu_only = + output.prop[idx].sppt_apu_only; + config_store.prop[i][j].fppt = output.prop[idx].fppt; + config_store.prop[i][j].stt_min = output.prop[idx].stt_min; + config_store.prop[i][j].stt_skin_temp[STT_TEMP_APU] = + output.prop[idx].stt_skin_temp[STT_TEMP_APU]; + config_store.prop[i][j].stt_skin_temp[STT_TEMP_HS2] = + output.prop[idx].stt_skin_temp[STT_TEMP_HS2]; + config_store.prop[i][j].fan_id = output.prop[idx].fan_id; + idx++; + } + } +} + +void amd_pmf_update_slider(struct amd_pmf_dev *dev, bool op, int idx, + struct amd_pmf_static_slider_granular *table) +{ + int src = amd_pmf_get_power_source(); + + if (op == SLIDER_OP_SET) { + amd_pmf_send_cmd(dev, SET_SPL, false, config_store.prop[src][idx].spl, NULL); + amd_pmf_send_cmd(dev, SET_FPPT, false, config_store.prop[src][idx].fppt, NULL); + amd_pmf_send_cmd(dev, SET_SPPT, false, config_store.prop[src][idx].sppt, NULL); + amd_pmf_send_cmd(dev, SET_SPPT_APU_ONLY, false, + config_store.prop[src][idx].sppt_apu_only, NULL); + 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); + amd_pmf_send_cmd(dev, SET_STT_LIMIT_HS2, false, + config_store.prop[src][idx].stt_skin_temp[STT_TEMP_HS2], 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); + amd_pmf_send_cmd(dev, GET_SPPT, true, ARG_NONE, &table->prop[src][idx].sppt); + amd_pmf_send_cmd(dev, GET_SPPT_APU_ONLY, true, ARG_NONE, + &table->prop[src][idx].sppt_apu_only); + amd_pmf_send_cmd(dev, GET_STT_MIN_LIMIT, true, ARG_NONE, + &table->prop[src][idx].stt_min); + amd_pmf_send_cmd(dev, GET_STT_LIMIT_APU, true, ARG_NONE, + (u32 *)&table->prop[src][idx].stt_skin_temp[STT_TEMP_APU]); + amd_pmf_send_cmd(dev, GET_STT_LIMIT_HS2, true, ARG_NONE, + (u32 *)&table->prop[src][idx].stt_skin_temp[STT_TEMP_HS2]); + } +} + +static int amd_pmf_profile_get(struct platform_profile_handler *pprof, + enum platform_profile_option *profile) +{ + struct amd_pmf_dev *pmf = container_of(pprof, struct amd_pmf_dev, pprof); + + *profile = pmf->current_profile; + return 0; +} + +u8 amd_pmf_get_pprof_modes(struct amd_pmf_dev *pmf) +{ + u8 mode; + + switch (pmf->current_profile) { + case PLATFORM_PROFILE_PERFORMANCE: + mode = POWER_MODE_PERFORMANCE; + break; + case PLATFORM_PROFILE_BALANCED: + mode = POWER_MODE_BALANCED_POWER; + break; + case PLATFORM_PROFILE_LOW_POWER: + mode = POWER_MODE_POWER_SAVER; + break; + default: + dev_err(pmf->dev, "Unknown Platform Profile.\n"); + break; + } + + return mode; +} + +int amd_pmf_profile_set(struct platform_profile_handler *pprof, + enum platform_profile_option profile) +{ + struct amd_pmf_dev *pmf = container_of(pprof, struct amd_pmf_dev, pprof); + u8 mode; + + pmf->current_profile = profile; + mode = amd_pmf_get_pprof_modes(pmf); + amd_pmf_update_slider(pmf, SLIDER_OP_SET, mode, NULL); + return 0; +} + +int amd_pmf_init_sps(struct amd_pmf_dev *dev) +{ + int err = 0; + + dev->pprof.profile_get = amd_pmf_profile_get; + dev->pprof.profile_set = amd_pmf_profile_set; + + /* Setup supported modes */ + set_bit(PLATFORM_PROFILE_LOW_POWER, dev->pprof.choices); + set_bit(PLATFORM_PROFILE_BALANCED, dev->pprof.choices); + set_bit(PLATFORM_PROFILE_PERFORMANCE, dev->pprof.choices); + + /* Create platform_profile structure and register */ + err = platform_profile_register(&dev->pprof); + if (err) { + dev_err(dev->dev, "Failed to register SPS support, this is most likely an SBIOS bug: %d\n", + err); + return -EEXIST; + } + + dev->current_profile = PLATFORM_PROFILE_BALANCED; + amd_pmf_load_defaults_sps(dev); + + return err; +} + +void amd_pmf_deinit_sps(struct amd_pmf_dev *dev) +{ + platform_profile_remove(); +}