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

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

[Victor Gallardo]

This patch adds GPCS, SGMII and M88E1112 PHY support
for the AMCC PPC460GT/EX processors.

Signed-off-by: Victor Gallardo <vgallardo@amcc.com>
---
 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
+
+#define DFLT_PHY_LESS_SPEED	100
+#define DFLT_PHY_LESS_DUPLEX	FULL
+
+/*
+ * 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 */
+};
+
+static const struct phy_less_port phy_less_port[] = {
+	CFG_PHY_LESS_PORTS	/* defined in board configuration file */
+};
+
 /*-----------------------------------------------------------------------------+
  * Global variables. TX and RX descriptors and buffers.
  *-----------------------------------------------------------------------------*/
@@ -611,8 +658,17 @@ int ppc_4xx_eth_setup_bridge(int devnum, bd_t * bis)
 
 #if defined(CONFIG_460EX)
 	mode = 9;
+	mfsdr(SDR0_ETH_CFG, eth_cfg);
+	if (((eth_cfg & SDR0_ETH_CFG_SGMII0_ENABLE) > 0) &&
+	    ((eth_cfg & SDR0_ETH_CFG_SGMII1_ENABLE) > 0))
+		mode = 11; /* config SGMII */
 #else
 	mode = 10;
+	mfsdr(SDR0_ETH_CFG, eth_cfg);
+	if (((eth_cfg & SDR0_ETH_CFG_SGMII0_ENABLE) > 0) &&
+	    ((eth_cfg & SDR0_ETH_CFG_SGMII1_ENABLE) > 0) &&
+	    ((eth_cfg & SDR0_ETH_CFG_SGMII2_ENABLE) > 0))
+		mode = 12; /* config SGMII */
 #endif
 
 	/* TODO:
@@ -635,6 +691,8 @@ int ppc_4xx_eth_setup_bridge(int devnum, bd_t * bis)
 	/*
 	 * Right now only 2*RGMII is supported. Please extend when needed.
 	 * sr - 2008-02-19
+	 * Add SGMII support.
+	 * vg - 2008-07-28
 	 */
 	switch (mode) {
 	case 1:
@@ -761,6 +819,20 @@ int ppc_4xx_eth_setup_bridge(int devnum, bd_t * bis)
 		bis->bi_phymode[2] = BI_PHYMODE_RGMII;
 		bis->bi_phymode[3] = BI_PHYMODE_RGMII;
 		break;
+	case 11:
+		/* 2 SGMII - 460EX */
+		bis->bi_phymode[0] = BI_PHYMODE_SGMII;
+		bis->bi_phymode[1] = BI_PHYMODE_SGMII;
+		bis->bi_phymode[2] = BI_PHYMODE_NONE;
+		bis->bi_phymode[3] = BI_PHYMODE_NONE;
+		break;
+	case 12:
+		/* 3 SGMII - 460GT */
+		bis->bi_phymode[0] = BI_PHYMODE_SGMII;
+		bis->bi_phymode[1] = BI_PHYMODE_SGMII;
+		bis->bi_phymode[2] = BI_PHYMODE_SGMII;
+		bis->bi_phymode[3] = BI_PHYMODE_NONE;
+		break;
 	default:
 		break;
 	}
@@ -945,6 +1017,48 @@ static int ppc_4xx_eth_init (struct eth_device *dev, bd_t * bis)
 	out_be32((void *)EMAC_M1 + hw_p->hw_addr, mode_reg);
 #endif /* defined(CONFIG_440GX) || defined(CONFIG_440SP) */
 
+#if defined(CONFIG_GPCS_PHY_ADDR) || defined(CONFIG_GPCS_PHY1_ADDR) || \
+    defined(CONFIG_GPCS_PHY2_ADDR) || defined(CONFIG_GPCS_PHY3_ADDR)
+	if (bis->bi_phymode[devnum] == BI_PHYMODE_SGMII) {
+		/*
+		 * In SGMII mode, GPCS access is needed for
+		 * communication with the internal SGMII SerDes.
+		 */
+		switch (devnum) {
+#if defined(CONFIG_GPCS_PHY_ADDR)
+		case 0:
+			reg = CONFIG_GPCS_PHY_ADDR;
+			break;
+#endif
+#if defined(CONFIG_GPCS_PHY1_ADDR)
+		case 1:
+			reg = CONFIG_GPCS_PHY1_ADDR;
+			break;
+#endif
+#if defined(CONFIG_GPCS_PHY2_ADDR)
+		case 2:
+			reg = CONFIG_GPCS_PHY2_ADDR;
+			break;
+#endif
+#if defined(CONFIG_GPCS_PHY3_ADDR)
+		case 3:
+			reg = CONFIG_GPCS_PHY3_ADDR;
+			break;
+#endif
+		}
+
+		mode_reg = in_be32((void *)EMAC_M1 + hw_p->hw_addr);
+		mode_reg |= EMAC_M1_MF_1000GPCS | EMAC_M1_IPPA_SET(reg);
+		out_be32((void *)EMAC_M1 + hw_p->hw_addr, mode_reg);
+
+		/* Configure GPCS interface to recommended setting for SGMII */
+		miiphy_reset(dev->name, reg);
+		miiphy_write(dev->name, reg, 0x04, 0x8120); /* AsymPause, FDX */
+		miiphy_write(dev->name, reg, 0x07, 0x2801); /* msg_pg, toggle */
+		miiphy_write(dev->name, reg, 0x00, 0x0140); /* 1Gbps, FDX     */
+	}
+#endif /* defined(CONFIG_GPCS_PHY_ADDR) */
+
 	/* wait for PHY to complete auto negotiation */
 	reg_short = 0;
 #ifndef CONFIG_CS8952_PHY
@@ -974,6 +1088,9 @@ static int ppc_4xx_eth_init (struct eth_device *dev, bd_t * bis)
 
 	bis->bi_phynum[devnum] = reg;
 
+	if (reg == CONFIG_PHY_LESS)
+		goto get_speed;
+
 #if defined(CONFIG_PHY_RESET)
 	/*
 	 * Reset the phy, only if its the first time through
@@ -986,6 +1103,33 @@ static int ppc_4xx_eth_init (struct eth_device *dev, bd_t * bis)
 		miiphy_write (dev->name, reg, 0x09, 0x0e00);
 		miiphy_write (dev->name, reg, 0x04, 0x01e1);
 #endif
+#if defined(CONFIG_M88E1112_PHY)
+		if (bis->bi_phymode[devnum] == BI_PHYMODE_SGMII) {
+			/*
+			 * Marvell 88E1112 PHY needs to have the SGMII MAC
+			 * interace (page 2) properly configured to
+			 * communicate with the 460EX/GT GPCS interface.
+			 */
+
+			/* Set access to Page 2 */
+			miiphy_write(dev->name, reg, 0x16, 0x0002);
+
+			miiphy_write(dev->name, reg, 0x00, 0x0040); /* 1Gbps */
+			miiphy_read(dev->name, reg, 0x10, &reg_short);
+			reg_short &= ~0x0C00; /* Preferred Media MASK */
+			reg_short |=  0x0800; /* Preferred Media Copper */
+			reg_short &= ~0x0380; /* Mode Select MASK */
+			reg_short |=  0x0280; /* Mode Select Copper only */
+			miiphy_write(dev->name, reg, 0x10, reg_short);
+			miiphy_read(dev->name, reg, 0x1a, &reg_short);
+			reg_short |= 0x8000; /* bypass Auto-Negotiation */
+			miiphy_write(dev->name, reg, 0x1a, reg_short);
+			miiphy_reset(dev->name, reg); /* reset MAC interface */
+
+			/* Reset access to Page 0 */
+			miiphy_write(dev->name, reg, 0x16, 0x0000);
+		}
+#endif /* defined(CONFIG_M88E1112_PHY) */
 		miiphy_reset (dev->name, reg);
 
 #if defined(CONFIG_440GX) || \
@@ -1080,8 +1224,22 @@ static int ppc_4xx_eth_init (struct eth_device *dev, bd_t * bis)
 	}
 #endif /* #ifndef CONFIG_CS8952_PHY */
 
-	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;
+
+		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;
+			}
+		}
+	} else {
+		speed = miiphy_speed(dev->name, reg);
+		duplex = miiphy_duplex(dev->name, reg);
+	}
 
 	if (hw_p->print_speed) {
 		hw_p->print_speed = 0;
diff --git a/cpu/ppc4xx/miiphy.c b/cpu/ppc4xx/miiphy.c
index c882720..d303598 100644
--- a/cpu/ppc4xx/miiphy.c
+++ b/cpu/ppc4xx/miiphy.c
@@ -180,8 +180,10 @@ int phy_setup_aneg (char *devname, unsigned char addr)
  *
  * sr: Currently on 460EX only EMAC0 works with MDIO, so we always
  * return EMAC0 offset here
+ * vg: For 460EX/460GT if internal GPCS PHY address is specified
+ * return appropriate EMAC offset
  */
-unsigned int miiphy_getemac_offset (void)
+unsigned int miiphy_getemac_offset(u8 addr)
 {
 #if (defined(CONFIG_440) && \
     !defined(CONFIG_440SP) && !defined(CONFIG_440SPE) && \
@@ -233,6 +235,39 @@ unsigned int miiphy_getemac_offset (void)
 		return 0x100;
 #endif
 
+#if defined(CONFIG_460EX) || defined(CONFIG_460GT)
+	u32 mode_reg;
+	u32 eoffset = 0;
+
+	switch (addr) {
+#if defined(CONFIG_HAS_ETH1) && defined(CONFIG_GPCS_PHY1_ADDR)
+	case CONFIG_GPCS_PHY1_ADDR:
+		mode_reg = in_be32((void *)EMAC_M1 + 0x100);
+		if (addr == EMAC_M1_IPPA_GET(mode_reg))
+			eoffset = 0x100;
+		break;
+#endif
+#if defined(CONFIG_HAS_ETH2) && defined(CONFIG_GPCS_PHY2_ADDR)
+	case CONFIG_GPCS_PHY2_ADDR:
+		mode_reg = in_be32((void *)EMAC_M1 + 0x300);
+		if (addr == EMAC_M1_IPPA_GET(mode_reg))
+			eoffset = 0x300;
+		break;
+#endif
+#if defined(CONFIG_HAS_ETH3) && defined(CONFIG_GPCS_PHY3_ADDR)
+	case CONFIG_GPCS_PHY3_ADDR:
+		mode_reg = in_be32((void *)EMAC_M1 + 0x400);
+		if (addr == EMAC_M1_IPPA_GET(mode_reg))
+			eoffset = 0x400;
+		break;
+#endif
+	default:
+		eoffset = 0;
+		break;
+	}
+	return eoffset;
+#endif
+
 	return 0;
 #endif
 }
@@ -262,7 +297,7 @@ static int emac_miiphy_command(u8 addr, u8 reg, int cmd, u16 value)
 	u32 emac_reg;
 	u32 sta_reg;
 
-	emac_reg = miiphy_getemac_offset();
+	emac_reg = miiphy_getemac_offset(addr);
 
 	/* wait for completion */
 	if (emac_miiphy_wait(emac_reg) != 0)
@@ -311,7 +346,7 @@ int emac4xx_miiphy_read (char *devname, unsigned char addr, unsigned char reg,
 	unsigned long sta_reg;
 	unsigned long emac_reg;
 
-	emac_reg = miiphy_getemac_offset ();
+	emac_reg = miiphy_getemac_offset(addr);
 
 	if (emac_miiphy_command(addr, reg, EMAC_STACR_READ, 0) != 0)
 		return -1;
diff --git a/include/ppc4xx_enet.h b/include/ppc4xx_enet.h
index b74c6fc..00669a7 100644
--- a/include/ppc4xx_enet.h
+++ b/include/ppc4xx_enet.h
@@ -376,6 +376,7 @@ typedef struct emac_4xx_hw_st {
 #define EMAC_M1_APP		(0x08000000)
 #define EMAC_M1_RSVD		(0x06000000)
 #define EMAC_M1_IST		(0x01000000)
+#define EMAC_M1_MF_1000GPCS	(0x00C00000)
 #define EMAC_M1_MF_1000MBPS	(0x00800000)	/* 0's for 10MBPS */
 #define EMAC_M1_MF_100MBPS	(0x00400000)
 #define EMAC_M1_RFS_MASK	(0x00380000)
@@ -394,6 +395,8 @@ typedef struct emac_4xx_hw_st {
 #define EMAC_M1_MWSW		(0x00007000)
 #define EMAC_M1_JUMBO_ENABLE	(0x00000800)
 #define EMAC_M1_IPPA		(0x000007c0)
+#define EMAC_M1_IPPA_SET(id)	(((id) & 0x1f) << 6)
+#define EMAC_M1_IPPA_GET(id)	(((id) >> 6) & 0x1f)
 #define EMAC_M1_OBCI_GT100	(0x00000020)
 #define EMAC_M1_OBCI_100	(0x00000018)
 #define EMAC_M1_OBCI_83		(0x00000010)
-- 
1.5.5

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

[Stefan Roese]

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@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
=====================================================================

[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@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
=====================================================================

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

[Victor Gallardo]

>> +
>> +/*
>> + * 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.

Maybe a better file for this would be in miiphy.h

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

[Ben Warren]

Hi Stefan and Victor,

Stefan Roese wrote:
> 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@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.
>
>   
I like the idea very much, but am not sure about the implementation.  
This problem has been around for a while (just search the archives for 
people wondering how to deal with a switch chip connected via rvMII or 
whatever).  The trickiest part of this is how to get the information to 
the driver.  I've always thought that the best way would be for board 
code to initialize each controller through proper C code (i.e. not 
CONFIG macros).  But there's definitely something to be said for doing 
it all through macros, since that's how Kconfig works.  Please have a 
look at the code that Andy Fleming recently submitted for the TSEC 
driver (it's in the main branch now).  He passes a tsec_info_struct into 
each call of tsec_initialize(), allowing all type of custom information 
to go in.  In my mind, that could be generalized to something that more 
than just TSEC, but let's take baby steps.

Incidentally, the term "Fixed PHY" has already been coined for what 
you're calling "PHY-less".  I suggest we standardize.

Anyway, I have to go to bed.  Eyes are starting to close and brain's 
sloowwwiing doowwn.

cheers,
Ben

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

[Stefan Roese]

On Thursday 04 September 2008, Ben Warren wrote:
> I like the idea very much, but am not sure about the implementation.
> This problem has been around for a while (just search the archives for
> people wondering how to deal with a switch chip connected via rvMII or
> whatever).  The trickiest part of this is how to get the information to
> the driver.  I've always thought that the best way would be for board
> code to initialize each controller through proper C code (i.e. not
> CONFIG macros).  But there's definitely something to be said for doing
> it all through macros, since that's how Kconfig works.  Please have a
> look at the code that Andy Fleming recently submitted for the TSEC
> driver (it's in the main branch now).  He passes a tsec_info_struct into
> each call of tsec_initialize(), allowing all type of custom information
> to go in.  In my mind, that could be generalized to something that more
> than just TSEC, but let's take baby steps.

Yes, this looks like a good approach. Not sure if we should go all the way or 
accept Victors approach for now. Moving to this parameter based 
initialization is a different matter that should really be done soon.

> Incidentally, the term "Fixed PHY" has already been coined for what
> you're calling "PHY-less".  I suggest we standardize.

Yes. Is there already a define available?

> Anyway, I have to go to bed.  Eyes are starting to close and brain's
> sloowwwiing doowwn.

Heh :)

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
=====================================================================

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

[Victor Gallardo]

Hi Stefan and Ben,

I saw what Andy Fleming's did. This is a bit to much work for what I
have time for.

I aggree, we need to take baby steps..

For now I'll change PHY-less to Fixed PHY and update some style issues.

-Victor Gallardo

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

On Thursday 04 September 2008, Ben Warren wrote:
> I like the idea very much, but am not sure about the implementation.
> This problem has been around for a while (just search the archives for

> people wondering how to deal with a switch chip connected via rvMII or

> whatever).  The trickiest part of this is how to get the information 
> to the driver.  I've always thought that the best way would be for 
> board code to initialize each controller through proper C code (i.e. 
> not CONFIG macros).  But there's definitely something to be said for 
> doing it all through macros, since that's how Kconfig works.  Please 
> have a look at the code that Andy Fleming recently submitted for the 
> TSEC driver (it's in the main branch now).  He passes a 
> tsec_info_struct into each call of tsec_initialize(), allowing all 
> type of custom information to go in.  In my mind, that could be 
> generalized to something that more than just TSEC, but let's take baby
steps.

Yes, this looks like a good approach. Not sure if we should go all the
way or accept Victors approach for now. Moving to this parameter based
initialization is a different matter that should really be done soon.

> Incidentally, the term "Fixed PHY" has already been coined for what 
> you're calling "PHY-less".  I suggest we standardize.

Yes. Is there already a define available?

> Anyway, I have to go to bed.  Eyes are starting to close and brain's 
> sloowwwiing doowwn.

Heh :)

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
=====================================================================

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

[Ben Warren]

Hi Victor,

Victor Gallardo wrote:
> Hi Stefan and Ben,
>
> I saw what Andy Fleming's did. This is a bit to much work for what I
> have time for.
>
>   
I understand completely.
> I aggree, we need to take baby steps..
>
> For now I'll change PHY-less to Fixed PHY and update some style issues.
>
> -Victor Gallardo
>
>   
Sounds like a good idea.  If you do that we can have this in 1.3.5 
release and then come up with a more unified approach for maybe the next 
release.

regards,
Ben