2004-11-10 08:28:16

by Colin Leroy

[permalink] [raw]
Subject: [PATCH] sungem: Fix stop_phy

Hi,

sungem driver's stop_phy differs quite a lot from Darwin's driver:
In darwin, the hack consisting in reading and writing the same
from/to MII_LPA is applied to every sungem card, not only bcm5201.
Also, it is done before the rest of the stuff, not after. Maybe order
matters.
In Linux sungem's driver, the BCM5221 phy is suspended as the BCM5201.
It is (completely) different in darwin.

The patch following this follows OS X a bit more closely. It works on
iBook G4 with a BCM5221, it would be nice if users of other PHYs could
test it. If I'm correct, stop_phy is called once during boot (I had an
oops from it during boot while hacking on it), so booting and checking
that network works should be enough.

Signed-off-by: Colin Leroy <[email protected]>
diff -ur a/drivers/net/sungem.c b/drivers/net/sungem.c
--- a/drivers/net/sungem.c 2004-10-18 23:55:28.000000000 +0200
+++ b/drivers/net/sungem.c 2004-11-10 09:16:55.660896544 +0100
@@ -2124,6 +2124,8 @@
*/
msleep(10);

+ /* FIXME OS X starts with disabling interrupts for BCM5201 */
+
/* Make sure we aren't polling PHY status change. We
* don't currently use that feature though
*/
@@ -2131,6 +2133,8 @@
mifcfg &= ~MIF_CFG_POLL;
writel(mifcfg, gp->regs + MIF_CFG);

+ bcm_link_partner_hack(&gp->phy_mii);
+
if (gp->wake_on_lan) {
/* Setup wake-on-lan */
} else {
@@ -2159,7 +2163,7 @@
}

if (found_mii_phy(gp) && gp->phy_mii.def->ops->suspend)
- gp->phy_mii.def->ops->suspend(&gp->phy_mii, 0 /* wake on lan options */);
+ gp->phy_mii.def->ops->suspend(&gp->phy_mii, gp->wake_on_lan);

if (!gp->wake_on_lan) {
/* According to Apple, we must set the MDIO pins to this begnign
diff -ur a/drivers/net/sungem_phy.c b/drivers/net/sungem_phy.c
--- a/drivers/net/sungem_phy.c 2004-10-18 23:53:45.000000000 +0200
+++ b/drivers/net/sungem_phy.c 2004-11-10 08:59:20.000000000 +0100
@@ -101,15 +101,17 @@
return 0;
}

-static int bcm5201_suspend(struct mii_phy* phy, int wol_options)
+void bcm_link_partner_hack(struct mii_phy *phy)
{
- if (!wol_options)
- phy_write(phy, MII_BCM5201_INTERRUPT, 0);
-
/* Here's a strange hack used by both MacOS 9 and X */
phy_write(phy, MII_LPA, phy_read(phy, MII_LPA));
-
+}
+
+static int bcm5201_suspend(struct mii_phy* phy, int wol_options)
+{
if (!wol_options) {
+ phy_write(phy, MII_BCM5201_INTERRUPT, 0);
+
#if 0 /* Commented out in Darwin... someone has those dawn docs ? */
u16 val = phy_read(phy, MII_BCM5201_AUXMODE2)
phy_write(phy, MII_BCM5201_AUXMODE2,
@@ -144,6 +146,20 @@
return 0;
}

+static int bcm5221_suspend(struct mii_phy* phy, int wol_options)
+{
+ if (!wol_options) {
+ u16 data = phy_read(phy, MII_BCM5221_TEST);
+ phy_write(phy, MII_BCM5221_TEST,
+ data | MII_BCM5221_TEST_ENABLE_SHADOWS);
+
+ data = phy_read(phy, MII_BCM5221_SHDOW_AUX_MODE4);
+ phy_write(phy, MII_BCM5221_SHDOW_AUX_MODE4,
+ data | MII_BCM5221_IDDQ);
+ }
+ return 0;
+}
+
static int bcm5400_init(struct mii_phy* phy)
{
u16 data;
@@ -662,7 +678,7 @@

/* Broadcom BCM 5221 */
static struct mii_phy_ops bcm5221_phy_ops = {
- .suspend = bcm5201_suspend,
+ .suspend = bcm5221_suspend,
.init = bcm5221_init,
.setup_aneg = genmii_setup_aneg,
.setup_forced = genmii_setup_forced,
diff -ur a/drivers/net/sungem_phy.h b/drivers/net/sungem_phy.h
--- a/drivers/net/sungem_phy.h 2004-10-18 23:54:40.000000000 +0200
+++ b/drivers/net/sungem_phy.h 2004-11-10 08:59:53.000000000 +0100
@@ -53,6 +53,7 @@
*/
extern int mii_phy_probe(struct mii_phy *phy, int mii_id);

+void bcm_link_partner_hack(struct mii_phy *phy);

/* MII definitions missing from mii.h */

@@ -81,6 +82,7 @@
#define MII_BCM5221_SHDOW_AUX_STAT2_APD 0x0020
#define MII_BCM5221_SHDOW_AUX_MODE4 0x1a
#define MII_BCM5221_SHDOW_AUX_MODE4_CLKLOPWR 0x0004
+#define MII_BCM5221_IDDQ 0x0001

/* MII BCM5400 1000-BASET Control register */
#define MII_BCM5400_GB_CONTROL 0x09


2004-11-10 16:46:48

by Colin Leroy

[permalink] [raw]
Subject: Re: [PATCH] sungem: Fix stop_phy

On 10 Nov 2004 at 09h11, Colin Leroy wrote:

Hi,

> oops from it during boot while hacking on it), so booting and checking
> that network works should be enough.

(also check sleep/resume if your laptop supports it, there may be
problems there...)

--
Colin

2004-11-10 23:06:00

by Benjamin Herrenschmidt

[permalink] [raw]
Subject: Re: [PATCH] sungem: Fix stop_phy

On Wed, 2004-11-10 at 09:27 +0100, Colin Leroy wrote:
> Hi,
>
> sungem driver's stop_phy differs quite a lot from Darwin's driver:
> In darwin, the hack consisting in reading and writing the same
> from/to MII_LPA is applied to every sungem card, not only bcm5201.
> Also, it is done before the rest of the stuff, not after. Maybe order
> matters.
> In Linux sungem's driver, the BCM5221 phy is suspended as the BCM5201.
> It is (completely) different in darwin.
>
> The patch following this follows OS X a bit more closely. It works on
> iBook G4 with a BCM5221, it would be nice if users of other PHYs could
> test it. If I'm correct, stop_phy is called once during boot (I had an
> oops from it during boot while hacking on it), so booting and checking
> that network works should be enough.

Which version of Darwin did you look at ? There have been changes since
I wrote that code. Note that currently, sungem stops everything (cell,
PHY, ...) when the interface is down for more than 10 seconds, though
I've been thinking about removing that "feature" since it prevents
polling the link...

> Signed-off-by: Colin Leroy <[email protected]>
> diff -ur a/drivers/net/sungem.c b/drivers/net/sungem.c
> --- a/drivers/net/sungem.c 2004-10-18 23:55:28.000000000 +0200
> +++ b/drivers/net/sungem.c 2004-11-10 09:16:55.660896544 +0100
> @@ -2124,6 +2124,8 @@
> */
> msleep(10);
>
> + /* FIXME OS X starts with disabling interrupts for BCM5201 */

PHY interrupts are used for WOL afaik

> /* Make sure we aren't polling PHY status change. We
> * don't currently use that feature though
> */

We don't use autopoll as it's somewhat buggy

> @@ -2131,6 +2133,8 @@
> mifcfg &= ~MIF_CFG_POLL;
> writel(mifcfg, gp->regs + MIF_CFG);
>
> + bcm_link_partner_hack(&gp->phy_mii);

I doubt this is useful, and I'd like to avoid those hacks that violate
the layering of sungem vs. sungem_phy.c

If you really want to read the LPA, then do so on all PHYs from
sungem.c. Clearing the interrupts can be done from the PHY init().

> if (gp->wake_on_lan) {
> /* Setup wake-on-lan */
> } else {
> @@ -2159,7 +2163,7 @@
> }
>
> if (found_mii_phy(gp) && gp->phy_mii.def->ops->suspend)
> - gp->phy_mii.def->ops->suspend(&gp->phy_mii, 0 /* wake on lan options */);
> + gp->phy_mii.def->ops->suspend(&gp->phy_mii, gp->wake_on_lan);
>
> if (!gp->wake_on_lan) {
> /* According to Apple, we must set the MDIO pins to this begnign
> diff -ur a/drivers/net/sungem_phy.c b/drivers/net/sungem_phy.c
> --- a/drivers/net/sungem_phy.c 2004-10-18 23:53:45.000000000 +0200
> +++ b/drivers/net/sungem_phy.c 2004-11-10 08:59:20.000000000 +0100
> @@ -101,15 +101,17 @@
> return 0;
> }
>
> -static int bcm5201_suspend(struct mii_phy* phy, int wol_options)
> +void bcm_link_partner_hack(struct mii_phy *phy)
> {
> - if (!wol_options)
> - phy_write(phy, MII_BCM5201_INTERRUPT, 0);
> -
> /* Here's a strange hack used by both MacOS 9 and X */
> phy_write(phy, MII_LPA, phy_read(phy, MII_LPA));
> -
> +}
> +
> +static int bcm5201_suspend(struct mii_phy* phy, int wol_options)
> +{
> if (!wol_options) {
> + phy_write(phy, MII_BCM5201_INTERRUPT, 0);
> +
> #if 0 /* Commented out in Darwin... someone has those dawn docs ? */
> u16 val = phy_read(phy, MII_BCM5201_AUXMODE2)
> phy_write(phy, MII_BCM5201_AUXMODE2,
> @@ -144,6 +146,20 @@
> return 0;
> }

The BCM52xx docs are available on Broadcom site.

> +static int bcm5221_suspend(struct mii_phy* phy, int wol_options)
> +{
> + if (!wol_options) {
> + u16 data = phy_read(phy, MII_BCM5221_TEST);
> + phy_write(phy, MII_BCM5221_TEST,
> + data | MII_BCM5221_TEST_ENABLE_SHADOWS);
> +
> + data = phy_read(phy, MII_BCM5221_SHDOW_AUX_MODE4);
> + phy_write(phy, MII_BCM5221_SHDOW_AUX_MODE4,
> + data | MII_BCM5221_IDDQ);
> + }
> + return 0;
> +}
> +
> static int bcm5400_init(struct mii_phy* phy)
> {
> u16 data;
> @@ -662,7 +678,7 @@
>
> /* Broadcom BCM 5221 */
> static struct mii_phy_ops bcm5221_phy_ops = {
> - .suspend = bcm5201_suspend,
> + .suspend = bcm5221_suspend,
> .init = bcm5221_init,
> .setup_aneg = genmii_setup_aneg,
> .setup_forced = genmii_setup_forced,
> diff -ur a/drivers/net/sungem_phy.h b/drivers/net/sungem_phy.h
> --- a/drivers/net/sungem_phy.h 2004-10-18 23:54:40.000000000 +0200
> +++ b/drivers/net/sungem_phy.h 2004-11-10 08:59:53.000000000 +0100
> @@ -53,6 +53,7 @@
> */
> extern int mii_phy_probe(struct mii_phy *phy, int mii_id);
>
> +void bcm_link_partner_hack(struct mii_phy *phy);
>
> /* MII definitions missing from mii.h */
>
> @@ -81,6 +82,7 @@
> #define MII_BCM5221_SHDOW_AUX_STAT2_APD 0x0020
> #define MII_BCM5221_SHDOW_AUX_MODE4 0x1a
> #define MII_BCM5221_SHDOW_AUX_MODE4_CLKLOPWR 0x0004
> +#define MII_BCM5221_IDDQ 0x0001
>
> /* MII BCM5400 1000-BASET Control register */
> #define MII_BCM5400_GB_CONTROL 0x09
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
--
Benjamin Herrenschmidt <[email protected]>