Re: [U-Boot] [PATCH] Revert "net: phy: delay only if reset handler is registered"

[Joe Hershberger]

Hi Fabio,

On Mon, Jan 4, 2016 at 8:24 AM, Fabio Estevam  wrote:
> Hi Jörg,
>
> On Mon, Jan 4, 2016 at 11:48 AM, Jörg Krause
>  wrote:
>
>> 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.

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

Thanks,
-Joe
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] Revert "net: phy: delay only if reset handler is registered"

[Fabio Estevam]

Hi Joe,

On Mon, Jan 4, 2016 at 4:35 PM, Joe Hershberger
 wrote:

> 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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] Revert "net: phy: delay only if reset handler is registered"

[Fabio Estevam]

Tom and Joe,

On Wed, Dec 23, 2015 at 1:42 PM, Fabio Estevam  wrote:

> Ethernet on mx6cuboxi is also broken because of 59370f3fcd13508
> (""net: phy: delay only if reset handler is registered").
>
> I tried doing:
>
> --- a/board/solidrun/mx6cuboxi/mx6cuboxi.c
> +++ b/board/solidrun/mx6cuboxi/mx6cuboxi.c
> @@ -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?

To avoid Ethernet breakage on at least Wandboard and Cubox-i boards on
2016.01, can we apply this revert patch in the meantime?
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] Revert "net: phy: delay only if reset handler is registered"

[Fabio Estevam]

Hi Jörg,

On Mon, Jan 4, 2016 at 11:48 AM, Jörg Krause
 wrote:

> 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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] Revert "net: phy: delay only if reset handler is registered"

[Fabio Estevam]

Hi Jörg,

On Mon, Jan 4, 2016 at 10:49 AM, Jörg Krause
 wrote:

> Do you know which PHY is used on mx6cuboxi?

It is an AR8035.

> 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.

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

It does not help.

Thanks,

Fabio Estevam
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] Revert "net: phy: delay only if reset handler is registered"

[Jörg Krause]

Hi Fabio,

On Mo, 2016-01-04 at 10:57 -0200, Fabio Estevam wrote:
> Hi Jörg,
> 
> On Mon, Jan 4, 2016 at 10:49 AM, Jörg Krause
>  wrote:
> 
> > Do you know which PHY is used on mx6cuboxi?
> 
> It is an AR8035.
> 
> > 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.
> 
> > 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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] Revert "net: phy: delay only if reset handler is registered"

[Jörg Krause]

Hi Fabio,

On Mi, 2015-12-23 at 13:42 -0200, Fabio Estevam wrote:
> Hi Jörg,
> 
> On Mon, Nov 23, 2015 at 4:16 PM, Fabio Estevam 
> wrote:
> > Hi Jörg ,
> > 
> > On Fri, Nov 20, 2015 at 6:37 PM, Fabio Estevam 
> > wrote:
> > 
> > > 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:
> 
> --- a/board/solidrun/mx6cuboxi/mx6cuboxi.c
> +++ b/board/solidrun/mx6cuboxi/mx6cuboxi.c
> @@ -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?

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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] Revert "net: phy: delay only if reset handler is registered"

[Fabio Estevam]

Hi Jörg ,

On Fri, Nov 20, 2015 at 6:37 PM, Fabio Estevam  wrote:

> 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.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] Revert "net: phy: delay only if reset handler is registered"

[Jörg Krause]

On Mo, 2015-11-23 at 16:16 -0200, Fabio Estevam wrote:
> Hi Jörg ,
> 
> On Fri, Nov 20, 2015 at 6:37 PM, Fabio Estevam 
> wrote:
> 
> > 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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] Revert "net: phy: delay only if reset handler is registered"

[Fabio Estevam]

Hi Jörg ,

On Thu, Nov 19, 2015 at 6:13 AM, Jörg Krause
 wrote:

> 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?
>
>> 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.

--- a/board/freescale/mx6sxsabresd/mx6sxsabresd.c
+++ b/board/freescale/mx6sxsabresd/mx6sxsabresd.c
@@ -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(_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(>pll_enet);
 reg |= BM_ANADIG_PLL_ENET_REF_25M_ENABLE;
 writel(reg, >pll_enet);

-return enable_fec_anatop_clock(0, ENET_125MHZ);
+return 0;
 }

 int board_eth_init(bd_t *bis)
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] Revert "net: phy: delay only if reset handler is registered"

[Jörg Krause]

On Mi, 2015-11-18 at 09:34 -0200, Fabio Estevam wrote:
> Hi Jörg,
> 
> On Wed, Nov 18, 2015 at 6:44 AM, Jörg Krause
>  wrote:
> 
> > 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.
> 
> > 
> > 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);
> > }
> 
> 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?

> 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

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] Revert "net: phy: delay only if reset handler is registered"

[Jörg Krause]

Hi Fabio,

On Di, 2015-11-17 at 14:25 -0200, Fabio Estevam wrote:
> This reverts commit 59370f3fcd135089c402c93720a87c688abe600c.
> 
> This commit breaks ethernet on at least mx6 Riotboard and
> mx6sxsabresd boards.
> 
> Reported-by: Catalin Crenguta 
> Signed-off-by: Fabio Estevam 
> ---
>  drivers/net/phy/phy.c | 8 +++-
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index d7364ff..9e68f1d 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -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);
>  }
>  

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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] Revert "net: phy: delay only if reset handler is registered"

[Fabio Estevam]

Hi Stefan,

On Wed, Nov 18, 2015 at 3:54 AM, Stefan Roese  wrote:

> 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@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.

Sure, I have even tried:

--- a/board/freescale/mx6sxsabresd/mx6sxsabresd.c
+++ b/board/freescale/mx6sxsabresd/mx6sxsabresd.c
@@ -163,8 +163,9 @@ static int setup_fec(void)

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

reg = readl(>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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] Revert "net: phy: delay only if reset handler is registered"

[Fabio Estevam]

Hi Jörg,

On Wed, Nov 18, 2015 at 6:44 AM, Jörg Krause
 wrote:

> 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.

>
> 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);
> }

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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH] Revert "net: phy: delay only if reset handler is registered"

[Fabio Estevam]

This reverts commit 59370f3fcd135089c402c93720a87c688abe600c.

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

Reported-by: Catalin Crenguta 
Signed-off-by: Fabio Estevam 
---
 drivers/net/phy/phy.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index d7364ff..9e68f1d 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -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);
 }
 
-- 
1.9.1

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] Revert "net: phy: delay only if reset handler is registered"

[Stefan Roese]

Hi Fabio,

On 17.11.2015 17:25, Fabio Estevam wrote:

This reverts commit 59370f3fcd135089c402c93720a87c688abe600c.

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

Reported-by: Catalin Crenguta 
Signed-off-by: Fabio Estevam 
---
  drivers/net/phy/phy.c | 8 +++-
  1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index d7364ff..9e68f1d 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -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);
  }


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@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

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot