Hi Patrick,
-----Original Message----- From: Patrick DELAUNAY patrick.delaunay@foss.st.com Sent: 2021年11月15日 18:42 To: Joakim Zhang qiangqing.zhang@nxp.com; joe.hershberger@ni.com; rfried.dev@gmail.com Cc: u-boot@lists.denx.de; Ye Li ye.li@nxp.com; daniil.stas@posteo.net; swarren@nvidia.com; Christophe ROULLIER christophe.roullier@foss.st.com; Marek Vasut marex@denx.de Subject: Re: [PATCH] net: eqos: connect and config PHY from probe stage instead of starting EQOS
Hi,
On 11/10/21 6:42 AM, Joakim Zhang wrote:
For EQOS ethernet, it will do phy_connect() and phy_config() when start the ethernet (eqos_srart()), users need wait seconds for PHY auto negotiation
s/eqos_srart()/eqos_start()/
to complete when do tftp boot. phy_config() -> board_phy_config() -> phydev->drv->config() // i.MX8MP/DXL use
realtek PHY
-> rtl8211f_config() -> genphy_config_aneg() ->genphy_restart_aneg()
// restart auto-nego, then need seconds to complete
To avoid wasting time, we can move PHY connection and configuration from eqos_start() to eqos_probe(). This patch also moves clock setting from eqos_start() to eqos_probe() as MDIO access need CSR clock, there is no function change.
Tested-on: i.MX8MP & i.MX8DXL
Before: u-boot=> run netboot Booting from net ... ethernet@30bf0000 Waiting for PHY auto negotiation to complete....... done BOOTP broadcast 1 DHCP client bound to address 10.193.102.192 (313 ms) Using ethernet@30bf0000 device TFTP from server 10.193.108.176; our IP address is 10.193.102.192; sending through gateway 10.193.102.254 After: u-boot=> run netboot Booting from net ... BOOTP broadcast 1 DHCP client bound to address 10.193.102.192 (454 ms) Using ethernet@30bf0000 device TFTP from server 10.193.108.176; our IP address is 10.193.102.192; sending through gateway 10.193.102.254
Signed-off-by: Joakim Zhang qiangqing.zhang@nxp.com
drivers/net/dwc_eth_qos.c | 132
+++++++++++++++++++-------------------
1 file changed, 67 insertions(+), 65 deletions(-)
diff --git a/drivers/net/dwc_eth_qos.c b/drivers/net/dwc_eth_qos.c index 585101804d..c1923fbe6b 100644 --- a/drivers/net/dwc_eth_qos.c +++ b/drivers/net/dwc_eth_qos.c @@ -1045,20 +1045,6 @@ static int eqos_start(struct udevice *dev) eqos->tx_desc_idx = 0; eqos->rx_desc_idx = 0;
- ret = eqos->config->ops->eqos_start_clks(dev);
- if (ret < 0) {
pr_err("eqos_start_clks() failed: %d", ret);goto err;- }
- ret = eqos->config->ops->eqos_start_resets(dev);
- if (ret < 0) {
pr_err("eqos_start_resets() failed: %d", ret);goto err_stop_clks;- }
- udelay(10);
- eqos->reg_access_ok = true;
=> as clock is moved in probe...
the line eqos->reg_access_ok = true should be also moved I think
or reg_access_ok can be removed, as reg_access_ok is always one when eqos_write_hwaddr is called
Why? I saw this variable "reg_access_ok " doesn't have initialize value. If remove this line, I think it will break the logic in eqos_write_hwaddr(), so I agree also move it into probe().
ret = wait_for_bit_le32(&eqos->dma_regs->mode, @@ -1066,68 +1052,34 @@ static int eqos_start(struct udevice *dev) eqos->config->swr_wait, false); if (ret) { pr_err("EQOS_DMA_MODE_SWR stuck");
goto err_stop_resets;
goto err;}
ret = eqos->config->ops->eqos_calibrate_pads(dev); if (ret < 0) { pr_err("eqos_calibrate_pads() failed: %d", ret);
goto err_stop_resets;
goto err;} rate = eqos->config->ops->eqos_get_tick_clk_rate(dev);
val = (rate / 1000000) - 1; writel(val, &eqos->mac_regs->us_tic_counter);
- /*
* if PHY was already connected and configured,* don't need to reconnect/reconfigure again*/- if (!eqos->phy) {
int addr = -1;-#ifdef CONFIG_DM_ETH_PHY
addr = eth_phy_get_addr(dev);-#endif -#ifdef DWC_NET_PHYADDR
addr = DWC_NET_PHYADDR;-#endif
eqos->phy = phy_connect(eqos->mii, addr, dev,eqos->config->interface(dev));if (!eqos->phy) {pr_err("phy_connect() failed");goto err_stop_resets;}if (eqos->max_speed) {ret = phy_set_supported(eqos->phy, eqos->max_speed);if (ret) {pr_err("phy_set_supported() failed: %d", ret);goto err_shutdown_phy;}}ret = phy_config(eqos->phy);if (ret < 0) {pr_err("phy_config() failed: %d", ret);goto err_shutdown_phy;}- }
- ret = phy_startup(eqos->phy); if (ret < 0) { pr_err("phy_startup() failed: %d", ret);
goto err_shutdown_phy;
goto err;}
if (!eqos->phy->link) { pr_err("No link");
goto err_shutdown_phy;
goto err;}
ret = eqos_adjust_link(dev); if (ret < 0) { pr_err("eqos_adjust_link() failed: %d", ret);
goto err_shutdown_phy;
goto err;}
/* Configure MTL */
@@ -1356,12 +1308,6 @@ static int eqos_start(struct udevice *dev) debug("%s: OK\n", __func__); return 0;
-err_shutdown_phy:
- phy_shutdown(eqos->phy);
-err_stop_resets:
- eqos->config->ops->eqos_stop_resets(dev);
-err_stop_clks:
- eqos->config->ops->eqos_stop_clks(dev); err: pr_err("FAILED: %d", ret); return ret;
@@ -1412,12 +1358,6 @@ static void eqos_stop(struct udevice *dev) clrbits_le32(&eqos->dma_regs->ch0_rx_control, EQOS_DMA_CH0_RX_CONTROL_SR);
- if (eqos->phy) {
phy_shutdown(eqos->phy);- }
- eqos->config->ops->eqos_stop_resets(dev);
- eqos->config->ops->eqos_stop_clks(dev);
- debug("%s: OK\n", __func__); }
@@ -1888,9 +1828,65 @@ static int eqos_probe(struct udevice *dev) eth_phy_set_mdio_bus(dev, eqos->mii); #endif
- ret = eqos->config->ops->eqos_start_clks(dev);
- if (ret < 0) {
pr_err("eqos_start_clks() failed: %d", ret);goto err_unregister_mdio;- }
- ret = eqos->config->ops->eqos_start_resets(dev);
- if (ret < 0) {
pr_err("eqos_start_resets() failed: %d", ret);goto err_stop_clks;- }
- udelay(10);
- /*
* if PHY was already connected and configured,* don't need to reconnect/reconfigure again*/- if (!eqos->phy) {
this test can be remove I think
because we have always eqos->phy = NULL in probe,
PHY was can't be configured before probe
and it should be done again after a remove...
Agree, I will remove this check.
Best Regards, Joakim Zhang
int addr = -1;+#ifdef CONFIG_DM_ETH_PHY
addr = eth_phy_get_addr(dev);+#endif +#ifdef DWC_NET_PHYADDR
addr = DWC_NET_PHYADDR;+#endif
eqos->phy = phy_connect(eqos->mii, addr, dev,eqos->config->interface(dev));if (!eqos->phy) {pr_err("phy_connect() failed");goto err_stop_resets;}if (eqos->max_speed) {ret = phy_set_supported(eqos->phy, eqos->max_speed);if (ret) {pr_err("phy_set_supported() failed: %d", ret);goto err_shutdown_phy;}}ret = phy_config(eqos->phy);if (ret < 0) {pr_err("phy_config() failed: %d", ret);goto err_shutdown_phy;}- }
- debug("%s: OK\n", __func__); return 0;
+err_shutdown_phy:
- phy_shutdown(eqos->phy);
+err_stop_resets:
- eqos->config->ops->eqos_stop_resets(dev);
+err_stop_clks:
- eqos->config->ops->eqos_stop_clks(dev);
+err_unregister_mdio:
- mdio_unregister(eqos->mii); err_free_mdio: mdio_free(eqos->mii); err_remove_resources_tegra:
@@ -1908,6 +1904,12 @@ static int eqos_remove(struct udevice *dev)
debug("%s(dev=%p):\n", __func__, dev);
- if (eqos->phy) {
phy_shutdown(eqos->phy);- }
- eqos->config->ops->eqos_stop_resets(dev);
- eqos->config->ops->eqos_stop_clks(dev);
- mdio_unregister(eqos->mii); mdio_free(eqos->mii); eqos->config->ops->eqos_remove_resources(dev);
Regards
Patrick