Re: Fix problems with Holly's DT representation of ethernet PHYs

[David Gibson <dwg@au1.ibm.com>]

On Fri, May 25, 2007 at 09:03:51AM -0500, Josh Boyer wrote:
> On Fri, 2007-05-25 at 14:38 +1000, David Gibson wrote:
> > 
> > Waah!  Third time lucky?
> 
> Not quite. :)
> 
> > 	- Second, the PHYs give only "bcm54xx" as a compatible
> > property.  This is unfortunate, because there are many bcm54xx PHY
> > models, and they have differences which can matter.  We add a more
> > precise compatible string, giving the precise PHY model (bcm5461A in
> > this case).
> 
> You don't actually do this.  Instead, you specify a
> txc-rxc-delay-disable property.

Oops, forgot to update the patch description.

> > -			PHY1: ethernet-phy@6000 {
> > -				device_type = "ethernet-phy";
> > -				compatible = "bcm54xx";
> > -				reg = <6000 50>;
> > -				phy-id = <1>;
> > +			PHY1: ethernet-phy@1 {
> > +				reg = <1>;
> > +				txc-rxc-delay-disable;
> >  			};
> 
> I would have rather we left the compatible = "bmc5461A" as well.  Though
> perhaps a comment would suffice instead.

Hrm, I was tending to go with Kumar's point that it's kind of simpler
and safer to probe the PHY type directly.

> > Index: working-2.6/arch/powerpc/sysdev/tsi108_dev.c
> > ===================================================================
> > --- working-2.6.orig/arch/powerpc/sysdev/tsi108_dev.c	2007-05-25 14:34:00.000000000 +1000
> > +++ working-2.6/arch/powerpc/sysdev/tsi108_dev.c	2007-05-25 14:38:32.000000000 +1000
> 
> > +
> >  		tsi_eth_data.regs = r[0].start;
> >  		tsi_eth_data.phyregs = res.start;
> >  		tsi_eth_data.phy = *phy_id;
> >  		tsi_eth_data.irq_num = irq_of_parse_and_map(np, 0);
> > -		if (of_device_is_compatible(phy, "bcm54xx"))
> > +
> > +		if (of_get_property(phy, "txc-rxc-delay-disable", NULL))
> >  			tsi_eth_data.phy_type = TSI108_PHY_BCM54XX;
> >  		of_node_put(phy);
> 
> At the very least this needs a comment explaining what exactly is being
> done here.  Right now, it's looking for some magical property and
> setting the PHY type to a Broadcom PHY...  very confusing to someone
> that hasn't followed the email thread.

Yeah, good point.  And more specifically I should put a FIXME comment
in, saying that the ethernet driver itself should be changed to
implement this workaround in a different way, rather than based on
this phy_type field, since the workaround isn't really related to the
PHY type.  I was assuming that cleanup would happen as part of the
port to phylib.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson