[PATCH] net: eqos: connect and config PHY from probe stage instead of starting EQOS
Joakim Zhang
qiangqing.zhang at nxp.com
Tue Nov 16 09:04:01 CET 2021
Hi Patrick,
> -----Original Message-----
> From: Patrick DELAUNAY <patrick.delaunay at foss.st.com>
> Sent: 2021年11月15日 18:42
> To: Joakim Zhang <qiangqing.zhang at nxp.com>; joe.hershberger at ni.com;
> rfried.dev at gmail.com
> Cc: u-boot at lists.denx.de; Ye Li <ye.li at nxp.com>; daniil.stas at posteo.net;
> swarren at nvidia.com; Christophe ROULLIER
> <christophe.roullier at foss.st.com>; Marek Vasut <marex at 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 at 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 at 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 at 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 at 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
More information about the U-Boot
mailing list