| Message ID | 1311072604-24840-1-git-send-email-sr@denx.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Tue, 2011-07-19 at 12:50 +0200, Stefan Roese wrote: > As noticed on a custom 440GX board using the Micrel KSZ8041 PHY in > fiber mode, a strapped fixed PHY configuration will currently restart > the autonegotiation. This patch checks the BMCR_ANENABLE bit and > skips this autonegotiation if its disabled. Won't that just break aneg on everything else ? IE, most other PHYs rely on ANENABLE being set further down this same function (especially if the FW doesn't do it but even then, we may reset PHYs along the way etc...) This is something that really a case where the device-tree should indicate that aneg shall not be performed and from there don't call setup_aneg at all. Cheers, Ben. > Signed-off-by: Stefan Roese <sr@denx.de> > Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> > --- > drivers/net/ibm_newemac/phy.c | 5 +++++ > 1 files changed, 5 insertions(+), 0 deletions(-) > > diff --git a/drivers/net/ibm_newemac/phy.c b/drivers/net/ibm_newemac/phy.c > index ac9d964..90afc58 100644 > --- a/drivers/net/ibm_newemac/phy.c > +++ b/drivers/net/ibm_newemac/phy.c > @@ -116,6 +116,11 @@ static int genmii_setup_aneg(struct mii_phy *phy, u32 advertise) > ctl = phy_read(phy, MII_BMCR); > if (ctl < 0) > return ctl; > + > + /* Don't start auto negotiation when its disabled in BMCR */ > + if (!(ctl & BMCR_ANENABLE)) > + return 0; > + > ctl &= ~(BMCR_FULLDPLX | BMCR_SPEED100 | BMCR_SPEED1000 | BMCR_ANENABLE); > > /* First clear the PHY */
Hi Ben, On Tuesday 19 July 2011 13:35:22 Benjamin Herrenschmidt wrote: > On Tue, 2011-07-19 at 12:50 +0200, Stefan Roese wrote: > > As noticed on a custom 440GX board using the Micrel KSZ8041 PHY in > > fiber mode, a strapped fixed PHY configuration will currently restart > > the autonegotiation. This patch checks the BMCR_ANENABLE bit and > > skips this autonegotiation if its disabled. > > Won't that just break aneg on everything else ? > > IE, most other PHYs rely on ANENABLE being set further down this same > function (especially if the FW doesn't do it but even then, we may reset > PHYs along the way etc...) If aneg is enabled for a PHY (e.g. not strapped to fixed configuration), I don't see how this patch will change the current aneg behaviour. Perhaps I'm missing something, but I tested this on some boards with aneg enabled (Sequoia etc). And I didn't notice any problems. > This is something that really a case where the device-tree should > indicate that aneg shall not be performed and from there don't call > setup_aneg at all. I feel that this BMCR_ANENABLE bit should be evaluated, but I have no strong preference here. If you prefer that this should be handled via a new dt property (phy-aneg = "disabled" ?), I can implement it this way. Just let me know. Thanks, Stefan
On Tue, 2011-07-19 at 13:59 +0200, Stefan Roese wrote: > Hi Ben, > > On Tuesday 19 July 2011 13:35:22 Benjamin Herrenschmidt wrote: > > On Tue, 2011-07-19 at 12:50 +0200, Stefan Roese wrote: > > > As noticed on a custom 440GX board using the Micrel KSZ8041 PHY in > > > fiber mode, a strapped fixed PHY configuration will currently restart > > > the autonegotiation. This patch checks the BMCR_ANENABLE bit and > > > skips this autonegotiation if its disabled. > > > > Won't that just break aneg on everything else ? > > > > IE, most other PHYs rely on ANENABLE being set further down this same > > function (especially if the FW doesn't do it but even then, we may reset > > PHYs along the way etc...) > > If aneg is enabled for a PHY (e.g. not strapped to fixed configuration), I > don't see how this patch will change the current aneg behaviour. Perhaps I'm > missing something, but I tested this on some boards with aneg enabled (Sequoia > etc). And I didn't notice any problems. But is aneg always enabled via straps ? The whole point of this function is that aneg can be enabled or disabled via the ethtool API (thus overriding whatever strapping)... I may be missing something but this patch looks like it would break this no ? > > This is something that really a case where the device-tree should > > indicate that aneg shall not be performed and from there don't call > > setup_aneg at all. > > I feel that this BMCR_ANENABLE bit should be evaluated, but I have no strong > preference here. If you prefer that this should be handled via a new dt > property (phy-aneg = "disabled" ?), I can implement it this way. Just let me > know. Don't we already have some bindings for PHY with a fixed setting ? I don't remember off hand, we need to dbl check. I don't like looking at BMCR because BCMR is a -control- register, something that we can (and will) whack with anything ourselves, which can be reset or reconfigured and I have learned to never trust board straps :-) Cheers, Ben.
On Tuesday 19 July 2011 14:29:30 Benjamin Herrenschmidt wrote: > > I feel that this BMCR_ANENABLE bit should be evaluated, but I have no > > strong preference here. If you prefer that this should be handled via a > > new dt property (phy-aneg = "disabled" ?), I can implement it this way. > > Just let me know. > > Don't we already have some bindings for PHY with a fixed setting ? I > don't remember off hand, we need to dbl check. The only related PHY property I found is "fixed-link" used in fs_enet-main.c. None in the emac driver. Here the description for "fixed-link": Documentation/devicetree/bindings/net/fsl-tsec-phy.txt: - fixed-link : <a b c d e> where a is emulated phy id - choose any, but unique to the all specified fixed-links, b is duplex - 0 half, 1 full, c is link speed - d#10/d#100/d#1000, d is pause - 0 no pause, 1 pause, e is asym_pause - 0 no asym_pause, 1 asym_pause. But what I really want to achieve, is to skip auto-negotiation (use the strapped configuration). And not to define this fixed configuration (again) in the device-tree. So I would prefer something like phy-aneg = "disabled". What do you think? Thanks, Stefan
diff --git a/drivers/net/ibm_newemac/phy.c b/drivers/net/ibm_newemac/phy.c index ac9d964..90afc58 100644 --- a/drivers/net/ibm_newemac/phy.c +++ b/drivers/net/ibm_newemac/phy.c @@ -116,6 +116,11 @@ static int genmii_setup_aneg(struct mii_phy *phy, u32 advertise) ctl = phy_read(phy, MII_BMCR); if (ctl < 0) return ctl; + + /* Don't start auto negotiation when its disabled in BMCR */ + if (!(ctl & BMCR_ANENABLE)) + return 0; + ctl &= ~(BMCR_FULLDPLX | BMCR_SPEED100 | BMCR_SPEED1000 | BMCR_ANENABLE); /* First clear the PHY */
As noticed on a custom 440GX board using the Micrel KSZ8041 PHY in fiber mode, a strapped fixed PHY configuration will currently restart the autonegotiation. This patch checks the BMCR_ANENABLE bit and skips this autonegotiation if its disabled. Signed-off-by: Stefan Roese <sr@denx.de> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org> --- drivers/net/ibm_newemac/phy.c | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)