[U-Boot] [PATCH 4/7] Create PHY Lib for U-Boot

[Detlev Zundel]

Hi Andy,

> Extends the mii_dev structure to participate in a full-blown MDIO and
> PHY driver scheme.  The mii_dev structure and miiphy calls are modified
> in such a way to allow the original mii command and miiphy
> infrastructure to work as before, but also to support a new set of APIs
> which allow (among other things) sharing of PHY driver code and 10G support
>
> The mii command will continue to support normal PHY management functions
> (Clause 22 of 802.3), but will not be changed to support 10G
> (Clause 45).
>
> The basic design is similar to PHY Lib from Linux, but simplified for
> U-Boot's network and driver infrastructure.
>
> We now have MDIO drivers and PHY drivers
>
> An MDIO driver provides:
> read
> write
> reset
>
> A PHY driver provides:
> (optionally): probe
> config - initial setup, starting of auto-negotiation
> startup - waiting for AN, and reading link state
> shutdown - any cleanup needed
>
> The ethernet drivers interact with the PHY Lib using these functions:
> phy_connect()
> phy_config()
> phy_startup()
> phy_shutdown()
>
> Each PHY driver can be configured separately, or all at once using
> phylib_all_drivers.h (added in the patch which adds the drivers)
>
> Signed-off-by: Andy Fleming <afleming at freescale.com>

Looks really good - a few comments:

[...]

> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> new file mode 100644
> index 0000000..ca35404
> --- /dev/null
> +++ b/drivers/net/phy/phy.c
> @@ -0,0 +1,705 @@
> +/*
> + * Generic PHY Management code
> + *
> + * This software may be used and distributed according to the
> + * terms of the GNU Public License, Version 2, incorporated
> + * herein by reference.
> + *
> + * Copyright 2011 Freescale Semiconductor, Inc.
> + * author Andy Fleming
> + *
> + * Based loosely off of Linux's PHY Lib

According to http://www.denx.de/wiki/U-Boot/Patches new files need
GPLv2+ headers and if I check Linux, I see GPLv2+ in
drivers/net/phy/phy.c (and other people have contributed under this), so
please change accrodingly.

> +/**
> + * genphy_update_link - update link status in @phydev
> + * @phydev: target phy_device struct
> + *
> + * Description: Update the value in phydev->link to reflect the
> + *   current link value.  In order to do this, we need to read
> + *   the status register twice, keeping the second value.
> + */
> +int genphy_update_link(struct phy_device *phydev)
> +{
> +	unsigned int mii_reg;
> +
> +	/*
> +	 * Wait if the link is up, and autonegotiation is in progress
> +	 * (ie - we're capable and it's not done)
> +	 */
> +	mii_reg = phy_read(phydev, MDIO_DEVAD_NONE, MII_BMSR);
> +
> +	/*
> +	 * If we already saw the link up, and it hasn't gone down, then
> +	 * we don't need to wait for autoneg again
> +	 */
> +	if (phydev->link && mii_reg & BMSR_LSTATUS)
> +		return 0;
> +
> +	if ((mii_reg & BMSR_ANEGCAPABLE) && !(mii_reg & BMSR_ANEGCOMPLETE)) {
> +		int i = 0;
> +
> +		printf("%s Waiting for PHY auto negotiation to complete",
> +			phydev->dev->name);
> +		while (!(mii_reg & BMSR_ANEGCOMPLETE)) {
> +			/*
> +			 * Timeout reached ?
> +			 */
> +			if (i > 5000) {
> +				printf(" TIMEOUT !\n");
> +				phydev->link = 0;
> +				return 0;
> +			}
> +
> +			if (ctrlc()) {
> +				puts("user interrupt!\n");
> +				phydev->link = 0;
> +				return -EINTR;
> +			}
> +
> +			if ((i++ % 500) == 0)
> +				printf(".");
> +
> +			udelay(1000);	/* 1 ms */
> +			mii_reg = phy_read(phydev, MDIO_DEVAD_NONE, MII_BMSR);
> +		}
> +		printf(" done\n");
> +		phydev->link = 1;
> +		udelay(500000);	/* another 500 ms (results in faster booting) */

I don't understand this, why does sleeping half a second speed up
booting?  A comment would be in order here.

> +	} else {
> +		/* Read the link a second time to clear the latched state */
> +		mii_reg = phy_read(phydev, MDIO_DEVAD_NONE, MII_BMSR);
> +
> +		if (mii_reg & BMSR_LSTATUS)
> +			phydev->link = 1;
> +		else
> +			phydev->link = 0;
> +	}
> +
> +	return 0;
> +}

[...]

> +struct phy_driver *get_phy_driver(struct phy_device *phydev,
> +				phy_interface_t interface)
> +{
> +	struct list_head *entry;
> +	int phy_id = phydev->phy_id;
> +	struct phy_driver *drv = NULL;
> +
> +	list_for_each(entry, &phy_drivers) {
> +		drv = list_entry(entry, struct phy_driver, list);
> +		if ((drv->uid & drv->mask) == (phy_id & drv->mask))
> +			return drv;
> +	}
> +
> +	/* If we made it here, there's no driver for this PHY */
> +	if (interface == PHY_INTERFACE_MODE_XGMII)
> +		return &gen10g_driver;
> +	else
> +		return &genphy_driver;
> +}

Maybe output a warning when the generic driver is used?  From what I see
later we should get a message from phy_connect, correct?  If this is the
case, then disregard this comment ;)

Apart from these minor things, the code looks really good.  I'm looking
forward to seeing this merged, thanks a lot!
  Detlev

-- 
Als ich entführt wurde, begannen meine Eltern aktiv zu werden.
Sie vermieteten mein Zimmer.
                                        -- Woody Allen
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu at denx.de