From 041099f67cd6d8b2b84912ec616e504710ebffe6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mart=C3=AD=20Bol=C3=ADvar?= Date: Mon, 19 Jul 2021 17:30:32 -0700 Subject: [PATCH] Bluetooth: Controller: clean up nRF DFE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The way we currently handle direction finding extension (DFE) support on Nordic nRF5 controllers relies on required devicetree properties related to DFE in the "nordic,nrf-radio" node. That doesn't make sense on radios without DFE support, though. Any .dtsi for an SoC without DFE support which has such a node would require extraneous DFE related properties like dfe-antenna-num. Instead of making the properties required, mark them optional. We indicate the presence of DFE support via a new 'dfe-supported' boolean property which the SoC .dtsi files can set (or not) depending on support. This gives us the opportunity to do some cleanup in the Kconfig, removing CONFIG_HAS_HW_NRF_RADIO_BLE_DF since we know from the devicetree whether DFE support is available. Handle that change appropriately in radio_df.c. This gives us an opportunity to improve readability in the devicetree-related macro magic in that file. Signed-off-by: Martí Bolívar --- dts/arm/nordic/nrf52820.dtsi | 3 +- dts/arm/nordic/nrf52833.dtsi | 3 +- dts/arm/nordic/nrf5340_cpunet.dtsi | 3 +- .../net/wireless/nordic,nrf-radio.yaml | 23 +- soc/arm/nordic_nrf/Kconfig.peripherals | 3 - soc/arm/nordic_nrf/nrf52/Kconfig.soc | 2 - soc/arm/nordic_nrf/nrf53/Kconfig.soc | 1 - .../bluetooth/controller/Kconfig.ll_sw_split | 5 +- .../ll_sw/nordic/hal/nrf5/radio/radio_df.c | 201 ++++++++++-------- 9 files changed, 135 insertions(+), 109 deletions(-) diff --git a/dts/arm/nordic/nrf52820.dtsi b/dts/arm/nordic/nrf52820.dtsi index 95111ce11b2..e72b6f3870f 100644 --- a/dts/arm/nordic/nrf52820.dtsi +++ b/dts/arm/nordic/nrf52820.dtsi @@ -56,8 +56,7 @@ reg = <0x40001000 0x1000>; interrupts = <1 NRF_DEFAULT_IRQ_PRIORITY>; status = "okay"; - dfe-antenna-num = <0>; - dfe-pdu-antenna = <0xFF>; + dfe-supported; }; uart0: uart@40002000 { diff --git a/dts/arm/nordic/nrf52833.dtsi b/dts/arm/nordic/nrf52833.dtsi index 3cb58c0bfb7..cd8e4ba68a7 100644 --- a/dts/arm/nordic/nrf52833.dtsi +++ b/dts/arm/nordic/nrf52833.dtsi @@ -55,8 +55,7 @@ reg = <0x40001000 0x1000>; interrupts = <1 NRF_DEFAULT_IRQ_PRIORITY>; status = "okay"; - dfe-antenna-num = <0>; - dfe-pdu-antenna = <0xFF>; + dfe-supported; }; uart0: uart@40002000 { diff --git a/dts/arm/nordic/nrf5340_cpunet.dtsi b/dts/arm/nordic/nrf5340_cpunet.dtsi index 6a756c6738a..cc6a5911d02 100644 --- a/dts/arm/nordic/nrf5340_cpunet.dtsi +++ b/dts/arm/nordic/nrf5340_cpunet.dtsi @@ -72,8 +72,7 @@ reg = <0x41008000 0x1000>; interrupts = <8 NRF_DEFAULT_IRQ_PRIORITY>; status = "okay"; - dfe-antenna-num = <0>; - dfe-pdu-antenna = <0xFF>; + dfe-supported; }; rng: random@41009000 { diff --git a/dts/bindings/net/wireless/nordic,nrf-radio.yaml b/dts/bindings/net/wireless/nordic,nrf-radio.yaml index fc4100c7731..1dcce92154e 100644 --- a/dts/bindings/net/wireless/nordic,nrf-radio.yaml +++ b/dts/bindings/net/wireless/nordic,nrf-radio.yaml @@ -16,6 +16,7 @@ description: | --------------------------- Some radios support the Bluetooth Direction Finding Extension (DFE). + The 'dfe-supported' property will be set when it is available. In this case, the 'dfegpio[n]-gpios' properties configure GPIO pins to use to drive antenna switching. @@ -91,22 +92,31 @@ properties: interrupts: required: true + dfe-supported: + type: boolean + description: | + If set, the radio hardware supports the Direction Finding Extension. + This property should be treated as read-only and should not be overridden; + the correct value is provided for your target's SoC already. + dfe-antenna-num: type: int - required: true + required: false description: | - Number of available antennas. + Number of available antennas for the Direction Finding Extension. - Value zero means no antenna available. If provided value different than - zero, it must be greater or equal to two. + This should only be set if dfe-supported is true. If you set this + property, the value must be at least two. dfe-pdu-antenna: type: int - required: true + required: false description: | Antenna switch pattern to be used for transmission of PDU before start of transmission of Constant Tone Extension. + This should only be set if dfe-supported is true. + This pattern is stored in SWITCHPATTERN[0] before actual antenna switching patterns. This pattern will also be used to drive GPIOs when the radio releases control of GPIOs used to switch antennas. @@ -115,7 +125,8 @@ properties: type: phandle-array required: false description: | - Pin select for DFE pin 0. + Pin select for DFE pin 0. This should only be set if dfe-supported + is true. For example, to use P0.2 on an nRF5x SoC: diff --git a/soc/arm/nordic_nrf/Kconfig.peripherals b/soc/arm/nordic_nrf/Kconfig.peripherals index 827963fdb4e..fb1e494ee9d 100644 --- a/soc/arm/nordic_nrf/Kconfig.peripherals +++ b/soc/arm/nordic_nrf/Kconfig.peripherals @@ -133,9 +133,6 @@ config HAS_HW_NRF_RADIO_BLE_CODED config HAS_HW_NRF_RADIO_IEEE802154 bool -config HAS_HW_NRF_RADIO_BLE_DF - bool - config HAS_HW_NRF_RNG bool diff --git a/soc/arm/nordic_nrf/nrf52/Kconfig.soc b/soc/arm/nordic_nrf/nrf52/Kconfig.soc index db9b9141799..43ccfd9543e 100644 --- a/soc/arm/nordic_nrf/nrf52/Kconfig.soc +++ b/soc/arm/nordic_nrf/nrf52/Kconfig.soc @@ -160,7 +160,6 @@ config SOC_NRF52820 select HAS_HW_NRF_RADIO_BLE_2M select HAS_HW_NRF_RADIO_TX_PWR_HIGH select HAS_HW_NRF_RADIO_BLE_CODED - select HAS_HW_NRF_RADIO_BLE_DF select HAS_HW_NRF_RNG select HAS_HW_NRF_RTC0 select HAS_HW_NRF_RTC1 @@ -296,7 +295,6 @@ config SOC_NRF52833 select HAS_HW_NRF_RADIO_TX_PWR_HIGH select HAS_HW_NRF_RADIO_BLE_CODED select HAS_HW_NRF_RADIO_IEEE802154 - select HAS_HW_NRF_RADIO_BLE_DF select HAS_HW_NRF_RNG select HAS_HW_NRF_RTC0 select HAS_HW_NRF_RTC1 diff --git a/soc/arm/nordic_nrf/nrf53/Kconfig.soc b/soc/arm/nordic_nrf/nrf53/Kconfig.soc index a8a754d79c8..14aaf1fc04d 100644 --- a/soc/arm/nordic_nrf/nrf53/Kconfig.soc +++ b/soc/arm/nordic_nrf/nrf53/Kconfig.soc @@ -86,7 +86,6 @@ config SOC_NRF5340_CPUNET select HAS_HW_NRF_RADIO_BLE_2M select HAS_HW_NRF_RADIO_BLE_CODED select HAS_HW_NRF_RADIO_IEEE802154 - select HAS_HW_NRF_RADIO_BLE_DF select HAS_HW_NRF_RNG select HAS_HW_NRF_RTC0 select HAS_HW_NRF_RTC1 diff --git a/subsys/bluetooth/controller/Kconfig.ll_sw_split b/subsys/bluetooth/controller/Kconfig.ll_sw_split index 0de174dcf25..ff4650fa10b 100644 --- a/subsys/bluetooth/controller/Kconfig.ll_sw_split +++ b/subsys/bluetooth/controller/Kconfig.ll_sw_split @@ -5,6 +5,9 @@ if BT_LL_SW_SPLIT +DT_PATH_NORDIC_RADIO := $(dt_nodelabel_path,radio) +DT_NORDIC_RADIO_DFE_SUPPORTED := $(dt_node_has_bool_prop,$(DT_PATH_NORDIC_RADIO),dfe-supported) + config BT_LLL_VENDOR_NORDIC bool "Use Nordic LLL" depends on SOC_COMPATIBLE_NRF @@ -31,7 +34,7 @@ config BT_LLL_VENDOR_NORDIC select BT_CTLR_SYNC_PERIODIC_SUPPORT select BT_CTLR_ADV_ISO_SUPPORT select BT_CTLR_SYNC_ISO_SUPPORT - select BT_CTLR_DF_SUPPORT if HAS_HW_NRF_RADIO_BLE_DF + select BT_CTLR_DF_SUPPORT if $(DT_NORDIC_RADIO_DFE_SUPPORTED) select BT_CTLR_CHAN_SEL_2_SUPPORT select BT_CTLR_MIN_USED_CHAN_SUPPORT select BT_CTLR_DTM_HCI_SUPPORT diff --git a/subsys/bluetooth/controller/ll_sw/nordic/hal/nrf5/radio/radio_df.c b/subsys/bluetooth/controller/ll_sw/nordic/hal/nrf5/radio/radio_df.c index 787956d8a6b..be9eac8e6bc 100644 --- a/subsys/bluetooth/controller/ll_sw/nordic/hal/nrf5/radio/radio_df.c +++ b/subsys/bluetooth/controller/ll_sw/nordic/hal/nrf5/radio/radio_df.c @@ -15,90 +15,138 @@ #include "radio_df.h" #include "radio_internal.h" -/* @brief Minimum antennas number required if antenna switching is enabled */ -#define DF_ANT_NUM_MIN 2 -/* @brief Value that used to check if PDU antenna pattern is not set */ -#define DF_PDU_ANT_NOT_SET 0xFF -/* @brief Value to set antenna GPIO pin as not connected */ -#define DF_GPIO_PIN_NOT_SET 0xFF -/* @brief Number of PSEL_DFEGPIO registers in Radio peripheral */ -#define DF_PSEL_GPIO_NUM 8 -/* @brief Maximum index number related with count of PSEL_DFEGPIO registers in - * Radio peripheral. It is used in macros only e.g. UTIL_LISTIFY that - * evaluate indices in an inclusive way. - */ -#define DF_PSEL_GPIO_NUM_MAX_IDX 7 +/* Devicetree node identifier for the radio node. */ +#define RADIO_NODE DT_NODELABEL(radio) -/* @brief Direction Finding antenna matrix configuration */ +/* Value to set for unconnected antenna GPIO pins. */ +#define DFE_PSEL_NOT_SET 0xFF +/* Number of PSEL_DFEGPIO[n] registers in the radio peripheral. */ +#define MAX_DFE_GPIO 8 +/* Run a macro 'fn' on each available DFE GPIO index, from 0 to + * MAX_DFE_GPIO-1, with the given parenthesized separator. + */ +#define FOR_EACH_DFE_GPIO(fn, sep) \ + FOR_EACH(fn, sep, 0, 1, 2, 3, 4, 5, 6, 7) + +/* Direction Finding antenna matrix configuration */ struct df_ant_cfg { uint8_t ant_num; /* Selection of GPIOs to be used to switch antennas by Radio */ - uint8_t dfe_gpio[DF_PSEL_GPIO_NUM]; + uint8_t dfe_gpio[MAX_DFE_GPIO]; }; -#define RADIO DT_NODELABEL(radio) -#define DFE_GPIO_PSEL(idx) \ - COND_CODE_1(DT_NODE_HAS_PROP(RADIO, dfegpio##idx##_gpios), \ - (NRF_DT_GPIOS_TO_PSEL(RADIO, dfegpio##idx##_gpios)), \ - (DF_GPIO_PIN_NOT_SET)) +#define DFE_GPIO_PSEL(idx) \ + NRF_DT_GPIOS_TO_PSEL_OR(RADIO_NODE, dfegpio##idx##_gpios, \ + DFE_PSEL_NOT_SET) #define DFE_GPIO_PIN_DISCONNECT (RADIO_PSEL_DFEGPIO_CONNECT_Disconnected << \ RADIO_PSEL_DFEGPIO_CONNECT_Pos) -#define COUNT_GPIO(idx, _) + DT_NODE_HAS_PROP(RADIO, dfegpio##idx##_gpios) -#define DFE_GPIO_NUM (UTIL_LISTIFY(DF_PSEL_GPIO_NUM, COUNT_GPIO, _)) +#define HAS_DFE_GPIO(idx) DT_NODE_HAS_PROP(RADIO_NODE, dfegpio##idx##_gpios) -/* DFE_GPIO_NUM_IS_ZERO is required to correctly compile COND_CODE_1 in - * DFE_GPIO_ALLOWED_ANT_NUM macro. DFE_GPIO_NUM does not expand to literal 1 - * So it is not possible to use it as a conditional in COND_CODE_1 argument. +/* The number of dfegpio[n]-gpios properties which are set. */ +#define DFE_GPIO_NUM (FOR_EACH_DFE_GPIO(HAS_DFE_GPIO, (+))) + +/* The minimum number of antennas required to enable antenna switching. */ +#define MIN_ANTENNA_NUM 2 + +/* The maximum number of antennas supported by the number of + * dfegpio[n]-gpios properties which are set. */ #if (DFE_GPIO_NUM > 0) -#define DFE_GPIO_NUM_IS_ZERO 1 +#define MAX_ANTENNA_NUM BIT(DFE_GPIO_NUM) #else -#define DFE_GPIO_NUM_IS_ZERO EMPTY +#define MAX_ANTENNA_NUM 0 #endif -#define DFE_GPIO_ALLOWED_ANT_NUM COND_CODE_1(DFE_GPIO_NUM_IS_ZERO, \ - (BIT(DFE_GPIO_NUM)), (0)) +/* PDU_ANTENNA is defined outside of the #if block below because + * radio_df_pdu_antenna_switch_pattern_get() can get called even when + * the preprocessor condition being tested is 0. In this case, we use + * the default value of 0. + */ +#define PDU_ANTENNA DT_PROP_OR(RADIO_NODE, dfe_pdu_antenna, 0) + +uint8_t radio_df_pdu_antenna_switch_pattern_get(void) +{ + return PDU_ANTENNA; +} #if defined(CONFIG_BT_CTLR_DF_ANT_SWITCH_TX) || \ defined(CONFIG_BT_CTLR_DF_ANT_SWITCH_RX) -/* Check if there is enough pins configured to represent each pattern - * for given antennas number. +/* + * Check that we have an antenna switch pattern for the DFE idle + * state. (In DFE idle state, the radio peripheral transmits or + * receives PDUs.) */ -BUILD_ASSERT((DT_PROP(RADIO, dfe_antenna_num) <= DFE_GPIO_ALLOWED_ANT_NUM), "Insufficient " - "number of GPIO pins configured."); -BUILD_ASSERT((DT_PROP(RADIO, dfe_antenna_num) >= DF_ANT_NUM_MIN), "Insufficient " - "number of antennas provided."); -BUILD_ASSERT(!IS_ENABLED(CONFIG_BT_CTLR_DF_INIT_ANT_SEL_GPIOS) || - (DT_PROP(RADIO, dfe_pdu_antenna) != DF_PDU_ANT_NOT_SET), "Missing " - "antenna pattern used to select antenna for PDU Tx."); -/* Check if dfegpio#-gios property has flag cell set to zero */ -#define DFE_GPIO_PIN_FLAGS(idx) (DT_GPIO_FLAGS(RADIO, dfegpio##idx##_gpios)) -#define DFE_GPIO_PIN_IS_FLAG_ZERO(idx) \ - COND_CODE_1(DT_NODE_HAS_PROP(RADIO, dfegpio##idx##_gpios), \ - (BUILD_ASSERT((DFE_GPIO_PIN_FLAGS(idx) == 0), \ - "Flags value of GPIO pin property must be" \ - "zero.")), \ - (EMPTY)) +#define HAS_PDU_ANTENNA DT_NODE_HAS_PROP(RADIO_NODE, dfe_pdu_antenna) -#define DFE_GPIO_PIN_LIST(idx, _) idx, -FOR_EACH(DFE_GPIO_PIN_IS_FLAG_ZERO, (;), - UTIL_LISTIFY(DF_PSEL_GPIO_NUM_MAX_IDX, DFE_GPIO_PIN_LIST)) +BUILD_ASSERT(HAS_PDU_ANTENNA, + "Missing antenna pattern used to select antenna for PDU Tx " + "during the DFE Idle state. " + "Set the dfe-pdu-antenna devicetree property."); -#if DT_NODE_HAS_STATUS(RADIO, okay) -const static struct df_ant_cfg ant_cfg = { - .ant_num = DT_PROP(RADIO, dfe_antenna_num), - .dfe_gpio = { - FOR_EACH(DFE_GPIO_PSEL, (,), - UTIL_LISTIFY(DF_PSEL_GPIO_NUM_MAX_IDX, DFE_GPIO_PIN_LIST)) +void radio_df_ant_switch_pattern_set(const uint8_t *patterns, uint8_t len) +{ + /* SWITCHPATTERN is like a moving pointer to an underlying buffer. + * Each write stores a value and moves the pointer to new free position. + * When read it returns number of stored elements since last write to + * CLEARPATTERN. There is no need to use a subscript operator. + * + * Some storage entries in the buffer has special purpose for DFE + * extension in radio: + * - SWITCHPATTERN[0] for idle period (PDU Tx/Rx), + * - SWITCHPATTERN[1] for guard and reference period, + * - SWITCHPATTERN[2] and following for switch-sampling slots. + * Due to that in SWITCHPATTER[0] there is stored a pattern provided by + * DTS property dfe_pdu_antenna. This limits number of supported antenna + * switch patterns by one. + */ + NRF_RADIO->SWITCHPATTERN = PDU_ANTENNA; + for (uint8_t idx = 0; idx < len; ++idx) { + NRF_RADIO->SWITCHPATTERN = patterns[idx]; } +} + +/* + * Check that the number of antennas has been set, and that enough + * pins are configured to represent each pattern for the given number + * of antennas. + */ + +#define HAS_ANTENNA_NUM DT_NODE_HAS_PROP(RADIO_NODE, dfe_antenna_num) + +BUILD_ASSERT(HAS_ANTENNA_NUM, + "You must set the dfe-antenna-num property in the radio node " + "to enable antenna switching."); + +#define ANTENNA_NUM DT_PROP_OR(RADIO_NODE, dfe_antenna_num, 0) + +BUILD_ASSERT(!HAS_ANTENNA_NUM || (ANTENNA_NUM <= MAX_ANTENNA_NUM), + "Insufficient number of GPIO pins configured. " + "Set more dfegpio[n]-gpios properties."); +BUILD_ASSERT(!HAS_ANTENNA_NUM || (ANTENNA_NUM >= MIN_ANTENNA_NUM), + "Insufficient number of antennas provided. " + "Increase the dfe-antenna-num property."); + +/* + * Check that each dfegpio[n]-gpios property has a zero flags cell. + */ + +#define ASSERT_DFE_GPIO_FLAGS_ARE_ZERO(idx) \ + BUILD_ASSERT(DT_GPIO_FLAGS(RADIO_NODE, dfegpio##idx##_gpios) == 0, \ + "The flags cell in each dfegpio[n]-gpios " \ + "property must be zero.") + +FOR_EACH_DFE_GPIO(ASSERT_DFE_GPIO_FLAGS_ARE_ZERO, (;)); + +/* Stores the dfegpio[n]-gpios property values. + */ +const static struct df_ant_cfg ant_cfg = { + .ant_num = ANTENNA_NUM, + .dfe_gpio = { FOR_EACH_DFE_GPIO(DFE_GPIO_PSEL, (,)) } }; -#else -#error "DF antenna switching feature requires dfe_antenna_num to be enabled in DTS" -#endif /* @brief Function configures Radio with information about GPIO pins that may be * used to drive antenna switching during CTE Tx/RX. @@ -111,10 +159,10 @@ void radio_df_ant_switching_pin_sel_cfg(void) { uint8_t pin_sel; - for (uint8_t idx = 0; idx < DF_PSEL_GPIO_NUM; ++idx) { + for (uint8_t idx = 0; idx < MAX_DFE_GPIO; ++idx) { pin_sel = ant_cfg.dfe_gpio[idx]; - if (pin_sel != DF_GPIO_PIN_NOT_SET) { + if (pin_sel != DFE_PSEL_NOT_SET) { nrf_radio_dfe_pattern_pin_set(NRF_RADIO, pin_sel, idx); @@ -141,12 +189,12 @@ void radio_df_ant_switching_gpios_cfg(void) { uint8_t pin_sel; - for (uint8_t idx = 0; idx < DF_PSEL_GPIO_NUM; ++idx) { + for (uint8_t idx = 0; idx < MAX_DFE_GPIO; ++idx) { pin_sel = ant_cfg.dfe_gpio[idx]; - if (pin_sel != DF_GPIO_PIN_NOT_SET) { + if (pin_sel != DFE_PSEL_NOT_SET) { nrf_gpio_cfg_output(pin_sel); - if (BIT(idx) & DT_PROP(RADIO, dfe_pdu_antenna)) { + if (BIT(idx) & PDU_ANTENNA) { nrf_gpio_pin_set(pin_sel); } else { nrf_gpio_pin_clear(pin_sel); @@ -316,33 +364,6 @@ void radio_df_ant_switch_pattern_clear(void) NRF_RADIO->CLEARPATTERN = RADIO_CLEARPATTERN_CLEARPATTERN_Clear; } -void radio_df_ant_switch_pattern_set(const uint8_t *patterns, uint8_t len) -{ - /* SWITCHPATTERN is like a moving pointer to underlying buffer. - * Each write stores a value and moves the pointer to new free position. - * When read it returns number of stored elements since last write to - * CLEARPATTERN. There is no need to use subscript operator. - * - * Some storage entries in the buffer has special purpose for DFE - * extension in radio: - * - SWITCHPATTERN[0] for idle period (PDU Tx/Rx), - * - SWITCHPATTERN[1] for guard and reference period, - * - SWITCHPATTERN[2] and following for switch-sampling slots. - * Due to that in SWITCHPATTER[0] there is stored a pattern provided by - * DTS property dfe_pdu_antenna. This limits number of supported antenna - * switch patterns by one. - */ - NRF_RADIO->SWITCHPATTERN = DT_PROP(RADIO, dfe_pdu_antenna); - for (uint8_t idx = 0; idx < len; ++idx) { - NRF_RADIO->SWITCHPATTERN = patterns[idx]; - } -} - -uint8_t radio_df_pdu_antenna_switch_pattern_get(void) -{ - return DT_PROP(RADIO, dfe_pdu_antenna); -} - void radio_df_reset(void) { radio_df_mode_set(RADIO_DFEMODE_DFEOPMODE_Disabled);