Revert "net: phy: delay only if reset handler is registered"

PATCH New Fabio Estevam 5 participants 17 messages Nov 17, 2015 – Jan 4, 2016

Subsystems (2)

Network drivers Ethernet PHY

lore

Share & tools

lore

Fabio Estevam

author

2015-11-17 16:25 UTC #0 lore raw

PATCH 1/1 Revert "net: phy: delay only if reset handler is registered"

This reverts commit 59370f3fcd135089c402c93720a87c688abe600c.

This commit breaks ethernet on at least mx6 Riotboard and mx6sxsabresd boards.

Reported-by Catalin Crenguta <[email protected]>

Signed-off-by Fabio Estevam <[email protected]>

---
 drivers/net/phy/phy.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

drivers/net/phy/phy.c +3 -5

index d7364ff..9e68f1d 100644
@@ -771,13 +771,11 @@ struct phy_device *phy_find_by_mask(struct mii_dev *bus, unsigned phy_mask,
 		phy_interface_t interface)
 {
 	/* Reset the bus */
-	if (bus->reset) {
+	if (bus->reset)
 		bus->reset(bus);
 
-		/* Wait 15ms to make sure the PHY has come out of hard reset */
-		udelay(15000);
-	}
-
+	/* Wait 15ms to make sure the PHY has come out of hard reset */
+	udelay(15000);
 	return get_phy_device_by_mask(bus, phy_mask, interface);
 }

Signature · 1 line

1.9.1

Stefan Roese

2015-11-18 05:54 UTC #1 lore raw

PATCH 1/1 Revert "net: phy: delay only if reset handler is registered"

Hi Fabio,

Quoted 21 lines · Fabio Estevam

This reverts commit 59370f3fcd135089c402c93720a87c688abe600c.
This commit breaks ethernet on at least mx6 Riotboard and mx6sxsabresd boards.
Reported-by Catalin Crenguta <[email protected]>
Signed-off-by Fabio Estevam <[email protected]>
---
  drivers/net/phy/phy.c | 8 +++-----
  1 file changed, 3 insertions(+), 5 deletions(-)
drivers/net/phy/phy.c +1 -1
index d7364ff..9e68f1d 100644
@@ -771,13 +771,11 @@ struct phy_device *phy_find_by_mask(struct mii_dev *bus, unsigned phy_mask,
  		phy_interface_t interface)
  {
  	/* Reset the bus */
-	if (bus->reset) {
+	if (bus->reset)

drivers/net/phy/phy.c struct phy_device *phy_find_by_mask(struct mii_dev *bus, unsigned phy_mask, · line 780

Quoted 10 lines

drivers/net/phy/phy.c +2 -4

  		bus->reset(bus);
 
-		/* Wait 15ms to make sure the PHY has come out of hard reset */
-		udelay(15000);
-	}
-
+	/* Wait 15ms to make sure the PHY has come out of hard reset */
+	udelay(15000);
  	return get_phy_device_by_mask(bus, phy_mask, interface);
  }

I'm not sure if this revert is the right way to solve this problem. Please take a look at my answer a few weeks ago:

https://www.mail-archive.com/u-boot at lists.denx.de/msg191206.html

As mentioned in my mail above, the delay should be added after the deassertion of the PHY reset signal. And not here in phy_find_by_mask() for all boards.

I just checked the code in mx6sxsabresd.c. Here also the delay is missing:

         /* Reset AR8031 PHY */
         gpio_direction_output(IMX_GPIO_NR(2, 7) , 0);
         udelay(500);
         gpio_set_value(IMX_GPIO_NR(2, 7), 1);

Could you please test with this change:

	/* Reset AR8031 PHY */
	gpio_direction_output(IMX_GPIO_NR(2, 7) , 0);
	udelay(500);
	gpio_set_value(IMX_GPIO_NR(2, 7), 1);

+ udelay(1500);

Or even better, check how long the reset needs to be inactive before the PHY starts to work.

Thanks,
Stefan

Fabio Estevam

2015-11-18 11:30 UTC #2 lore raw

PATCH 1/1 Revert "net: phy: delay only if reset handler is registered"

Hi Stefan,

Quoted 19 lines · Stefan Roese

I'm not sure if this revert is the right way to solve this problem. Please take a look at my answer a few weeks ago:
https://www.mail-archive.com/u-boot at lists.denx.de/msg191206.html
As mentioned in my mail above, the delay should be added after the deassertion of the PHY reset signal. And not here in phy_find_by_mask() for all boards.
I just checked the code in mx6sxsabresd.c. Here also the delay is missing:
        /* Reset AR8031 PHY */
        gpio_direction_output(IMX_GPIO_NR(2, 7) , 0);
        udelay(500);
        gpio_set_value(IMX_GPIO_NR(2, 7), 1);
Could you please test with this change:

Quoted 8 lines

        /* Reset AR8031 PHY */
        gpio_direction_output(IMX_GPIO_NR(2, 7) , 0);
        udelay(500);
        gpio_set_value(IMX_GPIO_NR(2, 7), 1);
+ udelay(1500);
Or even better, check how long the reset needs to be inactive before the PHY starts to work.

Sure, I have even tried:

board/freescale/mx6sxsabresd/mx6sxsabresd.c +2 -1

@@ -163,8 +163,9 @@ static int setup_fec(void)
 
        /* Reset AR8031 PHY */
        gpio_direction_output(IMX_GPIO_NR(2, 7) , 0);
-       udelay(500);
+       udelay(50000);
        gpio_set_value(IMX_GPIO_NR(2, 7), 1);
+       udelay(50000);
 
        reg = readl(&anatop->pll_enet);
        reg |= BM_ANADIG_PLL_ENET_REF_25M_ENABLE;

and it does not work here:

U-Boot 2016.01-rc1-00022-gfe52456-dirty (Nov 18 2015 - 09:28:02 -0200)

CPU:   Freescale i.MX6SX rev1.0 996 MHz (running at 792 MHz)
CPU:   Extended Commercial temperature grade (-20C to 105C) at 44C
Reset cause: POR
Board: MX6SX SABRE SDB
I2C:   ready
DRAM:  1 GiB
PMIC:  PFUZE100 ID=0x10
MMC:   FSL_SDHC: 0, FSL_SDHC: 1, FSL_SDHC: 2
PCI:   pcie phy link never came up
In:    serial
Out:   serial
Err:   serial
Net:   Board Net Initialization Failed

No ethernet found.
Hit any key to stop autoboot: 0
Booting from net ...
No ethernet found.
No ethernet found.
Bad Linux ARM zImage magic!

Regards,

Fabio Estevam

Jörg Krause

2015-11-18 08:44 UTC #3 lore raw

PATCH 1/1 Revert "net: phy: delay only if reset handler is registered"

In reply to Fabio Estevam

Hi Fabio,

Quoted 32 lines · Fabio Estevam

This reverts commit 59370f3fcd135089c402c93720a87c688abe600c.
This commit breaks ethernet on at least mx6 Riotboard and mx6sxsabresd boards.
Reported-by Catalin Crenguta <[email protected]>
Signed-off-by Fabio Estevam <[email protected]>
---
?drivers/net/phy/phy.c | 8 +++-----
?1 file changed, 3 insertions(+), 5 deletions(-)
drivers/net/phy/phy.c +0 -0
index d7364ff..9e68f1d 100644
@@ -771,13 +771,11 @@ struct phy_device *phy_find_by_mask(struct
 mii_dev *bus, unsigned phy_mask,
 ?		phy_interface_t interface)
 ?{
? /* Reset the bus */
drivers/net/phy/phy.c +1 -1
-	if (bus->reset) {
+	if (bus->reset)
? bus->reset(bus);
?
drivers/net/phy/phy.c +0 -1
-		/* Wait 15ms to make sure the PHY has come out of
hard reset */
drivers/net/phy/phy.c +1 -3
-		udelay(15000);
-	}
-
+	/* Wait 15ms to make sure the PHY has come out of hard reset
*/
drivers/net/phy/phy.c +1 -0
+	udelay(15000);

Quoted 3 lines

? return get_phy_device_by_mask(bus, phy_mask, interface);
?}
?

I think this is not the right thing to do here. It is true that the AR8035 ethernet chip of the RioTboard needs the clock to be stable for at least 1ms before RESET can be deasserted. This why it fails, if there is no MII reset function defined.

I think the right place to handle reset delays is the?board_eth_init(). The value of 15ms currently used looks to me like an arbitrary value. I am not sure where this value is comming from. Some chips need more, some less time to delay after a reset.

This is snippet of how I do it for a custom i.MX28-EVK-based board:

int board_eth_init(bd_t *bis) {
??cpu_eth_init(bis);
??/* Power-on FEC */
? gpio_direction_output(MX28_PAD_LCD_D21__GPIO_1_21, 0); ??/* Reset FEC PHY */ ??gpio_direction_output(MX28_PAD_ENET0_RX_CLK__GPIO_4_13, 0); ??/* Deassert nRST after 25 ms from power-up on (= t_purstd) */ ??udelay(25000); ??gpio_set_value(MX28_PAD_ENET0_RX_CLK__GPIO_4_13, 1); ??fecmxc_initialize_multi(bis, 0, 0, MXS_ENET0_BASE); }

Best regards
J?rg Krause

Fabio Estevam

2015-11-18 11:34 UTC #4 lore raw

PATCH 1/1 Revert "net: phy: delay only if reset handler is registered"

Hi J?rg,

Quoted 4 lines · J?rg Krause

I think this is not the right thing to do here. It is true that the AR8035 ethernet chip of the RioTboard needs the clock to be stable for at least 1ms before RESET can be deasserted. This why it fails, if there is no MII reset function defined.

In my case I am testing on a mx6sxsabresd which has two AR8031 chips.

Quoted 8 lines

I think the right place to handle reset delays is the board_eth_init(). The value of 15ms currently used looks to me like an arbitrary value. I am not sure where this value is comming from. Some chips need more, some less time to delay after a reset.
This is snippet of how I do it for a custom i.MX28-EVK-based board:

Quoted 11 lines

int board_eth_init(bd_t *bis) {
cpu_eth_init(bis);
/* Power-on FEC */ gpio_direction_output(MX28_PAD_LCD_D21__GPIO_1_21, 0); /* Reset FEC PHY */ gpio_direction_output(MX28_PAD_ENET0_RX_CLK__GPIO_4_13, 0); /* Deassert nRST after 25 ms from power-up on (= t_purstd) */ udelay(25000); gpio_set_value(MX28_PAD_ENET0_RX_CLK__GPIO_4_13, 1); fecmxc_initialize_multi(bis, 0, 0, MXS_ENET0_BASE);
}

We currently reset the Ethernet PHY inside setup_fec() inside board_eth_init().

Please check board/freescale/mx6sxsabresd/mx6sxsabresd.c.

I have also tried increasing the reset time and still do not have Ethernel functional.

Regards,

Fabio Estevam

Jörg Krause

2015-11-19 08:13 UTC #5 lore raw

PATCH 1/1 Revert "net: phy: delay only if reset handler is registered"

Quoted 35 lines · Fabio Estevam

Hi J?rg,
Quoted 5 lines · J?rg Krause
I think this is not the right thing to do here. It is true that the AR8035 ethernet chip of the RioTboard needs the clock to be stable for
at least 1ms before RESET can be deasserted. This why it fails, if there is no MII reset function defined.
In my case I am testing on a mx6sxsabresd which has two AR8031 chips.
Quoted 10 lines
I think the right place to handle reset delays is the board_eth_init().
The value of 15ms currently used looks to me like an arbitrary value. I
am not sure where this value is comming from. Some chips need more, some less time to delay after a reset.
This is snippet of how I do it for a custom i.MX28-EVK-based board:
Quoted 11 lines
int board_eth_init(bd_t *bis) {
? cpu_eth_init(bis);
? /* Power-on FEC */
? gpio_direction_output(MX28_PAD_LCD_D21__GPIO_1_21, 0); ? /* Reset FEC PHY */
? gpio_direction_output(MX28_PAD_ENET0_RX_CLK__GPIO_4_13, 0); ? /* Deassert nRST after 25 ms from power-up on (= t_purstd) */ ? udelay(25000);
? gpio_set_value(MX28_PAD_ENET0_RX_CLK__GPIO_4_13, 1); ? fecmxc_initialize_multi(bis, 0, 0, MXS_ENET0_BASE); }

Quoted 4 lines

We currently reset the Ethernet PHY inside setup_fec() inside board_eth_init().
Please check board/freescale/mx6sxsabresd/mx6sxsabresd.c.

Ok, I checked. This is what happens in?setup_fec():

1) Enable AR8031 power
2) Assert AR8031 RESET
3) Delay 0.5ms
4) Deassert AR8031 RESET
5) Enable anatop clock

Shouldn't the clock be enabled and become stable before deasserting the RESET line?

Quoted 2 lines

I have also tried increasing the reset time and still do not have Ethernel functional.

The AR8031 datasheets recommands a delay of 10ms.

Best regards
J?rg Krause

Fabio Estevam

2015-11-20 20:37 UTC #6 lore raw

PATCH 1/1 Revert "net: phy: delay only if reset handler is registered"

Hi J?rg ,

Quoted 8 lines · J?rg Krause

Ok, I checked. This is what happens in setup_fec():
1) Enable AR8031 power
2) Assert AR8031 RESET
3) Delay 0.5ms
4) Deassert AR8031 RESET
5) Enable anatop clock

Quoted 7 lines

Shouldn't the clock be enabled and become stable before deasserting the RESET line?
Quoted 2 lines
I have also tried increasing the reset time and still do not have Ethernel functional.
The AR8031 datasheets recommands a delay of 10ms.

Ok, I will test your proposal below on Monday when I get access to my mx6sxsabresd, thanks.

board/freescale/mx6sxsabresd/mx6sxsabresd.c +7 -3

@@ -150,11 +150,15 @@ static int setup_fec(void)
 {
     struct iomuxc *iomuxc_regs = (struct iomuxc *)IOMUXC_BASE_ADDR;
     struct anatop_regs *anatop = (struct anatop_regs *)ANATOP_BASE_ADDR;
-    int reg;
+    int reg, ret;
 
     /* Use 125MHz anatop loopback REF_CLK1 for ENET1 */
     clrsetbits_le32(&iomuxc_regs->gpr[1], IOMUX_GPR1_FEC1_MASK, 0);
 
+    ret = enable_fec_anatop_clock(0, ENET_125MHZ);
+    if (ret)
+        return ret;
+
     imx_iomux_v3_setup_multiple_pads(phy_control_pads,
                      ARRAY_SIZE(phy_control_pads));

@@ -163,14 +167,14 @@ static int setup_fec(void)
 
     /* Reset AR8031 PHY */
     gpio_direction_output(IMX_GPIO_NR(2, 7) , 0);
-    udelay(500);
+    mdelay(10);
     gpio_set_value(IMX_GPIO_NR(2, 7), 1);
 
     reg = readl(&anatop->pll_enet);
     reg |= BM_ANADIG_PLL_ENET_REF_25M_ENABLE;
     writel(reg, &anatop->pll_enet);
 
-    return enable_fec_anatop_clock(0, ENET_125MHZ);
+    return 0;
 }
 
 int board_eth_init(bd_t *bis)

Fabio Estevam

2015-11-23 18:16 UTC #7 lore raw

PATCH 1/1 Revert "net: phy: delay only if reset handler is registered"

Hi J?rg ,

Quoted 2 lines · Fabio Estevam

Ok, I will test your proposal below on Monday when I get access to my mx6sxsabresd, thanks.

Your proposal worked fine, thanks. Will send it as a formal patch.

Jörg Krause

2015-11-23 20:08 UTC #8 lore raw

PATCH 1/1 Revert "net: phy: delay only if reset handler is registered"

Quoted 10 lines · Fabio Estevam

Hi J?rg ,
Quoted 3 lines · Fabio Estevam
Ok, I will test your proposal below on Monday when I get access to my
mx6sxsabresd, thanks.
Your proposal worked fine, thanks. Will send it as a formal patch.

This is good news!

J?rg

Fabio Estevam

2015-12-23 15:42 UTC #9 lore raw

PATCH 1/1 Revert "net: phy: delay only if reset handler is registered"

In reply to Fabio Estevam

Hi J?rg,

Quoted 8 lines · Fabio Estevam

Hi J?rg ,
Quoted 2 lines · Fabio Estevam
Ok, I will test your proposal below on Monday when I get access to my mx6sxsabresd, thanks.
Your proposal worked fine, thanks. Will send it as a formal patch.

Ethernet on mx6cuboxi is also broken because of 59370f3fcd13508 (""net: phy: delay only if reset handler is registered").

I tried doing:

board/solidrun/mx6cuboxi/mx6cuboxi.c +1 -1

@@ -143,7 +143,7 @@ static void setup_iomux_enet(void)
        SETUP_IOMUX_PADS(enet_pads);
 
        gpio_direction_output(ETH_PHY_RESET, 0);
-       mdelay(2);
+       mdelay(10);
        gpio_set_value(ETH_PHY_RESET, 1);
 }

,but it did not help.

Any suggestions?

Thanks,

Fabio Estevam

2016-01-04 11:19 UTC #10 lore raw

PATCH 1/1 Revert "net: phy: delay only if reset handler is registered"

Tom and Joe,

Quoted 18 lines · Fabio Estevam

Ethernet on mx6cuboxi is also broken because of 59370f3fcd13508 (""net: phy: delay only if reset handler is registered").
I tried doing:
board/solidrun/mx6cuboxi/mx6cuboxi.c +1 -1
@@ -143,7 +143,7 @@ static void setup_iomux_enet(void)
        SETUP_IOMUX_PADS(enet_pads);
 
        gpio_direction_output(ETH_PHY_RESET, 0);
-       mdelay(2);
+       mdelay(10);
        gpio_set_value(ETH_PHY_RESET, 1);
 }
 
,but it did not help.

Quoted 1 line

Any suggestions?

To avoid Ethernet breakage on at least Wandboard and Cubox-i boards on 2016.01, can we apply this revert patch in the meantime?

Jörg Krause

2016-01-04 12:49 UTC #11 lore raw

PATCH 1/1 Revert "net: phy: delay only if reset handler is registered"

In reply to Fabio Estevam

Hi Fabio,

Quoted 34 lines · Fabio Estevam

Hi J?rg,
Quoted 10 lines · Fabio Estevam
Hi J?rg ,
Quoted 3 lines · Fabio Estevam
Ok, I will test your proposal below on Monday when I get access to my
mx6sxsabresd, thanks.
Your proposal worked fine, thanks. Will send it as a formal patch.
Ethernet on mx6cuboxi is also broken because of 59370f3fcd13508
(""net: phy: delay only if reset handler is registered").
I tried doing:
board/solidrun/mx6cuboxi/mx6cuboxi.c +1 -1
@@ -143,7 +143,7 @@ static void setup_iomux_enet(void)
 ????????SETUP_IOMUX_PADS(enet_pads);
 
 ????????gpio_direction_output(ETH_PHY_RESET, 0);
-???????mdelay(2);
+???????mdelay(10);
 ????????gpio_set_value(ETH_PHY_RESET, 1);
 ?}
 
,but it did not help.

Quoted 1 line

Any suggestions?

Sorry for the long delay due to christmas vacation.

Do you know which PHY is used on?mx6cuboxi?

I am wondering if the ANATOP clock setting is correct (25MHz)?

http://git.denx.de/?p=u-boot.git;a=blob;f=board/solidrun/mx6cuboxi/mx6c
uboxi.c;h=fc37f1eef06da5147e5403d4272d220836c9cfbc;hb=HEAD#l167

Does it help to increase the delay and set it to 15ms?

Best regards
J?rg Krause

Fabio Estevam

2016-01-04 12:57 UTC #12 lore raw

PATCH 1/1 Revert "net: phy: delay only if reset handler is registered"

Hi J?rg,

Quoted 1 line · J?rg Krause

Do you know which PHY is used on mx6cuboxi?

It is an AR8035.

Quoted 4 lines

I am wondering if the ANATOP clock setting is correct (25MHz)?
http://git.denx.de/?p=u-boot.git;a=blob;f=board/solidrun/mx6cuboxi/mx6c
uboxi.c;h=fc37f1eef06da5147e5403d4272d220836c9cfbc;hb=HEAD#l167

Yes, this seems correct for this board.

Quoted 1 line

Does it help to increase the delay and set it to 15ms?

It does not help.

Thanks,

Fabio Estevam

Jörg Krause

2016-01-04 13:48 UTC #13 lore raw

PATCH 1/1 Revert "net: phy: delay only if reset handler is registered"

Hi Fabio,

Quoted 15 lines · Fabio Estevam

Hi J?rg,
Quoted 1 line · J?rg Krause
Do you know which PHY is used on mx6cuboxi?
It is an AR8035.
Quoted 5 lines
I am wondering if the ANATOP clock setting is correct (25MHz)?
http://git.denx.de/?p=u-boot.git;a=blob;f=board/solidrun/mx6cuboxi/
mx6c uboxi.c;h=fc37f1eef06da5147e5403d4272d220836c9cfbc;hb=HEAD#l167

Quoted 5 lines

Yes, this seems correct for this board.
Quoted 1 line
Does it help to increase the delay and set it to 15ms?
It does not help.

mx6cuboxi.h defines?CONFIG_FEC_MXC and?CONFIG_PHYLIB, but does not use?cpu_eth_init() [which calls fecmxc_initialize()]. Is there any reason for this?

J?rg

Fabio Estevam

2016-01-04 14:24 UTC #14 lore raw

PATCH 1/1 Revert "net: phy: delay only if reset handler is registered"

Hi J?rg,

Quoted 3 lines · J?rg Krause

mx6cuboxi.h defines CONFIG_FEC_MXC and CONFIG_PHYLIB, but does not use cpu_eth_init() [which calls fecmxc_initialize()]. Is there any reason for this?

Will check it.

I am able to get Ethernet functional now if I do the same initialization as in mx6qsabresd, which uses AR8031.

Will test more and submit the patches later today.

Thanks

Joe Hershberger

2016-01-04 18:35 UTC #15 lore raw

PATCH 1/1 Revert "net: phy: delay only if reset handler is registered"

Hi Fabio,

Quoted 9 lines · Fabio Estevam

Hi J?rg,
Quoted 3 lines · J?rg Krause
mx6cuboxi.h defines CONFIG_FEC_MXC and CONFIG_PHYLIB, but does not use cpu_eth_init() [which calls fecmxc_initialize()]. Is there any reason for this?

Quoted 6 lines

Will check it.
I am able to get Ethernet functional now if I do the same initialization as in mx6qsabresd, which uses AR8031.
Will test more and submit the patches later today.

Does this mean you rescind the request to apply this revert patch?

Thanks,
-Joe

Fabio Estevam

2016-01-04 18:37 UTC #16 lore raw

PATCH 1/1 Revert "net: phy: delay only if reset handler is registered"

Hi Joe,

Quoted 1 line · Joe Hershberger

Does this mean you rescind the request to apply this revert patch?

That's correct. There is no need to revert the patch.

I have sent patches (via i.MX code) that fix Ethernet on mx6cuboxi.

Thanks