Re: [RFC v3 3/4] platform/x86/amd: dptc: Add platform profile support

[Antheas Kapenekakis]

On Sat, 7 Mar 2026 at 22:57, Rong Zhang <i@xxxxxxxx> wrote:
>
> Hi Antheas,
>
> On Sat, 2026-03-07 at 12:55 +0100, Antheas Kapenekakis wrote:
> > Register a platform_profile handler so the driver exposes standard
> > power profiles (low-power, balanced, performance, max-power) alongside
> > the manual tunable interface. max-power is only supplied when the
> > performance profile's preset values are below the tunable limits.
> >
> > When a non-custom profile is active, parameter writes are blocked
> > (-EPERM) and current_value reflects the profile's preset values.
> > Selecting the "custom" profile returns control to the user for manual
> > staging and committing. On resume, the active profile is automatically
> > re-applied.
> >
> > Assisted-by: Claude:claude-opus-4-6
> > Signed-off-by: Antheas Kapenekakis <lkml@xxxxxxxxxxx>
> > ---
> >  drivers/platform/x86/amd/Kconfig |   1 +
> >  drivers/platform/x86/amd/dptc.c  | 116 +++++++++++++++++++++++++++++--
> >  2 files changed, 113 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/platform/x86/amd/Kconfig b/drivers/platform/x86/amd/Kconfig
> > index d610092467fc..41ffbd722524 100644
> > --- a/drivers/platform/x86/amd/Kconfig
> > +++ b/drivers/platform/x86/amd/Kconfig
> > @@ -48,6 +48,7 @@ config AMD_ISP_PLATFORM
> >  config AMD_DPTC
> >       tristate "AMD Dynamic Power and Thermal Configuration Interface (DPTCi)"
> >       depends on X86_64 && ACPI && DMI
> > +     select ACPI_PLATFORM_PROFILE
> >       select FIRMWARE_ATTRIBUTES_CLASS
> >       help
> >         Driver for AMD AGESA ALIB Function 0x0C, the Dynamic Power and
> > diff --git a/drivers/platform/x86/amd/dptc.c b/drivers/platform/x86/amd/dptc.c
> > index acfe9cc01bab..1f07e2e6f278 100644
> > --- a/drivers/platform/x86/amd/dptc.c
> > +++ b/drivers/platform/x86/amd/dptc.c
> > @@ -23,6 +23,7 @@
> >  #include <linux/module.h>
> >  #include <linux/mutex.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/platform_profile.h>
> >  #include <linux/processor.h>
> >  #include <linux/sysfs.h>
> >  #include <linux/unaligned.h>
> > @@ -56,8 +57,13 @@ struct dptc_param_limits {
> >       u32 expanded_max;
> >  };
> >
> > +struct dptc_profile {
> > +     u32 vals[DPTC_NUM_PARAMS];      /* 0 = don't set / unstage this param */
> > +};
> > +
> >  struct dptc_device_limits {
> >       struct dptc_param_limits params[DPTC_NUM_PARAMS];
> > +     struct dptc_profile profiles[PLATFORM_PROFILE_LAST];
> >  };
> >
> >  struct dptc_param_desc {
> > @@ -85,6 +91,10 @@ static const struct dptc_device_limits limits_maxhh = { .params = {
> >       [DPTC_PPT_PL2_SPPT] = {  0,  4, 27,  82, 100 },
> >       [DPTC_PPT_PL3_FPPT] = {  0,  4, 40,  85, 100 },
> >       [DPTC_CPU_TEMP]     = { 60, 70, 95,  95, 100 },
> > +}, .profiles = {
> > +     [PLATFORM_PROFILE_LOW_POWER]   = { .vals = { 15, 15, 25, 0 } },
> > +     [PLATFORM_PROFILE_BALANCED]    = { .vals = { 25, 27, 40, 0 } },
> > +     [PLATFORM_PROFILE_PERFORMANCE] = { .vals = { 60, 63, 85, 0 } },
> >  }};
> >
> >  /* Substring matches against boot_cpu_data.x86_model_id; order matters. */
> > @@ -134,6 +144,9 @@ struct dptc_priv {
> >
> >       bool expanded;
> >
> > +     enum platform_profile_option profile;
> > +     struct device *ppdev;
> > +
> >       enum dptc_save_mode { SAVE_SINGLE, SAVE_BULK } save_mode;
> >
> >       u32 staged[DPTC_NUM_PARAMS];
> > @@ -269,6 +282,14 @@ static ssize_t dptc_current_value_show(struct kobject *kobj,
> >
> >       guard(mutex)(&dptc->lock);
> >
> > +     if (dptc->profile != PLATFORM_PROFILE_CUSTOM) {
> > +             u32 val = dptc->dev_limits->profiles[dptc->profile].vals[ps->idx];
> > +
> > +             if (!val)
> > +                     return sysfs_emit(buf, "\n");
> > +             return sysfs_emit(buf, "%u\n", val);
> > +     }
> > +
> >       if (!dptc->has_staged[ps->idx])
> >               return sysfs_emit(buf, "\n");
> >       return sysfs_emit(buf, "%u\n", dptc->staged[ps->idx]);
> > @@ -287,6 +308,8 @@ static ssize_t dptc_current_value_store(struct kobject *kobj,
> >       if (count == 1 && buf[0] == '\n') {
> >               guard(mutex)(&dptc->lock);
> >
> > +             if (dptc->profile != PLATFORM_PROFILE_CUSTOM)
> > +                     return -EPERM;
>
> It has nothing to do with permissions. I'd prefer -EBUSY.

I will defer to other drivers and update if necessary based on their choices.

> To maintain readability, usually a blank line should be added after
> returning early.
>
> >               dptc->has_staged[ps->idx] = false;
> >               return count;
> >       }
> > @@ -297,6 +320,8 @@ static ssize_t dptc_current_value_store(struct kobject *kobj,
> >
> >       guard(mutex)(&dptc->lock);
> >
> > +     if (dptc->profile != PLATFORM_PROFILE_CUSTOM)
> > +             return -EPERM;
> >       min = dptc_get_min(dptc, ps->idx);
> >       max = dptc_get_max(dptc, ps->idx);
> >       if (val < min || (max && val > max))
> > @@ -426,6 +451,8 @@ static ssize_t dptc_expanded_current_value_store(struct kobject *kobj,
> >
> >       guard(mutex)(&dptc->lock);
> >
> > +     if (dptc->profile != PLATFORM_PROFILE_CUSTOM)
> > +             return -EPERM;
>
> IMO kstrtou32() isn't really a blocker of acquiring the mutex earlier
> to prevent duplicating the same check. After all, firmware-attributes
> is not a performance critical interface, and dptc_save_settings_store()
> also calls sysfs_eq() with the mutex held.
>
> >       dptc->expanded = val;
> >       /* Clear staged values: limits changed, old values may be out of range */
> >       memset(dptc->has_staged, 0, sizeof(dptc->has_staged));
> > @@ -594,6 +621,78 @@ static void dptc_kset_unregister(void *data)
> >       kset_unregister(data);
> >  }
> >
> > +/* Platform profile */
> > +
> > +static void dptc_apply_profile(struct dptc_priv *dptc,
> > +                            enum platform_profile_option profile)
> > +{
> > +     const struct dptc_profile *pp;
> > +     int i;
> > +
> > +     memset(dptc->has_staged, 0, sizeof(dptc->has_staged));
> > +
> > +     if (profile == PLATFORM_PROFILE_CUSTOM)
> > +             return;
> > +
> > +     pp = &dptc->dev_limits->profiles[profile];
> > +     for (i = 0; i < DPTC_NUM_PARAMS; i++) {
> > +             if (!pp->vals[i])
> > +                     continue;
> > +             dptc->staged[i] = pp->vals[i];
> > +             dptc->has_staged[i] = true;
> > +     }
> > +}
> > +
> > +static int dptc_pp_probe(void *drvdata, unsigned long *choices)
> > +{
> > +     struct dptc_priv *dptc = drvdata;
> > +     int i, j;
> > +
> > +     set_bit(PLATFORM_PROFILE_CUSTOM, choices);
> > +     for (i = 0; i < PLATFORM_PROFILE_LAST; i++) {
> > +             for (j = 0; j < DPTC_NUM_PARAMS; j++) {
> > +                     if (dptc->dev_limits->profiles[i].vals[j]) {
> > +                             set_bit(i, choices);
> > +                             break;
> > +                     }
> > +             }
> > +     }
> > +     return 0;
> > +}
> > +
> > +static int dptc_pp_get(struct device *dev,
> > +                    enum platform_profile_option *profile)
> > +{
> > +     struct dptc_priv *dptc = dev_get_drvdata(dev);
> > +
> > +     guard(mutex)(&dptc->lock);
> > +
> > +     *profile = dptc->profile;
> > +     return 0;
> > +}
> > +
> > +static int dptc_pp_set(struct device *dev,
> > +                    enum platform_profile_option profile)
> > +{
> > +     struct dptc_priv *dptc = dev_get_drvdata(dev);
> > +     int ret = 0;
> > +
> > +     guard(mutex)(&dptc->lock);
> > +
> > +     dptc->profile = profile;
> > +     dptc_apply_profile(dptc, profile);
> > +     if (profile != PLATFORM_PROFILE_CUSTOM)
> > +             ret = dptc_alib_save(dptc);
>
> I'd prefer simply calling dptc_alib_save() in dptc_apply_profile()
> since the latter already checks the profile.

This way you will propagate an error because custom unstages all values.

Antheas

> > +
> > +     return ret;
> > +}
> > +
> > +static const struct platform_profile_ops dptc_pp_ops = {
> > +     .probe       = dptc_pp_probe,
> > +     .profile_get = dptc_pp_get,
> > +     .profile_set = dptc_pp_set,
> > +};
> > +
> >  static int dptc_resume(struct device *dev)
> >  {
> >       struct dptc_priv *dptc = dev_get_drvdata(dev);
> > @@ -601,10 +700,14 @@ static int dptc_resume(struct device *dev)
> >
> >       guard(mutex)(&dptc->lock);
> >
> > -     if (dptc->save_mode == SAVE_SINGLE)
> > +     if (dptc->profile != PLATFORM_PROFILE_CUSTOM) {
> > +             dptc_apply_profile(dptc, dptc->profile);
> >               ret = dptc_alib_save(dptc);
> > -     else
> > +     } else if (dptc->save_mode == SAVE_SINGLE) {
> > +             ret = dptc_alib_save(dptc);
> > +     } else {
> >               ret = 0;
> > +     }
> >
> >       if (ret)
> >               dev_warn(dev, "failed to restore settings on resume: %d\n", ret);
> > @@ -636,8 +739,7 @@ static int dptc_probe(struct platform_device *pdev)
> >                boot_cpu_data.x86_model_id);
> >
> >       dptc->fw_attr_dev = device_create(&firmware_attributes_class,
> > -                                       NULL, MKDEV(0, 0), NULL,
> > -                                       DRIVER_NAME);
> > +                                       NULL, MKDEV(0, 0), NULL, DRIVER_NAME);
>
> Squash the change into the previous patch.
>
> Thanks,
> Rong
>
> >       if (IS_ERR(dptc->fw_attr_dev))
> >               return PTR_ERR(dptc->fw_attr_dev);
> >
> > @@ -680,6 +782,12 @@ static int dptc_probe(struct platform_device *pdev)
> >       if (ret)
> >               return ret;
> >
> > +     dptc->profile = PLATFORM_PROFILE_CUSTOM;
> > +     dptc->ppdev = devm_platform_profile_register(dev, "amd-dptc", dptc,
> > +                                                  &dptc_pp_ops);
> > +     if (IS_ERR(dptc->ppdev))
> > +             return PTR_ERR(dptc->ppdev);
> > +
> >       return 0;
> >  }
> >
>