[U-Boot-Users] [PATCH 2/2 (resubmit)] NET: Add Ethernet 1000BASE-X support for PPC4xx

[Ben Warren <bwarren@qstreams.com>]

Larry,

I think this can be simplified a bit. More later on...

Larry Johnson wrote:
> This patch adds a new switch: "CONFIG_PHY_DYNAMIC_ANEG".  When this symbol
> is defined, the PHY will advertise it's capabilities for autonegotiation
> based on the capabilities shown in the PHY's status registers, including
> 1000BASE-X.  When "CONFIG_PHY_DYNAMIC_ANEG" is not defined, the PHY will
> advertise hard-coded capabilities, as before.
>
> Signed-off-by: Larry Johnson <lrj@acm.org>
> ---
>
>  common/miiphyutil.c |  155 +++++++++++++++++++++++++++++++++------------------
>  include/miiphy.h    |   21 +++++++
>  2 files changed, 121 insertions(+), 55 deletions(-)
>
> diff --git a/common/miiphyutil.c b/common/miiphyutil.c
> index 58ebc5e..b2f62d0 100644
> --- a/common/miiphyutil.c
> +++ b/common/miiphyutil.c
> @@ -344,101 +344,146 @@ int miiphy_reset (char *devname, unsigned char addr)
>
>  /*****************************************************************************
>   *
> - * Determine the ethernet speed (10/100).
> + * Determine the ethernet speed (10/100/1000).  Return 10 on error.
>   */
>  int miiphy_speed (char *devname, unsigned char addr)
>  {
> -	unsigned short reg;
> +	u16 bmcr;
>
>  #if defined(CONFIG_PHY_GIGE)
> -	if (miiphy_read (devname, addr, PHY_1000BTSR, &reg)) {
> -		printf ("PHY 1000BT Status read failed\n");
> -	} else {
> -		if (reg != 0xFFFF) {
> -			if ((reg & (PHY_1000BTSR_1000FD | PHY_1000BTSR_1000HD))
> -			    != 0) {
> -				return (_1000BASET);
> -			}
> +	u16 btsr;
> +
> +#if defined(CONFIG_PHY_DYNAMIC_ANEG)
>   
I don't think you need this CONFIG. It doesn't really do anything.
> +	u16 bmsr, exsr;
> +
> +	/* Check for 1000BASE-X. */
> +	if (miiphy_read (devname, addr, PHY_BMSR, &bmsr)) {
> +		printf ("PHY status");
> +		goto miiphy_read_failed;
> +	}
> +	if (bmsr & PHY_BMSR_EXT_STAT) {
> +
> +		if (miiphy_read (devname, addr, PHY_EXSR, &exsr)) {
> +			printf ("PHY extended status");
> +			goto miiphy_read_failed;
> +		}
> +		if (exsr & (PHY_EXSR_1000XF | PHY_EXSR_1000XH)) {
> +			/* 1000BASE-X */
> +			return _1000BASET;
>   
Per IEEE 802.3-2005, All PHYs with capabilities > 100Mbps must have 
PHY_MBSR_EXT_STAT set, and EXSR contains capability info for 1000BX and 
1000BT, so you can check for all four here. Please correct me if I'm wrong.
>  		}
>  	}
> +#endif /* defined(CONFIG_PHY_DYNAMIC_ANEG) */
> +
> +	/* Check for 1000BASE-T. */
> +	if (miiphy_read (devname, addr, PHY_1000BTSR, &btsr)) {
> +		printf ("PHY 1000BT status");
> +		goto miiphy_read_failed;
> +	}
> +	if (btsr != 0xFFFF &&
> +	    (btsr & (PHY_1000BTSR_1000FD | PHY_1000BTSR_1000HD))) {
> +		return _1000BASET;
> +	}
>   
And then you don't need to check 1000BTSR
>  #endif /* CONFIG_PHY_GIGE */
>
>  	/* Check Basic Management Control Register first. */
> -	if (miiphy_read (devname, addr, PHY_BMCR, &reg)) {
> -		puts ("PHY speed read failed, assuming 10bT\n");
> -		return (_10BASET);
> +	if (miiphy_read (devname, addr, PHY_BMCR, &bmcr)) {
> +		printf ("PHY speed");
> +		goto miiphy_read_failed;
>  	}
>  	/* Check if auto-negotiation is on. */
> -	if ((reg & PHY_BMCR_AUTON) != 0) {
> +	if (bmcr & PHY_BMCR_AUTON) {
>  		/* Get auto-negotiation results. */
> -		if (miiphy_read (devname, addr, PHY_ANLPAR, &reg)) {
> -			puts ("PHY AN speed read failed, assuming 10bT\n");
> -			return (_10BASET);
> -		}
> -		if ((reg & PHY_ANLPAR_100) != 0) {
> -			return (_100BASET);
> -		} else {
> -			return (_10BASET);
> +		u16 anlpar;
>   
Please move the anlpar declaration to the top of the function
> +
> +		if (miiphy_read (devname, addr, PHY_ANLPAR, &anlpar)) {
> +			printf ("PHY AN speed");
> +			goto miiphy_read_failed;
>  		}
> +		return (anlpar & PHY_ANLPAR_100) ? _100BASET : _10BASET;
>  	}
>  	/* Get speed from basic control settings. */
> -	else if (reg & PHY_BMCR_100MB) {
> -		return (_100BASET);
> -	} else {
> -		return (_10BASET);
> -	}
> +	return (bmcr & PHY_BMCR_100MB) ? _100BASET : _10BASET;
>
> +      miiphy_read_failed:
> +	printf (" read failed, assuming 10BASE-T\n");
> +	return _10BASET;
>  }
>
>  /*****************************************************************************
>   *
> - * Determine full/half duplex.
> + * Determine full/half duplex.  Return half on error.
>   */
>  int miiphy_duplex (char *devname, unsigned char addr)
>  {
> -	unsigned short reg;
> +	u16 bmcr;
>
>  #if defined(CONFIG_PHY_GIGE)
> -	if (miiphy_read (devname, addr, PHY_1000BTSR, &reg)) {
> -		printf ("PHY 1000BT Status read failed\n");
> -	} else {
> -		if ((reg != 0xFFFF) &&
> -		    (reg & (PHY_1000BTSR_1000FD | PHY_1000BTSR_1000HD))) {
> -			if ((reg & PHY_1000BTSR_1000FD) != 0) {
> -				return (FULL);
> -			} else {
> -				return (HALF);
> +	u16 btsr;
> +
> +#if defined(CONFIG_PHY_DYNAMIC_ANEG)
>   
Same as above. Don't need it, I don't think.
> +	u16 bmsr, exsr;
> +
> +	/* Check for 1000BASE-X. */
> +	if (miiphy_read (devname, addr, PHY_BMSR, &bmsr)) {
> +		printf ("PHY status");
> +		goto miiphy_read_failed;
> +	}
> +	if (bmsr & PHY_BMSR_EXT_STAT) {
> +
> +		if (miiphy_read (devname, addr, PHY_EXSR, &exsr)) {
> +			printf ("PHY extended status");
> +			goto miiphy_read_failed;
> +		}
> +		if (exsr & (PHY_EXSR_1000XF | PHY_EXSR_1000XH)) {
> +			/* 1000BASE-X */
> +			u16 anlpar;
> +
> +			if (miiphy_read (devname, addr, PHY_ANLPAR, &anlpar)) {
> +				printf ("1000BASE-X PHY AN duplex");
> +				goto miiphy_read_failed;
>  			}
> +			return (anlpar & PHY_X_ANLPAR_FD) ? FULL : HALF;
> +		}
> +	}
> +#endif /* defined(CONFIG_PHY_DYNAMIC_ANEG) */
> +
> +	/* Check for 1000BASE-T. */
> +	if (miiphy_read (devname, addr, PHY_1000BTSR, &btsr)) {
> +		printf ("PHY 1000BT status");
> +		goto miiphy_read_failed;
> +	}
> +	if (btsr != 0xFFFF) {
> +		if (btsr & PHY_1000BTSR_1000FD) {
> +			return FULL;
> +		} else if (btsr & PHY_1000BTSR_1000HD) {
> +			return HALF;
>  		}
>   
Same as above. All GigE capabilities are listed in EXSR
>  	}
>  #endif /* CONFIG_PHY_GIGE */
>
>  	/* Check Basic Management Control Register first. */
> -	if (miiphy_read (devname, addr, PHY_BMCR, &reg)) {
> -		puts ("PHY duplex read failed, assuming half duplex\n");
> -		return (HALF);
> +	if (miiphy_read (devname, addr, PHY_BMCR, &bmcr)) {
> +		puts ("PHY duplex");
> +		goto miiphy_read_failed;
>  	}
>  	/* Check if auto-negotiation is on. */
> -	if ((reg & PHY_BMCR_AUTON) != 0) {
> +	if (bmcr & PHY_BMCR_AUTON) {
>  		/* Get auto-negotiation results. */
> -		if (miiphy_read (devname, addr, PHY_ANLPAR, &reg)) {
> -			puts ("PHY AN duplex read failed, assuming half duplex\n");
> -			return (HALF);
> -		}
> +		u16 anlpar;
>   
See comment above
> -		if ((reg & (PHY_ANLPAR_10FD | PHY_ANLPAR_TXFD)) != 0) {
> -			return (FULL);
> -		} else {
> -			return (HALF);
> +		if (miiphy_read (devname, addr, PHY_ANLPAR, &anlpar)) {
> +			puts ("PHY AN duplex");
> +			goto miiphy_read_failed;
>  		}
> +		return (anlpar & (PHY_ANLPAR_10FD | PHY_ANLPAR_TXFD)) ?
> +		    FULL : HALF;
>  	}
>  	/* Get speed from basic control settings. */
> -	else if (reg & PHY_BMCR_DPLX) {
> -		return (FULL);
> -	} else {
> -		return (HALF);
> -	}
> +	return (bmcr & PHY_BMCR_DPLX) ? FULL : HALF;
>
> +      miiphy_read_failed:
> +	printf (" read failed, assuming half duplex\n");
> +	return HALF;
>  }
>
>  #ifdef CFG_FAULT_ECHO_LINK_DOWN
> diff --git a/include/miiphy.h b/include/miiphy.h
> index 42f2ad0..83234b9 100644
> --- a/include/miiphy.h
> +++ b/include/miiphy.h
> @@ -85,6 +85,7 @@ int bb_miiphy_write (char *devname, unsigned char addr,
>  #define PHY_ANLPNP		0x08
>  #define PHY_1000BTCR		0x09
>  #define PHY_1000BTSR		0x0A
> +#define PHY_EXSR		0x0F
>  #define PHY_PHYSTS		0x10
>  #define PHY_MIPSCR		0x11
>  #define PHY_MIPGSR		0x12
> @@ -118,6 +119,7 @@ int bb_miiphy_write (char *devname, unsigned char addr,
>  #define PHY_BMSR_100TXH		0x2000
>  #define PHY_BMSR_10TF		0x1000
>  #define PHY_BMSR_10TH		0x0800
> +#define PHY_BMSR_EXT_STAT	0x0100
>  #define PHY_BMSR_PRE_SUP	0x0040
>  #define PHY_BMSR_AUTN_COMP	0x0020
>  #define PHY_BMSR_RF		0x0010
> @@ -130,17 +132,30 @@ int bb_miiphy_write (char *devname, unsigned char addr,
>  #define PHY_ANLPAR_NP		0x8000
>  #define PHY_ANLPAR_ACK		0x4000
>  #define PHY_ANLPAR_RF		0x2000
> +#define PHY_ANLPAR_ASYMP	0x0800
> +#define PHY_ANLPAR_PAUSE	0x0400
>  #define PHY_ANLPAR_T4		0x0200
>  #define PHY_ANLPAR_TXFD		0x0100
>  #define PHY_ANLPAR_TX		0x0080
>  #define PHY_ANLPAR_10FD		0x0040
>  #define PHY_ANLPAR_10		0x0020
>  #define PHY_ANLPAR_100		0x0380	/* we can run at 100 */
> +/* phy ANLPAR 1000BASE-X */
> +#define PHY_X_ANLPAR_NP		0x8000
> +#define PHY_X_ANLPAR_ACK	0x4000
> +#define PHY_X_ANLPAR_RF_MASK	0x3000
> +#define PHY_X_ANLPAR_PAUSE_MASK	0x0180
> +#define PHY_X_ANLPAR_HD		0x0040
> +#define PHY_X_ANLPAR_FD		0x0020
>
>  #define PHY_ANLPAR_PSB_MASK	0x001f
>  #define PHY_ANLPAR_PSB_802_3	0x0001
>  #define PHY_ANLPAR_PSB_802_9	0x0002
>
> +/* phy 1000BTCR */
> +#define PHY_1000BTCR_1000FD	0x0200
> +#define PHY_1000BTCR_1000HD	0x0100
> +
>  /* phy 1000BTSR */
>  #define PHY_1000BTSR_MSCF	0x8000
>  #define PHY_1000BTSR_MSCR	0x4000
> @@ -149,4 +164,10 @@ int bb_miiphy_write (char *devname, unsigned char addr,
>  #define PHY_1000BTSR_1000FD	0x0800
>  #define PHY_1000BTSR_1000HD	0x0400
>
> +/* phy EXSR */
> +#define PHY_EXSR_1000XF		0x8000
> +#define PHY_EXSR_1000XH		0x4000
> +#define PHY_EXSR_1000TF		0x2000
> +#define PHY_EXSR_1000TH		0x1000
> +
>  #endif
>
> -------------------------------------------------------------------------
> This SF.net email is sponsored by: Splunk Inc.
> Still grepping through log files to find problems?  Stop.
> Now Search log events and configuration files using AJAX and a browser.
> Download your FREE copy of Splunk now >> http://get.splunk.com/
> _______________________________________________
> U-Boot-Users mailing list
> U-Boot-Users at lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/u-boot-users
>
>   
Nice work.

regards,
Ben