Re: [PATCH v2 1/1] platform/surface: platform_profile: add fan profile switching

From: Hans de Goede
Date: Mon Mar 25 2024 - 13:07:36 EST

Next message: Sebastian Reichel: "[PATCH v1 0/4] RK3588 GPU support"
Previous message: Jani Nikula: "[RFC 4/4] kernel/panic: add verbose logging of kernel taints in backtraces"
Next in thread: Ivor Wanders: "Re: [PATCH v2 1/1] platform/surface: platform_profile: add fan profile switching"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,

On 3/14/24 11:37 PM, Ivor Wanders wrote:
> Change naming from tmp to platform profile to clarify the module may
> interact with both the TMP and FAN subystems. Add functionality that
> switches the fan profile when the platform profile is changed when
> a fan is present.
>
> Signed-off-by: Ivor Wanders <ivor@xxxxxxxxxxxx>
> Link: https://github.com/linux-surface/kernel/pull/145
> Reviewed-by: Maximilian Luz <luzmaximilian@xxxxxxxxx>

Thank you for your patch, I've applied this patch to my review-hans
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans

> ---
> Changes in v2:
> - Added link entry to commit message.
> - Use u8 instead of char for the argument of __sam_fan_profile_set.
> - Made profile and profile_le variable const.
> ---
> ---
> .../surface/surface_aggregator_registry.c | 36 +++++---
> .../surface/surface_platform_profile.c | 88 ++++++++++++++++---
> 2 files changed, 100 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/platform/surface/surface_aggregator_registry.c b/drivers/platform/surface/surface_aggregator_registry.c
> index 035d6b4105cd..79e52eddabd0 100644
> --- a/drivers/platform/surface/surface_aggregator_registry.c
> +++ b/drivers/platform/surface/surface_aggregator_registry.c
> @@ -68,12 +68,26 @@ static const struct software_node ssam_node_bat_sb3base = {
> .parent = &ssam_node_hub_base,
> };
>
> -/* Platform profile / performance-mode device. */
> -static const struct software_node ssam_node_tmp_pprof = {
> +/* Platform profile / performance-mode device without a fan. */
> +static const struct software_node ssam_node_tmp_perf_profile = {
> .name = "ssam:01:03:01:00:01",
> .parent = &ssam_node_root,
> };
>
> +/* Platform profile / performance-mode device with a fan, such that
> + * the fan controller profile can also be switched.
> + */
> +static const struct property_entry ssam_node_tmp_perf_profile_has_fan[] = {
> + PROPERTY_ENTRY_BOOL("has_fan"),
> + { }
> +};
> +
> +static const struct software_node ssam_node_tmp_perf_profile_with_fan = {
> + .name = "ssam:01:03:01:00:01",
> + .parent = &ssam_node_root,
> + .properties = ssam_node_tmp_perf_profile_has_fan,
> +};
> +
> /* Fan speed function. */
> static const struct software_node ssam_node_fan_speed = {
> .name = "ssam:01:05:01:01:01",
> @@ -208,7 +222,7 @@ static const struct software_node ssam_node_pos_tablet_switch = {
> */
> static const struct software_node *ssam_node_group_gen5[] = {
> &ssam_node_root,
> - &ssam_node_tmp_pprof,
> + &ssam_node_tmp_perf_profile,
> NULL,
> };
>
> @@ -219,7 +233,7 @@ static const struct software_node *ssam_node_group_sb3[] = {
> &ssam_node_bat_ac,
> &ssam_node_bat_main,
> &ssam_node_bat_sb3base,
> - &ssam_node_tmp_pprof,
> + &ssam_node_tmp_perf_profile,
> &ssam_node_bas_dtx,
> &ssam_node_hid_base_keyboard,
> &ssam_node_hid_base_touchpad,
> @@ -233,7 +247,7 @@ static const struct software_node *ssam_node_group_sl3[] = {
> &ssam_node_root,
> &ssam_node_bat_ac,
> &ssam_node_bat_main,
> - &ssam_node_tmp_pprof,
> + &ssam_node_tmp_perf_profile,
> &ssam_node_hid_main_keyboard,
> &ssam_node_hid_main_touchpad,
> &ssam_node_hid_main_iid5,
> @@ -245,7 +259,7 @@ static const struct software_node *ssam_node_group_sl5[] = {
> &ssam_node_root,
> &ssam_node_bat_ac,
> &ssam_node_bat_main,
> - &ssam_node_tmp_pprof,
> + &ssam_node_tmp_perf_profile,
> &ssam_node_hid_main_keyboard,
> &ssam_node_hid_main_touchpad,
> &ssam_node_hid_main_iid5,
> @@ -258,7 +272,7 @@ static const struct software_node *ssam_node_group_sls[] = {
> &ssam_node_root,
> &ssam_node_bat_ac,
> &ssam_node_bat_main,
> - &ssam_node_tmp_pprof,
> + &ssam_node_tmp_perf_profile,
> &ssam_node_pos_tablet_switch,
> &ssam_node_hid_sam_keyboard,
> &ssam_node_hid_sam_penstash,
> @@ -274,7 +288,7 @@ static const struct software_node *ssam_node_group_slg1[] = {
> &ssam_node_root,
> &ssam_node_bat_ac,
> &ssam_node_bat_main,
> - &ssam_node_tmp_pprof,
> + &ssam_node_tmp_perf_profile,
> NULL,
> };
>
> @@ -283,7 +297,7 @@ static const struct software_node *ssam_node_group_sp7[] = {
> &ssam_node_root,
> &ssam_node_bat_ac,
> &ssam_node_bat_main,
> - &ssam_node_tmp_pprof,
> + &ssam_node_tmp_perf_profile,
> NULL,
> };
>
> @@ -293,7 +307,7 @@ static const struct software_node *ssam_node_group_sp8[] = {
> &ssam_node_hub_kip,
> &ssam_node_bat_ac,
> &ssam_node_bat_main,
> - &ssam_node_tmp_pprof,
> + &ssam_node_tmp_perf_profile,
> &ssam_node_kip_tablet_switch,
> &ssam_node_hid_kip_keyboard,
> &ssam_node_hid_kip_penstash,
> @@ -310,7 +324,7 @@ static const struct software_node *ssam_node_group_sp9[] = {
> &ssam_node_hub_kip,
> &ssam_node_bat_ac,
> &ssam_node_bat_main,
> - &ssam_node_tmp_pprof,
> + &ssam_node_tmp_perf_profile_with_fan,
> &ssam_node_fan_speed,
> &ssam_node_pos_tablet_switch,
> &ssam_node_hid_kip_keyboard,
> diff --git a/drivers/platform/surface/surface_platform_profile.c b/drivers/platform/surface/surface_platform_profile.c
> index a5a3941b3f43..3de864bc6610 100644
> --- a/drivers/platform/surface/surface_platform_profile.c
> +++ b/drivers/platform/surface/surface_platform_profile.c
> @@ -1,7 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0+
> /*
> * Surface Platform Profile / Performance Mode driver for Surface System
> - * Aggregator Module (thermal subsystem).
> + * Aggregator Module (thermal and fan subsystem).
> *
> * Copyright (C) 2021-2022 Maximilian Luz <luzmaximilian@xxxxxxxxx>
> */
> @@ -14,6 +14,7 @@
>
> #include <linux/surface_aggregator/device.h>
>
> +// Enum for the platform performance profile sent to the TMP module.
> enum ssam_tmp_profile {
> SSAM_TMP_PROFILE_NORMAL = 1,
> SSAM_TMP_PROFILE_BATTERY_SAVER = 2,
> @@ -21,15 +22,26 @@ enum ssam_tmp_profile {
> SSAM_TMP_PROFILE_BEST_PERFORMANCE = 4,
> };
>
> +// Enum for the fan profile sent to the FAN module. This fan profile is
> +// only sent to the EC if the 'has_fan' property is set. The integers are
> +// not a typo, they differ from the performance profile indices.
> +enum ssam_fan_profile {
> + SSAM_FAN_PROFILE_NORMAL = 2,
> + SSAM_FAN_PROFILE_BATTERY_SAVER = 1,
> + SSAM_FAN_PROFILE_BETTER_PERFORMANCE = 3,
> + SSAM_FAN_PROFILE_BEST_PERFORMANCE = 4,
> +};
> +
> struct ssam_tmp_profile_info {
> __le32 profile;
> __le16 unknown1;
> __le16 unknown2;
> } __packed;
>
> -struct ssam_tmp_profile_device {
> +struct ssam_platform_profile_device {
> struct ssam_device *sdev;
> struct platform_profile_handler handler;
> + bool has_fan;
> };
>
> SSAM_DEFINE_SYNC_REQUEST_CL_R(__ssam_tmp_profile_get, struct ssam_tmp_profile_info, {
> @@ -42,6 +54,13 @@ SSAM_DEFINE_SYNC_REQUEST_CL_W(__ssam_tmp_profile_set, __le32, {
> .command_id = 0x03,
> });
>
> +SSAM_DEFINE_SYNC_REQUEST_W(__ssam_fan_profile_set, u8, {
> + .target_category = SSAM_SSH_TC_FAN,
> + .target_id = SSAM_SSH_TID_SAM,
> + .command_id = 0x0e,
> + .instance_id = 0x01,
> +});
> +
> static int ssam_tmp_profile_get(struct ssam_device *sdev, enum ssam_tmp_profile *p)
> {
> struct ssam_tmp_profile_info info;
> @@ -57,12 +76,19 @@ static int ssam_tmp_profile_get(struct ssam_device *sdev, enum ssam_tmp_profile
>
> static int ssam_tmp_profile_set(struct ssam_device *sdev, enum ssam_tmp_profile p)
> {
> - __le32 profile_le = cpu_to_le32(p);
> + const __le32 profile_le = cpu_to_le32(p);
>
> return ssam_retry(__ssam_tmp_profile_set, sdev, &profile_le);
> }
>
> -static int convert_ssam_to_profile(struct ssam_device *sdev, enum ssam_tmp_profile p)
> +static int ssam_fan_profile_set(struct ssam_device *sdev, enum ssam_fan_profile p)
> +{
> + const u8 profile = p;
> +
> + return ssam_retry(__ssam_fan_profile_set, sdev->ctrl, &profile);
> +}
> +
> +static int convert_ssam_tmp_to_profile(struct ssam_device *sdev, enum ssam_tmp_profile p)
> {
> switch (p) {
> case SSAM_TMP_PROFILE_NORMAL:
> @@ -83,7 +109,8 @@ static int convert_ssam_to_profile(struct ssam_device *sdev, enum ssam_tmp_profi
> }
> }
>
> -static int convert_profile_to_ssam(struct ssam_device *sdev, enum platform_profile_option p)
> +
> +static int convert_profile_to_ssam_tmp(struct ssam_device *sdev, enum platform_profile_option p)
> {
> switch (p) {
> case PLATFORM_PROFILE_LOW_POWER:
> @@ -105,20 +132,42 @@ static int convert_profile_to_ssam(struct ssam_device *sdev, enum platform_profi
> }
> }
>
> +static int convert_profile_to_ssam_fan(struct ssam_device *sdev, enum platform_profile_option p)
> +{
> + switch (p) {
> + case PLATFORM_PROFILE_LOW_POWER:
> + return SSAM_FAN_PROFILE_BATTERY_SAVER;
> +
> + case PLATFORM_PROFILE_BALANCED:
> + return SSAM_FAN_PROFILE_NORMAL;
> +
> + case PLATFORM_PROFILE_BALANCED_PERFORMANCE:
> + return SSAM_FAN_PROFILE_BETTER_PERFORMANCE;
> +
> + case PLATFORM_PROFILE_PERFORMANCE:
> + return SSAM_FAN_PROFILE_BEST_PERFORMANCE;
> +
> + default:
> + /* This should have already been caught by platform_profile_store(). */
> + WARN(true, "unsupported platform profile");
> + return -EOPNOTSUPP;
> + }
> +}
> +
> static int ssam_platform_profile_get(struct platform_profile_handler *pprof,
> enum platform_profile_option *profile)
> {
> - struct ssam_tmp_profile_device *tpd;
> + struct ssam_platform_profile_device *tpd;
> enum ssam_tmp_profile tp;
> int status;
>
> - tpd = container_of(pprof, struct ssam_tmp_profile_device, handler);
> + tpd = container_of(pprof, struct ssam_platform_profile_device, handler);
>
> status = ssam_tmp_profile_get(tpd->sdev, &tp);
> if (status)
> return status;
>
> - status = convert_ssam_to_profile(tpd->sdev, tp);
> + status = convert_ssam_tmp_to_profile(tpd->sdev, tp);
> if (status < 0)
> return status;
>
> @@ -129,21 +178,32 @@ static int ssam_platform_profile_get(struct platform_profile_handler *pprof,
> static int ssam_platform_profile_set(struct platform_profile_handler *pprof,
> enum platform_profile_option profile)
> {
> - struct ssam_tmp_profile_device *tpd;
> + struct ssam_platform_profile_device *tpd;
> int tp;
>
> - tpd = container_of(pprof, struct ssam_tmp_profile_device, handler);
> + tpd = container_of(pprof, struct ssam_platform_profile_device, handler);
> +
> + tp = convert_profile_to_ssam_tmp(tpd->sdev, profile);
> + if (tp < 0)
> + return tp;
>
> - tp = convert_profile_to_ssam(tpd->sdev, profile);
> + tp = ssam_tmp_profile_set(tpd->sdev, tp);
> if (tp < 0)
> return tp;
>
> - return ssam_tmp_profile_set(tpd->sdev, tp);
> + if (tpd->has_fan) {
> + tp = convert_profile_to_ssam_fan(tpd->sdev, profile);
> + if (tp < 0)
> + return tp;
> + tp = ssam_fan_profile_set(tpd->sdev, tp);
> + }
> +
> + return tp;
> }
>
> static int surface_platform_profile_probe(struct ssam_device *sdev)
> {
> - struct ssam_tmp_profile_device *tpd;
> + struct ssam_platform_profile_device *tpd;
>
> tpd = devm_kzalloc(&sdev->dev, sizeof(*tpd), GFP_KERNEL);
> if (!tpd)
> @@ -154,6 +214,8 @@ static int surface_platform_profile_probe(struct ssam_device *sdev)
> tpd->handler.profile_get = ssam_platform_profile_get;
> tpd->handler.profile_set = ssam_platform_profile_set;
>
> + tpd->has_fan = device_property_read_bool(&sdev->dev, "has_fan");
> +
> set_bit(PLATFORM_PROFILE_LOW_POWER, tpd->handler.choices);
> set_bit(PLATFORM_PROFILE_BALANCED, tpd->handler.choices);
> set_bit(PLATFORM_PROFILE_BALANCED_PERFORMANCE, tpd->handler.choices);

Next message: Sebastian Reichel: "[PATCH v1 0/4] RK3588 GPU support"
Previous message: Jani Nikula: "[RFC 4/4] kernel/panic: add verbose logging of kernel taints in backtraces"
Next in thread: Ivor Wanders: "Re: [PATCH v2 1/1] platform/surface: platform_profile: add fan profile switching"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]