diff mbox

[U-Boot] phy/marvell: Rewrite the MV88E1111 phy config function based on kernel code

Message ID 1319178713-12472-2-git-send-email-tie-fei.zang@freescale.com
State Changes Requested
Headers show

Commit Message

Zang Roy-R61911 Oct. 21, 2011, 6:31 a.m. UTC 
The original m88e1111s_config() does not do the SGMII mode
initialization and is buggy. Rewrite the function according to
3.0.6 kernel function m88e1111_config_init() in drivers/net/phy/marvell.c

Signed-off-by: Roy Zang <tie-fei.zang@freescale.com>
Acked-by: Andy Fleming <afleming@freescale.com>
Cc: Kumar Gala <galak@kernel.crashing.org>
---
Tested on P1023RDS board
 drivers/net/phy/marvell.c |  100 ++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 94 insertions(+), 6 deletions(-)

Comments

Wolfgang Denk Oct. 23, 2011, 7:41 p.m. UTC | #1 
Dear Roy Zang,

In message <1319178713-12472-2-git-send-email-tie-fei.zang@freescale.com> you wrote:
> The original m88e1111s_config() does not do the SGMII mode
> initialization and is buggy. Rewrite the function according to
> 3.0.6 kernel function m88e1111_config_init() in drivers/net/phy/marvell.c
> 
> Signed-off-by: Roy Zang <tie-fei.zang@freescale.com>
> Acked-by: Andy Fleming <afleming@freescale.com>
> Cc: Kumar Gala <galak@kernel.crashing.org>
...
> +		/* soft reset */
> +		phy_write(phydev, MDIO_DEVAD_NONE, MII_BMCR, BMCR_RESET);
> +		do
> +			reg = phy_read(phydev, MDIO_DEVAD_NONE, MII_BMCR);
> +		while (reg & BMCR_RESET);
...
> +	/* soft reset */
> +	phy_write(phydev, MDIO_DEVAD_NONE, MII_BMCR, BMCR_RESET);
> +	do
> +		reg = phy_read(phydev, MDIO_DEVAD_NONE, MII_BMCR);
> +	while (reg & BMCR_RESET);

Do we really need this double reset?

Also, I dislike the potentially infinite loop here - please add a
timeout and an error exit.

Best regards,

Wolfgang Denk
Zang Roy-R61911 Oct. 24, 2011, 2:30 a.m. UTC | #2 
> -----Original Message-----
> From: Wolfgang Denk [mailto:wd@denx.de]
> Sent: Monday, October 24, 2011 3:42 AM
> To: Zang Roy-R61911
> Cc: u-boot@lists.denx.de; Kumar Gala
> Subject: Re: [U-Boot] [PATCH] phy/marvell: Rewrite the MV88E1111 phy config
> function based on kernel code
> 
> Dear Roy Zang,
> 
> In message <1319178713-12472-2-git-send-email-tie-fei.zang@freescale.com> you
> wrote:
> > The original m88e1111s_config() does not do the SGMII mode
> > initialization and is buggy. Rewrite the function according to
> > 3.0.6 kernel function m88e1111_config_init() in drivers/net/phy/marvell.c
> >
> > Signed-off-by: Roy Zang <tie-fei.zang@freescale.com>
> > Acked-by: Andy Fleming <afleming@freescale.com>
> > Cc: Kumar Gala <galak@kernel.crashing.org>
> ...
> > +		/* soft reset */
> > +		phy_write(phydev, MDIO_DEVAD_NONE, MII_BMCR, BMCR_RESET);
> > +		do
> > +			reg = phy_read(phydev, MDIO_DEVAD_NONE, MII_BMCR);
> > +		while (reg & BMCR_RESET);
> ...
> > +	/* soft reset */
> > +	phy_write(phydev, MDIO_DEVAD_NONE, MII_BMCR, BMCR_RESET);
> > +	do
> > +		reg = phy_read(phydev, MDIO_DEVAD_NONE, MII_BMCR);
> > +	while (reg & BMCR_RESET);
> 
> Do we really need this double reset?
The MV88E1111 user manual requests "any changes to HWCFG_MODE of Extended PHY Specific Status Register must be followed by software reset to take effect"
From the code flow, double reset is only for RTBI mode, which really doubly changes the HWCFG_MODE bits.

> 
> Also, I dislike the potentially infinite loop here - please add a
> timeout and an error exit.
This makes sense. Will update and resend.
Thanks.
Roy


> 
> Best regards,
> 
> Wolfgang Denk
> 
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> A supercomputer is a machine that runs an endless loop in 2 seconds.
diff mbox

Patch

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index bd1cdc4..635a114 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -43,6 +43,24 @@ 
 #define MIIM_88E1111_PHY_LED_DIRECT	0x4100
 #define MIIM_88E1111_PHY_LED_COMBINE	0x411C
 
+/* 88E1111 Extended PHY Specific Control Register */
+#define MIIM_88E1111_PHY_EXT_CR		0x14
+#define MIIM_88E1111_RX_DELAY		0x80
+#define MIIM_88E1111_TX_DELAY		0x2
+
+/* 88E1111 Extended PHY Specific Status Register */
+#define MIIM_88E1111_PHY_EXT_SR		0x1b
+#define MIIM_88E1111_HWCFG_MODE_MASK		0xf
+#define MIIM_88E1111_HWCFG_MODE_COPPER_RGMII	0xb
+#define MIIM_88E1111_HWCFG_MODE_FIBER_RGMII	0x3
+#define MIIM_88E1111_HWCFG_MODE_SGMII_NO_CLK	0x4
+#define MIIM_88E1111_HWCFG_MODE_COPPER_RTBI	0x9
+#define MIIM_88E1111_HWCFG_FIBER_COPPER_AUTO	0x8000
+#define MIIM_88E1111_HWCFG_FIBER_COPPER_RES	0x2000
+
+#define MIIM_88E1111_COPPER		0
+#define MIIM_88E1111_FIBER		1
+
 /* 88E1118 PHY defines */
 #define MIIM_88E1118_PHY_PAGE		22
 #define MIIM_88E1118_PHY_LED_PAGE	3
@@ -167,14 +185,84 @@  static int m88e1111s_config(struct phy_device *phydev)
 			(phydev->interface == PHY_INTERFACE_MODE_RGMII_ID) ||
 			(phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) ||
 			(phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID)) {
-		reg = phy_read(phydev, MDIO_DEVAD_NONE, 0x1b);
-		reg = (reg & 0xfff0) | 0xb;
-		phy_write(phydev, MDIO_DEVAD_NONE, 0x1b, reg);
-	} else {
-		phy_write(phydev, MDIO_DEVAD_NONE, 0x1b, 0x1f);
+		reg = phy_read(phydev,
+			MDIO_DEVAD_NONE, MIIM_88E1111_PHY_EXT_CR);
+		if ((phydev->interface == PHY_INTERFACE_MODE_RGMII) ||
+			(phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)) {
+			reg |= (MIIM_88E1111_RX_DELAY | MIIM_88E1111_TX_DELAY);
+		} else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) {
+			reg &= ~MIIM_88E1111_TX_DELAY;
+			reg |= MIIM_88E1111_RX_DELAY;
+		} else if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
+			reg &= ~MIIM_88E1111_RX_DELAY;
+			reg |= MIIM_88E1111_TX_DELAY;
+		}
+
+		phy_write(phydev,
+			MDIO_DEVAD_NONE, MIIM_88E1111_PHY_EXT_CR, reg);
+
+		reg = phy_read(phydev,
+			MDIO_DEVAD_NONE, MIIM_88E1111_PHY_EXT_SR);
+
+		reg &= ~(MIIM_88E1111_HWCFG_MODE_MASK);
+
+		if (reg & MIIM_88E1111_HWCFG_FIBER_COPPER_RES)
+			reg |= MIIM_88E1111_HWCFG_MODE_FIBER_RGMII;
+		else
+			reg |= MIIM_88E1111_HWCFG_MODE_COPPER_RGMII;
+
+		phy_write(phydev,
+			MDIO_DEVAD_NONE, MIIM_88E1111_PHY_EXT_SR, reg);
+	}
+
+	if (phydev->interface == PHY_INTERFACE_MODE_SGMII) {
+		reg = phy_read(phydev,
+			MDIO_DEVAD_NONE, MIIM_88E1111_PHY_EXT_SR);
+
+		reg &= ~(MIIM_88E1111_HWCFG_MODE_MASK);
+		reg |= MIIM_88E1111_HWCFG_MODE_SGMII_NO_CLK;
+		reg |= MIIM_88E1111_HWCFG_FIBER_COPPER_AUTO;
+
+		phy_write(phydev, MDIO_DEVAD_NONE,
+			MIIM_88E1111_PHY_EXT_SR, reg);
+	}
+
+	if (phydev->interface == PHY_INTERFACE_MODE_RTBI) {
+		reg = phy_read(phydev,
+			MDIO_DEVAD_NONE, MIIM_88E1111_PHY_EXT_CR);
+		reg |= (MIIM_88E1111_RX_DELAY | MIIM_88E1111_TX_DELAY);
+		phy_write(phydev,
+			MDIO_DEVAD_NONE, MIIM_88E1111_PHY_EXT_CR, reg);
+
+		reg = phy_read(phydev, MDIO_DEVAD_NONE,
+			MIIM_88E1111_PHY_EXT_SR);
+		reg &= ~(MIIM_88E1111_HWCFG_MODE_MASK |
+			MIIM_88E1111_HWCFG_FIBER_COPPER_RES);
+		reg |= 0x7 | MIIM_88E1111_HWCFG_FIBER_COPPER_AUTO;
+		phy_write(phydev, MDIO_DEVAD_NONE,
+			MIIM_88E1111_PHY_EXT_SR, reg);
+
+		/* soft reset */
+		phy_write(phydev, MDIO_DEVAD_NONE, MII_BMCR, BMCR_RESET);
+		do
+			reg = phy_read(phydev, MDIO_DEVAD_NONE, MII_BMCR);
+		while (reg & BMCR_RESET);
+
+		reg = phy_read(phydev, MDIO_DEVAD_NONE,
+			MIIM_88E1111_PHY_EXT_SR);
+		reg &= ~(MIIM_88E1111_HWCFG_MODE_MASK |
+			MIIM_88E1111_HWCFG_FIBER_COPPER_RES);
+		reg |= MIIM_88E1111_HWCFG_MODE_COPPER_RTBI |
+			MIIM_88E1111_HWCFG_FIBER_COPPER_AUTO;
+		phy_write(phydev, MDIO_DEVAD_NONE,
+			MIIM_88E1111_PHY_EXT_SR, reg);
 	}
 
-	phy_write(phydev, MDIO_DEVAD_NONE, 0x14, 0x0cd2);
+	/* soft reset */
+	phy_write(phydev, MDIO_DEVAD_NONE, MII_BMCR, BMCR_RESET);
+	do
+		reg = phy_read(phydev, MDIO_DEVAD_NONE, MII_BMCR);
+	while (reg & BMCR_RESET);
 
 	genphy_config_aneg(phydev);