Re: [PATCH 1/4] of/mdio: Add fixed link support

[Anton Vorontsov <avorontsov@ru.mvista.com>]

On Fri, Jun 26, 2009 at 05:33:26PM -0600, Grant Likely wrote:
> On Fri, Jun 26, 2009 at 4:29 PM, Anton
> Vorontsov<avorontsov@ru.mvista.com> wrote:
> > Currently the fixed link support is broken for all OF ethernet drivers,
> > an "OF MDIO rework" removed most of the support. Instead of re-adding
> > fixed-link stuff to the drivers, add the support to a framework, so we
> > won't duplicate any code.
> >
> > With this patch, if a node pointer is NULL, then of_phy_connect() will
> > try to find ethernet device's node, then will look for fixed-link
> > property, and if specified, it connects PHY as usual, via bus_id (fixed
> > link PHYs do not have any device tree nodes associated with them).
> >
> > Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
> 
> Ugh.  I do not like this approach.  I did not intend to break fixed
> links, but I do not think that this approach is the right fix.  There
> are several problems.
> 
> I don't like the fixed.c approach of creating a dummy phy to begin
> with.  I think it is an abuse of the device model to register a dummy
> mii_bus and have dummy devices registered on it.  It is a lot of code
> for what should be a very simple thing.  In particular, if a PHY is
> not specified, then the driver should use a static link configuration.
>  This is trivial to implement in an Ethernet driver and I do not think
> the dummy phy adds anything.

Dummy PHYs add more than you think, for example you'll have to
refactor or duplicate the code that is responsible for MAC settings
for a given mode (10/100/100, duplex, pause), and you'll have to
do netif_carrier_* handling yourself.

Not a problem per se, but you'll have to address this, and you'll
have two paths in the drivers.

> It hooks into the initialization path of *all* OF enabled net drivers,
> whether it wants it or not.  ie. The MPC5200 FEC driver does not want
> it because the fixed-link property is not part of the mpc5200-fec
> binding; it uses a current-speed property instead.  'fixed-link' has
> not been agreed upon to be applicable to all Ethernet bindings, and
> I'm not convinced that the format of it won't need to be changed for
> future Ethernet bindings.  A function for parsing fixed-link should be
> a library function that a driver can choose to call out to.  It should
> not be welded into the init path.
> 
> I also think parsing the device tree at device open time (when
> of_phy_connect is usually called) is best to be avoided.  fixed-link
> parsing should really happen at probe time and the values cached IMHO.
>  It's probably not significant, but I'd like to keep device tree reads
> constrained in the cold path (driver probe time) as opposed to the hot
> (or slightly less cool) device open path.

open() time isn't a hot path at all.

> Instead, I think that each driver should be more graceful about
> missing phy pointers and the init path should call out to a fixed-link
> parser function that sets the initial link settings.  Probably less
> than 5 lines of code per driver.
> 
> I'm sorry about breaking it.  It was my fault, and I'd be happy to fix
> it if you'd like me to,

Please do.

> but I don't think that this patch is the right
> approach.

OK, fine by me if you think you can do this stuff better. :-)

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2