Hi Patrick, > -----Original Message----- > From: Patrick DELAUNAY <patrick.delau...@foss.st.com> > Sent: 2021年11月15日 18:42 > To: Joakim Zhang <qiangqing.zh...@nxp.com>; joe.hershber...@ni.com; > rfried....@gmail.com > Cc: u-boot@lists.denx.de; Ye Li <ye...@nxp.com>; daniil.s...@posteo.net; > swar...@nvidia.com; Christophe ROULLIER > <christophe.roull...@foss.st.com>; Marek Vasut <ma...@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.zh...@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