Re: [PATCH] net: ibm_newemac: Don't start autonegotiation when disabled in BMCR (genmii)

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:

quoted

On Tue, 2011-07-19 at 12:50 +0200, Stefan Roese wrote:

quoted

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 ?

quoted

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.