| Message ID | 20240227125520.3153140-6-Shyam-sundar.S-k@amd.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | platform/x86/amd/pmf: Updates to amd-pmf driver | expand |
On Tue, 27 Feb 2024, Shyam Sundar S K wrote: > APMF spec has a newer section called the APTS (AMD Performance and > Thermal State) information, where each slider/power mode is associated > with an index number. > > Add support to get these indices for the Static Slider. > > Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com> > Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com> > Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> > --- > drivers/platform/x86/amd/pmf/acpi.c | 10 +++++++ > drivers/platform/x86/amd/pmf/pmf.h | 24 +++++++++++++++++ > drivers/platform/x86/amd/pmf/sps.c | 42 ++++++++++++++++++++++++++++- > 3 files changed, 75 insertions(+), 1 deletion(-) > > diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c > index 0fc8ad0ac3e9..28df45c058db 100644 > --- a/drivers/platform/x86/amd/pmf/acpi.c > +++ b/drivers/platform/x86/amd/pmf/acpi.c > @@ -96,6 +96,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_v2(struct amd_pmf_dev *pdev, > + struct apmf_static_slider_granular_output_v2 *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)); Wrong aligment. Please go through all the patches to check these. > diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c > index 33e23e25c8b1..dc4c7ccd4c43 100644 > --- a/drivers/platform/x86/amd/pmf/sps.c > +++ b/drivers/platform/x86/amd/pmf/sps.c > @@ -10,6 +10,7 @@ > > #include "pmf.h" > > +static struct amd_pmf_static_slider_granular_v2 config_store_v2; > static struct amd_pmf_static_slider_granular config_store; > > #ifdef CONFIG_AMD_PMF_DEBUG > @@ -63,10 +64,46 @@ static void amd_pmf_dump_sps_defaults(struct amd_pmf_static_slider_granular *dat > > pr_debug("Static Slider Data - END\n"); > } > + > +static void amd_pmf_dump_sps_defaults_v2(struct amd_pmf_static_slider_granular_v2 *data) > +{ > + pr_debug("Static Slider APTS state index data - BEGIN"); > + pr_debug("size: %u\n", data->size); > + pr_debug("ac_best_perf: %u\n", data->sps_idx.ac_best_perf); > + pr_debug("ac_balanced: %u\n", data->sps_idx.ac_balanced); > + pr_debug("ac_best_pwr_efficiency: %u\n", data->sps_idx.ac_best_pwr_efficiency); > + pr_debug("ac_energy_saver: %u\n", data->sps_idx.ac_energy_saver); > + pr_debug("dc_best_perf: %u\n", data->sps_idx.dc_best_perf); > + pr_debug("dc_balanced: %u\n", data->sps_idx.dc_balanced); > + pr_debug("dc_best_pwr_efficiency: %u\n", data->sps_idx.dc_best_pwr_efficiency); > + pr_debug("dc_battery_saver: %u\n", data->sps_idx.dc_battery_saver); I know these are debug only but what is the advantage of having the underscores in them? I think they'd read & match just fine without them (perhaps pwr->power would be better but it's up to you) and ac/dc can then be capitalized.
On Tue, 27 Feb 2024, Shyam Sundar S K wrote: > APMF spec has a newer section called the APTS (AMD Performance and > Thermal State) information, where each slider/power mode is associated > with an index number. > > Add support to get these indices for the Static Slider. > > Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com> > Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com> > Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> > --- > drivers/platform/x86/amd/pmf/acpi.c | 10 +++++++ > drivers/platform/x86/amd/pmf/pmf.h | 24 +++++++++++++++++ > drivers/platform/x86/amd/pmf/sps.c | 42 ++++++++++++++++++++++++++++- > 3 files changed, 75 insertions(+), 1 deletion(-) > > diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c > index 0fc8ad0ac3e9..28df45c058db 100644 > --- a/drivers/platform/x86/amd/pmf/acpi.c > +++ b/drivers/platform/x86/amd/pmf/acpi.c > @@ -96,6 +96,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_v2(struct amd_pmf_dev *pdev, > + struct apmf_static_slider_granular_output_v2 *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)); > +} > + > int apmf_get_static_slider_granular(struct amd_pmf_dev *pdev, > struct apmf_static_slider_granular_output *data) > { > diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h > index 5cad11369697..b27e96aeac23 100644 > --- a/drivers/platform/x86/amd/pmf/pmf.h > +++ b/drivers/platform/x86/amd/pmf/pmf.h > @@ -85,6 +85,7 @@ > #define MAX_OPERATION_PARAMS 4 > > #define PMF_IF_V1 1 > +#define PMF_IF_V2 2 > > struct sbios_hb_event_v2 { > u16 size; > @@ -264,6 +265,17 @@ struct amd_pmf_dev { > u16 pmf_if_version; > }; > > +struct apmf_sps_prop_granular_v2 { > + u8 ac_best_perf; > + u8 ac_balanced; > + u8 ac_best_pwr_efficiency; > + u8 ac_energy_saver; > + u8 dc_best_perf; > + u8 dc_balanced; > + u8 dc_best_pwr_efficiency; > + u8 dc_battery_saver; I started to wonder if these could be made into an two element array with 4xu8 in each, one for AC and DC because it would simplify some other code in the subsequent patches (and perhaps even in this patch)?
On 2/27/2024 19:43, Ilpo Järvinen wrote: > On Tue, 27 Feb 2024, Shyam Sundar S K wrote: > >> APMF spec has a newer section called the APTS (AMD Performance and >> Thermal State) information, where each slider/power mode is associated >> with an index number. >> >> Add support to get these indices for the Static Slider. >> >> Co-developed-by: Patil Rajesh Reddy <Patil.Reddy@amd.com> >> Signed-off-by: Patil Rajesh Reddy <Patil.Reddy@amd.com> >> Signed-off-by: Shyam Sundar S K <Shyam-sundar.S-k@amd.com> >> --- >> drivers/platform/x86/amd/pmf/acpi.c | 10 +++++++ >> drivers/platform/x86/amd/pmf/pmf.h | 24 +++++++++++++++++ >> drivers/platform/x86/amd/pmf/sps.c | 42 ++++++++++++++++++++++++++++- >> 3 files changed, 75 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c >> index 0fc8ad0ac3e9..28df45c058db 100644 >> --- a/drivers/platform/x86/amd/pmf/acpi.c >> +++ b/drivers/platform/x86/amd/pmf/acpi.c >> @@ -96,6 +96,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_v2(struct amd_pmf_dev *pdev, >> + struct apmf_static_slider_granular_output_v2 *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)); >> +} >> + >> int apmf_get_static_slider_granular(struct amd_pmf_dev *pdev, >> struct apmf_static_slider_granular_output *data) >> { >> diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h >> index 5cad11369697..b27e96aeac23 100644 >> --- a/drivers/platform/x86/amd/pmf/pmf.h >> +++ b/drivers/platform/x86/amd/pmf/pmf.h >> @@ -85,6 +85,7 @@ >> #define MAX_OPERATION_PARAMS 4 >> >> #define PMF_IF_V1 1 >> +#define PMF_IF_V2 2 >> >> struct sbios_hb_event_v2 { >> u16 size; >> @@ -264,6 +265,17 @@ struct amd_pmf_dev { >> u16 pmf_if_version; >> }; >> >> +struct apmf_sps_prop_granular_v2 { >> + u8 ac_best_perf; >> + u8 ac_balanced; >> + u8 ac_best_pwr_efficiency; >> + u8 ac_energy_saver; >> + u8 dc_best_perf; >> + u8 dc_balanced; >> + u8 dc_best_pwr_efficiency; >> + u8 dc_battery_saver; > > I started to wonder if these could be made into an two element array with > 4xu8 in each, one for AC and DC because it would simplify some other code > in the subsequent patches (and perhaps even in this patch)? > OK I understand your point. Let me give it a try. (I tried to retain the same struct fields across Linux and Windows so that its easy to maintain) Will address your remarks on other patches in the series in the new revision. Thanks, Shyam
diff --git a/drivers/platform/x86/amd/pmf/acpi.c b/drivers/platform/x86/amd/pmf/acpi.c index 0fc8ad0ac3e9..28df45c058db 100644 --- a/drivers/platform/x86/amd/pmf/acpi.c +++ b/drivers/platform/x86/amd/pmf/acpi.c @@ -96,6 +96,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_v2(struct amd_pmf_dev *pdev, + struct apmf_static_slider_granular_output_v2 *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)); +} + int apmf_get_static_slider_granular(struct amd_pmf_dev *pdev, struct apmf_static_slider_granular_output *data) { diff --git a/drivers/platform/x86/amd/pmf/pmf.h b/drivers/platform/x86/amd/pmf/pmf.h index 5cad11369697..b27e96aeac23 100644 --- a/drivers/platform/x86/amd/pmf/pmf.h +++ b/drivers/platform/x86/amd/pmf/pmf.h @@ -85,6 +85,7 @@ #define MAX_OPERATION_PARAMS 4 #define PMF_IF_V1 1 +#define PMF_IF_V2 2 struct sbios_hb_event_v2 { u16 size; @@ -264,6 +265,17 @@ struct amd_pmf_dev { u16 pmf_if_version; }; +struct apmf_sps_prop_granular_v2 { + u8 ac_best_perf; + u8 ac_balanced; + u8 ac_best_pwr_efficiency; + u8 ac_energy_saver; + u8 dc_best_perf; + u8 dc_balanced; + u8 dc_best_pwr_efficiency; + u8 dc_battery_saver; +} __packed; + struct apmf_sps_prop_granular { u32 fppt; u32 sppt; @@ -285,6 +297,16 @@ struct amd_pmf_static_slider_granular { struct apmf_sps_prop_granular prop[POWER_SOURCE_MAX][POWER_MODE_MAX]; }; +struct apmf_static_slider_granular_output_v2 { + u16 size; + struct apmf_sps_prop_granular_v2 sps_idx; +} __packed; + +struct amd_pmf_static_slider_granular_v2 { + u16 size; + struct apmf_sps_prop_granular_v2 sps_idx; +}; + struct os_power_slider { u16 size; u8 slider_event; @@ -634,6 +656,8 @@ const char *amd_pmf_source_as_str(unsigned int state); int apmf_update_fan_idx(struct amd_pmf_dev *pdev, bool manual, u32 idx); int amd_pmf_set_sps_power_limits(struct amd_pmf_dev *pmf); +int apmf_get_static_slider_granular_v2(struct amd_pmf_dev *dev, + struct apmf_static_slider_granular_output_v2 *data); /* Auto Mode Layer */ int apmf_get_auto_mode_def(struct amd_pmf_dev *pdev, struct apmf_auto_mode *data); diff --git a/drivers/platform/x86/amd/pmf/sps.c b/drivers/platform/x86/amd/pmf/sps.c index 33e23e25c8b1..dc4c7ccd4c43 100644 --- a/drivers/platform/x86/amd/pmf/sps.c +++ b/drivers/platform/x86/amd/pmf/sps.c @@ -10,6 +10,7 @@ #include "pmf.h" +static struct amd_pmf_static_slider_granular_v2 config_store_v2; static struct amd_pmf_static_slider_granular config_store; #ifdef CONFIG_AMD_PMF_DEBUG @@ -63,10 +64,46 @@ static void amd_pmf_dump_sps_defaults(struct amd_pmf_static_slider_granular *dat pr_debug("Static Slider Data - END\n"); } + +static void amd_pmf_dump_sps_defaults_v2(struct amd_pmf_static_slider_granular_v2 *data) +{ + pr_debug("Static Slider APTS state index data - BEGIN"); + pr_debug("size: %u\n", data->size); + pr_debug("ac_best_perf: %u\n", data->sps_idx.ac_best_perf); + pr_debug("ac_balanced: %u\n", data->sps_idx.ac_balanced); + pr_debug("ac_best_pwr_efficiency: %u\n", data->sps_idx.ac_best_pwr_efficiency); + pr_debug("ac_energy_saver: %u\n", data->sps_idx.ac_energy_saver); + pr_debug("dc_best_perf: %u\n", data->sps_idx.dc_best_perf); + pr_debug("dc_balanced: %u\n", data->sps_idx.dc_balanced); + pr_debug("dc_best_pwr_efficiency: %u\n", data->sps_idx.dc_best_pwr_efficiency); + pr_debug("dc_battery_saver: %u\n", data->sps_idx.dc_battery_saver); + pr_debug("Static Slider APTS state index data - END\n"); +} #else static void amd_pmf_dump_sps_defaults(struct amd_pmf_static_slider_granular *data) {} +static void amd_pmf_dump_sps_defaults_v2(struct amd_pmf_static_slider_granular_v2 *data) {} #endif +static void amd_pmf_load_defaults_sps_v2(struct amd_pmf_dev *dev) +{ + struct apmf_static_slider_granular_output_v2 output; + + memset(&config_store_v2, 0, sizeof(config_store_v2)); + apmf_get_static_slider_granular_v2(dev, &output); + + config_store_v2.size = output.size; + config_store_v2.sps_idx.ac_best_perf = output.sps_idx.ac_best_perf; + config_store_v2.sps_idx.ac_balanced = output.sps_idx.ac_balanced; + config_store_v2.sps_idx.ac_best_pwr_efficiency = output.sps_idx.ac_best_pwr_efficiency; + config_store_v2.sps_idx.ac_energy_saver = output.sps_idx.ac_energy_saver; + config_store_v2.sps_idx.dc_best_perf = output.sps_idx.dc_best_perf; + config_store_v2.sps_idx.dc_balanced = output.sps_idx.dc_balanced; + config_store_v2.sps_idx.dc_best_pwr_efficiency = output.sps_idx.dc_best_pwr_efficiency; + config_store_v2.sps_idx.dc_battery_saver = output.sps_idx.dc_battery_saver; + + amd_pmf_dump_sps_defaults_v2(&config_store_v2); +} + static void amd_pmf_load_defaults_sps(struct amd_pmf_dev *dev) { struct apmf_static_slider_granular_output output; @@ -256,7 +293,10 @@ int amd_pmf_init_sps(struct amd_pmf_dev *dev) dev->current_profile = PLATFORM_PROFILE_BALANCED; if (is_apmf_func_supported(dev, APMF_FUNC_STATIC_SLIDER_GRANULAR)) { - amd_pmf_load_defaults_sps(dev); + if (dev->pmf_if_version == PMF_IF_V2) + amd_pmf_load_defaults_sps_v2(dev); + else + amd_pmf_load_defaults_sps(dev); /* update SPS balanced power mode thermals */ amd_pmf_set_sps_power_limits(dev);