Re: [PATCH 2/2] phy: add smsc phy driver support

[Sascha Hauer <s.hauer@pengutronix.de>]

On Thu, Nov 15, 2012 at 10:31:44AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> as we to have specific phy init to fix chip issue link detection support

This patch was introduced to make the LAN8720 work with phylib. When
appplying this patch I believed that...

> +
> +static int lan87xx_config_init(struct phy_device *phydev)
> +{
> +	/*
> +	 * Make sure the EDPWRDOWN bit is NOT set. Setting this bit on
> +	 * LAN8710/LAN8720 PHY causes the PHY to misbehave, likely due
> +	 * to a bug on the chip.
> +	 *
> +	 * When the system is powered on with the network cable being
> +	 * disconnected all the way until after ifconfig ethX up is
> +	 * issued for the LAN port with this PHY, connecting the cable
> +	 * afterwards does not cause LINK change detection, while the
> +	 * expected behavior is the Link UP being detected.
> +	 */
> +	int rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
> +	if (rc < 0)
> +		return rc;
> +
> +	rc &= ~MII_LAN83C185_EDPWRDOWN;
> +
> +	rc = phy_write(phydev, MII_LAN83C185_CTRL_STATUS, rc);
> +	if (rc < 0)
> +		return rc;

... this change was responsible to let the phy driver correctly wait for
a link. As it just turned out, this change os totally irrelevant. The
change which makes this driver work is...

> +}, {
> +	.phy_id		= 0x0007c0f0, /* OUI=0x00800f, Model#=0x0f */
> +	.phy_id_mask	= 0xfffffff0,
> +	.drv.name	= "SMSC LAN8710/LAN8720",
> +
> +	.features	= (PHY_BASIC_FEATURES | SUPPORTED_Pause
> +				| SUPPORTED_Asym_Pause),

... this one. The phy now advertises SUPPORTED_Pause and
SUPPORTED_Asym_Pause which the generic phy driver does not do. This
causes the adv != oldadv check in genphy_config_advert to become true.
genphy_config_advert then returns '1' for advertise changed, which
causes genphy_config_aneg to call genphy_restart_aneg which calls
phy_aneg_done to wait for a link.

So this patch is bogus and by accident fixes a problem. The real problem
is that current phylib does not wait (reliably) for a link. This problem still
persists and is still very annoying. I just have a USB network adapter
here which needs a three second delay after discovering it to make it
work with barebox.

We need a phy_aneg_done() before trying to do network transfers and we
need that *unconditionally*.

Jean-Christophe, you can choose where you want to have this call, but it
will go in. We are carrying these phylib regressions for several months
now.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox