Date: Thu, 2 Apr 2009 09:55:03 +0900 From: Pyun YongHyeon <pyunyh@gmail.com> To: Marius Strobl <marius@alchemy.franken.de> Cc: "M. Warner Losh" <imp@bsdimp.com>, net@freebsd.org Subject: Re: Small change to ukphy Message-ID: <20090402005503.GB19091@michelle.cdnetworks.co.kr> In-Reply-To: <20090401211254.GA83780@alchemy.franken.de> References: <20090401.013246.-1253043078.imp@bsdimp.com> <20090401100939.GB12246@michelle.cdnetworks.co.kr> <20090401211254.GA83780@alchemy.franken.de>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, Apr 01, 2009 at 11:12:54PM +0200, Marius Strobl wrote:
> On Wed, Apr 01, 2009 at 07:09:39PM +0900, Pyun YongHyeon wrote:
> > On Wed, Apr 01, 2009 at 01:32:46AM -0600, M. Warner Losh wrote:
> > > I've encountered a number of PHY chips that need auto negotiation
> > > kicked off to come out of ISO state. This makes sense, because the
> > > ukphy driver never seems to take the PHY out of isolation state
> > > otherwise.
> > >
> > > Index: ukphy.c
> > > ===================================================================
> > > --- ukphy.c (revision 190463)
> > > +++ ukphy.c (working copy)
> > > @@ -146,6 +146,7 @@
> > > sc->mii_phy = ma->mii_phyno;
> > > sc->mii_service = ukphy_service;
> > > sc->mii_pdata = mii;
> > > + sc->mii_flags |= MIIF_FORCEANEG;
> > >
> > > mii->mii_instance++;
> > >
> > >
> > > This forces auto negotiation. The reason for this is that it takes it
> > > out of ISO state (Isolate). Once out of that state, things work
> >
> > If the purpose is to take PHY out of isolated state couldn't this
> > be handled in ifm_change_cb_t handler of parent interface? I guess
> > the callback can reset the PHY and subsequent mii_mediachg() call
> > may start auto-negotiation.
> >
> > > well. The question I have is will we properly go back into ISO state
> > > for PHYs that should be isolated.
> > >
> >
> > If the PHY requires special handing for ISO state in reset it may
> > need separated PHY driver as ukphy(4) does not set MIIF_NOISOLATE.
> > As you said it would be really great if we have a generic way to
> > pass various MII flags or driver specific information to mii(4).
> >
> > > NetBSD has many of its NIC drivers setting this flag. Their APIs
> > > allow them to set this directly at mii attach time. Ours don't, so
> > > none of our drivers set this flag.
> > >
> > > The other fix for this might be:
> > > Index: mii_physubr.c
> > > ===================================================================
> > > --- mii_physubr.c (revision 190463)
> > > +++ mii_physubr.c (working copy)
> > > @@ -113,7 +113,9 @@
> > > int bmcr, anar, gtcr;
> > >
> > > if (IFM_SUBTYPE(ife->ifm_media) == IFM_AUTO) {
> > > - if ((PHY_READ(sc, MII_BMCR) & BMCR_AUTOEN) == 0 ||
> > > + bmcr = PHY_READ(sc, MII_BMCR);
> > > + if ((bmcr & BMCR_AUTOEN) == 0 ||
> > > + (bmcr & BMCR_ISO) ||
> > > (sc->mii_flags & MIIF_FORCEANEG))
> > > (void) mii_phy_auto(sc);
> > > return;
> > >
> > > Which says that if auto negotiation is enabled, and ISO is set to go
> > > ahead and kick off an auto negotiation. I'm less sure of this path,
> > > but it is an alternative. Otherwise, we never write to the BMCR to
> > > take the device out of isolation. If there's a better place to do
> > > this, then I'm all ears.
> > >
> > > Either one of these hacks make several PC Cards that I have start to
> > > work... In fact, I'm starting to approach 100% (up from 50%) of my
> > > ed-based PC Cards working with this simple change (and others to the
> > > ed driver). I know that these cards are a little behind the leading
> > > edge, but I'd like to get them working since I've put a few hours into
> > > investigating things here.
> > >
> > > Comments?
> > >
>
> FYI, the idea I had for passing MIIF_DOPAUSE from the NIC
> drivers to the PHY drivers as required by the flow-control
> support without breaking the ABI was to use device flags.
> A proof-of-concept patch with an example application of
> that approach is:
> http://people.freebsd.org/~marius/mii_flags.diff
> One could even or the flags together in miibus_attach(),
> allowing MIIF_FORCEANEG etc to be additionally set via
> hints.
>
This looks good. As you know some PHY drivers(e.g. brgphy(4),
e1000phy(4)) have to know more information than mii flags. How
about passing one more pointer argument to mii_probe()? The
pointer would be used to point to a driver specific data.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090402005503.GB19091>