-
Notifications
You must be signed in to change notification settings - Fork 8.1k
drivers: bluetooth: silabs: Add hardware-dependent feature detection #97994
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
base: main
Are you sure you want to change the base?
Conversation
|
@jrintaha fyi |
soc/silabs/silabs_s2/Kconfig
Outdated
| def_bool $(dt_nodelabel_bool_prop,radio,ble-2mbps-supported) | ||
|
|
||
| config HAS_HW_EFR32_RADIO_BLE_CODED | ||
| def_bool $(dt_nodelabel_bool_prop,radio,ble-coded-phy-supported) |
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.
Do we need these intermediate symbols (HAS_HW_EFR32_RADIO_*)?
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 think it is easier to define dependencies with these, like for example HAS_HW_EFR32_RADIO_CTE_RX
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.
Not a blocker, but I have always had some doubts about the benefit of adding new global helper symbols. They are sometime used to hide some logic (now or in further PR) and may make the reader work more difficult.
|
|
||
| cs-supported: | ||
| type: boolean | ||
| description: | | ||
| If set, the radio hardware supports the BLE Channel Sounding feature. | ||
| This property should be treated as read-only and should not be overridden; | ||
| the correct value is provided for your target's SoC already. | ||
| cte-tx: | ||
| type: boolean | ||
| description: | | ||
| Indicates that the radio can transmit Constant Tone Extensions (CTE) | ||
| for Direction Finding in Angle of Departure (AoD) mode. | ||
| cte-rx: | ||
| type: boolean | ||
| description: | | ||
| Indicates that the radio can receive and sample Constant Tone Extensions (CTE) | ||
| for Direction Finding in Angle of Arrival (AoA) mode. Requires cte-tx capability. | ||
| tx-high-power-supported: | ||
| type: boolean | ||
| description: | | ||
| If set, indicates that the radio hardware supports high TX power settings. | ||
| This property should be treated as read-only and should not be overridden; | ||
| the correct value is provided for your target's SoC already. | ||
| ble-2mbps-supported: | ||
| type: boolean | ||
| description: | | ||
| If set, indicates that the radio hardware supports the 2 Mbps BLE mode. | ||
| This property should be treated as read-only and should not be overridden; | ||
| the correct value is provided for your target's SoC already. | ||
| ble-coded-phy-supported: | ||
| type: boolean | ||
| description: | | ||
| If set, indicates that the radio hardware supports coded BLE PHY. | ||
| This property should be treated as read-only and should not be overridden; | ||
| the correct value is provided for your target's SoC already. |
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.
Some duplication right?
Can we have a base radio binding that can be included?
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.
Agreed. Would be good to collect these into a base binding. @cvinayak were you thinking of a generic one for all radios or for Bluetooth LE radios in particular? If the latter would it be in dts/bindings/net/wireless or dts/bindings/bluetooth?. The properties should IMO also have the same namespace (likely ble-* which has been used for the existing properties). The base binding should also only contain features which can be shared accrosss multiple radios, i.e. in practice those features which are based on the Bluetooth standard.
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.
@ipankr just so we're clear, by "base binding" we mean something like a ble-radio.yaml which can be included by the actual radio bindings.
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.
Can we agree on a possible better naming of the features in this file? Or should I go as it is?
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.
Can we agree on a possible better naming of the features in this file? Or should I go as it is?
As I mentioned, at the very least I'd want them to have a common prefix. ble-* would have the benefit of not breaking downstream users of the existing properties.
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.
As I mentioned, at the very least I'd want them to have a common prefix.
ble-*would have the benefit of not breaking downstream users of the existing properties.
ble-*.yaml is fine by me. Do add ble-* prefix to Bluetooth technology specific features (tx-high-power-supported is generic wireless where as ble-cs- and ble-ctx- are Bluetooth technology). To keep the scope limited, this PR can include the ble-*.yaml to begin, other vendors and downstream can plan the re-use at their convenience. A github issue to track should suffice.
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 we're close to 4.3, I don't think I'd like to have a mix there where one radio binding uses the new base binding and another doesn't, so I'd suggest to update the nrf-radio.yaml here as well, if you don't mind (for what I've seen, it's just two ble- properties, right?)
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 its not too much work, and we have @nordicjm in attention. We can proceed to update nrf too, but not get too much out-of-scope.
| enum: | ||
| - highest | ||
|
|
||
| ble-tx-high-power-supported: |
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.
Even though this isn't based on any spec, but since there's a property with the identical name in the nRF binding, perhaps we'd still want this in the common base binding to avoid duplication (with a mention in the description that the meaning is implementation-specific?). Thoughts from @cvinayak?
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.
see https://github.com/zephyrproject-rtos/zephyr/pull/97994/files#r2451718718
tx-high-power-supported is very generic to wireless. Can we or do we already have a generic,radio.yaml ? this should go there.
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 would not put it into a common base, exactly because it is not in the spec and it is very vendor specific
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.
because it is not in the spec
Hence, requesting to revert back to tx-high-power-supported and associate it with Bluetooth.
| Use Silicon Labs Wiseconnect 3.x Bluetooth library to connect to the controller. | ||
|
|
||
| # SiWx917 BLE controller currently does not support HCI Command: Host Number of Completed Packets | ||
| configdefault BT_HCI_ACL_FLOW_CONTROL |
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.
wrong indent
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.
what exactly is wrong? It pass the compliance test.
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.
@ipankr all Kconfig definitions should start at the beginning of the line. Furthermore, I thought configdefault was only valid for Kconfig.defconfig files, but seems like there's no check for that either.
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.
there is no Kconfig option compliance check, not sure where you got that from. There is one check which checks spacing, it does not check any validity of Kconfig files, and another that checks for "Enable" in the name, and another that checks if it is loadable, that is the extent of testing, e.g. a 700 line file will pass CI
| select BT_CTLR_DF_ANT_SWITCH_1US_SUPPORT if HAS_HW_EFR32_RADIO_CTE_TX | ||
| select BT_CTLR_DF_ANT_SWITCH_2US_SUPPORT if HAS_HW_EFR32_RADIO_CTE_TX | ||
| select BT_SILABS_EFR32_HIGH_POWER if HAS_HW_EFR32_RADIO_TX_HIGH_POWER | ||
|
|
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.
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 would like to keep it for the readability.
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.
no thanks, remove, not allowed under https://docs.zephyrproject.org/latest/contribute/style/kconfig.html
| select PSA_CRYPTO | ||
| select SILABS_SISDK_PROTOCOL_CRYPTO | ||
| select HAS_BT_CTLR | ||
|
|
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.
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 would like to keep it for the readability.
| select BT_CTLR_SYNC_PERIODIC_SUPPORT | ||
| select BT_CTLR_SYNC_TRANSFER_RECEIVER_SUPPORT | ||
| select BT_CTLR_SYNC_TRANSFER_SENDER_SUPPORT | ||
|
|
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.
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 would like to keep it for the readability.
| Maximum queued advertising reports. | ||
| Additional advertising reports are dropped. | ||
|
|
||
| config BT_SILABS_EFR32_HIGH_POWER |
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 this when you have it in dts, and it has it's own kconfig below?
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.
It is a declaration. DT just selects it if hw supports it.
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.
there is a symbol in Kconfig which is auto generated, you don't need a duplicate symbol
| If set, indicates that the radio hardware supports coded BLE PHY. | ||
| tx-high-power-supported: | ||
| ble-tx-high-power-supported: |
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.
| ble-tx-high-power-supported: | |
| tx-high-power-supported: |
Isnt this use of front-end-module, or Tx gain in the multi-protocol radio, right? There is nothing Bluetooth Technology related here, right?
If this is specific to Bluetooth Technology LE Power Class 1 feature, then lets name it accordingly and update the description.
| enum: | ||
| - highest | ||
|
|
||
| ble-tx-high-power-supported: |
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.
see https://github.com/zephyrproject-rtos/zephyr/pull/97994/files#r2451718718
tx-high-power-supported is very generic to wireless. Can we or do we already have a generic,radio.yaml ? this should go there.
|
|
||
| cs-supported: | ||
| type: boolean | ||
| description: | | ||
| If set, the radio hardware supports the BLE Channel Sounding feature. | ||
| This property should be treated as read-only and should not be overridden; | ||
| the correct value is provided for your target's SoC already. | ||
| cte-tx: | ||
| type: boolean | ||
| description: | | ||
| Indicates that the radio can transmit Constant Tone Extensions (CTE) | ||
| for Direction Finding in Angle of Departure (AoD) mode. | ||
| cte-rx: | ||
| type: boolean | ||
| description: | | ||
| Indicates that the radio can receive and sample Constant Tone Extensions (CTE) | ||
| for Direction Finding in Angle of Arrival (AoA) mode. Requires cte-tx capability. | ||
| tx-high-power-supported: | ||
| type: boolean | ||
| description: | | ||
| If set, indicates that the radio hardware supports high TX power settings. | ||
| This property should be treated as read-only and should not be overridden; | ||
| the correct value is provided for your target's SoC already. | ||
| ble-2mbps-supported: | ||
| type: boolean | ||
| description: | | ||
| If set, indicates that the radio hardware supports the 2 Mbps BLE mode. | ||
| This property should be treated as read-only and should not be overridden; | ||
| the correct value is provided for your target's SoC already. | ||
| ble-coded-phy-supported: | ||
| type: boolean | ||
| description: | | ||
| If set, indicates that the radio hardware supports coded BLE PHY. | ||
| This property should be treated as read-only and should not be overridden; | ||
| the correct value is provided for your target's SoC already. |
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 its not too much work, and we have @nordicjm in attention. We can proceed to update nrf too, but not get too much out-of-scope.
| ble-coded-phy-supported; | ||
| cs-supported; | ||
| ble-cs-supported; | ||
| dfe-supported; |
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.
dfe-supported ? ble-dfe-supported, right?
Vendor specific configuration is moved to a separate file. Signed-off-by: Ivan Pankratov <[email protected]>
Create a shared base binding (ble-radio.yaml) for common Bluetooth LE radio hardware capabilities to avoid duplication between vendors and ensure consistent property naming across the ecosystem. Properties are prefixed with 'ble-' and ordered chronologically by Bluetooth Core Specification version (5.0, 5.1, 6.0). Each property indicates a hardware capability, not current enablement state. Signed-off-by: Ivan Pankratov <[email protected]>
|



Summary
This PR creates a shared base binding for common Bluetooth LE radio hardware capabilities to avoid duplication between vendors and ensure consistent property naming across the Zephyr ecosystem.
Changes
New Base Binding
Updated Vendor Bindings
Silabs:
Nordic:
Silicon Labs Device Trees
Updated all Series 2 SoCs with hardware-supported features:
Nordic Device Trees
Updated with consistent property naming:
Kconfig Integration
Silabs - Added hardware capability flags (auto-detected from DT):
Silabs HCI driver automatically selects controller features based on hardware capabilities.
Nordic - Updated Kconfig to reference renamed properties:
Testing
Verified on all affected hardware (11 board configurations):
Design Philosophy
This approach harmonizes devicetree bindings across vendors while respecting each vendor's driver architecture.