Merge branch 'bugfix/eth_link_reset_v5.1' into 'release/v5.1'

fix(esp_eth): Fixed Ethernet link reset when Ethernet is stopped (v5.1)

See merge request espressif/esp-idf!29329

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
     *

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
  *

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;
@@ -312,10 +313,16 @@ esp_err_t esp_eth_stop(esp_eth_handle_t hdl)
 
     eth_link_t expected_link = ETH_LINK_UP;
     if (atomic_compare_exchange_strong(&eth_driver->link, &expected_link, ETH_LINK_DOWN)){
-        // MAC is stopped by setting link down
+        // to ensure backwards compatibility, check if set_link function is not NULL
-        ESP_GOTO_ON_ERROR(mac->set_link(mac, ETH_LINK_DOWN), err, TAG, "ethernet mac set link failed");
+        if (phy->set_link != NULL) {
-        ESP_GOTO_ON_ERROR(esp_event_post(ETH_EVENT, ETHERNET_EVENT_DISCONNECTED, &eth_driver, sizeof(esp_eth_driver_t *), 0), err,
+            // 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, &eth_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, &eth_driver, sizeof(esp_eth_driver_t *), 0),

components/esp_eth/src/esp_eth_mac_esp.c

@@ -156,10 +156,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");

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;
 

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 <stdlib.h>
 #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;

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;

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, &eth_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++) {
+    for(int j = 0; j < 2; j++) {
-        // start Ethernet driver
+        // run the start/stop test with disabled auto-negotiation
-        TEST_ESP_OK(esp_eth_start(eth_handle));
+        if (j > 0) {
-        /* wait for IP lease */
+            ESP_LOGI(TAG, "Run with auto-negotiation disabled...");
-        bits = xEventGroupWaitBits(eth_event_group, ETH_GOT_IP_BIT, true, true, pdMS_TO_TICKS(ETH_GET_IP_TIMEOUT_MS));
+            bool auto_nego_en = false;
-        TEST_ASSERT((bits & ETH_GOT_IP_BIT) == ETH_GOT_IP_BIT);
+            bool exp_autoneg_en;
-        // stop Ethernet driver
+            TEST_ESP_OK(esp_eth_ioctl(eth_handle, ETH_CMD_S_AUTONEGO, &auto_nego_en));
-        TEST_ESP_OK(esp_eth_stop(eth_handle));
+            TEST_ESP_OK(esp_eth_ioctl(eth_handle, ETH_CMD_G_AUTONEGO, &exp_autoneg_en));
-        /* wait for connection stop */
+            TEST_ASSERT_EQUAL(false, exp_autoneg_en);
-        bits = xEventGroupWaitBits(eth_event_group, ETH_STOP_BIT, true, true, pdMS_TO_TICKS(ETH_STOP_TIMEOUT_MS));
+// *** LAN8720 deviation ***
-        TEST_ASSERT((bits & ETH_STOP_BIT) == ETH_STOP_BIT);
+// 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 = &reg_val;
+            TEST_ESP_OK(esp_eth_ioctl(eth_handle, ETH_CMD_READ_PHY_REG, &reg));
+            reg_val |= 0x8000;
+            TEST_ESP_OK(esp_eth_ioctl(eth_handle, ETH_CMD_WRITE_PHY_REG, &reg));
+            uint32_t reg_val_act;
+            reg.reg_value_p = &reg_val_act;
+            TEST_ESP_OK(esp_eth_ioctl(eth_handle, ETH_CMD_READ_PHY_REG, &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));