Skip to content

Conversation

@ragurram26
Copy link
Contributor

To enhance throughput performance on the SiWx91x series, certain configurations need to be enabled in the SiWx91x.

help
Enable this option to improve the throughput performance of the SiWx91x
series. This feature provides enhanced data transfer capabilities and
improved network performance.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why anyone wouldn't want to enable this option? If there is not reason to not enable it, maybe we don't need a config parameter.

(BTW, this symbol is orphan. You can definitely remove it then).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This description is just a serie a paraphrase of the title. You have to guide the user in his choice: why he should want to not enable it.

Typically, it would probably makes sense to warn will break support for WPA3 and MPF (so so generate some security weakness).

It would be nice to include a couple of number if you have ones.

SL_SI91X_CUSTOM_FEAT_EXTENSION_VALID)
: (SL_SI91X_CUSTOM_FEAT_EXTENSION_VALID |
SL_SI91X_CUSTOM_FEAT_ASYNC_CONNECTION_STATUS |
SL_SI91X_CUSTOM_FEAT_RTC_FROM_HOST),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need the condition? Why not just

.custom_feature_bit_map =
       SL_SI91X_CUSTOM_FEAT_EXTENSION_VALID |
       SL_SI91X_CUSTOM_FEAT_SOC_CLK_CONFIG_160MHZ;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My question is still relevant.

@ragurram26 ragurram26 force-pushed the Throughput_configurations branch 3 times, most recently from 92977e7 to 576dc52 Compare October 23, 2025 09:04
help
Enable this option to improve the throughput performance of the SiWx91x
series. This feature provides enhanced data transfer capabilities and
improved network performance.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This description is just a serie a paraphrase of the title. You have to guide the user in his choice: why he should want to not enable it.

Typically, it would probably makes sense to warn will break support for WPA3 and MPF (so so generate some security weakness).

It would be nice to include a couple of number if you have ones.

SL_SI91X_EXT_FEAT_FRONT_END_SWITCH_PINS_ULP_GPIO_4_5_0;
if (!IS_ENABLED(CONFIG_WIFI_SIWX91X_HIGH_THROUGHPUT)) {
boot_config->ext_custom_feature_bit_map |= SL_SI91X_EXT_FEAT_IEEE_80211W;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you confirm the rest of the driver still behave properly in the various MFP scenario (I assume sl_wifi_set_mfp() will return an error and everything is fine).

BTW, I think disabling 11w should be a separated parameters (I don't think many user want it). Do you know what is the performance benefit of this parameter alone?

SL_SI91X_CUSTOM_FEAT_EXTENSION_VALID)
: (SL_SI91X_CUSTOM_FEAT_EXTENSION_VALID |
SL_SI91X_CUSTOM_FEAT_ASYNC_CONNECTION_STATUS |
SL_SI91X_CUSTOM_FEAT_RTC_FROM_HOST),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My question is still relevant.

@ragurram26 ragurram26 force-pushed the Throughput_configurations branch 7 times, most recently from 42c44e2 to f226c12 Compare November 3, 2025 15:22
@ragurram26 ragurram26 changed the title drivers: wifi: siwx91x: SiWx91x high performance configurations To improve SiWx91x performance Nov 3, 2025
}
.ta_pool = {.tx_ratio_in_buffer_pool = 1,
.rx_ratio_in_buffer_pool = 1,
.global_ratio_in_buffer_pool = 1}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New line after opening brace:

		.ta_pool = {
			.tx_ratio_in_buffer_pool     = 1,
			.rx_ratio_in_buffer_pool     = 1,
			.global_ratio_in_buffer_pool = 1,
		}

I have not been to understand how these ratio work despite the documentation in sl_wifi_device.h. If all the values are identical, every buffer type has one third of the memory? Then, how the new values are different from the previous ones?
Or every buffer type get x/10 of the memory? Then, how it worked when these values were 0?

Copy link
Contributor

@jerome-pouiller jerome-pouiller Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the record: when this structure was filled with zeros, the NWP only reserved a small fixed numbers (~10) of RX/TX buffers. With the new code, the number of buffers is a ratio (1/3 Tx, 1/3 Rx) of the available RAM.

@ragurram26 ragurram26 force-pushed the Throughput_configurations branch from f226c12 to db5eaa1 Compare November 4, 2025 05:13
@ragurram26 ragurram26 force-pushed the Throughput_configurations branch from db5eaa1 to 937f705 Compare November 4, 2025 05:26
* to NWP, better are the WiFi and BLE performances.
*/
reg = <0x00000400 DT_SIZE_K(255)>;
reg = <0x00000400 DT_SIZE_K(319)>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand right, you give less memory to NWP and you got better perf ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original values has been computed to offload socket/TCP/IP to the NWP. However, since the Zephyr is taking care about these layers, the NWP is less solicited than initially expected. So, it makes sense to allocate more memory to the M4. Probably the user would want to allocate more memory to the NWP if he enables CONFIG_WIFI_SILABS_SIWX91X_NET_STACK_OFFLOAD.

@ragurram26, maye the sentence "Less memory is allocated to Zephyr, more memory is allocated to NWP, better are the WiFi and BLE performances." is not accurate anymore?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ragurram26 don't you believe the comment should be updated?

* to NWP, better are the WiFi and BLE performances.
*/
reg = <0x00000400 DT_SIZE_K(255)>;
reg = <0x00000400 DT_SIZE_K(319)>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original values has been computed to offload socket/TCP/IP to the NWP. However, since the Zephyr is taking care about these layers, the NWP is less solicited than initially expected. So, it makes sense to allocate more memory to the M4. Probably the user would want to allocate more memory to the NWP if he enables CONFIG_WIFI_SILABS_SIWX91X_NET_STACK_OFFLOAD.

@ragurram26, maye the sentence "Less memory is allocated to Zephyr, more memory is allocated to NWP, better are the WiFi and BLE performances." is not accurate anymore?

@jhedberg jhedberg changed the title To improve SiWx91x performance soc: silabs: siwx91x: Improve performance Nov 6, 2025
@ragurram26 ragurram26 force-pushed the Throughput_configurations branch 5 times, most recently from 3577df3 to 93eeeee Compare November 13, 2025 04:13
jhedberg
jhedberg previously approved these changes Nov 24, 2025
- deep-sleep-with-ram-retention
required: true

soc-120mhz-clk:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a standard naming for this property? If it's specific to Silabs, perhaps it should have some appropriate prefix? (e.g. silabs,)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me like a clock-frequency property would be better, potentially with an enum to limit the selection to allowed values.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, I am a bit confused by the name. What soc means here? Is it clock the NWP clock? If this parameter is not set what is the frequency of the NWP?

Copy link
Contributor

@jerome-pouiller jerome-pouiller Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SL_SI91X_CUSTOM_FEAT_SOC_CLK_CONFIG_160MHZ also exists. Why we don't use 160MHz?

I assume 160MHZ is exclusive wit 120MHZ, so a boolean is probably not the right type.

@jhedberg jhedberg dismissed their stale review November 24, 2025 10:31

Dismissing my review until my question is answered

@ragurram26 ragurram26 force-pushed the Throughput_configurations branch from 93eeeee to ea8b5f6 Compare November 25, 2025 04:56
To enhance throughput performance on the SiWx91x series, Added
some Siwx91x configurations by deafult.

Signed-off-by: Rahul Gurram <[email protected]>
Added support for 120Mhz soc clock via DTS.Setting the clock
to 120 MHz can improve throughput

Signed-off-by: Rahul Gurram <[email protected]>
@ragurram26 ragurram26 force-pushed the Throughput_configurations branch from ea8b5f6 to 26af633 Compare November 25, 2025 04:56
@sonarqubecloud
Copy link

Please retry analysis of this Pull-Request directly on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants