Message-ID: <c2b86d8f-d148-4ad8-aa46-f94b9598be80@gmail.com>
Date: Tue, 14 Oct 2025 00:26:19 +0200
From: Denis Benato <benato.denis96@...il.com>
To: ALOK TIWARI <alok.a.tiwari@...cle.com>, linux-kernel@...r.kernel.org
Cc: platform-driver-x86@...r.kernel.org, Hans de Goede <hdegoede@...hat.com>,
 Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 "Limonciello, Mario" <mario.limonciello@....com>,
 "Luke D . Jones" <luke@...nes.dev>,
 Derek John Clark <derekjohn.clark@...il.com>,
 Mateusz Schyboll <dragonn@...pl>, porfet828@...il.com
Subject: Re: [External] : [PATCH v13 8/8] platform/x86: asus-armoury: add
 ppt_* and nv_* tuning knobs


On 10/13/25 21:50, Denis Benato wrote:
> On 10/13/25 20:25, ALOK TIWARI wrote:
>>
>> On 10/13/2025 11:35 PM, Denis Benato wrote:
>>> From: "Luke D. Jones" <luke@...nes.dev>
>>>
>>> Adds the ppt_* and nv_* tuning knobs that are available via WMI methods
>>> and adds proper min/max levels plus defaults.
>>>
>>> The min/max are defined by ASUS and typically gained by looking at what
>>> they allow in the ASUS Armoury Crate application - ASUS does not share
>>> the values outside of this. It could also be possible to gain the AMD
>>> values by use of ryzenadj and testing for the minimum stable value.
>>>
>>> The general rule of thumb for adding to the match table is that if the
>>> model range has a single CPU used throughout, then the DMI match can
>>> omit the last letter of the model number as this is the GPU model.
>>>
>>> If a min or max value is not provided it is assumed that the particular
>>> setting is not supported. for example ppt_pl2_sppt_min/max is not set.
>>> If a <ppt_setting>_def is not set then the default is assumed to be
>>> <ppt_setting>_max
>>>
>>> It is assumed that at least AC settings are available so that the
>>> firmware attributes will be created - if no DC table is available
>>> and power is on DC, then reading the attributes is -ENODEV.
>>>
>>> Signed-off-by: Denis Benato <benato.denis96@...il.com>
>>> Signed-off-by: Luke D. Jones <luke@...nes.dev>
>>> Reviewed-by: Mario Limonciello <mario.limonciello@....com>
>>> Tested-by: Mateusz Schyboll <dragonn@...pl>
>>> ---
>>>   drivers/platform/x86/asus-armoury.c        |  296 ++++-
>>>   drivers/platform/x86/asus-armoury.h        | 1210 ++++++++++++++++++++
>>>   include/linux/platform_data/x86/asus-wmi.h |    3 +
>>>   3 files changed, 1503 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/asus-armoury.c b/drivers/platform/x86/asus-armoury.c
>>> index e27f964aebf8..918aea6fba1e 100644
>>> --- a/drivers/platform/x86/asus-armoury.c
>>> +++ b/drivers/platform/x86/asus-armoury.c
>>> @@ -27,6 +27,7 @@
>>>   #include <linux/mutex.h>
>>>   #include <linux/platform_data/x86/asus-wmi.h>
>>>   #include <linux/printk.h>
>>> +#include <linux/power_supply.h>
>>>   #include <linux/types.h>
>>>     #include "asus-armoury.h"
>>> @@ -45,6 +46,17 @@
>>>   #define ASUS_MINI_LED_2024_STRONG 0x01
>>>   #define ASUS_MINI_LED_2024_OFF    0x02
>>>   +/* Power tunable attribute name defines */
>>> +#define ATTR_PPT_PL1_SPL        "ppt_pl1_spl"
>>> +#define ATTR_PPT_PL2_SPPT       "ppt_pl2_sppt"
>>> +#define ATTR_PPT_PL3_FPPT       "ppt_pl3_fppt"
>>> +#define ATTR_PPT_APU_SPPT       "ppt_apu_sppt"
>>> +#define ATTR_PPT_PLATFORM_SPPT  "ppt_platform_sppt"
>>> +#define ATTR_NV_DYNAMIC_BOOST   "nv_dynamic_boost"
>>> +#define ATTR_NV_TEMP_TARGET     "nv_temp_target"
>>> +#define ATTR_NV_BASE_TGP        "nv_base_tgp"
>>> +#define ATTR_NV_TGP             "nv_tgp"
>>> +
>>>   #define ASUS_POWER_CORE_MASK    GENMASK(15, 8)
>>>   #define ASUS_PERF_CORE_MASK        GENMASK(7, 0)
>>>   @@ -73,11 +85,26 @@ struct cpu_cores {
>>>       u32 max_power_cores;
>>>   };
>>>   +struct rog_tunables {
>>> +    const struct power_limits *power_limits;
>>> +    u32 ppt_pl1_spl;            // cpu
>>> +    u32 ppt_pl2_sppt;            // cpu
>>> +    u32 ppt_pl3_fppt;            // cpu
>>> +    u32 ppt_apu_sppt;            // plat
>>> +    u32 ppt_platform_sppt;        // plat
>>> +
>>> +    u32 nv_dynamic_boost;
>>> +    u32 nv_temp_target;
>>> +    u32 nv_tgp;
>>> +};
>>> +
>>>   static struct asus_armoury_priv {
>>>       struct device *fw_attr_dev;
>>>       struct kset *fw_attr_kset;
>>>         struct cpu_cores *cpu_cores;
>>> +    /* Index 0 for DC, 1 for AC */
>>> +    struct rog_tunables *rog_tunables[2];
>>>       u32 mini_led_dev_id;
>>>       u32 gpu_mux_dev_id;
>>>       /*
>>> @@ -719,7 +746,34 @@ static ssize_t cores_efficiency_current_value_store(struct kobject *kobj,
>>>   ATTR_GROUP_CORES_RW(cores_efficiency, "cores_efficiency",
>>>               "Set the max available efficiency cores");
>>>   +/* Define helper to access the current power mode tunable values */
>>> +static inline struct rog_tunables *get_current_tunables(void)
>>> +{
>>> +    return asus_armoury
>>> +        .rog_tunables[power_supply_is_system_supplied() ? 1 : 0];
>>> +}
>>> +
>>>   /* Simple attribute creation */
>>> +ATTR_GROUP_ROG_TUNABLE(ppt_pl1_spl, ATTR_PPT_PL1_SPL, ASUS_WMI_DEVID_PPT_PL1_SPL,
>>> +               "Set the CPU slow package limit");
>>> +ATTR_GROUP_ROG_TUNABLE(ppt_pl2_sppt, ATTR_PPT_PL2_SPPT, ASUS_WMI_DEVID_PPT_PL2_SPPT,
>>> +               "Set the CPU fast package limit");
>>> +ATTR_GROUP_ROG_TUNABLE(ppt_pl3_fppt, ATTR_PPT_PL3_FPPT, ASUS_WMI_DEVID_PPT_FPPT,
>> why not ASUS_WMI_DEVID_PPT_PL3_FPPT ? 
>>
> I simply didn't touch anything that was not brought up, but I see that it appears to be a more consistent name.
>
> Will use that name for v14, thanks!
Unfortunately taking a closer look I discovered that macro has been introduced over
2 years ago in commit e0b278e7b5da62c3ebb156a8b7d76a739da2d953
"platform/x86: asus-wmi: expose dGPU and CPU tunables for ROG"
and it is not introduced as part of this commit series.

I think it would be best to create an ah-hoc commit when this driver is merged to change the name in both,
or do I send the name change now and rework this driver? what do you think?

Honestly given the large number of people already running this and the request of having it upstream
the road that will make it merge sooner is the one I would like to take.

Thanks,
Denis

>>> +               "Set the CPU fastest package limit");
>>> +ATTR_GROUP_ROG_TUNABLE(ppt_apu_sppt, ATTR_PPT_APU_SPPT, ASUS_WMI_DEVID_PPT_APU_SPPT,
>>> +               "Set the APU package limit");
>>> +ATTR_GROUP_ROG_TUNABLE(ppt_platform_sppt, ATTR_PPT_PLATFORM_SPPT, ASUS_WMI_DEVID_PPT_PLAT_SPPT,
>>> +               "Set the platform package limit");
>>> +ATTR_GROUP_ROG_TUNABLE(nv_dynamic_boost, ATTR_NV_DYNAMIC_BOOST, ASUS_WMI_DEVID_NV_DYN_BOOST,
>>> +               "Set the Nvidia dynamic boost limit");
>>> +ATTR_GROUP_ROG_TUNABLE(nv_temp_target, ATTR_NV_TEMP_TARGET, ASUS_WMI_DEVID_NV_THERM_TARGET,
>>> +               "Set the Nvidia max thermal limit");
>>> +ATTR_GROUP_ROG_TUNABLE(nv_tgp, "nv_tgp", ASUS_WMI_DEVID_DGPU_SET_TGP,
>>> +               "Set the additional TGP on top of the base TGP");
>>> +ATTR_GROUP_INT_VALUE_ONLY_RO(nv_base_tgp, ATTR_NV_BASE_TGP, ASUS_WMI_DEVID_DGPU_BASE_TGP,
>>> +                 "Read the base TGP value");
>>> +
>>> +
>>>   ATTR_GROUP_ENUM_INT_RO(charge_mode, "charge_mode", ASUS_WMI_DEVID_CHARGE_MODE, "0;1;2",
>>>                  "Show the current mode of charging");
>>>   @@ -746,6 +800,16 @@ static const struct asus_attr_group armoury_attr_groups[] = {
>>>       { &cores_efficiency_attr_group, ASUS_WMI_DEVID_CORES_MAX },
>>>       { &cores_performance_attr_group, ASUS_WMI_DEVID_CORES_MAX },
>>>   +    { &ppt_pl1_spl_attr_group, ASUS_WMI_DEVID_PPT_PL1_SPL },
>>> +    { &ppt_pl2_sppt_attr_group, ASUS_WMI_DEVID_PPT_PL2_SPPT },
>>> +    { &ppt_pl3_fppt_attr_group, ASUS_WMI_DEVID_PPT_FPPT },
>>> +    { &ppt_apu_sppt_attr_group, ASUS_WMI_DEVID_PPT_APU_SPPT },
>>> +    { &ppt_platform_sppt_attr_group, ASUS_WMI_DEVID_PPT_PLAT_SPPT },
>>> +    { &nv_dynamic_boost_attr_group, ASUS_WMI_DEVID_NV_DYN_BOOST },
>>> +    { &nv_temp_target_attr_group, ASUS_WMI_DEVID_NV_THERM_TARGET },
>>> +    { &nv_base_tgp_attr_group, ASUS_WMI_DEVID_DGPU_BASE_TGP },
>>> +    { &nv_tgp_attr_group, ASUS_WMI_DEVID_DGPU_SET_TGP },
>>> +
>>>       { &charge_mode_attr_group, ASUS_WMI_DEVID_CHARGE_MODE },
>>>       { &boot_sound_attr_group, ASUS_WMI_DEVID_BOOT_SOUND },
>>>       { &mcu_powersave_attr_group, ASUS_WMI_DEVID_MCU_POWERSAVE },
>>> @@ -754,8 +818,75 @@ static const struct asus_attr_group armoury_attr_groups[] = {
>>>       { &screen_auto_brightness_attr_group, ASUS_WMI_DEVID_SCREEN_AUTO_BRIGHTNESS },
>>>   };
>>>   +/**
>>> + * is_power_tunable_attr - Determines if an attribute is a power-related tunable
>>> + * @name: The name of the attribute to check
>>> + *
>>> + * This function checks if the given attribute name is related to power tuning.
>>> + *
>>> + * Return: true if the attribute is a power-related tunable, false otherwise
>>> + */
>>> +static bool is_power_tunable_attr(const char *name)
>>> +{
>>> +    static const char * const power_tunable_attrs[] = {
>>> +        ATTR_PPT_PL1_SPL,    ATTR_PPT_PL2_SPPT,
>>> +        ATTR_PPT_PL3_FPPT,    ATTR_PPT_APU_SPPT,
>>> +        ATTR_PPT_PLATFORM_SPPT, ATTR_NV_DYNAMIC_BOOST,
>>> +        ATTR_NV_TEMP_TARGET,    ATTR_NV_BASE_TGP,
>>> +        ATTR_NV_TGP
>>> +    };
>>> +
>>> +    for (unsigned int i = 0; i < ARRAY_SIZE(power_tunable_attrs); i++) {
>>> +        if (!strcmp(name, power_tunable_attrs[i]))
>>> +            return true;
>>> +    }
>>> +
>>> +    return false;
>>> +}
>>> +
>>> +/**
>>> + * has_valid_limit - Checks if a power-related attribute has a valid limit value
>>> + * @name: The name of the attribute to check
>>> + * @limits: Pointer to the power_limits structure containing limit values
>>> + *
>>> + * This function checks if a power-related attribute has a valid limit value.
>>> + * It returns false if limits is NULL or if the corresponding limit value is zero.
>>> + *
>>> + * Return: true if the attribute has a valid limit value, false otherwise
>>> + */
>>> +static bool has_valid_limit(const char *name, const struct power_limits *limits)
>>> +{
>>> +    u32 limit_value = 0;
>>> +
>>> +    if (!limits)
>>> +        return false;
>>> +
>>> +    if (!strcmp(name, ATTR_PPT_PL1_SPL))
>>> +        limit_value = limits->ppt_pl1_spl_max;
>>> +    else if (!strcmp(name, ATTR_PPT_PL2_SPPT))
>>> +        limit_value = limits->ppt_pl2_sppt_max;
>>> +    else if (!strcmp(name, ATTR_PPT_PL3_FPPT))
>>> +        limit_value = limits->ppt_pl3_fppt_max;
>>> +    else if (!strcmp(name, ATTR_PPT_APU_SPPT))
>>> +        limit_value = limits->ppt_apu_sppt_max;
>>> +    else if (!strcmp(name, ATTR_PPT_PLATFORM_SPPT))
>>> +        limit_value = limits->ppt_platform_sppt_max;
>>> +    else if (!strcmp(name, ATTR_NV_DYNAMIC_BOOST))
>>> +        limit_value = limits->nv_dynamic_boost_max;
>>> +    else if (!strcmp(name, ATTR_NV_TEMP_TARGET))
>>> +        limit_value = limits->nv_temp_target_max;
>>> +    else if (!strcmp(name, ATTR_NV_BASE_TGP) ||
>>> +         !strcmp(name, ATTR_NV_TGP))
>>> +        limit_value = limits->nv_tgp_max;
>>> +
>>> +    return limit_value > 0;
>>> +}
>>> +
>>>   static int asus_fw_attr_add(void)
>>>   {
>>> +    const struct power_limits *limits;
>>> +    bool should_create;
>>> +    const char *name;
>>>       int err, i;
>>>         asus_armoury.fw_attr_dev = device_create(&firmware_attributes_class, NULL, MKDEV(0, 0),
>>> @@ -812,12 +943,30 @@ static int asus_fw_attr_add(void)
>>>           if (!asus_wmi_is_present(armoury_attr_groups[i].wmi_devid))
>>>               continue;
>>>   -        err = sysfs_create_group(&asus_armoury.fw_attr_kset->kobj,
>>> -                     armoury_attr_groups[i].attr_group);
>>> -        if (err) {
>>> -            pr_err("Failed to create sysfs-group for %s\n",
>>> -                   armoury_attr_groups[i].attr_group->name);
>>> -            goto err_remove_groups;
>>> +        /* Always create by default, unless PPT is not present */
>>> +        should_create = true;
>>> +        name = armoury_attr_groups[i].attr_group->name;
>>> +
>>> +        /* Check if this is a power-related tunable requiring limits */
>>> +        if (asus_armoury.rog_tunables[1] && asus_armoury.rog_tunables[1]->power_limits &&
>>> +            is_power_tunable_attr(name)) {
>>> +            limits = asus_armoury.rog_tunables[1]->power_limits;
>>> +            /* Check only AC, if DC is not present then AC won't be either */
>>> +            should_create = has_valid_limit(name, limits);
>>> +            if (!should_create) {
>>> +                pr_debug("Missing max value on %s for tunable: %s\n",
>>> +                     dmi_get_system_info(DMI_BOARD_NAME), name);
>> dmi_get_system_info can return NULL 
> ouch! v14 here I come.
>>> +            }
>>> +        }
>>> +
>>> +        if (should_create) {
>>> +            err = sysfs_create_group(&asus_armoury.fw_attr_kset->kobj,
>>> +                armoury_attr_groups[i].attr_group);
>>> +            if (err) {
>>> +                pr_err("Failed to create sysfs-group for %s\n",
>>> +                       armoury_attr_groups[i].attr_group->name);
>>> +                goto err_remove_groups;
>>> +            }
>>>           }
>>>       }
>>>  
>> Thanks,
>> Alok 
> Thanks,
> Denis B.

Powered by blists - more mailing lists