From ed304d5173449550dc6678a9540ab47420d8f7f4 Mon Sep 17 00:00:00 2001 From: Ondrej Kosta Date: Fri, 16 Feb 2024 12:34:01 +0100 Subject: [PATCH] fix(esp_eth): Fixed Ethernet link reset when Ethernet is stopped --- components/esp_eth/include/esp_eth_phy.h | 15 +++++- .../esp_eth/include/esp_eth_phy_802_3.h | 10 ++++ components/esp_eth/src/esp_eth.c | 17 +++++-- components/esp_eth/src/esp_eth_mac_esp.c | 2 + components/esp_eth/src/esp_eth_phy_802_3.c | 21 ++++++++ .../esp_eth/src/esp_eth_phy_ksz8851snl.c | 18 ++++++- components/esp_eth/src/esp_eth_phy_w5500.c | 18 ++++++- .../test_apps/main/esp_eth_test_apps.c | 51 ++++++++++++++----- 8 files changed, 132 insertions(+), 20 deletions(-) diff --git a/components/esp_eth/include/esp_eth_phy.h b/components/esp_eth/include/esp_eth_phy.h index ebba324f6b..07bcf8fb93 100644 --- a/components/esp_eth/include/esp_eth_phy.h +++ b/components/esp_eth/include/esp_eth_phy.h @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2019-2021 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2019-2024 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -128,6 +128,19 @@ struct esp_eth_phy_s { */ esp_err_t (*get_link)(esp_eth_phy_t *phy); + /** + * @brief Set Ethernet PHY link status + * + * @param[in] phy: Ethernet PHY instance + * @param[in] link new link status + * + * @return + * - ESP_OK: set Ethernet PHY link status successfully + * - ESP_FAIL: set Ethernet PHY link status failed because some error occurred + * + */ + esp_err_t (*set_link)(esp_eth_phy_t *phy, eth_link_t link); + /** * @brief Power control of Ethernet PHY * diff --git a/components/esp_eth/include/esp_eth_phy_802_3.h b/components/esp_eth/include/esp_eth_phy_802_3.h index 5eabdba53d..23bf075ab7 100644 --- a/components/esp_eth/include/esp_eth_phy_802_3.h +++ b/components/esp_eth/include/esp_eth_phy_802_3.h @@ -139,6 +139,16 @@ esp_err_t esp_eth_phy_802_3_set_speed(phy_802_3_t *phy_802_3, eth_speed_t speed) */ esp_err_t esp_eth_phy_802_3_set_duplex(phy_802_3_t *phy_802_3, eth_duplex_t duplex); +/** + * @brief Set Ethernet PHY link status + * + * @param phy_802_3 IEEE 802.3 PHY object infostructure + * @param link new link status + * @return + * - ESP_OK: Ethernet PHY link set successfuly + */ +esp_err_t esp_eth_phy_802_3_set_link(phy_802_3_t *phy_802_3, eth_link_t link); + /** * @brief Initialize Ethernet PHY * diff --git a/components/esp_eth/src/esp_eth.c b/components/esp_eth/src/esp_eth.c index 425a286a5b..2d36299267 100644 --- a/components/esp_eth/src/esp_eth.c +++ b/components/esp_eth/src/esp_eth.c @@ -303,6 +303,7 @@ esp_err_t esp_eth_stop(esp_eth_handle_t hdl) esp_err_t ret = ESP_OK; esp_eth_driver_t *eth_driver = (esp_eth_driver_t *)hdl; ESP_GOTO_ON_FALSE(eth_driver, ESP_ERR_INVALID_ARG, err, TAG, "ethernet driver handle can't be null"); + esp_eth_phy_t *phy = eth_driver->phy; esp_eth_mac_t *mac = eth_driver->mac; // check if driver has started esp_eth_fsm_t expected_fsm = ESP_ETH_FSM_START; @@ -311,11 +312,17 @@ esp_err_t esp_eth_stop(esp_eth_handle_t hdl) ESP_GOTO_ON_ERROR(esp_timer_stop(eth_driver->check_link_timer), err, TAG, "stop link timer 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"); + if (atomic_compare_exchange_strong(ð_driver->link, &expected_link, ETH_LINK_DOWN)){ + // to ensure backwards compatibility, check if set_link function is not NULL + if (phy->set_link != NULL) { + // MAC is stopped by setting link down at PHY layer + ESP_GOTO_ON_ERROR(phy->set_link(phy, ETH_LINK_DOWN), err, TAG, "ethernet phy reset link failed"); + } else { + // MAC is stopped by setting link down at MAC layer + 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), diff --git a/components/esp_eth/src/esp_eth_mac_esp.c b/components/esp_eth/src/esp_eth_mac_esp.c index d59e1e33b4..12901ce27e 100644 --- a/components/esp_eth/src/esp_eth_mac_esp.c +++ b/components/esp_eth/src/esp_eth_mac_esp.c @@ -155,10 +155,12 @@ static esp_err_t emac_esp32_set_link(esp_eth_mac_t *mac, eth_link_t link) case ETH_LINK_UP: ESP_GOTO_ON_ERROR(esp_intr_enable(emac->intr_hdl), err, TAG, "enable interrupt failed"); emac_esp32_start(mac); + ESP_LOGD(TAG, "emac started"); break; case ETH_LINK_DOWN: ESP_GOTO_ON_ERROR(esp_intr_disable(emac->intr_hdl), err, TAG, "disable interrupt failed"); emac_esp32_stop(mac); + ESP_LOGD(TAG, "emac stopped"); break; default: ESP_GOTO_ON_FALSE(false, ESP_ERR_INVALID_ARG, err, TAG, "unknown link status"); 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 785a0c0feb..91debb7876 100644 --- a/components/esp_eth/src/esp_eth_phy_802_3.c +++ b/components/esp_eth/src/esp_eth_phy_802_3.c @@ -87,6 +87,12 @@ static esp_err_t set_duplex(esp_eth_phy_t *phy, eth_duplex_t duplex) return esp_eth_phy_802_3_set_duplex(phy_802_3, duplex); } +static esp_err_t set_link(esp_eth_phy_t *phy, eth_link_t link) +{ + phy_802_3_t *phy_802_3 = esp_eth_phy_into_phy_802_3(phy); + return esp_eth_phy_802_3_set_link(phy_802_3, link); +} + static esp_err_t init(esp_eth_phy_t *phy) { phy_802_3_t *phy_802_3 = esp_eth_phy_into_phy_802_3(phy); @@ -344,6 +350,20 @@ err: return ret; } +esp_err_t esp_eth_phy_802_3_set_link(phy_802_3_t *phy_802_3, eth_link_t link) +{ + esp_err_t ret = ESP_OK; + esp_eth_mediator_t *eth = phy_802_3->eth; + + if (phy_802_3->link_status != link) { + phy_802_3->link_status = link; + // link status changed, inmiedately report to upper layers + ESP_GOTO_ON_ERROR(eth->on_state_changed(eth, ETH_STATE_LINK, (void *)phy_802_3->link_status), err, TAG, "change link failed"); + } +err: + return ret; +} + esp_err_t esp_eth_phy_802_3_init(phy_802_3_t *phy_802_3) { return esp_eth_phy_802_3_basic_phy_init(phy_802_3); @@ -495,6 +515,7 @@ esp_err_t esp_eth_phy_802_3_obj_config_init(phy_802_3_t *phy_802_3, const eth_ph phy_802_3->parent.set_speed = set_speed; phy_802_3->parent.set_duplex = set_duplex; phy_802_3->parent.del = del; + phy_802_3->parent.set_link = set_link; phy_802_3->parent.get_link = NULL; phy_802_3->parent.custom_ioctl = NULL; diff --git a/components/esp_eth/src/esp_eth_phy_ksz8851snl.c b/components/esp_eth/src/esp_eth_phy_ksz8851snl.c index 3739b63a0b..982c1c9b24 100644 --- a/components/esp_eth/src/esp_eth_phy_ksz8851snl.c +++ b/components/esp_eth/src/esp_eth_phy_ksz8851snl.c @@ -3,7 +3,7 @@ * * SPDX-License-Identifier: MIT * - * SPDX-FileContributor: 2021-2023 Espressif Systems (Shanghai) CO LTD + * SPDX-FileContributor: 2021-2024 Espressif Systems (Shanghai) CO LTD */ #include #include "esp_check.h" @@ -223,6 +223,21 @@ static esp_err_t phy_ksz8851_get_link(esp_eth_phy_t *phy) return ksz8851_update_link_duplex_speed(ksz8851); } +static esp_err_t phy_ksz8851_set_link(esp_eth_phy_t *phy, eth_link_t link) +{ + esp_err_t ret = ESP_OK; + phy_ksz8851snl_t *ksz8851 = __containerof(phy, phy_ksz8851snl_t, parent); + esp_eth_mediator_t *eth = ksz8851->eth; + + if (ksz8851->link_status != link) { + ksz8851->link_status = link; + // link status changed, inmiedately report to upper layers + ESP_GOTO_ON_ERROR(eth->on_state_changed(eth, ETH_STATE_LINK, (void *)ksz8851->link_status), err, TAG, "change link failed"); + } +err: + return ret; +} + static esp_err_t phy_ksz8851_set_addr(esp_eth_phy_t *phy, uint32_t addr) { phy_ksz8851snl_t *ksz8851 = __containerof(phy, phy_ksz8851snl_t, parent); @@ -361,6 +376,7 @@ esp_eth_phy_t *esp_eth_phy_new_ksz8851snl(const eth_phy_config_t *config) ksz8851->parent.deinit = phy_ksz8851_deinit; ksz8851->parent.autonego_ctrl = phy_ksz8851_autonego_ctrl; ksz8851->parent.get_link = phy_ksz8851_get_link; + ksz8851->parent.set_link = phy_ksz8851_set_link; ksz8851->parent.pwrctl = phy_ksz8851_pwrctl; ksz8851->parent.set_addr = phy_ksz8851_set_addr; ksz8851->parent.get_addr = phy_ksz8851_get_addr; diff --git a/components/esp_eth/src/esp_eth_phy_w5500.c b/components/esp_eth/src/esp_eth_phy_w5500.c index 3aea2cb0b1..d211681c1c 100644 --- a/components/esp_eth/src/esp_eth_phy_w5500.c +++ b/components/esp_eth/src/esp_eth_phy_w5500.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2020-2021 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2020-2024 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Apache-2.0 */ @@ -115,6 +115,21 @@ err: return ret; } +static esp_err_t w5500_set_link(esp_eth_phy_t *phy, eth_link_t link) +{ + esp_err_t ret = ESP_OK; + phy_w5500_t *w5500 = __containerof(phy, phy_w5500_t, parent); + esp_eth_mediator_t *eth = w5500->eth; + + if (w5500->link_status != link) { + w5500->link_status = link; + // link status changed, inmiedately report to upper layers + ESP_GOTO_ON_ERROR(eth->on_state_changed(eth, ETH_STATE_LINK, (void *)w5500->link_status), err, TAG, "change link failed"); + } +err: + return ret; +} + static esp_err_t w5500_reset(esp_eth_phy_t *phy) { esp_err_t ret = ESP_OK; @@ -362,6 +377,7 @@ esp_eth_phy_t *esp_eth_phy_new_w5500(const eth_phy_config_t *config) w5500->parent.set_mediator = w5500_set_mediator; w5500->parent.autonego_ctrl = w5500_autonego_ctrl; w5500->parent.get_link = w5500_get_link; + w5500->parent.set_link = w5500_set_link; w5500->parent.pwrctl = w5500_pwrctl; w5500->parent.get_addr = w5500_get_addr; w5500->parent.set_addr = w5500_set_addr; diff --git a/components/esp_eth/test_apps/main/esp_eth_test_apps.c b/components/esp_eth/test_apps/main/esp_eth_test_apps.c index 9a58b7b5fe..69eec14e3a 100644 --- a/components/esp_eth/test_apps/main/esp_eth_test_apps.c +++ b/components/esp_eth/test_apps/main/esp_eth_test_apps.c @@ -1,5 +1,5 @@ /* - * SPDX-FileCopyrightText: 2022-2023 Espressif Systems (Shanghai) CO LTD + * SPDX-FileCopyrightText: 2022-2024 Espressif Systems (Shanghai) CO LTD * * SPDX-License-Identifier: Unlicense OR CC0-1.0 */ @@ -552,17 +552,44 @@ TEST_CASE("ethernet start/stop stress test with IP stack", "[ethernet]") TEST_ESP_OK(esp_event_handler_register(ETH_EVENT, ESP_EVENT_ANY_ID, ð_event_handler, eth_event_group)); TEST_ESP_OK(esp_event_handler_register(IP_EVENT, IP_EVENT_ETH_GOT_IP, &got_ip_event_handler, eth_event_group)); - for (int i = 0; i < 10; i++) { - // start Ethernet driver - TEST_ESP_OK(esp_eth_start(eth_handle)); - /* wait for IP lease */ - bits = xEventGroupWaitBits(eth_event_group, ETH_GOT_IP_BIT, true, true, pdMS_TO_TICKS(ETH_GET_IP_TIMEOUT_MS)); - TEST_ASSERT((bits & ETH_GOT_IP_BIT) == ETH_GOT_IP_BIT); - // stop Ethernet driver - TEST_ESP_OK(esp_eth_stop(eth_handle)); - /* wait for connection stop */ - bits = xEventGroupWaitBits(eth_event_group, ETH_STOP_BIT, true, true, pdMS_TO_TICKS(ETH_STOP_TIMEOUT_MS)); - TEST_ASSERT((bits & ETH_STOP_BIT) == ETH_STOP_BIT); + for(int j = 0; j < 2; j++) { + // run the start/stop test with disabled auto-negotiation + if (j > 0) { + ESP_LOGI(TAG, "Run with auto-negotiation disabled..."); + bool auto_nego_en = false; + bool exp_autoneg_en; + TEST_ESP_OK(esp_eth_ioctl(eth_handle, ETH_CMD_S_AUTONEGO, &auto_nego_en)); + TEST_ESP_OK(esp_eth_ioctl(eth_handle, ETH_CMD_G_AUTONEGO, &exp_autoneg_en)); + TEST_ASSERT_EQUAL(false, exp_autoneg_en); +// *** LAN8720 deviation *** +// Rationale: When the device is in manual 100BASE-TX or 10BASE-T modes with Auto-MDIX enabled, the PHY does not link to a +// link partner that is configured for auto-negotiation. See LAN8720 errata for more details. +#ifdef CONFIG_TARGET_ETH_PHY_DEVICE_LAN8720 + esp_eth_phy_reg_rw_data_t reg; + uint32_t reg_val; + reg.reg_addr = 27; + reg.reg_value_p = ®_val; + TEST_ESP_OK(esp_eth_ioctl(eth_handle, ETH_CMD_READ_PHY_REG, ®)); + reg_val |= 0x8000; + TEST_ESP_OK(esp_eth_ioctl(eth_handle, ETH_CMD_WRITE_PHY_REG, ®)); + uint32_t reg_val_act; + reg.reg_value_p = ®_val_act; + TEST_ESP_OK(esp_eth_ioctl(eth_handle, ETH_CMD_READ_PHY_REG, ®)); + TEST_ASSERT_EQUAL(reg_val, reg_val_act); +#endif + } + for (int i = 0; i < 10; i++) { + // start Ethernet driver + TEST_ESP_OK(esp_eth_start(eth_handle)); + /* wait for IP lease */ + bits = xEventGroupWaitBits(eth_event_group, ETH_GOT_IP_BIT, true, true, pdMS_TO_TICKS(ETH_GET_IP_TIMEOUT_MS)); + TEST_ASSERT((bits & ETH_GOT_IP_BIT) == ETH_GOT_IP_BIT); + // stop Ethernet driver + TEST_ESP_OK(esp_eth_stop(eth_handle)); + /* wait for connection stop */ + bits = xEventGroupWaitBits(eth_event_group, ETH_STOP_BIT, true, true, pdMS_TO_TICKS(ETH_STOP_TIMEOUT_MS)); + TEST_ASSERT((bits & ETH_STOP_BIT) == ETH_STOP_BIT); + } } TEST_ESP_OK(esp_eth_del_netif_glue(glue));