From b61bd2364c1a4267f489910987d7d2c0d14e62c2 Mon Sep 17 00:00:00 2001 From: Vinayak Kariappa Chettimada Date: Mon, 25 Sep 2023 10:17:22 +0200 Subject: [PATCH] Bluetooth: Controller: nrf53: Fix back-to-back Tx Rx implementation Back-to-back Tx Rx implementation was incorrect for nRF53 that uses DPPI. Both current and next DPPI channels where enabled in the implementation which only worked correctly to have the right tIFS when current and next PDU length were same. Fix ensures that the correct current DPPI is subscribed to by the radio subscribe. The implementation has been refactor to be able to use the current sw_tifs_toggle value in the HAL implementation. Signed-off-by: Vinayak Kariappa Chettimada --- .../ll_sw/nordic/hal/nrf5/radio/radio.c | 30 ++++- .../ll_sw/nordic/hal/nrf5/radio/radio.h | 2 + .../nordic/hal/nrf5/radio/radio_nrf5_dppi.h | 108 ++++++++++++------ .../nordic/hal/nrf5/radio/radio_nrf5_ppi.h | 76 ++++++------ .../controller/ll_sw/nordic/lll/lll_adv_iso.c | 2 +- 5 files changed, 142 insertions(+), 76 deletions(-) diff --git a/subsys/bluetooth/controller/ll_sw/nordic/hal/nrf5/radio/radio.c b/subsys/bluetooth/controller/ll_sw/nordic/hal/nrf5/radio/radio.c index 0b41a9bbfb6..a657ab95dfe 100644 --- a/subsys/bluetooth/controller/ll_sw/nordic/hal/nrf5/radio/radio.c +++ b/subsys/bluetooth/controller/ll_sw/nordic/hal/nrf5/radio/radio.c @@ -718,7 +718,7 @@ void sw_switch(uint8_t dir_curr, uint8_t dir_next, uint8_t phy_curr, uint8_t fla #endif /* CONFIG_BT_CTLR_DF_PHYEND_OFFSET_COMPENSATION_ENABLE */ uint32_t delay; - hal_radio_sw_switch_setup(cc, ppi, sw_tifs_toggle); + hal_radio_sw_switch_setup(sw_tifs_toggle); /* NOTE: As constants are passed to dir_curr and dir_next, the * compiler should optimize out the redundant code path @@ -736,7 +736,7 @@ void sw_switch(uint8_t dir_curr, uint8_t dir_next, uint8_t phy_curr, uint8_t fla hal_radio_tx_chain_delay_ns_get(phy_curr, flags_curr)); - hal_radio_b2b_txen_on_sw_switch(ppi); + hal_radio_b2b_txen_on_sw_switch(cc, ppi); } else { /* If RX PHY is LE Coded, calculate for S8 coding. * Assumption being, S8 has higher delay. @@ -746,7 +746,7 @@ void sw_switch(uint8_t dir_curr, uint8_t dir_next, uint8_t phy_curr, uint8_t fla flags_next) + hal_radio_rx_chain_delay_ns_get(phy_curr, 1)); - hal_radio_txen_on_sw_switch(ppi); + hal_radio_txen_on_sw_switch(cc, ppi); } #if defined(CONFIG_BT_CTLR_DF_PHYEND_OFFSET_COMPENSATION_ENABLE) @@ -839,7 +839,7 @@ void sw_switch(uint8_t dir_curr, uint8_t dir_next, uint8_t phy_curr, uint8_t fla flags_curr)) + (EVENT_CLOCK_JITTER_US << 1); - hal_radio_rxen_on_sw_switch(ppi); + hal_radio_rxen_on_sw_switch(cc, ppi); } else { delay = HAL_RADIO_NS2US_CEIL( hal_radio_rx_ready_delay_ns_get(phy_next, @@ -848,7 +848,7 @@ void sw_switch(uint8_t dir_curr, uint8_t dir_next, uint8_t phy_curr, uint8_t fla flags_curr)) + (EVENT_CLOCK_JITTER_US << 1); - hal_radio_b2b_rxen_on_sw_switch(ppi); + hal_radio_b2b_rxen_on_sw_switch(cc, ppi); } @@ -982,6 +982,26 @@ void radio_switch_complete_and_b2b_rx(uint8_t phy_curr, uint8_t flags_curr, #endif /* !CONFIG_BT_CTLR_TIFS_HW */ } +void radio_switch_complete_and_b2b_tx_disable(void) +{ +#if defined(CONFIG_BT_CTLR_TIFS_HW) + NRF_RADIO->SHORTS = (RADIO_SHORTS_READY_START_Msk | RADIO_SHORTS_END_DISABLE_Msk); +#else /* CONFIG_BT_CTLR_TIFS_HW */ + NRF_RADIO->SHORTS = (RADIO_SHORTS_READY_START_Msk | NRF_RADIO_SHORTS_PDU_END_DISABLE); + hal_radio_sw_switch_b2b_tx_disable(sw_tifs_toggle); +#endif /* !CONFIG_BT_CTLR_TIFS_HW */ +} + +void radio_switch_complete_and_b2b_rx_disable(void) +{ +#if defined(CONFIG_BT_CTLR_TIFS_HW) + NRF_RADIO->SHORTS = (RADIO_SHORTS_READY_START_Msk | RADIO_SHORTS_END_DISABLE_Msk); +#else /* CONFIG_BT_CTLR_TIFS_HW */ + NRF_RADIO->SHORTS = (RADIO_SHORTS_READY_START_Msk | NRF_RADIO_SHORTS_PDU_END_DISABLE); + hal_radio_sw_switch_b2b_rx_disable(sw_tifs_toggle); +#endif /* !CONFIG_BT_CTLR_TIFS_HW */ +} + void radio_switch_complete_and_disable(void) { #if defined(CONFIG_BT_CTLR_TIFS_HW) diff --git a/subsys/bluetooth/controller/ll_sw/nordic/hal/nrf5/radio/radio.h b/subsys/bluetooth/controller/ll_sw/nordic/hal/nrf5/radio/radio.h index 600d7e95481..4bf833b233a 100644 --- a/subsys/bluetooth/controller/ll_sw/nordic/hal/nrf5/radio/radio.h +++ b/subsys/bluetooth/controller/ll_sw/nordic/hal/nrf5/radio/radio.h @@ -106,6 +106,8 @@ void radio_switch_complete_and_b2b_tx(uint8_t phy_curr, uint8_t flags_curr, uint8_t phy_next, uint8_t flags_next); void radio_switch_complete_and_b2b_rx(uint8_t phy_curr, uint8_t flags_curr, uint8_t phy_next, uint8_t flags_next); +void radio_switch_complete_and_b2b_tx_disable(void); +void radio_switch_complete_and_b2b_rx_disable(void); void radio_switch_complete_and_disable(void); uint8_t radio_phy_flags_rx_get(void); diff --git a/subsys/bluetooth/controller/ll_sw/nordic/hal/nrf5/radio/radio_nrf5_dppi.h b/subsys/bluetooth/controller/ll_sw/nordic/hal/nrf5/radio/radio_nrf5_dppi.h index 787755866ab..1fdf7d90d30 100644 --- a/subsys/bluetooth/controller/ll_sw/nordic/hal/nrf5/radio/radio_nrf5_dppi.h +++ b/subsys/bluetooth/controller/ll_sw/nordic/hal/nrf5/radio/radio_nrf5_dppi.h @@ -455,10 +455,7 @@ static inline void hal_sw_switch_timer_clear_ppi_config(void) #endif /* CONFIG_BT_CTLR_DF_PHYEND_OFFSET_COMPENSATION_ENABLE */ -static inline void hal_radio_sw_switch_setup( - uint8_t compare_reg, - uint8_t radio_enable_ppi, - uint8_t ppi_group_index) +static inline void hal_radio_sw_switch_setup(uint8_t ppi_group_index) { /* Set up software switch mechanism for next Radio switch. */ @@ -466,7 +463,7 @@ static inline void hal_radio_sw_switch_setup( * over PPI[] */ HAL_SW_SWITCH_GROUP_TASK_ENABLE_PPI_REGISTER_EVT = - HAL_SW_SWITCH_GROUP_TASK_ENABLE_PPI_EVT; + HAL_SW_SWITCH_GROUP_TASK_ENABLE_PPI_EVT; nrf_dppi_subscribe_set(NRF_DPPIC, HAL_SW_DPPI_TASK_EN_FROM_IDX(SW_SWITCH_TIMER_TASK_GROUP(ppi_group_index)), HAL_SW_SWITCH_GROUP_TASK_ENABLE_PPI); @@ -476,55 +473,70 @@ static inline void hal_radio_sw_switch_setup( nrf_dppi_subscribe_clear(NRF_DPPIC, HAL_SW_DPPI_TASK_EN_FROM_IDX(SW_SWITCH_TIMER_TASK_GROUP(other_grp))); +} +static inline void hal_radio_txen_on_sw_switch(uint8_t compare_reg_index, uint8_t radio_enable_ppi) +{ /* Wire SW Switch timer event to the * PPI[] for enabling Radio. Do * not wire the task; it is done by the caller of * the function depending on the desired direction * (TX/RX). */ - HAL_SW_SWITCH_RADIO_ENABLE_PPI_REGISTER_EVT(compare_reg) = + HAL_SW_SWITCH_RADIO_ENABLE_PPI_REGISTER_EVT(compare_reg_index) = HAL_SW_SWITCH_RADIO_ENABLE_PPI_EVT(radio_enable_ppi); + + nrf_radio_subscribe_set(NRF_RADIO, NRF_RADIO_TASK_TXEN, radio_enable_ppi); } -static inline void hal_radio_txen_on_sw_switch(uint8_t ppi) +static inline void hal_radio_b2b_txen_on_sw_switch(uint8_t compare_reg_index, + uint8_t radio_enable_ppi) { - nrf_radio_subscribe_set(NRF_RADIO, NRF_RADIO_TASK_TXEN, ppi); -} - -static inline void hal_radio_b2b_txen_on_sw_switch(uint8_t ppi) -{ - /* NOTE: Calling radio_tmr_start/radio_tmr_start_us/radio_tmr_start_now - * after the radio_switch_complete_and_b2b_tx() call would have - * changed the PPI channel to HAL_RADIO_ENABLE_ON_TICK_PPI as we - * cannot double buffer the subscribe buffer. Hence, lets have - * both DPPI channel enabled (other one was enabled by the DPPI - * group when the Radio End occurred) so that when both timer - * trigger one of the DPPI is correct in the radio tx - * subscription. + /* Wire SW Switch timer event to the + * PPI[] for enabling Radio. Do + * not wire the task; it is done by the caller of + * the function depending on the desired direction + * (TX/RX). */ - nrf_radio_subscribe_set(NRF_RADIO, NRF_RADIO_TASK_TXEN, ppi); - nrf_dppi_channels_enable(NRF_DPPIC, BIT(ppi)); + HAL_SW_SWITCH_RADIO_ENABLE_PPI_REGISTER_EVT(compare_reg_index) = + HAL_SW_SWITCH_RADIO_ENABLE_PPI_EVT(radio_enable_ppi); + + uint8_t prev_ppi_idx = (compare_reg_index + 0x01) & 0x01; + + radio_enable_ppi = HAL_SW_SWITCH_RADIO_ENABLE_PPI(prev_ppi_idx); + nrf_radio_subscribe_set(NRF_RADIO, NRF_RADIO_TASK_TXEN, radio_enable_ppi); } -static inline void hal_radio_rxen_on_sw_switch(uint8_t ppi) +static inline void hal_radio_rxen_on_sw_switch(uint8_t compare_reg_index, uint8_t radio_enable_ppi) { - nrf_radio_subscribe_set(NRF_RADIO, NRF_RADIO_TASK_RXEN, ppi); -} - -static inline void hal_radio_b2b_rxen_on_sw_switch(uint8_t ppi) -{ - /* NOTE: Calling radio_tmr_start/radio_tmr_start_us/radio_tmr_start_now - * after the radio_switch_complete_and_b2b_rx() call would have - * changed the PPI channel to HAL_RADIO_ENABLE_ON_TICK_PPI as we - * cannot double buffer the subscribe buffer. Hence, lets have - * both DPPI channel enabled (other one was enabled by the DPPI - * group when the Radio End occurred) so that when both timer - * trigger one of the DPPI is correct in the radio rx - * subscription. + /* Wire SW Switch timer event to the + * PPI[] for enabling Radio. Do + * not wire the task; it is done by the caller of + * the function depending on the desired direction + * (TX/RX). */ - nrf_radio_subscribe_set(NRF_RADIO, NRF_RADIO_TASK_RXEN, ppi); - nrf_dppi_channels_enable(NRF_DPPIC, BIT(ppi)); + HAL_SW_SWITCH_RADIO_ENABLE_PPI_REGISTER_EVT(compare_reg_index) = + HAL_SW_SWITCH_RADIO_ENABLE_PPI_EVT(radio_enable_ppi); + + nrf_radio_subscribe_set(NRF_RADIO, NRF_RADIO_TASK_RXEN, radio_enable_ppi); +} + +static inline void hal_radio_b2b_rxen_on_sw_switch(uint8_t compare_reg_index, + uint8_t radio_enable_ppi) +{ + /* Wire SW Switch timer event to the + * PPI[] for enabling Radio. Do + * not wire the task; it is done by the caller of + * the function depending on the desired direction + * (TX/RX). + */ + HAL_SW_SWITCH_RADIO_ENABLE_PPI_REGISTER_EVT(compare_reg_index) = + HAL_SW_SWITCH_RADIO_ENABLE_PPI_EVT(radio_enable_ppi); + + uint8_t prev_ppi_idx = (compare_reg_index + 0x01) & 0x01; + + radio_enable_ppi = HAL_SW_SWITCH_RADIO_ENABLE_PPI(prev_ppi_idx); + nrf_radio_subscribe_set(NRF_RADIO, NRF_RADIO_TASK_RXEN, radio_enable_ppi); } static inline void hal_radio_sw_switch_disable(void) @@ -540,6 +552,26 @@ static inline void hal_radio_sw_switch_disable(void) HAL_SW_DPPI_TASK_EN_FROM_IDX(SW_SWITCH_TIMER_TASK_GROUP(1))); } +static inline void hal_radio_sw_switch_b2b_tx_disable(uint8_t compare_reg_index) +{ + hal_radio_sw_switch_disable(); + + uint8_t prev_ppi_idx = (compare_reg_index + 0x01) & 0x01; + uint8_t radio_enable_ppi = HAL_SW_SWITCH_RADIO_ENABLE_PPI(prev_ppi_idx); + + nrf_radio_subscribe_set(NRF_RADIO, NRF_RADIO_TASK_TXEN, radio_enable_ppi); +} + +static inline void hal_radio_sw_switch_b2b_rx_disable(uint8_t compare_reg_index) +{ + hal_radio_sw_switch_disable(); + + uint8_t prev_ppi_idx = (compare_reg_index + 0x01) & 0x01; + uint8_t radio_enable_ppi = HAL_SW_SWITCH_RADIO_ENABLE_PPI(prev_ppi_idx); + + nrf_radio_subscribe_set(NRF_RADIO, NRF_RADIO_TASK_RXEN, radio_enable_ppi); +} + static inline void hal_radio_sw_switch_cleanup(void) { hal_radio_sw_switch_disable(); diff --git a/subsys/bluetooth/controller/ll_sw/nordic/hal/nrf5/radio/radio_nrf5_ppi.h b/subsys/bluetooth/controller/ll_sw/nordic/hal/nrf5/radio/radio_nrf5_ppi.h index d964129bfa1..44cec4621a3 100644 --- a/subsys/bluetooth/controller/ll_sw/nordic/hal/nrf5/radio/radio_nrf5_ppi.h +++ b/subsys/bluetooth/controller/ll_sw/nordic/hal/nrf5/radio/radio_nrf5_ppi.h @@ -420,10 +420,7 @@ static inline void hal_sw_switch_timer_clear_ppi_config(void) #define HAL_SW_SWITCH_RADIO_ENABLE_PPI_TASK_RX \ ((uint32_t)&(NRF_RADIO->TASKS_RXEN)) -static inline void hal_radio_sw_switch_setup( - uint8_t compare_reg, - uint8_t radio_enable_ppi, - uint8_t ppi_group_index) +static inline void hal_radio_sw_switch_setup(uint8_t ppi_group_index) { /* Set up software switch mechanism for next Radio switch. */ @@ -435,53 +432,58 @@ static inline void hal_radio_sw_switch_setup( HAL_SW_SWITCH_GROUP_TASK_ENABLE_PPI, HAL_SW_SWITCH_GROUP_TASK_ENABLE_PPI_EVT, HAL_SW_SWITCH_GROUP_TASK_ENABLE_PPI_TASK(ppi_group_index)); +} +static inline void hal_radio_txen_on_sw_switch(uint8_t compare_reg_index, uint8_t radio_enable_ppi) +{ /* Wire SW Switch timer event to the * PPI[] for enabling Radio. Do * not wire the task; it is done by the caller of * the function depending on the desired direction * (TX/RX). */ - nrf_ppi_event_endpoint_setup( - NRF_PPI, - radio_enable_ppi, - HAL_SW_SWITCH_RADIO_ENABLE_PPI_EVT(compare_reg)); + nrf_ppi_event_endpoint_setup(NRF_PPI, radio_enable_ppi, + HAL_SW_SWITCH_RADIO_ENABLE_PPI_EVT(compare_reg_index)); + + nrf_ppi_task_endpoint_setup(NRF_PPI, radio_enable_ppi, + HAL_SW_SWITCH_RADIO_ENABLE_PPI_TASK_TX); } -static inline void hal_radio_txen_on_sw_switch(uint8_t ppi) -{ - nrf_ppi_task_endpoint_setup( - NRF_PPI, - ppi, - HAL_SW_SWITCH_RADIO_ENABLE_PPI_TASK_TX); -} - -static inline void hal_radio_b2b_txen_on_sw_switch(uint8_t ppi) +static inline void hal_radio_b2b_txen_on_sw_switch(uint8_t compare_reg_index, + uint8_t radio_enable_ppi) { /* NOTE: As independent PPI are used to trigger the Radio Tx task, * double buffers implementation works for sw_switch using PPIs, * simply reuse the hal_radio_txen_on_sw_switch() functon to set * the next PPIs task to be Radio Tx enable. */ - hal_radio_txen_on_sw_switch(ppi); + hal_radio_txen_on_sw_switch(compare_reg_index, radio_enable_ppi); } -static inline void hal_radio_rxen_on_sw_switch(uint8_t ppi) +static inline void hal_radio_rxen_on_sw_switch(uint8_t compare_reg_index, uint8_t radio_enable_ppi) { - nrf_ppi_task_endpoint_setup( - NRF_PPI, - ppi, - HAL_SW_SWITCH_RADIO_ENABLE_PPI_TASK_RX); -} - -static inline void hal_radio_b2b_rxen_on_sw_switch(uint8_t ppi) -{ - /* NOTE: As independent PPI are used to trigger the Radio Rx task, - * double buffers implementation works for sw_switch using PPIs, - * simply reuse the hal_radio_rxen_on_sw_switch() functon to set - * the next PPIs task to be Radio Rx enable. + /* Wire SW Switch timer event to the + * PPI[] for enabling Radio. Do + * not wire the task; it is done by the caller of + * the function depending on the desired direction + * (TX/RX). */ - hal_radio_rxen_on_sw_switch(ppi); + nrf_ppi_event_endpoint_setup(NRF_PPI, radio_enable_ppi, + HAL_SW_SWITCH_RADIO_ENABLE_PPI_EVT(compare_reg_index)); + + nrf_ppi_task_endpoint_setup(NRF_PPI, radio_enable_ppi, + HAL_SW_SWITCH_RADIO_ENABLE_PPI_TASK_RX); +} + +static inline void hal_radio_b2b_rxen_on_sw_switch(uint8_t compare_reg_index, + uint8_t radio_enable_ppi) +{ + /* NOTE: As independent PPI are used to trigger the Radio Tx task, + * double buffers implementation works for sw_switch using PPIs, + * simply reuse the hal_radio_txen_on_sw_switch() functon to set + * the next PPIs task to be Radio Tx enable. + */ + hal_radio_rxen_on_sw_switch(compare_reg_index, radio_enable_ppi); } static inline void hal_radio_sw_switch_disable(void) @@ -496,6 +498,16 @@ static inline void hal_radio_sw_switch_disable(void) BIT(HAL_SW_SWITCH_GROUP_TASK_ENABLE_PPI)); } +static inline void hal_radio_sw_switch_b2b_tx_disable(uint8_t compare_reg_index) +{ + hal_radio_sw_switch_disable(); +} + +static inline void hal_radio_sw_switch_b2b_rx_disable(uint8_t compare_reg_index) +{ + hal_radio_sw_switch_disable(); +} + static inline void hal_radio_sw_switch_cleanup(void) { hal_radio_sw_switch_disable(); diff --git a/subsys/bluetooth/controller/ll_sw/nordic/lll/lll_adv_iso.c b/subsys/bluetooth/controller/ll_sw/nordic/lll/lll_adv_iso.c index 87849d0e1fd..20e3fe528e1 100644 --- a/subsys/bluetooth/controller/ll_sw/nordic/lll/lll_adv_iso.c +++ b/subsys/bluetooth/controller/ll_sw/nordic/lll/lll_adv_iso.c @@ -717,7 +717,7 @@ static void isr_tx_common(void *param, pkt_flags); } - radio_switch_complete_and_disable(); + radio_switch_complete_and_b2b_tx_disable(); radio_isr_set(isr_done_term, lll); } else {