[PATCH] phy/micrel: KSZ8031RNL RMII clock reconfiguration bug

[PATCH] phy/micrel: KSZ8031RNL RMII clock reconfiguration bug

[Bruno Thomsen]

Bug: Unable to send and receive Ethernet packets with Micrel PHY.

Affected devices:
KSZ8031RNL (commercial temp)
KSZ8031RNLI (industrial temp)

Description:
PHY device is correctly detected during probe.
PHY power-up default is 25MHz crystal clock input
and output 50MHz RMII clock to MAC.
Reconfiguration of PHY to input 50MHz RMII clock from MAC
causes PHY to become unresponsive if clock source is changed
after Operation Mode Strap Override (OMSO) register setup.

Cause:
Long lead times on parts where clock setup match circuit design
forces the usage of similar parts with wrong default setup.

Solution:
Swapped KSZ8031 register setup and added phy_write return code validation.

Tested with Freescale i.MX28 Fast Ethernet Controler (fec).

Signed-off-by: Bruno Thomsen <bth@kamstrup.dk>
---
 drivers/net/phy/micrel.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 011dbda..ec3f646 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -194,8 +194,10 @@ static int ksz8021_config_init(struct phy_device *phydev)
 	if (rc)
 		dev_err(&phydev->dev, "failed to set led mode\n");
 
-	phy_write(phydev, MII_KSZPHY_OMSO, val);
 	rc = ksz_config_flags(phydev);
+	if (rc < 0)
+		return rc;
+	rc = phy_write(phydev, MII_KSZPHY_OMSO, val);
 	return rc < 0 ? rc : 0;
 }
 
-- 
1.9.1

Re: [PATCH] phy/micrel: KSZ8031RNL RMII clock reconfiguration bug

[Angelo Dureghello]

I add some useful info:
there is a carrier change after phy is attached, but link state arrives 
as zero.

[root@xb ~]# cat /sys/class/net/eth0/carrier
carrier          carrier_changes
[root@xb ~]# cat /sys/class/net/eth0/carrier
0
[root@xb ~]# cat /sys/class/net/eth0/carrier_changes
1

Regards,
Angelo


On 09/10/2014 16:48, Bruno Thomsen wrote:
> Bug: Unable to send and receive Ethernet packets with Micrel PHY.
>
> Affected devices:
> KSZ8031RNL (commercial temp)
> KSZ8031RNLI (industrial temp)
>
> Description:
> PHY device is correctly detected during probe.
> PHY power-up default is 25MHz crystal clock input
> and output 50MHz RMII clock to MAC.
> Reconfiguration of PHY to input 50MHz RMII clock from MAC
> causes PHY to become unresponsive if clock source is changed
> after Operation Mode Strap Override (OMSO) register setup.
>
> Cause:
> Long lead times on parts where clock setup match circuit design
> forces the usage of similar parts with wrong default setup.
>
> Solution:
> Swapped KSZ8031 register setup and added phy_write return code validation.
>
> Tested with Freescale i.MX28 Fast Ethernet Controler (fec).
>
> Signed-off-by: Bruno Thomsen <bth@kamstrup.dk>
> ---
>   drivers/net/phy/micrel.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index 011dbda..ec3f646 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -194,8 +194,10 @@ static int ksz8021_config_init(struct phy_device *phydev)
>   	if (rc)
>   		dev_err(&phydev->dev, "failed to set led mode\n");
>   
> -	phy_write(phydev, MII_KSZPHY_OMSO, val);
>   	rc = ksz_config_flags(phydev);
> +	if (rc < 0)
> +		return rc;
> +	rc = phy_write(phydev, MII_KSZPHY_OMSO, val);
>   	return rc < 0 ? rc : 0;
>   }
>   

RE: [PATCH] phy/micrel: KSZ8031RNL RMII clock reconfiguration bug

[Bruno Thomsen]

Hey again

> I add some useful info:
> there is a carrier change after phy is attached, but link state arrives as zero.
>
> [root@xb ~]# cat /sys/class/net/eth0/carrier
> carrier          carrier_changes
> [root@xb ~]# cat /sys/class/net/eth0/carrier
> 0
> [root@xb ~]# cat /sys/class/net/eth0/carrier_changes
> 1

Just to clarify, this patch fixes a real bug and from your first bug rapport it sounded like a similar issue.


On 09/10/2014 16:48, Bruno Thomsen wrote:
> Bug: Unable to send and receive Ethernet packets with Micrel PHY.
>
> Affected devices:
> KSZ8031RNL (commercial temp)
> KSZ8031RNLI (industrial temp)
>
> Description:
> PHY device is correctly detected during probe.
> PHY power-up default is 25MHz crystal clock input and output 50MHz 
> RMII clock to MAC.
> Reconfiguration of PHY to input 50MHz RMII clock from MAC causes PHY 
> to become unresponsive if clock source is changed after Operation Mode 
> Strap Override (OMSO) register setup.
>

Venlig hilsen / Best regards

Kamstrup A/S <http://www.kamstrup.dk> 
Bruno Thomsen
Development engineer
Technology

Kamstrup A/S
Industrivej 28
DK-8660 Skanderborg
Tel:	 +45 89 93 10 00	 
Fax:	 +45 89 93 10 01	 
Dir:	 +45 89 93 13 94	 
E-mail:	 bth@kamstrup.dk	 
Web:	 www.kamstrup.dk	

Re: [PATCH] phy/micrel: KSZ8031RNL RMII clock reconfiguration bug

[David Miller]

From: Bruno Thomsen <bth@kamstrup.dk>
Date: Thu, 9 Oct 2014 16:48:14 +0200

> Bug: Unable to send and receive Ethernet packets with Micrel PHY.
> 
> Affected devices:
> KSZ8031RNL (commercial temp)
> KSZ8031RNLI (industrial temp)
> 
> Description:
> PHY device is correctly detected during probe.
> PHY power-up default is 25MHz crystal clock input
> and output 50MHz RMII clock to MAC.
> Reconfiguration of PHY to input 50MHz RMII clock from MAC
> causes PHY to become unresponsive if clock source is changed
> after Operation Mode Strap Override (OMSO) register setup.
> 
> Cause:
> Long lead times on parts where clock setup match circuit design
> forces the usage of similar parts with wrong default setup.
> 
> Solution:
> Swapped KSZ8031 register setup and added phy_write return code validation.
> 
> Tested with Freescale i.MX28 Fast Ethernet Controler (fec).
> 
> Signed-off-by: Bruno Thomsen <bth@kamstrup.dk>

Applied, thank you.

Re: phy/micrel: KSZ8031RNL RMII clock reconfiguration bug

[Johan Hovold]

Hi Bruno,

On Thu, Oct 09, 2014 at 04:48:14PM +0200, Bruno Thomsen wrote:
> Bug: Unable to send and receive Ethernet packets with Micrel PHY.
> 
> Affected devices:
> KSZ8031RNL (commercial temp)
> KSZ8031RNLI (industrial temp)
> 
> Description:
> PHY device is correctly detected during probe.
> PHY power-up default is 25MHz crystal clock input
> and output 50MHz RMII clock to MAC.
> Reconfiguration of PHY to input 50MHz RMII clock from MAC
> causes PHY to become unresponsive if clock source is changed
> after Operation Mode Strap Override (OMSO) register setup.
> 
> Cause:
> Long lead times on parts where clock setup match circuit design
> forces the usage of similar parts with wrong default setup.
> 
> Solution:
> Swapped KSZ8031 register setup and added phy_write return code validation.
> 
> Tested with Freescale i.MX28 Fast Ethernet Controler (fec).
> 
> Signed-off-by: Bruno Thomsen <bth@kamstrup.dk>
> ---
>  drivers/net/phy/micrel.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index 011dbda..ec3f646 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -194,8 +194,10 @@ static int ksz8021_config_init(struct phy_device *phydev)
>  	if (rc)
>  		dev_err(&phydev->dev, "failed to set led mode\n");
>  
> -	phy_write(phydev, MII_KSZPHY_OMSO, val);
>  	rc = ksz_config_flags(phydev);
> +	if (rc < 0)
> +		return rc;
> +	rc = phy_write(phydev, MII_KSZPHY_OMSO, val);
>  	return rc < 0 ? rc : 0;
>  }

As you may have seen by now, I've been working on refactoring the
micrel phy driver to be able to use common initialisation code.

Specifically, I've added generic support for disabling the broadcast
address, which is what the MII_KSZPHY_OMSO write above does.

Generally you want this to be the first thing you do in order to avoid
unnecessary reconfigurations. If we ever were to allow concurrent
probing this would also be a requirement.

Could you provide some detail about the setup were you find that the PHY
becomes unresponsive without your patch? Do you have more than one PHY
on the bus? Using what addresses? And using what clock modes (i.e. 25
MHz or 50 MHz)?

Also, what exactly do you mean by "unresponsive"? Are you still able to
read the PHY registers for example?

Thanks,
Johan

RE: phy/micrel: KSZ8031RNL RMII clock reconfiguration bug

[Bruno Thomsen]

Hi Johan,

> As you may have seen by now, I've been working on refactoring the micrel phy driver to be able to use common initialisation code.
>
> Specifically, I've added generic support for disabling the broadcast address, which is what the MII_KSZPHY_OMSO write above does.
>
> Generally you want this to be the first thing you do in order to avoid unnecessary reconfigurations. If we ever were to allow concurrent probing this would also be a requirement.
>
> Could you provide some detail about the setup were you find that the PHY becomes unresponsive without your patch? Do you have more than one PHY on the bus? Using what addresses? And using what clock modes (i.e. 25 MHz or 50 MHz)?
> 
> Also, what exactly do you mean by "unresponsive"? Are you still able to read the PHY registers for example?

I think it sounds like a good idea to refactor the init code.

My setup:
iMX28 processor with dual Ethernet MAC; FEC0 (enabled) and FEC1 (disabled).
There is a single KSZ8031 PHY that receives 50MHz RMII clock from the MAC.
I am unable to read PHY registers from both user-land tools and extra debug PHY reads in driver code.

Boot trace:
[   22.277785] fec 800f0000.ethernet eth0: Freescale FEC PHY driver [Micrel KSZ8031] (mii_bus:phy_addr=800f0000.etherne:00, irq=-1)
[   22.292527] IPv6: ADDRCONF(NETDEV_UP): eth0: link is not ready
[   24.276217] libphy: 800f0000.etherne:00 - Link is Up - 100/Full
[   24.285094] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready


Venlig hilsen / Best regards

Kamstrup A/S <http://www.kamstrup.dk> 
Bruno Thomsen
Development engineer
Technology

Kamstrup A/S
Industrivej 28
DK-8660 Skanderborg
Tel:	 +45 89 93 10 00	 
Fax:	 +45 89 93 10 01	 
Dir:	 +45 89 93 13 94	 
E-mail:	 bth@kamstrup.dk	 
Web:	 www.kamstrup.dk

Re: phy/micrel: KSZ8031RNL RMII clock reconfiguration bug

[Johan Hovold]

On Wed, Nov 12, 2014 at 12:17:57PM +0000, Bruno Thomsen wrote:
> Hi Johan,
> 
> > As you may have seen by now, I've been working on refactoring the
> > micrel phy driver to be able to use common initialisation code.
> >
> > Specifically, I've added generic support for disabling the broadcast
> > address, which is what the MII_KSZPHY_OMSO write above does.
> >
> > Generally you want this to be the first thing you do in order to
> > avoid unnecessary reconfigurations. If we ever were to allow
> > concurrent probing this would also be a requirement.
> >
> > Could you provide some detail about the setup were you find that the
> > PHY becomes unresponsive without your patch? Do you have more than
> > one PHY on the bus? Using what addresses? And using what clock modes
> > (i.e. 25 MHz or 50 MHz)?
> > 
> > Also, what exactly do you mean by "unresponsive"? Are you still able
> > to read the PHY registers for example?
>  
> I think it sounds like a good idea to refactor the init code.
> 
> My setup:
> iMX28 processor with dual Ethernet MAC; FEC0 (enabled) and FEC1 (disabled).
> There is a single KSZ8031 PHY that receives 50MHz RMII clock from the MAC.
> I am unable to read PHY registers from both user-land tools and extra
> debug PHY reads in driver code.

Did you specify a led-mode as well, or was the Operation Mode Strap
Override (OMSO) write the first access after the soft reset?

Did you try any other workarounds besides setting the clock mode before
doing the OMSO write?

And REF_CLK (pin 16) is not connected? 

> Boot trace:
> [   22.277785] fec 800f0000.ethernet eth0: Freescale FEC PHY driver [Micrel KSZ8031] (mii_bus:phy_addr=800f0000.etherne:00, irq=-1)
> [   22.292527] IPv6: ADDRCONF(NETDEV_UP): eth0: link is not ready
> [   24.276217] libphy: 800f0000.etherne:00 - Link is Up - 100/Full
> [   24.285094] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready

Ok, so you use a single PHY strapped to address 0. 

Would you able to test my series on your setup, and possibly a couple of
diagnostic patches on top?

Thanks,
Johan

RE: phy/micrel: KSZ8031RNL RMII clock reconfiguration bug

[Bruno Thomsen]

> Did you specify a led-mode as well, or was the Operation Mode Strap Override (OMSO) write the first access after the soft reset?
No led-mode was specified so OMSO was the first write.

> Did you try any other workarounds besides setting the clock mode before doing the OMSO write?
I spend around 2 weeks hunting down the bug.
During which I tried many things in both the Freescale FEC MAC driver and the Micrel PHY driver.
Changing the init and the probe flow as well as adding a lot of extra debug traces.

> And REF_CLK (pin 16) is not connected? 
Yes, pin 16 is floating.

> Would you able to test my series on your setup, and possibly a couple of diagnostic patches on top?
Sure, I can try later this week.

/Bruno

Re: phy/micrel: KSZ8031RNL RMII clock reconfiguration bug

[Johan Hovold]

On Mon, Nov 17, 2014 at 02:56:45PM +0000, Bruno Thomsen wrote:
> > Did you specify a led-mode as well, or was the Operation Mode Strap
> > Override (OMSO) write the first access after the soft reset?
>
> No led-mode was specified so OMSO was the first write.

Did you try setting a led-mode before changing the clock mode? Perhaps
that could also trigger the problem (i.e. when the clock-mode change
isn't the first write after reset).

> > Would you able to test my series on your setup, and possibly a
> > couple of diagnostic patches on top?
>
> Sure, I can try later this week.

Much appreciated. I'll resend the last part of the series (which is not
already in linux-next) and make sure to put you on CC.

Thanks,
Johan