-
Notifications
You must be signed in to change notification settings - Fork 8.3k
soc: silabs: siwg917: Add configurable NWP boot features via DTS and Kconfig #98655
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
soc: silabs: siwg917: Add configurable NWP boot features via DTS and Kconfig #98655
Conversation
3fbda12 to
4f1c22f
Compare
jerome-pouiller
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice improvements!
| type: boolean | ||
| description: | | ||
| Disables automatic 40 MHz XTAL frequency correction. | ||
| Enable this for calibration applications, or when using a custom XTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you use a "negative" boolean (disable-), the word "Enable" is twisted (you mean enabling the disabling, that's it?).
Short, I suggest to avoid "Negative" boolean arithmetic.
| Enables Real-Time Clock (RTC) synchronization from the host system. | ||
| When enabled, the NWP uses the host's RTC for timekeeping and certificate | ||
| validation timing. | ||
| Enable this when the host MCU provides a valid RTC source. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this feature is not enabled, how the NWP retrieve time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If disabled, NWP uses SNTP for timekeeping, but RTC is needed for certificate validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: Currently, Zephyr do not implement TLS offloading. NWP certificate validation is never used. So, I believe mentioning certificate validation is confusing.
BTW, what is the purpose of timekeeping outside of certificate validation?
| the external RF switch. Use this on boards with an external front-end | ||
| switch network. | ||
| This corresponds to BIT(29) – SL_SI91X_EXT_FEAT_FRONT_END_SWITCH_PINS_ULP_GPIO_4_5_0. | ||
| Mutually exclusive with 'front-end-internal-switch'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you prefer an enum rather than two dependent boolean:
antenna-selection:
type: string
description: [...]
enum:
- "none"
- "ext-gpios"
- "internal"
If you sort properly the enum values, you could directly assign it to bits 29-30 (BTW, I don't get the difference between SL_SI91X_EXT_FEAT_FRONT_END_INTERNAL_SWITCH and no front-end at all).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The internal switch mode uses the SiWx917’s on-chip T/R switch logic, while “no front-end” means no RF switching is active.
Internal mode doesn’t use GPIOs and doesn’t support RF_BLE_TX (8 dBm).
Agreed, aligning enum values to bits [30:29] makes sense.
drivers/wifi/siwx91x/Kconfig.siwx91x
Outdated
| config WIFI_SILABS_SIWX91X_FEAT_HIDE_PSK_CREDENTIALS | ||
| bool "Hide PSK and Sensitive Credentials" | ||
| help | ||
| Hides sensitive credentials such as PSK, PMK, and EAP. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean, in the logs?
| { | ||
| uint32_t ext_feat = 0; | ||
|
|
||
| #if DT_NODE_HAS_PROP(NWP_NODE, front_end_internal_switch) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe you need preprocessor macros. if (DT_NODE_HAS_PROP(NWP_NODE, front_end_internal_switch)) { ... } should work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moreover, I think you should retrieve the configuration in the driver initialization and put it in the siwx91x_nwp_config struct like it was done for power-profile or stack-size in SIWX91X_NWP_DEFINE.
It adds an intermediate step but allows to not have DT macro inside of the code.
Martinhoff-maker
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good improvements ! Only have a question: Do you take care that if the Bluetooth is also activated (coex), it will not conflict ?
| }; | ||
|
|
||
| &nwp { | ||
| front-end-switch-gpios; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you need to set this properties here if it's already present in siwg917.dtsi ?
| { | ||
| uint32_t ext_feat = 0; | ||
|
|
||
| #if DT_NODE_HAS_PROP(NWP_NODE, front_end_internal_switch) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moreover, I think you should retrieve the configuration in the driver initialization and put it in the siwx91x_nwp_config struct like it was done for power-profile or stack-size in SIWX91X_NWP_DEFINE.
It adds an intermediate step but allows to not have DT macro inside of the code.
4f1c22f to
3577970
Compare
3577970 to
c5815f0
Compare
| Enables Real-Time Clock (RTC) synchronization from the host system. | ||
| When enabled, the NWP uses the host's RTC for timekeeping and certificate | ||
| validation timing. | ||
| Enable this when the host MCU provides a valid RTC source. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: Currently, Zephyr do not implement TLS offloading. NWP certificate validation is never used. So, I believe mentioning certificate validation is confusing.
BTW, what is the purpose of timekeeping outside of certificate validation?
| Enables 1.8 V operation support for the NWP power domain. | ||
| Enable this if the board powers the NWP with 1.8 V instead of 3.3 V. | ||
| disable-xtal-correction: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename in enable-xtal-correction and inverse the logic in the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. This logic allows to directly map the Wiseconnect flags... why not.
| void (*config_irq)(const struct device *dev); | ||
| uint32_t stack_size; | ||
| uint8_t power_profile; | ||
| const char *antenna_selection; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use DT_INST_ENUM_IDX() to automatically convert the string into an int.
| .disable_xtal_correction = DT_PROP_OR(DT_DRV_INST(inst), \ | ||
| disable_xtal_correction, false), \ | ||
| .qspi_80mhz_clk = DT_PROP_OR(DT_DRV_INST(inst), qspi_80mhz_clk, false), \ | ||
| .antenna_selection = DT_PROP_OR(DT_DRV_INST(inst), antenna_selection, "none"), \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need DT_PROP_OR(). From api-usage.rst:
Use DT_PROP() [...] will expand to either 0 or 1 depending on if the property
is present or absent.
You can even use DT_INST_PROP() so you won't need DT_DRV_INST().
|
|
||
| if (IS_ENABLED(CONFIG_WIFI_SILABS_SIWX91X_FEAT_SECURITY_PSK)) { | ||
| boot_config->feature_bit_map |= SL_SI91X_FEAT_SECURITY_PSK; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For readability, does it make sense to set this bit if CONFIG_WIFI_SILABS_SIWX91X is not set? (BTW, maybe CONFIG_WIFI_SILABS_SIWX91X_ROAMING_USE_DEAUTH should also be relocated)
| } | ||
| } | ||
|
|
||
| if (cfg->antenna_selection) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In fact, you can write:
#define SL_SI91X_EXT_FEAT_FRONT_END_MSK (BIT(30) | BIT(29))
[...]
boot_config->ext_custom_feature_bit_map &= ~SL_SI91X_EXT_FEAT_FRONT_END_MASK;
boot_config->ext_custom_feature_bit_map |= FIELD_PREP(SL_SI91X_EXT_FEAT_FRONT_END_MSK,
cfg->antenna_selection);
[...]
.antenna_selection = DT_INST_ENUM_IDX(inst, antenna_selection),
| } | ||
| } | ||
|
|
||
| static void siwx91x_apply_nwp_config(const struct device *dev, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything in this file is related to nwp. Maybe we should rename this function in siwx91x_apply_hw_config()?
Add new Devicetree properties under the SiWx91x NWP node to describe hardware-specific boot configuration options. The properties are documented in the SiWx91x NWP YAML binding. Signed-off-by: Arunmani Alagarsamy <arunmani.a@silabs.com>
Add support for configuring hardware-specific boot feature bitmaps through Devicetree and software-specific boot configurations through Kconfig options. Signed-off-by: Arunmani Alagarsamy <arunmani.a@silabs.com>
c5815f0 to
294e0bb
Compare
|



Adds support for configuring SiWx91x NWP boot features using Devicetree and Kconfig.
Introduces new DTS properties for hardware-specific options (e.g., RTC sync, 1.8 V support, XTAL correction, QSPI 80 MHz, front-end switch modes) and Kconfig entries for software-defined boot configurations.