RE: [PATCH] net: eqos: connect and config PHY from probe stage instead of starting EQOS

2021-11-16 Thread Joakim Zhang

Hi Ramon,

> -Original Message-
> From: Ramon Fried 
> Sent: 2021年11月17日 12:20
> To: Joakim Zhang 
> Cc: Joe Hershberger ; U-Boot Mailing List
> ; Ye Li ; Patrick Delaunay
> ; Daniil Stas ;
> Stephen Warren 
> Subject: Re: [PATCH] net: eqos: connect and config PHY from probe stage
> instead of starting EQOS
> 
> On Tue, Nov 16, 2021 at 1:02 PM Joakim Zhang 
> wrote:
> >
> >
> > > -Original Message-
> > > From: Ramon Fried 
> > > Sent: 2021年11月16日 18:45
> > > To: Joakim Zhang 
> > > Cc: Joe Hershberger ; U-Boot Mailing List
> > > ; Ye Li ; Patrick Delaunay
> > > ; Daniil Stas
> > > ; Stephen Warren 
> > > Subject: Re: [PATCH] net: eqos: connect and config PHY from probe
> > > stage instead of starting EQOS
> > >
> > > On Tue, Nov 16, 2021 at 10:04 AM Joakim Zhang
> > > 
> > > wrote:
> > > >
> > > >
> > > > Hi Ramon,
> > > >
> > > > > -Original Message-
> > > > > From: Ramon Fried 
> > > > > Sent: 2021年11月16日 13:57
> > > > > To: Joakim Zhang 
> > > > > Cc: Joe Hershberger ; U-Boot Mailing
> > > > > List ; Ye Li ; Patrick
> > > > > Delaunay ; Daniil Stas
> > > > > ; Stephen Warren 
> > > > > Subject: Re: [PATCH] net: eqos: connect and config PHY from
> > > > > probe stage instead of starting EQOS
> > > > >
> > > > > On Wed, Nov 10, 2021 at 7: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 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@30bf Waiting for PHY auto negotiation to
> complete...
> > > > > > done BOOTP broadcast 1 DHCP client bound to address
> > > > > > 10.193.102.192
> > > > > > (313 ms) Using ethernet@30bf 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@30bf device TFTP from server 10.193.108.176; our
> > > > > > IP address is 10.193.102.192; sending through gateway
> > > > > > 10.193.102.254
> > > > > >
> > > > > How much time do you save exactly, Is it the ~140ms you show
> > > > > here at the commit log ?
> > > >
> > > > Exactly not. This time points to DHCP client bound to address, not
> > > > the time
> > > this patch saves.
> > > >
> > > > How can we evaluate the time we save? Please see the log:
> > > >
> > > > Before:
> > > > Booting from net ...
> > > > ethernet@30bf Waiting for 

Re: [PATCH] net: eqos: connect and config PHY from probe stage instead of starting EQOS

2021-11-16 Thread Ramon Fried
On Tue, Nov 16, 2021 at 1:02 PM Joakim Zhang  wrote:
>
>
> > -Original Message-
> > From: Ramon Fried 
> > Sent: 2021年11月16日 18:45
> > To: Joakim Zhang 
> > Cc: Joe Hershberger ; U-Boot Mailing List
> > ; Ye Li ; Patrick Delaunay
> > ; Daniil Stas ;
> > Stephen Warren 
> > Subject: Re: [PATCH] net: eqos: connect and config PHY from probe stage
> > instead of starting EQOS
> >
> > On Tue, Nov 16, 2021 at 10:04 AM Joakim Zhang 
> > wrote:
> > >
> > >
> > > Hi Ramon,
> > >
> > > > -Original Message-
> > > > From: Ramon Fried 
> > > > Sent: 2021年11月16日 13:57
> > > > To: Joakim Zhang 
> > > > Cc: Joe Hershberger ; U-Boot Mailing List
> > > > ; Ye Li ; Patrick Delaunay
> > > > ; Daniil Stas
> > > > ; Stephen Warren 
> > > > Subject: Re: [PATCH] net: eqos: connect and config PHY from probe
> > > > stage instead of starting EQOS
> > > >
> > > > On Wed, Nov 10, 2021 at 7: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 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@30bf Waiting for PHY auto negotiation to complete...
> > > > > done BOOTP broadcast 1 DHCP client bound to address 10.193.102.192
> > > > > (313 ms) Using ethernet@30bf 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@30bf device TFTP from server 10.193.108.176; our IP
> > > > > address is 10.193.102.192; sending through gateway 10.193.102.254
> > > > >
> > > > How much time do you save exactly, Is it the ~140ms you show here at
> > > > the commit log ?
> > >
> > > Exactly not. This time points to DHCP client bound to address, not the 
> > > time
> > this patch saves.
> > >
> > > How can we evaluate the time we save? Please see the log:
> > >
> > > Before:
> > > Booting from net ...
> > > ethernet@30bf Waiting for PHY auto negotiation to complete...
> > done
> > > BOOTP broadcast 1
> > > After:
> > > Booting from net ...
> > > BOOTP broadcast 1
> > >
> > > And you need to check the related code:
> > > drivers/net/dwc_eth_qos.c: phy_startup ->...->
> > > drivers/net/phy/phy.c: genphy_update_link()
> > >
> > >
> > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsour
> > >
> > ce.denx.de%2Fu-boot%2Fu-boot%2F-%2Fblob%2Fmaster%2Fdrivers%2Fnet
> > %2Fphy
> > > %2Fphy.c%23L225data=04%7C01%7Cqiangqing.zhang%40nxp.com%
> > 7C59322db
> > >
> > 193a54788a09308d9a8ee2cfc%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%
> > 7C0%7
> > >
> > C637726563178464522%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw
>

RE: [PATCH] net: eqos: connect and config PHY from probe stage instead of starting EQOS

2021-11-16 Thread Joakim Zhang

> -Original Message-
> From: Ramon Fried 
> Sent: 2021年11月16日 18:45
> To: Joakim Zhang 
> Cc: Joe Hershberger ; U-Boot Mailing List
> ; Ye Li ; Patrick Delaunay
> ; Daniil Stas ;
> Stephen Warren 
> Subject: Re: [PATCH] net: eqos: connect and config PHY from probe stage
> instead of starting EQOS
> 
> On Tue, Nov 16, 2021 at 10:04 AM Joakim Zhang 
> wrote:
> >
> >
> > Hi Ramon,
> >
> > > -Original Message-
> > > From: Ramon Fried 
> > > Sent: 2021年11月16日 13:57
> > > To: Joakim Zhang 
> > > Cc: Joe Hershberger ; U-Boot Mailing List
> > > ; Ye Li ; Patrick Delaunay
> > > ; Daniil Stas
> > > ; Stephen Warren 
> > > Subject: Re: [PATCH] net: eqos: connect and config PHY from probe
> > > stage instead of starting EQOS
> > >
> > > On Wed, Nov 10, 2021 at 7: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 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@30bf Waiting for PHY auto negotiation to complete...
> > > > done BOOTP broadcast 1 DHCP client bound to address 10.193.102.192
> > > > (313 ms) Using ethernet@30bf 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@30bf device TFTP from server 10.193.108.176; our IP
> > > > address is 10.193.102.192; sending through gateway 10.193.102.254
> > > >
> > > How much time do you save exactly, Is it the ~140ms you show here at
> > > the commit log ?
> >
> > Exactly not. This time points to DHCP client bound to address, not the time
> this patch saves.
> >
> > How can we evaluate the time we save? Please see the log:
> >
> > Before:
> > Booting from net ...
> > ethernet@30bf Waiting for PHY auto negotiation to complete...
> done
> > BOOTP broadcast 1
> > After:
> > Booting from net ...
> > BOOTP broadcast 1
> >
> > And you need to check the related code:
> > drivers/net/dwc_eth_qos.c: phy_startup ->...->
> > drivers/net/phy/phy.c: genphy_update_link()
> >
> >
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fsour
> >
> ce.denx.de%2Fu-boot%2Fu-boot%2F-%2Fblob%2Fmaster%2Fdrivers%2Fnet
> %2Fphy
> > %2Fphy.c%23L225data=04%7C01%7Cqiangqing.zhang%40nxp.com%
> 7C59322db
> >
> 193a54788a09308d9a8ee2cfc%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%
> 7C0%7
> >
> C637726563178464522%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw
> MDAiLCJQIj
> >
> oiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=usRIR7L
> exBnk
> > dnDO3U8hHYtMAIWT8bJ0Q7wgTElUjHs%3Dreserved=0
> >
> > while (!(mii_reg & BMSR_ANEGCOMPLETE)) {
> > /*
> >  * Timeout reached ?
> >  */
> > if (i > (PHY_ANEG_TIMEOUT / 50)) {
> > printf(" TIMEOUT !\n");
> > phydev->link = 0;
> >  

RE: [PATCH] net: eqos: connect and config PHY from probe stage instead of starting EQOS

2021-11-16 Thread Joakim Zhang

Hi Ramon,

> -Original Message-
> From: Ramon Fried 
> Sent: 2021年11月16日 13:57
> To: Joakim Zhang 
> Cc: Joe Hershberger ; U-Boot Mailing List
> ; Ye Li ; Patrick Delaunay
> ; Daniil Stas ;
> Stephen Warren 
> Subject: Re: [PATCH] net: eqos: connect and config PHY from probe stage
> instead of starting EQOS
> 
> On Wed, Nov 10, 2021 at 7: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 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@30bf Waiting for PHY auto negotiation to complete...
> > done BOOTP broadcast 1 DHCP client bound to address 10.193.102.192
> > (313 ms) Using ethernet@30bf 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@30bf device TFTP from server 10.193.108.176; our IP
> > address is 10.193.102.192; sending through gateway 10.193.102.254
> >
> How much time do you save exactly, Is it the ~140ms you show here at the
> commit log ?

Exactly not. This time points to DHCP client bound to address, not the time 
this patch saves.

How can we evaluate the time we save? Please see the log:

Before:
Booting from net ...
ethernet@30bf Waiting for PHY auto negotiation to complete... done
BOOTP broadcast 1
After:
Booting from net ...
BOOTP broadcast 1   

And you need to check the related code:
drivers/net/dwc_eth_qos.c: phy_startup ->...->
drivers/net/phy/phy.c: genphy_update_link()

https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/net/phy/phy.c#L225

while (!(mii_reg & BMSR_ANEGCOMPLETE)) {
/*
 * Timeout reached ?
 */
if (i > (PHY_ANEG_TIMEOUT / 50)) {
printf(" TIMEOUT !\n");
phydev->link = 0;
return -ETIMEDOUT;
}

if (ctrlc()) {
puts("user interrupt!\n");
phydev->link = 0;
return -EINTR;
}

if ((i++ % 10) == 0)
printf(".");

mii_reg = phy_read(phydev, MDIO_DEVAD_NONE, MII_BMSR);
mdelay(50); /* 50 ms */
}

We can see that one "." is about 500ms, so from the log, we save about 
500*7=3500ms=3.5s, this also means that PHY needs about 3.5s to complete 
auto-nego.

I also described this in the commit message, "users need wait seconds for PHY 
auto negotiation to complete when do tftp boot", may not quite clear, I will 
improve it.

Best Regards,
Joakim Zhang


RE: [PATCH] net: eqos: connect and config PHY from probe stage instead of starting EQOS

2021-11-16 Thread Joakim Zhang

Hi Patrick,

> -Original Message-
> From: Patrick DELAUNAY 
> Sent: 2021年11月15日 18:42
> To: Joakim Zhang ; joe.hershber...@ni.com;
> rfried@gmail.com
> Cc: u-boot@lists.denx.de; Ye Li ; daniil.s...@posteo.net;
> swar...@nvidia.com; Christophe ROULLIER
> ; Marek Vasut 
> 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@30bf Waiting for PHY auto negotiation to complete...
> > done BOOTP broadcast 1 DHCP client bound to address 10.193.102.192
> > (313 ms) Using ethernet@30bf 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@30bf 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 
> > ---
> >   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(>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 / 100) - 1;
> > writel(val, >mac_regs->us_tic_counter);
> >
> > -   /*
> > -* if PHY was already connected and configured,
> > -* don't need to reconnec

Re: [PATCH] net: eqos: connect and config PHY from probe stage instead of starting EQOS

2021-11-16 Thread Ramon Fried
On Tue, Nov 16, 2021 at 10:04 AM Joakim Zhang  wrote:
>
>
> Hi Ramon,
>
> > -Original Message-
> > From: Ramon Fried 
> > Sent: 2021年11月16日 13:57
> > To: Joakim Zhang 
> > Cc: Joe Hershberger ; U-Boot Mailing List
> > ; Ye Li ; Patrick Delaunay
> > ; Daniil Stas ;
> > Stephen Warren 
> > Subject: Re: [PATCH] net: eqos: connect and config PHY from probe stage
> > instead of starting EQOS
> >
> > On Wed, Nov 10, 2021 at 7: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 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@30bf Waiting for PHY auto negotiation to complete...
> > > done BOOTP broadcast 1 DHCP client bound to address 10.193.102.192
> > > (313 ms) Using ethernet@30bf 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@30bf device TFTP from server 10.193.108.176; our IP
> > > address is 10.193.102.192; sending through gateway 10.193.102.254
> > >
> > How much time do you save exactly, Is it the ~140ms you show here at the
> > commit log ?
>
> Exactly not. This time points to DHCP client bound to address, not the time 
> this patch saves.
>
> How can we evaluate the time we save? Please see the log:
>
> Before:
> Booting from net ...
> ethernet@30bf Waiting for PHY auto negotiation to complete... done
> BOOTP broadcast 1
> After:
> Booting from net ...
> BOOTP broadcast 1
>
> And you need to check the related code:
> drivers/net/dwc_eth_qos.c: phy_startup ->...->
> drivers/net/phy/phy.c: genphy_update_link()
>
> https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/net/phy/phy.c#L225
>
> while (!(mii_reg & BMSR_ANEGCOMPLETE)) {
> /*
>  * Timeout reached ?
>  */
> if (i > (PHY_ANEG_TIMEOUT / 50)) {
> printf(" TIMEOUT !\n");
> phydev->link = 0;
> return -ETIMEDOUT;
> }
>
> if (ctrlc()) {
> puts("user interrupt!\n");
> phydev->link = 0;
> return -EINTR;
> }
>
> if ((i++ % 10) == 0)
> printf(".");
>
> mii_reg = phy_read(phydev, MDIO_DEVAD_NONE, MII_BMSR);
> mdelay(50); /* 50 ms */
> }
>
> We can see that one "." is about 500ms, so from the log, we save about 
> 500*7=3500ms=3.5s, this also means that PHY needs about 3.5s to complete 
> auto-nego.
>
> I also described this in the commit message, "users need wait seconds for PHY 
> auto negotiation to complete when do tftp boot", may not quite clear, I will 
> improve it.
>
> Best Regards,
> Joakim Zhang

NACK.
Hi Joakim.
It's currently working like that for all network drivers, it's not specific
What you're currently suggesting is that the link will be brought up
automatically for all users, even if they're not interested in network
booting.
That won't work.
Thanks,
Ramon


Re: [PATCH] net: eqos: connect and config PHY from probe stage instead of starting EQOS

2021-11-15 Thread Ramon Fried
On Wed, Nov 10, 2021 at 7: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
> 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@30bf Waiting for PHY auto negotiation to complete... done
> BOOTP broadcast 1
> DHCP client bound to address 10.193.102.192 (313 ms)
> Using ethernet@30bf 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@30bf device
> TFTP from server 10.193.108.176; our IP address is 10.193.102.192; sending 
> through gateway 10.193.102.254
>
How much time do you save exactly, Is it the ~140ms you show here at
the commit log ?

> Signed-off-by: Joakim Zhang 
> ---
>  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;
>
> ret = wait_for_bit_le32(>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 / 100) - 1;
> writel(val, >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: 

Re: [PATCH] net: eqos: connect and config PHY from probe stage instead of starting EQOS

2021-11-15 Thread Patrice CHOTARD
Hi All

For information

Another patch from Marek will collide with this one => 
https://patchwork.ozlabs.org/project/uboot/patch/2023022352.231762-1-ma...@denx.de/

Patrice


On 11/15/21 11:41 AM, Patrick DELAUNAY wrote:
> 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@30bf Waiting for PHY auto negotiation to complete... done
>> BOOTP broadcast 1
>> DHCP client bound to address 10.193.102.192 (313 ms)
>> Using ethernet@30bf 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@30bf 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 
>> ---
>>   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
> 
>>     ret = wait_for_bit_le32(>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 / 100) - 1;
>>   writel(val, >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 

Re: [PATCH] net: eqos: connect and config PHY from probe stage instead of starting EQOS

2021-11-15 Thread Patrick DELAUNAY

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@30bf Waiting for PHY auto negotiation to complete... done
BOOTP broadcast 1
DHCP client bound to address 10.193.102.192 (313 ms)
Using ethernet@30bf 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@30bf 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 
---
  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

  
  	ret = wait_for_bit_le32(>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 / 100) - 1;

writel(val, >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 */

@@ 

[PATCH] net: eqos: connect and config PHY from probe stage instead of starting EQOS

2021-11-10 Thread Joakim Zhang
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
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@30bf Waiting for PHY auto negotiation to complete... done
BOOTP broadcast 1
DHCP client bound to address 10.193.102.192 (313 ms)
Using ethernet@30bf 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@30bf 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 
---
 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;
 
ret = wait_for_bit_le32(>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 / 100) - 1;
writel(val, >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:
-