drivers: wifi: siwx91x: Add power-save support #86537

ArunmaniAlagarsamy2710 · 2025-03-03T06:00:08Z

Added power-save mode support to optimize station wake-up timing
and reduce power consumption. The device currently supports
only legacy power-save mode.

Implemented power-save features:

ps ON/OFF : By default, the device operates in
fast PSP mode.
ps_listen_interval : Moves to DTIM wakeup if set to zero.
ps_exit_strategy : Updates mode if enabled; otherwise, follows
the configured exit strategy when power-save
is enabled.
ps_wakeup_mode : Configures the wake-up behavior.
ps_timeout : Defines the timeout duration for power-save mode.

drivers/wifi/siwx91x/siwx91x_wifi.c

drivers/wifi/siwx91x/Kconfig.siwx91x

drivers/wifi/siwx91x/siwx91x_wifi.c

krish2718 · 2025-03-26T13:15:34Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+					sl_ps_profile.profile = req_ps_mode;
+				}
+			} else {
+				return -EAGAIN;


did you mean -EALREADY?

krish2718 · 2025-03-26T13:18:37Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+			}
+		} else if (params->enabled == WIFI_PS_DISABLED) {
+			if (sl_ps_profile.profile == HIGH_PERFORMANCE) {
+				return -EAGAIN;


did you mean -EALREADY?

jhedberg · 2025-03-28T08:08:32Z

drivers/wifi/siwx91x/Kconfig.siwx91x

+
+config WIFI_SIWX91X_ENHANCED_MAX_PSP
+	bool "Enhanced Max PSP Support"
+	default n


n is the "default default", so you can just remove this line

jhedberg · 2025-03-28T08:10:10Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+	case WIFI_PS_PARAM_STATE:
+		if (params->enabled == WIFI_PS_ENABLED) {
+			if (sl_ps_profile.profile == HIGH_PERFORMANCE) {
+				if (siwx91x_ps_cfg.req_ps_mode == HIGH_PERFORMANCE) {
+					sl_ps_profile.profile = ASSOCIATED_POWER_SAVE_LOW_LATENCY;
+				} else {
+					sl_ps_profile.profile = siwx91x_ps_cfg.req_ps_mode;
+				}
+			} else {
+				return -EALREADY;
+			}
+
+			if (sl_ps_profile.dtim_aligned_type == 0) {
+				beacon_interval = siwx91x_get_beacon_interval(dev);
+				if (beacon_interval < 0) {
+					params->fail_reason = WIFI_PS_PARAM_FAIL_DEVICE_NOT_CONNECTED;


The compliance check is failing because of the long (> 100) lines. To me it looks like this function could use some refactoring into smaller helper functions, so that you don't need to do so deep indentations.

jerome-pouiller · 2025-03-27T13:06:27Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+		return -EINVAL;
+	}
+
+	get_wifi_current_performance_profile(&sl_ps_profile);


This is a private function. Use sl_wifi_get_performance_profile() instead.

jerome-pouiller · 2025-03-27T13:21:38Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+	if (status != SL_STATUS_OK) {
+		params->fail_reason = WIFI_PS_PARAM_FAIL_CMD_EXEC_FAIL;
+		LOG_ERR("Failed to set power save profile: 0x%x", status);
+		return -EINVAL;


EINVAL is used when the caller didn't provide the right arguments. Here, I rather believe the error is unexpected. I would rather return EIO.

jerome-pouiller · 2025-03-28T12:43:55Z

drivers/wifi/siwx91x/Kconfig.siwx91x

+	help
+	  Enable this option to allow the device to configure
+	  Enhanced Maximum Power Save (PSP) support during
+	  device initialization.


Enhanced Maximum Power Save is not standard feature. It makes sense to explain what are the drawbacks and the advantage of this option.

This option only makes sense if WIFI_SILABS_SIWX91X is selected.

jerome-pouiller · 2025-03-28T12:45:15Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+	/* HIGH_PERFORMANCE by default */
+	sl_si91x_performance_profile_t req_ps_mode;
+	uint16_t listen_interval;
+} siwx91x_ps_cfg;


Avoid global variable. These variables should be a part of struct siwx91x_dev.

BTW, avoid to declare struct/variables in the middle of the code.

jerome-pouiller · 2025-03-28T12:47:12Z

drivers/wifi/siwx91x/siwx91x_wifi.c

@@ -709,6 +712,270 @@ static int siwx91x_stats(const struct device *dev, struct net_stats_wifi *stats)
 }
 #endif

+struct siwx91x_ps_profile {
+	/* HIGH_PERFORMANCE by default */


We can fix the code rather than require a comment:

[...] siwx91x_ps_cfg = { .req_ps_mode = HIGH_PERFORMANCE, }

jerome-pouiller · 2025-03-28T12:52:29Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+			}
+		} else {
+			return -EALREADY;
+		}


I have to admit I don't understand what the above lines are doing. Maybe a comment could help.

jerome-pouiller · 2025-03-28T12:55:03Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+		break;
+	case WIFI_PS_DISABLED:
+		if (sl_ps_profile->profile == HIGH_PERFORMANCE) {
+			return -EALREADY;


I don't think it make sense to report an error if the profile is already selected. EALREADY exists for cases the function won't do what the user expect if it is called twice (eg. wifi_connect()).

jerome-pouiller · 2025-03-28T12:57:08Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+			return -EINVAL;
+		}
+
+		sl_ps_profile.monitor_interval = (uint16_t)params->timeout_ms;


Is the cast required?

jerome-pouiller · 2025-03-28T13:00:21Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+		if (beacon_interval > 0) {
+			/* converts MSEC to beacon unit */
+			config->ps_params.listen_interval =
+				(sl_ps_profile.listen_interval / beacon_interval);


Parentheses are useless.

jerome-pouiller · 2025-03-28T13:00:23Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+
+	if (sl_ps_profile.dtim_aligned_type == 1) {
+		config->ps_params.wakeup_mode = WIFI_PS_WAKEUP_MODE_DTIM;
+	} else if (sl_ps_profile.dtim_aligned_type == 0) {


If you consider dtim_aligned_type is a boolean, test it as a boolean:

if (sl_ps_profile.dtim_aligned_type) { ... } else { ... }

jerome-pouiller · 2025-03-28T16:56:50Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+	if (sidev->state != WIFI_STATE_COMPLETED) {
+		/* fail reason */
+		LOG_ERR("Device not connected");
+		return -EIO;


You probably meant EINVAL, EBUSY or EAGAIN

jerome-pouiller · 2025-03-28T16:57:05Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+	sl_status_t status;
+
+	if (sidev->state != WIFI_STATE_COMPLETED) {
+		/* fail reason */


Useless comment.

jerome-pouiller · 2025-03-28T17:01:12Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+		return -EINVAL;
+	}
+
+	return sys_get_le16(sl_stat.beacon_interval);


Until now, we didn't considered the host CPU could be big endian. I believe it does not make sense to do it for sl_stat.beacon_interval.

jerome-pouiller · 2025-03-28T17:04:11Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+		*listen_interval = SIWX91X_PS_MAX_LISTEN_INTERVAL_MS;
+	}
+
+	return 0;


I suggest to return the result. Hence, you will be consistent with siwx91x_get_beacon_interval().

jerome-pouiller · 2025-03-28T17:04:34Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+
+	__ASSERT(sl_ps_profile, "sl_ps_profile cannot be NULL");
+
+	switch(state) {


Missing space after switch.

ArunmaniAlagarsamy2710 · 2025-04-02T05:56:36Z

Rebased to reduce the CI failures

jerome-pouiller · 2025-04-04T14:56:44Z

soc/silabs/silabs_siwx91x/siwg917/nwp.c

@@ -61,7 +62,9 @@ int siwg91x_get_nwp_config(int wifi_oper_mode, sl_wifi_device_configuration_t *g

 #ifdef CONFIG_WIFI_SILABS_SIWX91X
 		boot_config->ext_tcp_ip_feature_bit_map = SL_SI91X_CONFIG_FEAT_EXTENSION_VALID;
+#ifdef CONFIG_WIFI_SIWX91X_ENHANCED_MAX_PSP


You can use IS_ENABLED().

jerome-pouiller · 2025-04-04T14:57:51Z

drivers/wifi/siwx91x/siwx91x_wifi.c

 	sl_status_t status;

 	sidev->state = WIFI_STATE_INTERFACE_DISABLED;
 	sidev->iface = iface;

+	siwx91x_ps_cfg->req_ps_mode = HIGH_PERFORMANCE;
+	siwx91x_ps_cfg->listen_interval = 0;
+	siwx91x_ps_cfg->listen_interval_wakeup = false;


You can statically assign these values during declaration:

static struct siwx91x_dev sidev = { .siwx91x_ps_cfg.req_ps_mode = HIGH_PERFORMANCE, };

Initialization of listen_interval and listen_interval_wakeup are not required because static variable are already filled with 0s.

BTW, do we need to introduce yet another struct for PS parameters? I feel the only information we need to store is req_ps_mode (to restore it on join).

jerome-pouiller · 2025-04-04T15:04:44Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+
+		status = siwx91x_connected_ap_beacon_interval(&beacon_interval);
+		if (status == SL_STATUS_OK && beacon_interval > 0) {
+			/* converts MSEC to beacon unit */


MSEC is not SI: Convert ms in number of beacons. (BTW, it's obvious, the comment is not required)

jerome-pouiller · 2025-04-04T15:17:02Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+	__ASSERT(listen_interval, "listen_interval cannot be NULL");
+
+	if (sidev->state != WIFI_STATE_COMPLETED) {
+		siwx91x_ps_cfg->listen_interval_wakeup = true;


This side effect is unexpected in function called get_xxx().

jerome-pouiller · 2025-04-04T15:51:26Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+	}
+
+	params.enabled = WIFI_PS_ENABLED;
+	params.type = WIFI_PS_PARAM_STATE;


Prefer assignment during declaration:

struct wifi_ps_params params = { .type = WIFI_PS_PARAM_STATE, .enabled = WIFI_PS_ENABLED, };

ArunmaniAlagarsamy2710 · 2025-04-08T14:44:15Z

Rebased to resolve conflicts

jerome-pouiller

This code is still super difficult to understand.

jerome-pouiller · 2025-04-09T09:29:45Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+	case WIFI_PS_PARAM_LISTEN_INTERVAL:
+		sl_ps_profile->listen_interval = params->listen_interval;
+		sl_ps_profile->dtim_aligned_type = 0;
+		return 0;


Is this return expected? I feel listen_interval will be never applied.

jerome-pouiller · 2025-04-09T09:31:34Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+
+static int siwx91x_set_power_save(const struct device *dev, struct wifi_ps_params *params)
+{
+	static sl_si91x_performance_profile_t exit_strategy_mode = { HIGH_PERFORMANCE };


Avoid global variables. This variable should probably go in siwx91x_dev.

BTW, I feel this variable is redundant with sl_ps_profile->profile.

Actually, we need the exit_strategy_mode variable to store the power save profile in case the user configures the exit strategy before the power save enabled. If we directly use sl_ps_profile->profile, power save would be enabled immediately even if the user hasn't explicitly enabled it.

jerome-pouiller · 2025-04-09T11:23:41Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+static int siwx91x_convert_li_bcn_unit_to_ms(uint16_t bcn_li)
+{
+	uint16_t beacon_interval;
+	uint16_t ms_li;


Why uint16_t? Unless you have good reason use int.

jerome-pouiller · 2025-04-09T11:25:16Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+		return -EAGAIN;
+	}
+
+	ms_li = bcn_li * beacon_interval;


If you want to use uint16_t, you have to add a comment explaining why you guarantee there is no overflow here.

jerome-pouiller · 2025-04-09T11:27:04Z

drivers/wifi/siwx91x/siwx91x_wifi.c

+static int siwx91x_convert_li_bcn_unit_to_ms(uint16_t bcn_li)
+{
+	uint16_t beacon_interval;
+	uint16_t ms_li;


The variable names lack of consistency in the function. I suggest:

listen_interval

beacon_interval

listen_interval_ms

jerome-pouiller · 2025-04-09T11:40:49Z

drivers/wifi/siwx91x/siwx91x_wifi.c

 static int siwx91x_configure_ps_mode(const struct device *dev, enum wifi_ps state,
-				     sl_si91x_performance_profile_t exit_strategy_mode)
+				     sl_si91x_performance_profile_t exit_strategy_mode,
+				     uint16_t bcn_li)
 {


Same comment than for siwx91x_schedule_listen_interval_wakeup().

jerome-pouiller · 2025-04-09T11:41:48Z

drivers/wifi/siwx91x/siwx91x_wifi.c

@@ -795,6 +864,7 @@ static int siwx91x_set_power_save(const struct device *dev, struct wifi_ps_param
 	struct siwx91x_dev *sidev = dev->data;
 	sl_wifi_performance_profile_t *sl_ps_profile = &sidev->sl_ps_profile;
 	sl_wifi_interface_t interface;
+	static uint16_t bcn_li;


Please (really) avoid global variables.

jerome-pouiller · 2025-04-09T11:42:02Z

drivers/wifi/siwx91x/siwx91x_wifi.c

@@ -816,7 +886,8 @@ static int siwx91x_set_power_save(const struct device *dev, struct wifi_ps_param

 	switch (params->type) {
 	case WIFI_PS_PARAM_STATE:
-		status = siwx91x_configure_ps_mode(dev, params->enabled, exit_strategy_mode);
+		status = siwx91x_configure_ps_mode(dev, params->enabled, exit_strategy_mode,
+						   bcn_li);


Value of may be bcn_li is undefined.

jerome-pouiller · 2025-04-09T11:44:58Z

drivers/wifi/siwx91x/siwx91x_wifi.c

-	siwx91x_on_join_ipv6(sidev);
-
-	return 0;
-}


Could you relocate this function in a separate commit so the review will be easier?

jerome-pouiller · 2025-04-09T11:51:55Z

drivers/wifi/siwx91x/siwx91x_wifi.c

-				if (status < 0) {
-					return status;
+				if (status == -EINVAL) {
+					sl_ps_profile->profile = HIGH_PERFORMANCE;


Is it related to this commit?

EINVAL means "Invalid argument". This is a bit weird to fix this error by changing the profile.

What happens now if status < 0 && status != -EINVAL ?

Added power-save mode support to optimize station wake-up timing and reduce power consumption. The device currently supports only legacy power-save mode. Implemented power-save features: - ps ON/OFF : By default, the device operates in fast PSP mode. - ps_exit_strategy : Updates mode if enabled; otherwise, follows the configured exit strategy when power-save is enabled. - ps_wakeup_mode : Configures the wake-up behavior. - ps_timeout : Defines the timeout duration for power-save mode. Signed-off-by: Arunmani Alagarsamy <[email protected]>

Adds support to configure listen interval in power save mode. Zephyr sets listen interval based on beacon units, while SDK expects it in ms. The setting takes effect after connection. If set to 0, device switches to DTIM wakeup mode. Signed-off-by: Arunmani Alagarsamy <[email protected]>

Move the siwx91x_on_join() function to make it more accessible for power save feature integration. No functional changes. Signed-off-by: Arunmani Alagarsamy <[email protected]>

Allow users to enable listen interval wakeup mode before connection. Power save is automatically activated after connection if configured beforehand. Signed-off-by: Arunmani Alagarsamy <[email protected]>

ArunmaniAlagarsamy2710 · 2025-04-11T06:45:58Z

Rebased to resolve conflicts

ArunmaniAlagarsamy2710 force-pushed the dev/power_save branch 2 times, most recently from 1830008 to 7e6b30a Compare March 18, 2025 10:57

ArunmaniAlagarsamy2710 force-pushed the dev/power_save branch from 7e6b30a to a5f1cb2 Compare March 21, 2025 14:26

NiteshKShah reviewed Mar 26, 2025

View reviewed changes

drivers/wifi/siwx91x/siwx91x_wifi.c Show resolved Hide resolved

NiteshKShah reviewed Mar 26, 2025

View reviewed changes

drivers/wifi/siwx91x/siwx91x_wifi.c Outdated Show resolved Hide resolved