From 256d4579695a67d5fabaa3b678da46c59757790e Mon Sep 17 00:00:00 2001 From: Ondrej Kosta Date: Mon, 30 Jan 2023 09:28:44 +0100 Subject: [PATCH] esp_eth: possible start/stop issue fixed ESP32 EMAC could hang when stopped/started multiple times at 10Mbps speed mode --- components/esp_eth/src/esp_eth.c | 19 +++++++--- components/esp_eth/src/esp_eth_mac_esp.c | 6 +-- components/esp_eth/src/esp_eth_phy_802_3.c | 20 ++++------ .../esp_eth/src/esp_eth_phy_ksz8851snl.c | 18 +++------ components/esp_eth/src/esp_eth_phy_w5500.c | 16 ++------ components/hal/emac_hal.c | 38 +++++++++++++------ components/hal/esp32/include/hal/emac_ll.h | 5 +++ .../eth_enc28j60/esp_eth_phy_enc28j60.c | 8 +--- 8 files changed, 65 insertions(+), 65 deletions(-) diff --git a/components/esp_eth/src/esp_eth.c b/components/esp_eth/src/esp_eth.c index 566c96b1c5..b11af704ff 100644 --- a/components/esp_eth/src/esp_eth.c +++ b/components/esp_eth/src/esp_eth.c @@ -50,7 +50,7 @@ typedef struct { bool auto_nego_en; eth_speed_t speed; eth_duplex_t duplex; - eth_link_t link; + _Atomic eth_link_t link; atomic_int ref_count; void *priv; _Atomic esp_eth_fsm_t fsm; @@ -129,7 +129,7 @@ static esp_err_t eth_on_state_changed(esp_eth_mediator_t *eth, esp_eth_state_t s case ETH_STATE_LINK: { eth_link_t link = (eth_link_t)args; ESP_GOTO_ON_ERROR(mac->set_link(mac, link), err, TAG, "ethernet mac set link failed"); - eth_driver->link = link; + atomic_store(ð_driver->link, link); if (link == ETH_LINK_UP) { ESP_GOTO_ON_ERROR(esp_event_post(ETH_EVENT, ETHERNET_EVENT_CONNECTED, ð_driver, sizeof(esp_eth_driver_t *), 0), err, TAG, "send ETHERNET_EVENT_CONNECTED event failed"); @@ -205,7 +205,7 @@ esp_err_t esp_eth_driver_install(const esp_eth_config_t *config, esp_eth_handle_ atomic_init(ð_driver->fsm, ESP_ETH_FSM_STOP); eth_driver->mac = mac; eth_driver->phy = phy; - eth_driver->link = ETH_LINK_DOWN; + atomic_init(ð_driver->link, ETH_LINK_DOWN); eth_driver->duplex = ETH_DUPLEX_HALF; eth_driver->speed = ETH_SPEED_10M; eth_driver->stack_input = config->stack_input; @@ -309,7 +309,14 @@ esp_err_t esp_eth_stop(esp_eth_handle_t hdl) ESP_GOTO_ON_FALSE(atomic_compare_exchange_strong(ð_driver->fsm, &expected_fsm, ESP_ETH_FSM_STOP), ESP_ERR_INVALID_STATE, err, TAG, "driver not started yet"); ESP_GOTO_ON_ERROR(esp_timer_stop(eth_driver->check_link_timer), err, TAG, "stop link timer failed"); - ESP_GOTO_ON_ERROR(mac->stop(mac), err, TAG, "stop mac failed"); + + eth_link_t expected_link = ETH_LINK_UP; + if (atomic_compare_exchange_strong(ð_driver->link, &expected_link, ETH_LINK_DOWN)){ + // MAC is stopped by setting link down + ESP_GOTO_ON_ERROR(mac->set_link(mac, ETH_LINK_DOWN), err, TAG, "ethernet mac set link failed"); + ESP_GOTO_ON_ERROR(esp_event_post(ETH_EVENT, ETHERNET_EVENT_DISCONNECTED, ð_driver, sizeof(esp_eth_driver_t *), 0), err, + TAG, "send ETHERNET_EVENT_DISCONNECTED event failed"); + } ESP_GOTO_ON_ERROR(esp_event_post(ETH_EVENT, ETHERNET_EVENT_STOP, ð_driver, sizeof(esp_eth_driver_t *), 0), err, TAG, "send ETHERNET_EVENT_STOP event failed"); @@ -336,9 +343,9 @@ esp_err_t esp_eth_transmit(esp_eth_handle_t hdl, void *buf, size_t length) esp_err_t ret = ESP_OK; esp_eth_driver_t *eth_driver = (esp_eth_driver_t *)hdl; - if (atomic_load(ð_driver->fsm) != ESP_ETH_FSM_START) { + if (atomic_load(ð_driver->link) != ETH_LINK_UP) { ret = ESP_ERR_INVALID_STATE; - ESP_LOGD(TAG, "Ethernet is not started"); + ESP_LOGD(TAG, "Ethernet link is not up, can't transmit"); goto err; } diff --git a/components/esp_eth/src/esp_eth_mac_esp.c b/components/esp_eth/src/esp_eth_mac_esp.c index 4e5735077e..a1dd3c157f 100644 --- a/components/esp_eth/src/esp_eth_mac_esp.c +++ b/components/esp_eth/src/esp_eth_mac_esp.c @@ -34,7 +34,7 @@ static const char *TAG = "esp.emac"; #define PHY_OPERATION_TIMEOUT_US (1000) -#define MAC_STOP_TIMEOUT_US (250) +#define MAC_STOP_TIMEOUT_US (2500) // this is absolute maximum for 10Mbps, it is 10 times faster for 100Mbps #define FLOW_CONTROL_LOW_WATER_MARK (CONFIG_ETH_DMA_RX_BUFFER_NUM / 3) #define FLOW_CONTROL_HIGH_WATER_MARK (FLOW_CONTROL_LOW_WATER_MARK * 2) @@ -262,7 +262,6 @@ static esp_err_t emac_esp32_receive(esp_eth_mac_t *mac, uint8_t *buf, uint32_t * ESP_GOTO_ON_FALSE(buf && length, ESP_ERR_INVALID_ARG, err, TAG, "can't set buf and length to null"); uint32_t receive_len = emac_hal_receive_frame(&emac->hal, buf, expected_len, &emac->frames_remain, &emac->free_rx_descriptor); /* we need to check the return value in case the buffer size is not enough */ - ESP_LOGD(TAG, "receive len= %d", receive_len); ESP_GOTO_ON_FALSE(expected_len >= receive_len, ESP_ERR_INVALID_SIZE, err, TAG, "received buffer longer than expected"); *length = receive_len; return ESP_OK; @@ -370,8 +369,6 @@ static esp_err_t emac_esp32_init(esp_eth_mac_t *mac) ESP_GOTO_ON_FALSE(to < emac->sw_reset_timeout_ms / 10, ESP_ERR_TIMEOUT, err, TAG, "reset timeout"); /* set smi clock */ emac_hal_set_csr_clock_range(&emac->hal, esp_clk_apb_freq()); - /* reset descriptor chain */ - emac_hal_reset_desc_chain(&emac->hal); /* init mac registers by default */ emac_hal_init_mac_default(&emac->hal); /* init dma registers with selected EMAC-DMA configuration */ @@ -406,6 +403,7 @@ static esp_err_t emac_esp32_deinit(esp_eth_mac_t *mac) static esp_err_t emac_esp32_start(esp_eth_mac_t *mac) { emac_esp32_t *emac = __containerof(mac, emac_esp32_t, parent); + /* reset descriptor chain */ emac_hal_reset_desc_chain(&emac->hal); emac_hal_start(&emac->hal); return ESP_OK; diff --git a/components/esp_eth/src/esp_eth_phy_802_3.c b/components/esp_eth/src/esp_eth_phy_802_3.c index 5a315451f4..b9725ae337 100644 --- a/components/esp_eth/src/esp_eth_phy_802_3.c +++ b/components/esp_eth/src/esp_eth_phy_802_3.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2022 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2022-2023 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -230,12 +230,9 @@ static esp_err_t eth_phy_802_3_set_speed(esp_eth_phy_t *phy, eth_speed_t speed) phy_802_3_t *phy_802_3 = __containerof(phy, phy_802_3_t, parent); esp_eth_mediator_t *eth = phy_802_3->eth; - if (phy_802_3->link_status == ETH_LINK_UP) { - /* Since the link is going to be reconfigured, consider it down for a while */ - phy_802_3->link_status = ETH_LINK_DOWN; - /* Indicate to upper stream apps the link is cosidered down */ - ESP_GOTO_ON_ERROR(eth->on_state_changed(eth, ETH_STATE_LINK, (void *)phy_802_3->link_status), err, TAG, "change link failed"); - } + /* Since the link is going to be reconfigured, consider it down to be status updated once the driver re-started */ + phy_802_3->link_status = ETH_LINK_DOWN; + /* Set speed */ bmcr_reg_t bmcr; ESP_GOTO_ON_ERROR(eth->phy_reg_read(eth, phy_802_3->addr, ETH_PHY_BMCR_REG_ADDR, &(bmcr.val)), err, TAG, "read BMCR failed"); @@ -253,12 +250,9 @@ static esp_err_t eth_phy_802_3_set_duplex(esp_eth_phy_t *phy, eth_duplex_t duple phy_802_3_t *phy_802_3 = __containerof(phy, phy_802_3_t, parent); esp_eth_mediator_t *eth = phy_802_3->eth; - if (phy_802_3->link_status == ETH_LINK_UP) { - /* Since the link is going to be reconfigured, consider it down for a while */ - phy_802_3->link_status = ETH_LINK_DOWN; - /* Indicate to upper stream apps the link is cosidered down */ - ESP_GOTO_ON_ERROR(eth->on_state_changed(eth, ETH_STATE_LINK, (void *)phy_802_3->link_status), err, TAG, "change link failed"); - } + /* Since the link is going to be reconfigured, consider it down to be status updated once the driver re-started */ + phy_802_3->link_status = ETH_LINK_DOWN; + /* Set duplex mode */ bmcr_reg_t bmcr; ESP_GOTO_ON_ERROR(eth->phy_reg_read(eth, phy_802_3->addr, ETH_PHY_BMCR_REG_ADDR, &(bmcr.val)), err, TAG, "read BMCR failed"); diff --git a/components/esp_eth/src/esp_eth_phy_ksz8851snl.c b/components/esp_eth/src/esp_eth_phy_ksz8851snl.c index 066a6c7db9..52446d9b21 100644 --- a/components/esp_eth/src/esp_eth_phy_ksz8851snl.c +++ b/components/esp_eth/src/esp_eth_phy_ksz8851snl.c @@ -288,12 +288,9 @@ static esp_err_t phy_ksz8851_set_speed(esp_eth_phy_t *phy, eth_speed_t speed) phy_ksz8851snl_t *ksz8851 = __containerof(phy, phy_ksz8851snl_t, parent); esp_eth_mediator_t *eth = ksz8851->eth; - if (ksz8851->link_status == ETH_LINK_UP) { - /* Since the link is going to be reconfigured, consider it down for a while */ - ksz8851->link_status = ETH_LINK_DOWN; - /* Indicate to upper stream apps the link is cosidered down */ - ESP_GOTO_ON_ERROR(eth->on_state_changed(eth, ETH_STATE_LINK, (void *)ksz8851->link_status), err, TAG, "change link failed"); - } + /* Since the link is going to be reconfigured, consider it down to be status updated once the driver re-started */ + ksz8851->link_status = ETH_LINK_DOWN; + /* Set speed */ uint32_t control; ESP_GOTO_ON_ERROR(eth->phy_reg_read(eth, ksz8851->addr, KSZ8851_P1CR, &control), err, TAG, "P1CR read failed"); @@ -315,12 +312,9 @@ static esp_err_t phy_ksz8851_set_duplex(esp_eth_phy_t *phy, eth_duplex_t duplex) phy_ksz8851snl_t *ksz8851 = __containerof(phy, phy_ksz8851snl_t, parent); esp_eth_mediator_t *eth = ksz8851->eth; - if (ksz8851->link_status == ETH_LINK_UP) { - /* Since the link is going to be reconfigured, consider it down for a while */ - ksz8851->link_status = ETH_LINK_DOWN; - /* Indicate to upper stream apps the link is cosidered down */ - ESP_GOTO_ON_ERROR(eth->on_state_changed(eth, ETH_STATE_LINK, (void *)ksz8851->link_status), err, TAG, "change link failed"); - } + /* Since the link is going to be reconfigured, consider it down to be status updated once the driver re-started */ + ksz8851->link_status = ETH_LINK_DOWN; + /* Set duplex mode */ uint32_t control; ESP_GOTO_ON_ERROR(eth->phy_reg_read(eth, ksz8851->addr, KSZ8851_P1CR, &control), err, TAG, "P1CR read failed"); diff --git a/components/esp_eth/src/esp_eth_phy_w5500.c b/components/esp_eth/src/esp_eth_phy_w5500.c index 79a4c951ce..3aea2cb0b1 100644 --- a/components/esp_eth/src/esp_eth_phy_w5500.c +++ b/components/esp_eth/src/esp_eth_phy_w5500.c @@ -259,12 +259,8 @@ static esp_err_t w5500_set_speed(esp_eth_phy_t *phy, eth_speed_t speed) phy_w5500_t *w5500 = __containerof(phy, phy_w5500_t, parent); esp_eth_mediator_t *eth = w5500->eth; - if (w5500->link_status == ETH_LINK_UP) { - /* Since the link is going to be reconfigured, consider it down for a while */ - w5500->link_status = ETH_LINK_DOWN; - /* Indicate to upper stream apps the link is cosidered down */ - ESP_GOTO_ON_ERROR(eth->on_state_changed(eth, ETH_STATE_LINK, (void *)w5500->link_status), err, TAG, "change link failed"); - } + /* Since the link is going to be reconfigured, consider it down to be status updated once the driver re-started */ + w5500->link_status = ETH_LINK_DOWN; phycfg_reg_t phycfg; ESP_GOTO_ON_ERROR(eth->phy_reg_read(eth, w5500->addr, W5500_REG_PHYCFGR, (uint32_t *) & (phycfg.val)), err, TAG, "read PHYCFG failed"); @@ -298,12 +294,8 @@ static esp_err_t w5500_set_duplex(esp_eth_phy_t *phy, eth_duplex_t duplex) phy_w5500_t *w5500 = __containerof(phy, phy_w5500_t, parent); esp_eth_mediator_t *eth = w5500->eth; - if (w5500->link_status == ETH_LINK_UP) { - /* Since the link is going to be reconfigured, consider it down for a while */ - w5500->link_status = ETH_LINK_DOWN; - /* Indicate to upper stream apps the link is cosidered down */ - ESP_GOTO_ON_ERROR(eth->on_state_changed(eth, ETH_STATE_LINK, (void *)w5500->link_status), err, TAG, "change link failed"); - } + /* Since the link is going to be reconfigured, consider it down to be status updated once the driver re-started */ + w5500->link_status = ETH_LINK_DOWN; phycfg_reg_t phycfg; ESP_GOTO_ON_ERROR(eth->phy_reg_read(eth, w5500->addr, W5500_REG_PHYCFGR, (uint32_t *) & (phycfg.val)), err, TAG, "read PHYCFG failed"); diff --git a/components/hal/emac_hal.c b/components/hal/emac_hal.c index 95ca5f0dfe..d5497b66d6 100644 --- a/components/hal/emac_hal.c +++ b/components/hal/emac_hal.c @@ -23,6 +23,18 @@ typedef struct { uint32_t copy_len; }__attribute__((packed)) emac_hal_auto_buf_info_t; +static esp_err_t emac_hal_flush_trans_fifo(emac_hal_context_t *hal) +{ + emac_ll_flush_trans_fifo_enable(hal->dma_regs, true); + /* no other writes to the Operation Mode register until the flush tx fifo bit is cleared */ + for (uint32_t i = 0; i < 1000; i++) { + if (emac_ll_get_flush_trans_fifo(hal->dma_regs) == 0) { + return ESP_OK; + } + } + return ESP_ERR_TIMEOUT; +} + void emac_hal_iomux_init_mii(void) { /* TX_CLK to GPIO0 */ @@ -288,7 +300,7 @@ void emac_hal_init_dma_default(emac_hal_context_t *hal, emac_hal_dma_config_t *h /* Disable Transmit Store Forward */ emac_ll_trans_store_forward_enable(hal->dma_regs, false); /* Flush Transmit FIFO */ - emac_ll_flush_trans_fifo_enable(hal->dma_regs, true); + emac_hal_flush_trans_fifo(hal); /* Transmit Threshold Control */ emac_ll_set_transmit_threshold(hal->dma_regs, EMAC_LL_TRANSMIT_THRESHOLD_CONTROL_64); /* Disable Forward Error Frame */ @@ -344,22 +356,21 @@ void emac_hal_start(emac_hal_context_t *hal) { /* Enable Ethernet MAC and DMA Interrupt */ emac_ll_enable_corresponding_intr(hal->dma_regs, EMAC_LL_CONFIG_ENABLE_INTR_MASK); - - /* Flush Transmit FIFO */ - emac_ll_flush_trans_fifo_enable(hal->dma_regs, true); - - /* Start DMA transmission */ - emac_ll_start_stop_dma_transmit(hal->dma_regs, true); - /* Start DMA reception */ - emac_ll_start_stop_dma_receive(hal->dma_regs, true); + /* Clear all pending interrupts */ + emac_ll_clear_all_pending_intr(hal->dma_regs); /* Enable transmit state machine of the MAC for transmission on the MII */ emac_ll_transmit_enable(hal->mac_regs, true); + /* Start DMA transmission */ + /* Note that the EMAC Databook states the DMA could be started prior enabling + the MAC transmitter. However, it turned out that such order may cause the MAC + transmitter hangs */ + emac_ll_start_stop_dma_transmit(hal->dma_regs, true); + + /* Start DMA reception */ + emac_ll_start_stop_dma_receive(hal->dma_regs, true); /* Enable receive state machine of the MAC for reception from the MII */ emac_ll_receive_enable(hal->mac_regs, true); - - /* Clear all pending interrupts */ - emac_ll_clear_all_pending_intr(hal->dma_regs); } esp_err_t emac_hal_stop(emac_hal_context_t *hal) @@ -385,6 +396,9 @@ esp_err_t emac_hal_stop(emac_hal_context_t *hal) /* Stop DMA reception */ emac_ll_start_stop_dma_receive(hal->dma_regs, false); + /* Flush Transmit FIFO */ + emac_hal_flush_trans_fifo(hal); + /* Disable Ethernet MAC and DMA Interrupt */ emac_ll_disable_all_intr(hal->dma_regs); diff --git a/components/hal/esp32/include/hal/emac_ll.h b/components/hal/esp32/include/hal/emac_ll.h index dc3817f20f..a93e44bdd6 100644 --- a/components/hal/esp32/include/hal/emac_ll.h +++ b/components/hal/esp32/include/hal/emac_ll.h @@ -417,6 +417,11 @@ static inline void emac_ll_flush_trans_fifo_enable(emac_dma_dev_t *dma_regs, boo dma_regs->dmaoperation_mode.flush_tx_fifo = enable; } +static inline bool emac_ll_get_flush_trans_fifo(emac_dma_dev_t *dma_regs) +{ + return dma_regs->dmaoperation_mode.flush_tx_fifo; +} + static inline void emac_ll_set_transmit_threshold(emac_dma_dev_t *dma_regs, uint32_t threshold) { dma_regs->dmaoperation_mode.tx_thresh_ctrl = threshold; diff --git a/examples/ethernet/enc28j60/components/eth_enc28j60/esp_eth_phy_enc28j60.c b/examples/ethernet/enc28j60/components/eth_enc28j60/esp_eth_phy_enc28j60.c index d6b30046ca..94c7f68b08 100644 --- a/examples/ethernet/enc28j60/components/eth_enc28j60/esp_eth_phy_enc28j60.c +++ b/examples/ethernet/enc28j60/components/eth_enc28j60/esp_eth_phy_enc28j60.c @@ -224,12 +224,8 @@ esp_err_t enc28j60_set_duplex(esp_eth_phy_t *phy, eth_duplex_t duplex) esp_eth_mediator_t *eth = enc28j60->eth; phcon1_reg_t phcon1; - if (enc28j60->link_status == ETH_LINK_UP) { - /* Since the link is going to be reconfigured, consider it down for a while */ - enc28j60->link_status = ETH_LINK_DOWN; - /* Indicate to upper stream apps the link is cosidered down */ - PHY_CHECK(eth->on_state_changed(eth, ETH_STATE_LINK, (void *)enc28j60->link_status), "change link failed", err); - } + /* Since the link is going to be reconfigured, consider it down to be status updated once the driver re-started */ + enc28j60->link_status = ETH_LINK_DOWN; PHY_CHECK(eth->phy_reg_read(eth, enc28j60->addr, 0, &phcon1.val) == ESP_OK, "read PHCON1 failed", err);