Fix problems with Holly's DT representation of ethernet PHYs

Fix problems with Holly's DT representation of ethernet PHYs

[David Gibson]

This patch fixes some problems with the way the Ethernet PHYs are
represented in the device tree for the Holly board.  This means
changes to the dts itself, and to the code with instantiates the
tsi108 ethernet platform devices based on the device tree.

	- First, and most importantly, the PHYs are given with an
identical 'reg' property.  This reg currently encodes the accessible
register used to initiate mdio interaction with the PHYs, rather than
a meaningful address on the parent bus (mdio in this case), which is
incorrect.  Instead we give the address of these registers as 'reg' in
the mdio node itself, and encode the ID of each phy in their 'reg'
propertys.  The PHY's unit name addresses are updated to match.
	- 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).

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Index: working-2.6/arch/powerpc/boot/dts/holly.dts
===================================================================
--- working-2.6.orig/arch/powerpc/boot/dts/holly.dts	2007-05-24 13:55:38.000000000 +1000
+++ working-2.6/arch/powerpc/boot/dts/holly.dts	2007-05-24 14:11:56.000000000 +1000
@@ -63,19 +63,20 @@
 		mdio@6000 {
 			device_type = "mdio";
 			compatible = "tsi-ethernet";
+			reg = <6000 50>;
+			#address-cells = <1>;
+			#size-cells = <0>;
 
-			PHY1: ethernet-phy@6000 {
+			PHY1: ethernet-phy@1 {
 				device_type = "ethernet-phy";
-				compatible = "bcm54xx";
-				reg = <6000 50>;
-				phy-id = <1>;
+				compatible = "bcm5461A", "bcm54xx";
+				reg = <1>;
 			};
 
-			PHY2: ethernet-phy@6400 {
+			PHY2: ethernet-phy@2 {
 				device_type = "ethernet-phy";
-				compatible = "bcm54xx";
-				reg = <6000 50>;
-				phy-id = <2>;
+				compatible = "bcm5461A", "bcm54xx";
+				reg = <2>;
 			};
 		};
 
Index: working-2.6/arch/powerpc/sysdev/tsi108_dev.c
===================================================================
--- working-2.6.orig/arch/powerpc/sysdev/tsi108_dev.c	2007-05-24 13:55:38.000000000 +1000
+++ working-2.6/arch/powerpc/sysdev/tsi108_dev.c	2007-05-24 14:12:11.000000000 +1000
@@ -75,9 +75,8 @@ static int __init tsi108_eth_of_init(voi
 	     (np = of_find_compatible_node(np, "network", "tsi-ethernet")) != NULL;
 	     i++) {
 		struct resource r[2];
-		struct device_node *phy;
+		struct device_node *phy, *mdio;
 		hw_info tsi_eth_data;
-		const unsigned int *id;
 		const unsigned int *phy_id;
 		const void *mac_addr;
 		const phandle *ph;
@@ -119,13 +118,13 @@ static int __init tsi108_eth_of_init(voi
 			goto unreg;
 		}
 
-		id = of_get_property(phy, "reg", NULL);
-		phy_id = of_get_property(phy, "phy-id", NULL);
-		ret = of_address_to_resource(phy, 0, &res);
-		if (ret) {
-			of_node_put(phy);
+		phy_id = of_get_property(phy, "reg", NULL);
+
+		mdio = of_get_parent(phy);
+		ret = of_address_to_resource(mdio, 0, &res);
+		of_node_put(mdio);
+		if (ret)
 			goto unreg;
-		}
 		tsi_eth_data.regs = r[0].start;
 		tsi_eth_data.phyregs = res.start;
 		tsi_eth_data.phy = *phy_id;


-- 
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

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

[Kumar Gala]

On May 23, 2007, at 11:16 PM, David Gibson wrote:

> This patch fixes some problems with the way the Ethernet PHYs are
> represented in the device tree for the Holly board.  This means
> changes to the dts itself, and to the code with instantiates the
> tsi108 ethernet platform devices based on the device tree.
>
> 	- First, and most importantly, the PHYs are given with an
> identical 'reg' property.  This reg currently encodes the accessible
> register used to initiate mdio interaction with the PHYs, rather than
> a meaningful address on the parent bus (mdio in this case), which is
> incorrect.  Instead we give the address of these registers as 'reg' in
> the mdio node itself, and encode the ID of each phy in their 'reg'
> propertys.  The PHY's unit name addresses are updated to match.
> 	- 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).

Is the compatible really need here?  We are able to provide and bind  
drivers based on MII_PHYSID1/2.  I don't see what putting the  
compatible proper gets us.

> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
>
> Index: working-2.6/arch/powerpc/boot/dts/holly.dts
> ===================================================================
> --- working-2.6.orig/arch/powerpc/boot/dts/holly.dts	2007-05-24  
> 13:55:38.000000000 +1000
> +++ working-2.6/arch/powerpc/boot/dts/holly.dts	2007-05-24  
> 14:11:56.000000000 +1000
> @@ -63,19 +63,20 @@
>  		mdio@6000 {
>  			device_type = "mdio";
>  			compatible = "tsi-ethernet";
> +			reg = <6000 50>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
>
> -			PHY1: ethernet-phy@6000 {
> +			PHY1: ethernet-phy@1 {
>  				device_type = "ethernet-phy";
> -				compatible = "bcm54xx";
> -				reg = <6000 50>;
> -				phy-id = <1>;
> +				compatible = "bcm5461A", "bcm54xx";
> +				reg = <1>;
>  			};
>
> -			PHY2: ethernet-phy@6400 {
> +			PHY2: ethernet-phy@2 {
>  				device_type = "ethernet-phy";
> -				compatible = "bcm54xx";
> -				reg = <6000 50>;
> -				phy-id = <2>;
> +				compatible = "bcm5461A", "bcm54xx";
> +				reg = <2>;
>  			};
>  		};
>
> Index: working-2.6/arch/powerpc/sysdev/tsi108_dev.c
> ===================================================================
> --- working-2.6.orig/arch/powerpc/sysdev/tsi108_dev.c	2007-05-24  
> 13:55:38.000000000 +1000
> +++ working-2.6/arch/powerpc/sysdev/tsi108_dev.c	2007-05-24  
> 14:12:11.000000000 +1000
> @@ -75,9 +75,8 @@ static int __init tsi108_eth_of_init(voi
>  	     (np = of_find_compatible_node(np, "network", "tsi- 
> ethernet")) != NULL;
>  	     i++) {
>  		struct resource r[2];
> -		struct device_node *phy;
> +		struct device_node *phy, *mdio;
>  		hw_info tsi_eth_data;
> -		const unsigned int *id;
>  		const unsigned int *phy_id;
>  		const void *mac_addr;
>  		const phandle *ph;
> @@ -119,13 +118,13 @@ static int __init tsi108_eth_of_init(voi
>  			goto unreg;
>  		}
>
> -		id = of_get_property(phy, "reg", NULL);
> -		phy_id = of_get_property(phy, "phy-id", NULL);
> -		ret = of_address_to_resource(phy, 0, &res);
> -		if (ret) {
> -			of_node_put(phy);
> +		phy_id = of_get_property(phy, "reg", NULL);
> +
> +		mdio = of_get_parent(phy);
> +		ret = of_address_to_resource(mdio, 0, &res);
> +		of_node_put(mdio);
> +		if (ret)
>  			goto unreg;
> -		}
>  		tsi_eth_data.regs = r[0].start;
>  		tsi_eth_data.phyregs = res.start;
>  		tsi_eth_data.phy = *phy_id;
>
>
> -- 
> 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
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

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

[David Gibson]

On Wed, May 23, 2007 at 11:22:34PM -0500, Kumar Gala wrote:
> 
> On May 23, 2007, at 11:16 PM, David Gibson wrote:
> 
> > This patch fixes some problems with the way the Ethernet PHYs are
> > represented in the device tree for the Holly board.  This means
> > changes to the dts itself, and to the code with instantiates the
> > tsi108 ethernet platform devices based on the device tree.
> >
> > 	- First, and most importantly, the PHYs are given with an
> > identical 'reg' property.  This reg currently encodes the accessible
> > register used to initiate mdio interaction with the PHYs, rather than
> > a meaningful address on the parent bus (mdio in this case), which is
> > incorrect.  Instead we give the address of these registers as 'reg' in
> > the mdio node itself, and encode the ID of each phy in their 'reg'
> > propertys.  The PHY's unit name addresses are updated to match.
> > 	- 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).
> 
> Is the compatible really need here?  We are able to provide and bind  
> drivers based on MII_PHYSID1/2.  I don't see what putting the  
> compatible proper gets us.

That's a good point (except that having a node without compatible is
kind of odd).  At the moment the code which instantiates the platform
device looks at compatible to enable a workaround.  But actually that
workaround is holly specific, rather than related to the PHY model,
and so should be encoded differently.

-- 
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

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

[Segher Boessenkool]

Looks okay but...

> +				compatible = "bcm5461A", "bcm54xx";

... lowercase please.


Segher

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

[Segher Boessenkool]

>> Is the compatible really need here?  We are able to provide and bind
>> drivers based on MII_PHYSID1/2.  I don't see what putting the
>> compatible proper gets us.
>
> That's a good point (except that having a node without compatible is
> kind of odd).

Not really; "compatible" is for the cases where "name"
alone isn't enough.

> At the moment the code which instantiates the platform
> device looks at compatible to enable a workaround.  But actually that
> workaround is holly specific, rather than related to the PHY model,
> and so should be encoded differently.

There is no real value in encoding it in the device
tree, even.


Segher

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

[Josh Boyer]

On Wed, 2007-05-23 at 23:22 -0500, Kumar Gala wrote:
> On May 23, 2007, at 11:16 PM, David Gibson wrote:
> 
> > This patch fixes some problems with the way the Ethernet PHYs are
> > represented in the device tree for the Holly board.  This means
> > changes to the dts itself, and to the code with instantiates the
> > tsi108 ethernet platform devices based on the device tree.
> >
> > 	- First, and most importantly, the PHYs are given with an
> > identical 'reg' property.  This reg currently encodes the accessible
> > register used to initiate mdio interaction with the PHYs, rather than
> > a meaningful address on the parent bus (mdio in this case), which is
> > incorrect.  Instead we give the address of these registers as 'reg' in
> > the mdio node itself, and encode the ID of each phy in their 'reg'
> > propertys.  The PHY's unit name addresses are updated to match.
> > 	- 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).
> 
> Is the compatible really need here?  We are able to provide and bind  
> drivers based on MII_PHYSID1/2.  I don't see what putting the  
> compatible proper gets us.

I think it's needed until the TSI driver switches to phylib.  Right now,
it assumes that a Marvell phy is used, which is what is present on the
Taiga MPC7448HPC2 board.  The Holly board uses the Broadcom phy and we
added the compatible code for that.

josh

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

[Josh Boyer]

On Thu, 2007-05-24 at 14:16 +1000, David Gibson wrote:
> This patch fixes some problems with the way the Ethernet PHYs are
> represented in the device tree for the Holly board.  This means
> changes to the dts itself, and to the code with instantiates the
> tsi108 ethernet platform devices based on the device tree.
> 
> 	- First, and most importantly, the PHYs are given with an
> identical 'reg' property.  This reg currently encodes the accessible
> register used to initiate mdio interaction with the PHYs, rather than
> a meaningful address on the parent bus (mdio in this case), which is
> incorrect.  Instead we give the address of these registers as 'reg' in
> the mdio node itself, and encode the ID of each phy in their 'reg'
> propertys.  The PHY's unit name addresses are updated to match.

You'll need to fixup the mpc7448hpc2 board's DTS with the same change
then, as it shares the same bridge and ethernet code.

> 	- 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).

This part is fine with me.

josh

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

[David Gibson]

On Thu, May 24, 2007 at 08:45:48AM -0500, Josh Boyer wrote:
> On Thu, 2007-05-24 at 14:16 +1000, David Gibson wrote:
> > This patch fixes some problems with the way the Ethernet PHYs are
> > represented in the device tree for the Holly board.  This means
> > changes to the dts itself, and to the code with instantiates the
> > tsi108 ethernet platform devices based on the device tree.
> > 
> > 	- First, and most importantly, the PHYs are given with an
> > identical 'reg' property.  This reg currently encodes the accessible
> > register used to initiate mdio interaction with the PHYs, rather than
> > a meaningful address on the parent bus (mdio in this case), which is
> > incorrect.  Instead we give the address of these registers as 'reg' in
> > the mdio node itself, and encode the ID of each phy in their 'reg'
> > propertys.  The PHY's unit name addresses are updated to match.
> 
> You'll need to fixup the mpc7448hpc2 board's DTS with the same change
> then, as it shares the same bridge and ethernet code.

Ah, yes.  Done.

> > 	- 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).
> 
> This part is fine with me.
> 
> josh
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
> 

-- 
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

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

[David Gibson]

On Thu, May 24, 2007 at 08:44:20AM -0500, Josh Boyer wrote:
> On Wed, 2007-05-23 at 23:22 -0500, Kumar Gala wrote:
> > On May 23, 2007, at 11:16 PM, David Gibson wrote:
> > 
> > > This patch fixes some problems with the way the Ethernet PHYs are
> > > represented in the device tree for the Holly board.  This means
> > > changes to the dts itself, and to the code with instantiates the
> > > tsi108 ethernet platform devices based on the device tree.
> > >
> > > 	- First, and most importantly, the PHYs are given with an
> > > identical 'reg' property.  This reg currently encodes the accessible
> > > register used to initiate mdio interaction with the PHYs, rather than
> > > a meaningful address on the parent bus (mdio in this case), which is
> > > incorrect.  Instead we give the address of these registers as 'reg' in
> > > the mdio node itself, and encode the ID of each phy in their 'reg'
> > > propertys.  The PHY's unit name addresses are updated to match.
> > > 	- 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).
> > 
> > Is the compatible really need here?  We are able to provide and bind  
> > drivers based on MII_PHYSID1/2.  I don't see what putting the  
> > compatible proper gets us.
> 
> I think it's needed until the TSI driver switches to phylib.  Right now,
> it assumes that a Marvell phy is used, which is what is present on the
> Taiga MPC7448HPC2 board.  The Holly board uses the Broadcom phy and we
> added the compatible code for that.

I think Ben's suggested approach of using a special property in the
phy node to indicate that this workaround is necessary is better.
After all, the workaround isn't actually related to the type of PHY,
which is what compatible encodes, but to how it's wired up.  Revised
patch below addressing this and other matters.

Fix problems with Holly's DT representation of ethernet PHYs

This patch fixes some problems with the way the Ethernet PHYs are
represented in the device tree for the Holly board.  This means
changes to the dts itself, and to the code with instantiates the
tsi108 ethernet platform devices based on the device tree.

	- First, and most importantly, the PHYs are given with an
identical 'reg' property.  This reg currently encodes the accessible
register used to initiate mdio interaction with the PHYs, rather than
a meaningful address on the parent bus (mdio in this case), which is
incorrect.  Instead we give the address of these registers as 'reg' in
the mdio node itself, and encode the ID of each phy in their 'reg'
propertys.  The PHY's unit name addresses are updated to match.
	- 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).

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Index: working-2.6/arch/powerpc/boot/dts/holly.dts
===================================================================
--- working-2.6.orig/arch/powerpc/boot/dts/holly.dts	2007-05-25 13:27:01.000000000 +1000
+++ working-2.6/arch/powerpc/boot/dts/holly.dts	2007-05-25 13:53:03.000000000 +1000
@@ -60,22 +60,21 @@
 			reg = <7000 400>;
 		};
 
-		mdio@6000 {
+		MDIO: mdio@6000 {
 			device_type = "mdio";
 			compatible = "tsi-ethernet";
+			reg = <6000 50>;
+			#address-cells = <1>;
+			#size-cells = <0>;
 
-			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;
 			};
 
-			PHY2: ethernet-phy@6400 {
-				device_type = "ethernet-phy";
-				compatible = "bcm54xx";
-				reg = <6000 50>;
-				phy-id = <2>;
+			PHY2: ethernet-phy@2 {
+				reg = <2>;
+				txc-rxc-delay-disable;
 			};
 		};
 
@@ -88,6 +87,7 @@
 			local-mac-address = [ 00 00 00 00 00 00 ];
 			interrupt-parent = < &/tsi109@c0000000/pic@7400 >;
 			interrupts = <10 2>;
+			mdio-handle = <&MDIO>;
 			phy-handle = <&PHY1>;
 		};
 
@@ -100,6 +100,7 @@
 			local-mac-address = [ 00 00 00 00 00 00 ];
 			interrupt-parent = < &/tsi109@c0000000/pic@7400 >;
 			interrupts = <11 2>;
+			mdio-handle = <&MDIO>;
 			phy-handle = <&PHY2>;
 		};
 
Index: working-2.6/arch/powerpc/sysdev/tsi108_dev.c
===================================================================
--- working-2.6.orig/arch/powerpc/sysdev/tsi108_dev.c	2007-05-08 14:58:06.000000000 +1000
+++ working-2.6/arch/powerpc/sysdev/tsi108_dev.c	2007-05-25 14:09:16.000000000 +1000
@@ -75,9 +75,9 @@ static int __init tsi108_eth_of_init(voi
 	     (np = of_find_compatible_node(np, "network", "tsi-ethernet")) != NULL;
 	     i++) {
 		struct resource r[2];
-		struct device_node *phy;
+		struct device_node *phy, *mdio;
 		hw_info tsi_eth_data;
-		const unsigned int *id;
+		const void *
 		const unsigned int *phy_id;
 		const void *mac_addr;
 		const phandle *ph;
@@ -111,6 +111,13 @@ static int __init tsi108_eth_of_init(voi
 		if (mac_addr)
 			memcpy(tsi_eth_data.mac_addr, mac_addr, 6);
 
+		ph = of_get_property(np, "mdio-handle", NULL);
+		mdio = of_find_node_by_phandle(*ph);
+		ret = of_address_to_resource(mdio, 0, &res);
+		of_node_put(mdio);
+		if (ret)
+			goto unreg;
+
 		ph = of_get_property(np, "phy-handle", NULL);
 		phy = of_find_node_by_phandle(*ph);
 
@@ -119,20 +126,17 @@ static int __init tsi108_eth_of_init(voi
 			goto unreg;
 		}
 
-		id = of_get_property(phy, "reg", NULL);
-		phy_id = of_get_property(phy, "phy-id", NULL);
-		ret = of_address_to_resource(phy, 0, &res);
-		if (ret) {
-			of_node_put(phy);
-			goto unreg;
-		}
+		phy_id = of_get_property(phy, "reg", NULL);
+
 		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"))
 			tsi_eth_data.phy_type = TSI108_PHY_BCM54XX;
 		of_node_put(phy);
+
 		ret =
 		    platform_device_add_data(tsi_eth_dev, &tsi_eth_data,
 					     sizeof(hw_info));
Index: working-2.6/arch/powerpc/boot/dts/mpc7448hpc2.dts
===================================================================
--- working-2.6.orig/arch/powerpc/boot/dts/mpc7448hpc2.dts	2007-05-21 12:47:17.000000000 +1000
+++ working-2.6/arch/powerpc/boot/dts/mpc7448hpc2.dts	2007-05-25 14:03:16.000000000 +1000
@@ -58,24 +58,23 @@
 			compatible  = "tsi-i2c";
 		};
 
-		mdio@6000 {
+		MDIO: mdio@6000 {
 			device_type = "mdio";
 			compatible = "tsi-ethernet";
+			reg = <6000 50>;
+			#address-cells = <1>;
+			#size-cells = <0>;
 
-			phy8: ethernet-phy@6000 {
+			phy8: ethernet-phy@8 {
 				interrupt-parent = <&mpic>;
 				interrupts = <2 1>;
-				reg = <6000 50>;
-				phy-id = <8>;
-				device_type = "ethernet-phy";
+				reg = <8>;
 			};
 
-			phy9: ethernet-phy@6400 {
+			phy9: ethernet-phy@9 {
 				interrupt-parent = <&mpic>;
 				interrupts = <2 1>;
-				reg = <6000 50>;
-				phy-id = <9>;
-				device_type = "ethernet-phy";
+				reg = <9>;
 			};
 
 		};
@@ -89,6 +88,7 @@
 			address = [ 00 06 D2 00 00 01 ];
 			interrupts = <10 2>;
 			interrupt-parent = <&mpic>;
+			mdio-handle = <&MDIO>;
 			phy-handle = <&phy8>;
 		};
 
@@ -102,6 +102,7 @@
 			address = [ 00 06 D2 00 00 02 ];
 			interrupts = <11 2>;
 			interrupt-parent = <&mpic>;
+			mdio-handle = <&MDIO>;
 			phy-handle = <&phy9>;
 		};
 


-- 
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

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

[David Gibson]

On Fri, May 25, 2007 at 02:23:59PM +1000, David Gibson wrote:
> On Thu, May 24, 2007 at 08:44:20AM -0500, Josh Boyer wrote:
> > On Wed, 2007-05-23 at 23:22 -0500, Kumar Gala wrote:
> > > On May 23, 2007, at 11:16 PM, David Gibson wrote:
> > > 
> > > > This patch fixes some problems with the way the Ethernet PHYs are
> > > > represented in the device tree for the Holly board.  This means
> > > > changes to the dts itself, and to the code with instantiates the
> > > > tsi108 ethernet platform devices based on the device tree.
> > > >
> > > > 	- First, and most importantly, the PHYs are given with an
> > > > identical 'reg' property.  This reg currently encodes the accessible
> > > > register used to initiate mdio interaction with the PHYs, rather than
> > > > a meaningful address on the parent bus (mdio in this case), which is
> > > > incorrect.  Instead we give the address of these registers as 'reg' in
> > > > the mdio node itself, and encode the ID of each phy in their 'reg'
> > > > propertys.  The PHY's unit name addresses are updated to match.
> > > > 	- 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).
> > > 
> > > Is the compatible really need here?  We are able to provide and bind  
> > > drivers based on MII_PHYSID1/2.  I don't see what putting the  
> > > compatible proper gets us.
> > 
> > I think it's needed until the TSI driver switches to phylib.  Right now,
> > it assumes that a Marvell phy is used, which is what is present on the
> > Taiga MPC7448HPC2 board.  The Holly board uses the Broadcom phy and we
> > added the compatible code for that.
> 
> I think Ben's suggested approach of using a special property in the
> phy node to indicate that this workaround is necessary is better.
> After all, the workaround isn't actually related to the type of PHY,
> which is what compatible encodes, but to how it's wired up.  Revised
> patch below addressing this and other matters.

Grah! Forgot quilt ref.  Now a copy that might actually work.

Fix problems with Holly's DT representation of ethernet PHYs

This patch fixes some problems with the way the Ethernet PHYs are
represented in the device tree for the Holly board.  This means
changes to the dts itself, and to the code with instantiates the
tsi108 ethernet platform devices based on the device tree.

	- First, and most importantly, the PHYs are given with an
identical 'reg' property.  This reg currently encodes the accessible
register used to initiate mdio interaction with the PHYs, rather than
a meaningful address on the parent bus (mdio in this case), which is
incorrect.  Instead we give the address of these registers as 'reg' in
the mdio node itself, and encode the ID of each phy in their 'reg'
propertys.  The PHY's unit name addresses are updated to match.
	- 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).

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Index: working-2.6/arch/powerpc/boot/dts/holly.dts
===================================================================
--- working-2.6.orig/arch/powerpc/boot/dts/holly.dts	2007-05-25 14:34:00.000000000 +1000
+++ working-2.6/arch/powerpc/boot/dts/holly.dts	2007-05-25 14:34:01.000000000 +1000
@@ -60,22 +60,21 @@
 			reg = <7000 400>;
 		};
 
-		mdio@6000 {
+		MDIO: mdio@6000 {
 			device_type = "mdio";
 			compatible = "tsi-ethernet";
+			reg = <6000 50>;
+			#address-cells = <1>;
+			#size-cells = <0>;
 
-			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;
 			};
 
-			PHY2: ethernet-phy@6400 {
-				device_type = "ethernet-phy";
-				compatible = "bcm54xx";
-				reg = <6000 50>;
-				phy-id = <2>;
+			PHY2: ethernet-phy@2 {
+				reg = <2>;
+				txc-rxc-delay-disable;
 			};
 		};
 
@@ -88,6 +87,7 @@
 			local-mac-address = [ 00 00 00 00 00 00 ];
 			interrupt-parent = < &/tsi109@c0000000/pic@7400 >;
 			interrupts = <10 2>;
+			mdio-handle = <&MDIO>;
 			phy-handle = <&PHY1>;
 		};
 
@@ -100,6 +100,7 @@
 			local-mac-address = [ 00 00 00 00 00 00 ];
 			interrupt-parent = < &/tsi109@c0000000/pic@7400 >;
 			interrupts = <11 2>;
+			mdio-handle = <&MDIO>;
 			phy-handle = <&PHY2>;
 		};
 
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:36:48.000000000 +1000
@@ -75,9 +75,8 @@ static int __init tsi108_eth_of_init(voi
 	     (np = of_find_compatible_node(np, "network", "tsi-ethernet")) != NULL;
 	     i++) {
 		struct resource r[2];
-		struct device_node *phy;
+		struct device_node *phy, *mdio;
 		hw_info tsi_eth_data;
-		const unsigned int *id;
 		const unsigned int *phy_id;
 		const void *mac_addr;
 		const phandle *ph;
@@ -111,6 +110,13 @@ static int __init tsi108_eth_of_init(voi
 		if (mac_addr)
 			memcpy(tsi_eth_data.mac_addr, mac_addr, 6);
 
+		ph = of_get_property(np, "mdio-handle", NULL);
+		mdio = of_find_node_by_phandle(*ph);
+		ret = of_address_to_resource(mdio, 0, &res);
+		of_node_put(mdio);
+		if (ret)
+			goto unreg;
+
 		ph = of_get_property(np, "phy-handle", NULL);
 		phy = of_find_node_by_phandle(*ph);
 
@@ -119,20 +125,17 @@ static int __init tsi108_eth_of_init(voi
 			goto unreg;
 		}
 
-		id = of_get_property(phy, "reg", NULL);
-		phy_id = of_get_property(phy, "phy-id", NULL);
-		ret = of_address_to_resource(phy, 0, &res);
-		if (ret) {
-			of_node_put(phy);
-			goto unreg;
-		}
+		phy_id = of_get_property(phy, "reg", NULL);
+
 		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);
+
 		ret =
 		    platform_device_add_data(tsi_eth_dev, &tsi_eth_data,
 					     sizeof(hw_info));
Index: working-2.6/arch/powerpc/boot/dts/mpc7448hpc2.dts
===================================================================
--- working-2.6.orig/arch/powerpc/boot/dts/mpc7448hpc2.dts	2007-05-25 14:34:00.000000000 +1000
+++ working-2.6/arch/powerpc/boot/dts/mpc7448hpc2.dts	2007-05-25 14:34:01.000000000 +1000
@@ -58,24 +58,23 @@
 			compatible  = "tsi-i2c";
 		};
 
-		mdio@6000 {
+		MDIO: mdio@6000 {
 			device_type = "mdio";
 			compatible = "tsi-ethernet";
+			reg = <6000 50>;
+			#address-cells = <1>;
+			#size-cells = <0>;
 
-			phy8: ethernet-phy@6000 {
+			phy8: ethernet-phy@8 {
 				interrupt-parent = <&mpic>;
 				interrupts = <2 1>;
-				reg = <6000 50>;
-				phy-id = <8>;
-				device_type = "ethernet-phy";
+				reg = <8>;
 			};
 
-			phy9: ethernet-phy@6400 {
+			phy9: ethernet-phy@9 {
 				interrupt-parent = <&mpic>;
 				interrupts = <2 1>;
-				reg = <6000 50>;
-				phy-id = <9>;
-				device_type = "ethernet-phy";
+				reg = <9>;
 			};
 
 		};
@@ -89,6 +88,7 @@
 			address = [ 00 06 D2 00 00 01 ];
 			interrupts = <10 2>;
 			interrupt-parent = <&mpic>;
+			mdio-handle = <&MDIO>;
 			phy-handle = <&phy8>;
 		};
 
@@ -102,6 +102,7 @@
 			address = [ 00 06 D2 00 00 02 ];
 			interrupts = <11 2>;
 			interrupt-parent = <&mpic>;
+			mdio-handle = <&MDIO>;
 			phy-handle = <&phy9>;
 		};
 


-- 
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

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

[David Gibson]

On Fri, May 25, 2007 at 02:37:28PM +1000, David Gibson wrote:
> On Fri, May 25, 2007 at 02:23:59PM +1000, David Gibson wrote:
> > On Thu, May 24, 2007 at 08:44:20AM -0500, Josh Boyer wrote:
> > > On Wed, 2007-05-23 at 23:22 -0500, Kumar Gala wrote:
> > > > On May 23, 2007, at 11:16 PM, David Gibson wrote:
> > > > 
> > > > > This patch fixes some problems with the way the Ethernet PHYs are
> > > > > represented in the device tree for the Holly board.  This means
> > > > > changes to the dts itself, and to the code with instantiates the
> > > > > tsi108 ethernet platform devices based on the device tree.
> > > > >
> > > > > 	- First, and most importantly, the PHYs are given with an
> > > > > identical 'reg' property.  This reg currently encodes the accessible
> > > > > register used to initiate mdio interaction with the PHYs, rather than
> > > > > a meaningful address on the parent bus (mdio in this case), which is
> > > > > incorrect.  Instead we give the address of these registers as 'reg' in
> > > > > the mdio node itself, and encode the ID of each phy in their 'reg'
> > > > > propertys.  The PHY's unit name addresses are updated to match.
> > > > > 	- 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).
> > > > 
> > > > Is the compatible really need here?  We are able to provide and bind  
> > > > drivers based on MII_PHYSID1/2.  I don't see what putting the  
> > > > compatible proper gets us.
> > > 
> > > I think it's needed until the TSI driver switches to phylib.  Right now,
> > > it assumes that a Marvell phy is used, which is what is present on the
> > > Taiga MPC7448HPC2 board.  The Holly board uses the Broadcom phy and we
> > > added the compatible code for that.
> > 
> > I think Ben's suggested approach of using a special property in the
> > phy node to indicate that this workaround is necessary is better.
> > After all, the workaround isn't actually related to the type of PHY,
> > which is what compatible encodes, but to how it's wired up.  Revised
> > patch below addressing this and other matters.
> 
> Grah! Forgot quilt ref.  Now a copy that might actually work.

Waah!  Third time lucky?

Fix problems with Holly's DT representation of ethernet PHYs

This patch fixes some problems with the way the Ethernet PHYs are
represented in the device tree for the Holly board.  This means
changes to the dts itself, and to the code with instantiates the
tsi108 ethernet platform devices based on the device tree.

	- First, and most importantly, the PHYs are given with an
identical 'reg' property.  This reg currently encodes the accessible
register used to initiate mdio interaction with the PHYs, rather than
a meaningful address on the parent bus (mdio in this case), which is
incorrect.  Instead we give the address of these registers as 'reg' in
the mdio node itself, and encode the ID of each phy in their 'reg'
propertys.  The PHY's unit name addresses are updated to match.
	- 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).

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>

Index: working-2.6/arch/powerpc/boot/dts/holly.dts
===================================================================
--- working-2.6.orig/arch/powerpc/boot/dts/holly.dts	2007-05-25 14:34:00.000000000 +1000
+++ working-2.6/arch/powerpc/boot/dts/holly.dts	2007-05-25 14:34:01.000000000 +1000
@@ -60,22 +60,21 @@
 			reg = <7000 400>;
 		};
 
-		mdio@6000 {
+		MDIO: mdio@6000 {
 			device_type = "mdio";
 			compatible = "tsi-ethernet";
+			reg = <6000 50>;
+			#address-cells = <1>;
+			#size-cells = <0>;
 
-			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;
 			};
 
-			PHY2: ethernet-phy@6400 {
-				device_type = "ethernet-phy";
-				compatible = "bcm54xx";
-				reg = <6000 50>;
-				phy-id = <2>;
+			PHY2: ethernet-phy@2 {
+				reg = <2>;
+				txc-rxc-delay-disable;
 			};
 		};
 
@@ -88,6 +87,7 @@
 			local-mac-address = [ 00 00 00 00 00 00 ];
 			interrupt-parent = < &/tsi109@c0000000/pic@7400 >;
 			interrupts = <10 2>;
+			mdio-handle = <&MDIO>;
 			phy-handle = <&PHY1>;
 		};
 
@@ -100,6 +100,7 @@
 			local-mac-address = [ 00 00 00 00 00 00 ];
 			interrupt-parent = < &/tsi109@c0000000/pic@7400 >;
 			interrupts = <11 2>;
+			mdio-handle = <&MDIO>;
 			phy-handle = <&PHY2>;
 		};
 
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
@@ -75,9 +75,8 @@ static int __init tsi108_eth_of_init(voi
 	     (np = of_find_compatible_node(np, "network", "tsi-ethernet")) != NULL;
 	     i++) {
 		struct resource r[2];
-		struct device_node *phy;
+		struct device_node *phy, *mdio;
 		hw_info tsi_eth_data;
-		const unsigned int *id;
 		const unsigned int *phy_id;
 		const void *mac_addr;
 		const phandle *ph;
@@ -111,6 +110,13 @@ static int __init tsi108_eth_of_init(voi
 		if (mac_addr)
 			memcpy(tsi_eth_data.mac_addr, mac_addr, 6);
 
+		ph = of_get_property(np, "mdio-handle", NULL);
+		mdio = of_find_node_by_phandle(*ph);
+		ret = of_address_to_resource(mdio, 0, &res);
+		of_node_put(mdio);
+		if (ret)
+			goto unreg;
+
 		ph = of_get_property(np, "phy-handle", NULL);
 		phy = of_find_node_by_phandle(*ph);
 
@@ -119,20 +125,17 @@ static int __init tsi108_eth_of_init(voi
 			goto unreg;
 		}
 
-		id = of_get_property(phy, "reg", NULL);
-		phy_id = of_get_property(phy, "phy-id", NULL);
-		ret = of_address_to_resource(phy, 0, &res);
-		if (ret) {
-			of_node_put(phy);
-			goto unreg;
-		}
+		phy_id = of_get_property(phy, "reg", NULL);
+
 		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);
+
 		ret =
 		    platform_device_add_data(tsi_eth_dev, &tsi_eth_data,
 					     sizeof(hw_info));
Index: working-2.6/arch/powerpc/boot/dts/mpc7448hpc2.dts
===================================================================
--- working-2.6.orig/arch/powerpc/boot/dts/mpc7448hpc2.dts	2007-05-25 14:34:00.000000000 +1000
+++ working-2.6/arch/powerpc/boot/dts/mpc7448hpc2.dts	2007-05-25 14:34:01.000000000 +1000
@@ -58,24 +58,23 @@
 			compatible  = "tsi-i2c";
 		};
 
-		mdio@6000 {
+		MDIO: mdio@6000 {
 			device_type = "mdio";
 			compatible = "tsi-ethernet";
+			reg = <6000 50>;
+			#address-cells = <1>;
+			#size-cells = <0>;
 
-			phy8: ethernet-phy@6000 {
+			phy8: ethernet-phy@8 {
 				interrupt-parent = <&mpic>;
 				interrupts = <2 1>;
-				reg = <6000 50>;
-				phy-id = <8>;
-				device_type = "ethernet-phy";
+				reg = <8>;
 			};
 
-			phy9: ethernet-phy@6400 {
+			phy9: ethernet-phy@9 {
 				interrupt-parent = <&mpic>;
 				interrupts = <2 1>;
-				reg = <6000 50>;
-				phy-id = <9>;
-				device_type = "ethernet-phy";
+				reg = <9>;
 			};
 
 		};
@@ -89,6 +88,7 @@
 			address = [ 00 06 D2 00 00 01 ];
 			interrupts = <10 2>;
 			interrupt-parent = <&mpic>;
+			mdio-handle = <&MDIO>;
 			phy-handle = <&phy8>;
 		};
 
@@ -102,6 +102,7 @@
 			address = [ 00 06 D2 00 00 02 ];
 			interrupts = <11 2>;
 			interrupt-parent = <&mpic>;
+			mdio-handle = <&MDIO>;
 			phy-handle = <&phy9>;
 		};
 


-- 
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

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

[Josh Boyer]

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.

> -			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.

> 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.

josh

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

[Segher Boessenkool]

> Waah!  Third time lucky?

You wish :-)

> 	- 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 completely removed the "compatible" properties instead.
Bad idea.


Segher

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

[David Gibson]

On Fri, May 25, 2007 at 04:11:59PM +0200, Segher Boessenkool wrote:
> > Waah!  Third time lucky?
> 
> You wish :-)
> 
> > 	- 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 completely removed the "compatible" properties instead.
> Bad idea.

Um... weren't you the one that was just saying compatible properties
aren't necessary if you can distinguish the hardware in other ways?

-- 
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

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

[David Gibson]

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

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

[Benjamin Herrenschmidt]

On Mon, 2007-05-28 at 09:30 +1000, David Gibson wrote:
> On Fri, May 25, 2007 at 04:11:59PM +0200, Segher Boessenkool wrote:
> > > Waah!  Third time lucky?
> > 
> > You wish :-)
> > 
> > > 	- 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 completely removed the "compatible" properties instead.
> > Bad idea.
> 
> Um... weren't you the one that was just saying compatible properties
> aren't necessary if you can distinguish the hardware in other ways?

In that case however, the 54xx are fairly different from each other, so
please use something more specific.

Ben.

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

[Segher Boessenkool]

>>> 	- 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 completely removed the "compatible" properties instead.
>> Bad idea.
>
> Um... weren't you the one that was just saying compatible properties
> aren't necessary if you can distinguish the hardware in other ways?

The OS device driver doesn't need "compatible" if it
can probe the device some other way; it doesn't need
the device node at all, even.  You still should have
a "compatible" property (or, old style, a specific
"name" property) if you want the OS to be able to use
the device node to recognise the device (i.e., if a
device node for the device exists at all: always).


Segher

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

[Segher Boessenkool]

> In that case however, the 54xx are fairly different from each other, so
> please use something more specific.

_Never_ use the "xx" stuff, it's meaningless.


Segher

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

[Benjamin Herrenschmidt]

On Mon, 2007-05-28 at 13:07 +0200, Segher Boessenkool wrote:
> > In that case however, the 54xx are fairly different from each other, so
> > please use something more specific.
> 
> _Never_ use the "xx" stuff, it's meaningless.

Pretty much yeah... either you are compatible with a precise version
(the first one of the serie, like an hypothetical 5400) or you are not
in which case you have your precise model name in there.

Ben.

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

[David Gibson]

On Mon, May 28, 2007 at 01:06:11PM +0200, Segher Boessenkool wrote:
> >>> 	- 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 completely removed the "compatible" properties instead.
> >> Bad idea.
> >
> > Um... weren't you the one that was just saying compatible properties
> > aren't necessary if you can distinguish the hardware in other ways?
> 
> The OS device driver doesn't need "compatible" if it
> can probe the device some other way; it doesn't need
> the device node at all, even.  You still should have
> a "compatible" property (or, old style, a specific
> "name" property) if you want the OS to be able to use
> the device node to recognise the device (i.e., if a
> device node for the device exists at all: always).

Hrm.  Ok.  compatible property restored.

-- 
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

Fix problems with Holly's DT representation of ethernet PHYs

[David Gibson]

This patch fixes some problems with the way the Ethernet PHYs are
represented in the device tree for the Holly board.  This means
changes to the dts itself, and to the code with instantiates the
tsi108 ethernet platform devices based on the device tree.

	- First, and most importantly, the PHYs are given with an
identical 'reg' property.  This reg currently encodes the accessible
register used to initiate mdio interaction with the PHYs, rather than
a meaningful address on the parent bus (mdio in this case), which is
incorrect.  Instead we give the address of these registers as 'reg' in
the mdio node itself, and encode the ID of each phy in their 'reg'
propertyies.  The PHY's unit name addresses are updated to match.
	- Second, the PHYs give only "bcm54xx" as a compatible
property.  This is broken, since there are many bcm54xx PHY models,
and they have differences which can matter.  We replace the compatible
property with the precise PHY model, bcm5461a in this case.
	- Third, the node representing the mdio link had a compatible
property of "tsi-ethernet", identical to the ethernet MAC nodes.  This
is clearly incorrect, since it's a different sort of device, and
replaced with "tsi-mdio".
	- Finally, currently the platform device constructor enables a
workaround in the tsi108 ethernet driver based on the compatible
property of the PHY.  This is incorrect, because the workaround in
question is necessary due to the board's wiring of the PHY, not the
model of PHY itself.  This patch alters the constructor to instead
enable the workaround based on a new special property in the PHY node.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---

Fixed various problems pointed out in feedback.

Index: working-2.6/arch/powerpc/boot/dts/holly.dts
===================================================================
--- working-2.6.orig/arch/powerpc/boot/dts/holly.dts	2007-05-29 14:38:58.000000000 +1000
+++ working-2.6/arch/powerpc/boot/dts/holly.dts	2007-05-29 14:49:55.000000000 +1000
@@ -60,22 +60,23 @@
 			reg = <7000 400>;
 		};
 
-		mdio@6000 {
+		MDIO: mdio@6000 {
 			device_type = "mdio";
-			compatible = "tsi-ethernet";
+			compatible = "tsi-mdio";
+			reg = <6000 50>;
+			#address-cells = <1>;
+			#size-cells = <0>;
 
-			PHY1: ethernet-phy@6000 {
-				device_type = "ethernet-phy";
-				compatible = "bcm54xx";
-				reg = <6000 50>;
-				phy-id = <1>;
+			PHY1: ethernet-phy@1 {
+				compatible = "bcm5461a";
+				reg = <1>;
+				txc-rxc-delay-disable;
 			};
 
-			PHY2: ethernet-phy@6400 {
-				device_type = "ethernet-phy";
-				compatible = "bcm54xx";
-				reg = <6000 50>;
-				phy-id = <2>;
+			PHY2: ethernet-phy@2 {
+				compatible = "bcm5461a";
+				reg = <2>;
+				txc-rxc-delay-disable;
 			};
 		};
 
@@ -88,6 +89,7 @@
 			local-mac-address = [ 00 00 00 00 00 00 ];
 			interrupt-parent = < &/tsi109@c0000000/pic@7400 >;
 			interrupts = <10 2>;
+			mdio-handle = <&MDIO>;
 			phy-handle = <&PHY1>;
 		};
 
@@ -100,6 +102,7 @@
 			local-mac-address = [ 00 00 00 00 00 00 ];
 			interrupt-parent = < &/tsi109@c0000000/pic@7400 >;
 			interrupts = <11 2>;
+			mdio-handle = <&MDIO>;
 			phy-handle = <&PHY2>;
 		};
 
Index: working-2.6/arch/powerpc/sysdev/tsi108_dev.c
===================================================================
--- working-2.6.orig/arch/powerpc/sysdev/tsi108_dev.c	2007-05-08 14:58:06.000000000 +1000
+++ working-2.6/arch/powerpc/sysdev/tsi108_dev.c	2007-05-29 14:57:20.000000000 +1000
@@ -75,9 +75,8 @@ static int __init tsi108_eth_of_init(voi
 	     (np = of_find_compatible_node(np, "network", "tsi-ethernet")) != NULL;
 	     i++) {
 		struct resource r[2];
-		struct device_node *phy;
+		struct device_node *phy, *mdio;
 		hw_info tsi_eth_data;
-		const unsigned int *id;
 		const unsigned int *phy_id;
 		const void *mac_addr;
 		const phandle *ph;
@@ -111,6 +110,13 @@ static int __init tsi108_eth_of_init(voi
 		if (mac_addr)
 			memcpy(tsi_eth_data.mac_addr, mac_addr, 6);
 
+		ph = of_get_property(np, "mdio-handle", NULL);
+		mdio = of_find_node_by_phandle(*ph);
+		ret = of_address_to_resource(mdio, 0, &res);
+		of_node_put(mdio);
+		if (ret)
+			goto unreg;
+
 		ph = of_get_property(np, "phy-handle", NULL);
 		phy = of_find_node_by_phandle(*ph);
 
@@ -119,20 +125,25 @@ static int __init tsi108_eth_of_init(voi
 			goto unreg;
 		}
 
-		id = of_get_property(phy, "reg", NULL);
-		phy_id = of_get_property(phy, "phy-id", NULL);
-		ret = of_address_to_resource(phy, 0, &res);
-		if (ret) {
-			of_node_put(phy);
-			goto unreg;
-		}
+		phy_id = of_get_property(phy, "reg", NULL);
+
 		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"))
+
+		/* Some boards with the TSI108 bridge (e.g. Holly)
+		 * have a miswiring of the ethernet PHYs which
+		 * requires a workaround.  The special
+		 * "txc-rxc-delay-disable" property enables this
+		 * workaround.  FIXME: Need to port the tsi108_eth
+		 * driver itself to phylib and use a non-misleading
+		 * name for the workaround flag - it's not actually to
+		 * do with the model of PHY in use */
+		if (of_get_property(phy, "txc-rxc-delay-disable", NULL))
 			tsi_eth_data.phy_type = TSI108_PHY_BCM54XX;
 		of_node_put(phy);
+
 		ret =
 		    platform_device_add_data(tsi_eth_dev, &tsi_eth_data,
 					     sizeof(hw_info));
Index: working-2.6/arch/powerpc/boot/dts/mpc7448hpc2.dts
===================================================================
--- working-2.6.orig/arch/powerpc/boot/dts/mpc7448hpc2.dts	2007-05-21 12:47:17.000000000 +1000
+++ working-2.6/arch/powerpc/boot/dts/mpc7448hpc2.dts	2007-05-29 14:38:58.000000000 +1000
@@ -58,24 +58,23 @@
 			compatible  = "tsi-i2c";
 		};
 
-		mdio@6000 {
+		MDIO: mdio@6000 {
 			device_type = "mdio";
 			compatible = "tsi-ethernet";
+			reg = <6000 50>;
+			#address-cells = <1>;
+			#size-cells = <0>;
 
-			phy8: ethernet-phy@6000 {
+			phy8: ethernet-phy@8 {
 				interrupt-parent = <&mpic>;
 				interrupts = <2 1>;
-				reg = <6000 50>;
-				phy-id = <8>;
-				device_type = "ethernet-phy";
+				reg = <8>;
 			};
 
-			phy9: ethernet-phy@6400 {
+			phy9: ethernet-phy@9 {
 				interrupt-parent = <&mpic>;
 				interrupts = <2 1>;
-				reg = <6000 50>;
-				phy-id = <9>;
-				device_type = "ethernet-phy";
+				reg = <9>;
 			};
 
 		};
@@ -89,6 +88,7 @@
 			address = [ 00 06 D2 00 00 01 ];
 			interrupts = <10 2>;
 			interrupt-parent = <&mpic>;
+			mdio-handle = <&MDIO>;
 			phy-handle = <&phy8>;
 		};
 
@@ -102,6 +102,7 @@
 			address = [ 00 06 D2 00 00 02 ];
 			interrupts = <11 2>;
 			interrupt-parent = <&mpic>;
+			mdio-handle = <&MDIO>;
 			phy-handle = <&phy9>;
 		};
 


-- 
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

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

[Segher Boessenkool]

> +		MDIO: mdio@6000 {
>  			device_type = "mdio";
> -			compatible = "tsi-ethernet";
> +			compatible = "tsi-mdio";

Hrm, did I miss this before?  A more exact "compatible"
property would be better ("tsi109-mdio" "tsi108-mdio" or
something like that).

Rest looks good,


Segher

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

[Josh Boyer]

On Tue, 2007-05-29 at 08:43 +0200, Segher Boessenkool wrote:
> > +		MDIO: mdio@6000 {
> >  			device_type = "mdio";
> > -			compatible = "tsi-ethernet";
> > +			compatible = "tsi-mdio";
> 
> Hrm, did I miss this before?  A more exact "compatible"
> property would be better ("tsi109-mdio" "tsi108-mdio" or
> something like that).

If we must, then tsi108-mdio is what I would recommend.  They are the
same between 108, 109, and 110 as far as I know, so it's the lowest
common denominator.

josh

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

[Segher Boessenkool]

>>> +			compatible = "tsi-mdio";
>>
>> Hrm, did I miss this before?  A more exact "compatible"
>> property would be better ("tsi109-mdio" "tsi108-mdio" or
>> something like that).
>
> If we must, then tsi108-mdio is what I would recommend.  They are the
> same between 108, 109, and 110 as far as I know, so it's the lowest
> common denominator.

[Assuming what is really on the board is a tsi109...]

I recommend putting both 109 and 108 in the property, in
that order; that way, if you need to do something special
on the tsi109 implementation (something you might not yet
know about perhaps, maybe a bug; or some extra feature on
the tsi109 device that the driver doesn't handle yet), the
driver has a chance to do that.  If the driver doesn't care
and only uses tsi108 features, it obviously can probe for
tsi108 only and all will be fine as well.


Segher

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

[Josh Boyer]

On Tue, 2007-05-29 at 16:49 +0200, Segher Boessenkool wrote:
> >>> +			compatible = "tsi-mdio";
> >>
> >> Hrm, did I miss this before?  A more exact "compatible"
> >> property would be better ("tsi109-mdio" "tsi108-mdio" or
> >> something like that).
> >
> > If we must, then tsi108-mdio is what I would recommend.  They are the
> > same between 108, 109, and 110 as far as I know, so it's the lowest
> > common denominator.
> 
> [Assuming what is really on the board is a tsi109...]

For Holly, yes.  For Taiga, it's 108.  For Hackberry, it's 110.

> I recommend putting both 109 and 108 in the property, in
> that order; that way, if you need to do something special
> on the tsi109 implementation (something you might not yet
> know about perhaps, maybe a bug; or some extra feature on
> the tsi109 device that the driver doesn't handle yet), the
> driver has a chance to do that.  If the driver doesn't care
> and only uses tsi108 features, it obviously can probe for
> tsi108 only and all will be fine as well.

*shrug*  Either way works for me.

We're adding these compatible properties to DTS files and the drivers at
the same time.  Unless (until?) there are firmwares for these boards
that start specifying something else in a real device tree, it really
doesn't matter much.  Not that there's anything wrong with your
reasoning.  Just seems like we're trying really hard to define something
"correctly" when we control what's on both sides :).

josh

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

[David Gibson]

On Tue, May 29, 2007 at 08:43:38AM +0200, Segher Boessenkool wrote:
> > +		MDIO: mdio@6000 {
> >  			device_type = "mdio";
> > -			compatible = "tsi-ethernet";
> > +			compatible = "tsi-mdio";
> 
> Hrm, did I miss this before?  A more exact "compatible"
> property would be better ("tsi109-mdio" "tsi108-mdio" or
> something like that).

Hrm, yeah.  I thought about that, but left it as tsi-mdio to match the
other rather non-specific compatible properties ("tsi-ethernet",
"tsi-brige", "tsi-i2c" and so forth).

But I guess I'd better fix that too.  Next version of the patch fixes
a whole bunch of places to use tsi108/tsi109 compatible properties.

-- 
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

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

[Benjamin Herrenschmidt]

On Tue, 2007-05-29 at 14:29 -0500, Josh Boyer wrote:
> 
> *shrug*  Either way works for me.
> 
> We're adding these compatible properties to DTS files and the drivers
> at
> the same time.  Unless (until?) there are firmwares for these boards
> that start specifying something else in a real device tree, it really
> doesn't matter much.  Not that there's anything wrong with your
> reasoning.  Just seems like we're trying really hard to define
> something
> "correctly" when we control what's on both sides :).
> 

I do think Segher is right there... compatible 109 and then 108

Ben.

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

[David Gibson]

On Wed, May 30, 2007 at 09:17:15PM +1000, Benjamin Herrenschmidt wrote:
> On Tue, 2007-05-29 at 14:29 -0500, Josh Boyer wrote:
> > 
> > *shrug*  Either way works for me.
> > 
> > We're adding these compatible properties to DTS files and the drivers
> > at
> > the same time.  Unless (until?) there are firmwares for these boards
> > that start specifying something else in a real device tree, it really
> > doesn't matter much.  Not that there's anything wrong with your
> > reasoning.  Just seems like we're trying really hard to define
> > something
> > "correctly" when we control what's on both sides :).
> > 
> 
> I do think Segher is right there... compatible 109 and then 108

That's what I've done in the new patch.

-- 
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

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

[Segher Boessenkool]

> We're adding these compatible properties to DTS files and the drivers 
> at
> the same time.  Unless (until?) there are firmwares for these boards
> that start specifying something else in a real device tree, it really
> doesn't matter much.

Sure.  So you have time to work out things now, I
suggest you take advantage of that :-)

> Not that there's anything wrong with your
> reasoning.  Just seems like we're trying really hard to define 
> something
> "correctly" when we control what's on both sides :).

Right now, and in your case, you do.  OTOH, the
goal is to have the DTS be a well-established
stable interface between the firmware/bootloader/
bootwrapper and the kernel; there is no room for
either side of that interface playing dirty tricks,
not on any board ;-)

Also, the DTS files in the kernel source tree should
server as a best-of-breed example for people doing
custom device trees for their own boards.  We better
whip them into good shape or we'll all look foolish...


Segher

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

[Josh Boyer]

On Wed, 2007-05-30 at 13:36 +0200, Segher Boessenkool wrote:
> > We're adding these compatible properties to DTS files and the drivers 
> > at
> > the same time.  Unless (until?) there are firmwares for these boards
> > that start specifying something else in a real device tree, it really
> > doesn't matter much.
> 
> Sure.  So you have time to work out things now, I
> suggest you take advantage of that :-)
> 
> > Not that there's anything wrong with your
> > reasoning.  Just seems like we're trying really hard to define 
> > something
> > "correctly" when we control what's on both sides :).
> 
> Right now, and in your case, you do.  OTOH, the
> goal is to have the DTS be a well-established
> stable interface between the firmware/bootloader/
> bootwrapper and the kernel; there is no room for
> either side of that interface playing dirty tricks,
> not on any board ;-)
> 
> Also, the DTS files in the kernel source tree should
> server as a best-of-breed example for people doing
> custom device trees for their own boards.  We better
> whip them into good shape or we'll all look foolish...

Yeah, I know.  Ignore my earlier email.  I blame it on lack of sleep.

The only issue we might have in the future is if DT capable firmware for
these boards shows up and does something completely different.
Hopefully that won't happen.

josh

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

[Segher Boessenkool]

>>> Not that there's anything wrong with your
>>> reasoning.  Just seems like we're trying really hard to define
>>> something
>>> "correctly" when we control what's on both sides :).
>>
>> Right now, and in your case, you do.  OTOH, the
>> goal is to have the DTS be a well-established
>> stable interface between the firmware/bootloader/
>> bootwrapper and the kernel; there is no room for
>> either side of that interface playing dirty tricks,
>> not on any board ;-)
>>
>> Also, the DTS files in the kernel source tree should
>> server as a best-of-breed example for people doing
>> custom device trees for their own boards.  We better
>> whip them into good shape or we'll all look foolish...
>
> Yeah, I know.  Ignore my earlier email.  I blame it on lack of sleep.
>
> The only issue we might have in the future is if DT capable firmware 
> for
> these boards shows up and does something completely different.

Yeah, that's exactly the same problem as we would have
if we would code our device trees without trying to at
least create an informal binding for the nodes in question:
total chaos.

> Hopefully that won't happen.

Hopefully, indeed.

If a third party constructs a board with some weird
device tree, then they probably have a big set of Linux
patches to go with that.  Now either they work with the
kernel community to get that integrated into mainline
(which means they need to do a lot of changes to the DTS
as well if it indeed is weird / wrong, so they better
start doing that *before* selling the boards); or they
can happily maintain their own kernel fork, like so many
companies already do.

I don't see a problem here :-)


Segher

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

[Zang Roy-r61911]

>=20
> On Tue, 2007-05-29 at 16:49 +0200, Segher Boessenkool wrote:
> > >>> +			compatible =3D "tsi-mdio";
> > >>
> > >> Hrm, did I miss this before?  A more exact "compatible"
> > >> property would be better ("tsi109-mdio" "tsi108-mdio" or
> > >> something like that).
> > >
> > > If we must, then tsi108-mdio is what I would recommend. =20
> They are the
> > > same between 108, 109, and 110 as far as I know, so it's=20
> the lowest
> > > common denominator.
> >=20
> > [Assuming what is really on the board is a tsi109...]
>=20
> For Holly, yes.  For Taiga, it's 108.  For Hackberry, it's 110.

For Taiga, it's 108 or 109.
The new manufactured taiga board with 109 chip!
Roy