[U-Boot] [PATCH 1/1 v3] ppc4xx: Add support for GPCS, SGMII and M88E1112 PHY

[Victor Gallardo]

Hi Stefan,

OK. I agree, I will remove DFLT_PHY_LESS_SPEED and DFLT_PHY_LESS_DUPLEX.

In terms of a generic PHY-less approach. I'll wait for Ben's input.

-Victor Gallardo

-----Original Message-----
From: Stefan Roese [mailto:sr at denx.de] 
Sent: Wednesday, September 03, 2008 12:23 AM
To: u-boot at lists.denx.de
Cc: Victor Gallardo; Ben Warren
Subject: Re: [U-Boot] [PATCH 1/1 v3] ppc4xx: Add support for GPCS, SGMII
and M88E1112 PHY

On Wednesday 03 September 2008, Victor Gallardo wrote:
> This patch adds GPCS, SGMII and M88E1112 PHY support for the AMCC 
> PPC460GT/EX processors.
>
> Signed-off-by: Victor Gallardo <vgallardo at amcc.com>
> ---

A good idea is to keep a history of what changed in the patch revisions
here in this area (after the "---"). Something like:

v2:
- Added comments to GPCS PHY setup
- Minor coding style cleanup

v3: 
- Generalized the PHY-less configuration even more

Please find some more comments below.

>  cpu/ppc4xx/4xx_enet.c |  162
> ++++++++++++++++++++++++++++++++++++++++++++++++- cpu/ppc4xx/miiphy.c
|  
> 41 ++++++++++++-
>  include/ppc4xx_enet.h |    3 +
>  3 files changed, 201 insertions(+), 5 deletions(-)
>
> diff --git a/cpu/ppc4xx/4xx_enet.c b/cpu/ppc4xx/4xx_enet.c index 
> 8a38335..e137bac 100644
> --- a/cpu/ppc4xx/4xx_enet.c
> +++ b/cpu/ppc4xx/4xx_enet.c
> @@ -198,6 +198,7 @@
>  #define BI_PHYMODE_RMII  8
>  #endif
>  #endif
> +#define BI_PHYMODE_SGMII 9
>
>  #if defined(CONFIG_440SP) || defined(CONFIG_440SPE) || \
>      defined(CONFIG_440EPX) || defined(CONFIG_440GRX) || \ @@ -216,6 
> +217,52 @@
>  #define MAL_RX_CHAN_MUL	1
>  #endif
>
> +/*-------------------------------------------------------------------
> +-+
> + * PHY-less support for Ethernet Ports.
> + 
> +*--------------------------------------------------------------------
> +*/
> +
> +/*
> + * Some boards do not have a PHY for each ethernet port.
> + * For example on Arches board (2 CPU system) eth0 does not have
> + * a PHY, both CPU's are wired directly together (AC coupled)
> + * using SGMII0.
> + *
> + * In these cases :
> + *    1) set the appropriate CONFIG_PHY_ADDR equal to CONFIG_PHY_LESS
> + *       to detect that the specified ethernet port does not have a
PHY.
> + *    2) Then define CFG_PHY_LESS_PORT and CFG_PHY_LESS_PORTS in
board
> + *       configuration file. For example on the Arches board we would
do
> + *       the following.
> + *         #define CFG_PHY_LESS_PORT(devnum,speed,duplex) \
> + *                 { devnum, speed, duplex},
> + *         #define CFG_PHY_LESS_PORTS \
> + *                 CFG_PHY_LESS_PORT(0,1000,FULL)
> + */
> +#if !defined(CONFIG_PHY_LESS)
> +#define CONFIG_PHY_LESS	0xFFFFFFFF /* PHY-less mode */
> +#endif

If we agree that this is a good generic approach for this PHY-less
handling, then we should probably move this to a common header so that
other ethernet driver can use this too.

Ben, what do you think?

And the description should be moved to a common place too. Either the
toplevel README, or a new README.xxx in the doc directory.

> +
> +#define DFLT_PHY_LESS_SPEED	100
> +#define DFLT_PHY_LESS_DUPLEX	FULL

Do we really need these defaults? Please see my comment further down.

> +
> +/*
> + * CFG_PHY_LESS_PORTS tells us about ethernet ports that have no PHY
> + * attached to them.
> + */
> +#ifndef CFG_PHY_LESS_PORTS
> +#define CFG_PHY_LESS_PORTS
> +#endif
> +
> +struct phy_less_port {
> +	unsigned int devnum;	/* ethernet port */
> +	unsigned int speed;	/* specified speed 10,100 or 1000 */
> +	unsigned int duplex;	/* specified duplex FULL or HALF */
> +};

Again, if we agree that this is a good "solution", then this should be
moved into a common header, probably net.h.

<snip>

> -	speed = miiphy_speed (dev->name, reg);
> -	duplex = miiphy_duplex (dev->name, reg);
> +get_speed:
> +	if (reg == CONFIG_PHY_LESS) {
> +		speed = DFLT_PHY_LESS_SPEED;
> +		duplex = DFLT_PHY_LESS_DUPLEX;

I don't think we need this defaults here. Remove them and...

> +		for (i = 0; i < ARRAY_SIZE(phy_less_port); i++) {
> +			if (devnum == phy_less_port[i].devnum) {
> +				speed = phy_less_port[i].speed;
> +				duplex = phy_less_port[i].duplex;
> +				break;
> +			}
> +		}

...add this here:

		if (i == ARRAY_SIZE(phy_less_port)) {
			printf("ERROR: PHY (%s) not configured
correctly!\n",
				dev->name);
			return -1;
		}

If the PHY-less device is not in the list, then this is a
misconfiguration which should be fixed IMHO.

Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================