On 11/06/26 18:34, Jai Luthra wrote:

Hello Jai,

Hi Siddharth,

Thank you for the patch.

Quoting Siddharth Vadapalli (2026-06-11 17:36:50)
Currently, phy_config() is invoked every time during am65_cpsw_start()
which guarantees robustness and also allows recovering from link
failures by resetting the Ethernet PHY. While this is beneficial for
system stability, for use-cases where speed is prioritized over
robustness, the existing implementation is insufficient.

To support use-cases prioritizing speed while continuing to retain the
existing behavior for robustness, sample the environment variable named
"am65_cpsw_phy_config_once" to determine whether phy_config() should
continue to be invoked once per invocation of am65_cpsw_start(), OR, it
should only be invoked across invocations. Since the environment variable
is not set by default, robustness is prioritized over speed. Use-cases
which require speed can set "am65_cpsw_phy_config_once" to any of:
'1', 'y', 'Y', 't' and 'T'
based on the implementation of the env_get_yesno() helper function.

Signed-off-by: Siddharth Vadapalli <[email protected]>
---

Hello,

This patch is based on commit
3cdce049f90 erge tag 'u-boot-rockchip-20260610' of 
https://source.denx.de/u-boot/custodians/u-boot-rockchip
of the master branch of U-Boot.

Patch has been tested on J784S4 EVM which uses MCU CPSW2G for Ethernet
functionality. The following test logs demonstrate the default behavior
being retained in the absence of 'am65_cpsw_phy_config_once' environment
variable being set to a logical true, followed by setting the environment
variable to a logical true ('y') and observing the PHY Autonegotiation
process being skipped, followed by setting the environment variable to a
logical false ('n' but could be anything that isn't logically true based
on the env_get_yesno() helper function) and verifying that we go back to
the default behavior of performing PHY Autonegotiation again:
https://gist.github.com/Siddharth-Vadapalli-at-TI/6229b5a3b7d80cd0c2037a6364406bbe

Regards,
Siddharth.

  drivers/net/ti/am65-cpsw-nuss.c | 32 +++++++++++++++++++++++++++-----
  1 file changed, 27 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ti/am65-cpsw-nuss.c b/drivers/net/ti/am65-cpsw-nuss.c
index 84a3e44ecbc..a5cca52f674 100644
--- a/drivers/net/ti/am65-cpsw-nuss.c
+++ b/drivers/net/ti/am65-cpsw-nuss.c
@@ -18,6 +18,7 @@
  #include <dm/pinctrl.h>
  #include <dma-uclass.h>
  #include <dm/of_access.h>
+#include <env.h>
  #include <miiphy.h>
  #include <net.h>
  #include <phy.h>
@@ -104,6 +105,7 @@ struct am65_cpsw_port {
         fdt_addr_t      macsl_base;
         bool            disabled;
         u32             mac_control;
+       bool            phy_configured;
  };
struct am65_cpsw_common {
@@ -313,7 +315,7 @@ static int am65_cpsw_start(struct udevice *dev)
         struct am65_cpsw_port *port = &common->ports[priv->port_id];
         struct am65_cpsw_port *port0 = &common->ports[0];
         struct ti_udma_drv_chan_cfg_data *dma_rx_cfg_data;
-       int ret, i;
+       int ret, i, skip_phy_config_env;
if (common->started)
                 return 0;
@@ -426,10 +428,30 @@ static int am65_cpsw_start(struct udevice *dev)
                        port->port_sgmii_base + AM65_CPSW_SGMII_CONTROL_REG);
         }
- ret = phy_config(priv->phydev);
-       if (ret < 0) {
-               dev_err(dev, "phy_config failed: %d", ret);
-               goto err_dis_rx;
+       /*
+        * Invoking phy_config() every time that am65_cpsw_start() is executed 
will
+        * make the link robust. However, it delays every networking command 
such as
+        * 'dhcp' and 'tftp'. For use-cases that prioritize speed over 
robustness,
+        * phy_config() should be invoked only once. To support both use-cases,
+        * sample the environment variable 'am65_cpsw_phy_config_once' to switch
+        * between:
+        * a) Invoke phy_config() every time - Default behavior for robustness
+        * b) Invoke phy_config() once per driver probe - User configurable 
behavior
+        *    for speed.
+        */
+       skip_phy_config_env = env_get_yesno("am65_cpsw_phy_config_once");
+
+       /* If environment variable is not defined, assume it to be false. */
+       if (skip_phy_config_env == -1)
+               skip_phy_config_env = 0;
+
+       if (!port->phy_configured || !skip_phy_config_env) {
+               ret = phy_config(priv->phydev);
+               if (ret < 0) {
+                       dev_err(dev, "phy_config failed: %d", ret);
+                       goto err_dis_rx;
+               }
+               port->phy_configured = true;
         }

Tested-by: Jai Luthra <[email protected]> [on BeagleY-AI]

Thank you for testing the patch.


Although I was surprised there's no way to check the status of the phy link
through some controller bit that can maybe mark phy as unconfigured on next
attempt? Apologies if that's stupid, I don't understand networking =)
You do ask a good question. Maybe I should have clarified that in response to Nora's question.

The issue discovered during automation is that the PHY reports the Link being UP, but doesn't send any packets out. Based on the statistics in the MAC (Network Controller / CPSW), the packets are being sent from the MAC to the PHY, but the PHY isn't transmitting them on the Wire. At this stage, simply resetting the Ethernet PHY by writing to its registers using the mii command fixes the issue. Therefore, it seems to be a rare situation wherein the PHY ends up in a bad state and the Link Status alone isn't sufficient to figure out whether the PHY is working. Hence, resetting the PHY on every networking command and starting from scratch not only reduces the occurrences of the issue, but also helps us recover from the issue by rerunning the networking commands.

Since certain users may prefer speed over stability, or alternatively, since users may prefer speed by default with the option to recover on the same boot in case they end up in such a situation, using an environment variable seemed to be the most flexible approach (build-time configs will only provide speed or stability but won't allow the user to switch between them at runtime).

I hope this clarifies your question. Let me know otherwise.

Regards,
Siddharth.

Reply via email to