All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] MDIO: FSL_PQ_MDIO: Fix bug on incorrect offset of tbipa register
@ 2013-06-12 12:47 ` Oded Gabbay
  0 siblings, 0 replies; 24+ messages in thread
From: Oded Gabbay @ 2013-06-12 12:47 UTC (permalink / raw)
  To: benh, paulus, leoli, galak, B38951, Dongsheng.Wang, bigeasy,
	stef.van.os, timur, davem, ogabbay
  Cc: linuxppc-dev, linux-kernel, netdev

This patch fixes a bug in the fsl_pq_mdio.c module and in relevant device-tree
files regarding the correct offset of the tbipa register in the eTSEC
controller in some of Freescale's PQ3 and QorIQ SoC.
The bug happens when the mdio in the device tree is configured to be compatible
to "fsl,gianfar-tbi". Because the mdio device in the device tree points to
addresses 25520, 26520 or 27520 (depends on the controller ID), the variable
priv->map at function fsl_pq_mdio_probe, points to that address. However,
later in the function there is a write to register tbipa that is actually
located at 25030, 26030 or 27030. Because the correct address is not io mapped,
the contents are written to a different register in the controller.
The fix sets the address of the mdio device to start at 25000, 26000 or 27000
and changes the mii_offset field to 0x520 in the relevant entry
(fsl,gianfar-tbi) of the fsl_pq_mdio_match array.

Note: This patch may break MDIO functionallity of some old Freescale's SoC
until Freescale will fix their device tree files. Basically, every device tree
which contains an mdio device that is compatible to "fsl,gianfar-tbi" should be
examined.

Signed-off-by: Oded Gabbay <ogabbay@advaoptical.com>
---
 arch/powerpc/boot/dts/fsl/pq3-etsec1-1.dtsi    | 4 ++--
 arch/powerpc/boot/dts/fsl/pq3-etsec1-2.dtsi    | 4 ++--
 arch/powerpc/boot/dts/fsl/pq3-etsec1-3.dtsi    | 4 ++--
 arch/powerpc/boot/dts/ge_imp3a.dts             | 4 ++--
 arch/powerpc/boot/dts/mpc8536ds.dtsi           | 4 ++--
 arch/powerpc/boot/dts/mpc8544ds.dtsi           | 2 +-
 arch/powerpc/boot/dts/mpc8548cds.dtsi          | 6 +++---
 arch/powerpc/boot/dts/mpc8568mds.dts           | 2 +-
 arch/powerpc/boot/dts/mpc8572ds.dtsi           | 6 +++---
 arch/powerpc/boot/dts/mpc8572ds_camp_core0.dts | 4 ++--
 arch/powerpc/boot/dts/mpc8572ds_camp_core1.dts | 2 +-
 arch/powerpc/boot/dts/p2020ds.dtsi             | 4 ++--
 arch/powerpc/boot/dts/p2020rdb-pc.dtsi         | 4 ++--
 arch/powerpc/boot/dts/p2020rdb.dts             | 4 ++--
 arch/powerpc/boot/dts/ppa8548.dts              | 6 +++---
 drivers/net/ethernet/freescale/fsl_pq_mdio.c   | 2 +-
 16 files changed, 31 insertions(+), 31 deletions(-)

diff --git a/arch/powerpc/boot/dts/fsl/pq3-etsec1-1.dtsi b/arch/powerpc/boot/dts/fsl/pq3-etsec1-1.dtsi
index 96693b4..d38bf63 100644
--- a/arch/powerpc/boot/dts/fsl/pq3-etsec1-1.dtsi
+++ b/arch/powerpc/boot/dts/fsl/pq3-etsec1-1.dtsi
@@ -46,9 +46,9 @@ ethernet@25000 {
 	interrupts = <35 2 0 0 36 2 0 0 40 2 0 0>;
 };
 
-mdio@25520 {
+mdio@25000 {
 	#address-cells = <1>;
 	#size-cells = <0>;
 	compatible = "fsl,gianfar-tbi";
-	reg = <0x25520 0x20>;
+	reg = <0x25000 0x1000>;
 };
diff --git a/arch/powerpc/boot/dts/fsl/pq3-etsec1-2.dtsi b/arch/powerpc/boot/dts/fsl/pq3-etsec1-2.dtsi
index 6b3fab1..6290b49 100644
--- a/arch/powerpc/boot/dts/fsl/pq3-etsec1-2.dtsi
+++ b/arch/powerpc/boot/dts/fsl/pq3-etsec1-2.dtsi
@@ -46,9 +46,9 @@ ethernet@26000 {
 	interrupts = <31 2 0 0 32 2 0 0 33 2 0 0>;
 };
 
-mdio@26520 {
+mdio@26000 {
 	#address-cells = <1>;
 	#size-cells = <0>;
 	compatible = "fsl,gianfar-tbi";
-	reg = <0x26520 0x20>;
+	reg = <0x26000 0x1000>;
 };
diff --git a/arch/powerpc/boot/dts/fsl/pq3-etsec1-3.dtsi b/arch/powerpc/boot/dts/fsl/pq3-etsec1-3.dtsi
index 0da592d..5296811 100644
--- a/arch/powerpc/boot/dts/fsl/pq3-etsec1-3.dtsi
+++ b/arch/powerpc/boot/dts/fsl/pq3-etsec1-3.dtsi
@@ -46,9 +46,9 @@ ethernet@27000 {
 	interrupts = <37 2 0 0 38 2 0 0 39 2 0 0>;
 };
 
-mdio@27520 {
+mdio@27000 {
 	#address-cells = <1>;
 	#size-cells = <0>;
 	compatible = "fsl,gianfar-tbi";
-	reg = <0x27520 0x20>;
+	reg = <0x27000 0x1000>;
 };
diff --git a/arch/powerpc/boot/dts/ge_imp3a.dts b/arch/powerpc/boot/dts/ge_imp3a.dts
index fefae41..49d9b4e 100644
--- a/arch/powerpc/boot/dts/ge_imp3a.dts
+++ b/arch/powerpc/boot/dts/ge_imp3a.dts
@@ -174,14 +174,14 @@
 			};
 		};
 
-		mdio@25520 {
+		mdio@25000 {
 			tbi1: tbi-phy@11 {
 				reg = <0x11>;
 				device_type = "tbi-phy";
 			};
 		};
 
-		mdio@26520 {
+		mdio@26000 {
 			status = "disabled";
 		};
 
diff --git a/arch/powerpc/boot/dts/mpc8536ds.dtsi b/arch/powerpc/boot/dts/mpc8536ds.dtsi
index 7c3dde8..c4df5a1 100644
--- a/arch/powerpc/boot/dts/mpc8536ds.dtsi
+++ b/arch/powerpc/boot/dts/mpc8536ds.dtsi
@@ -227,11 +227,11 @@
 		phy-connection-type = "rgmii-id";
 	};
 
-	mdio@26520 {
+	mdio@26000 {
 		#address-cells = <1>;
 		#size-cells = <0>;
 		compatible = "fsl,gianfar-tbi";
-		reg = <0x26520 0x20>;
+		reg = <0x26000 0x1000>;
 
 		tbi1: tbi-phy@11 {
 			reg = <0x11>;
diff --git a/arch/powerpc/boot/dts/mpc8544ds.dtsi b/arch/powerpc/boot/dts/mpc8544ds.dtsi
index b219d03..ba051b2 100644
--- a/arch/powerpc/boot/dts/mpc8544ds.dtsi
+++ b/arch/powerpc/boot/dts/mpc8544ds.dtsi
@@ -111,7 +111,7 @@
 		phy-connection-type = "rgmii-id";
 	};
 
-	mdio@26520 {
+	mdio@26000 {
 		tbi1: tbi-phy@11 {
 			reg = <0x11>;
 			device_type = "tbi-phy";
diff --git a/arch/powerpc/boot/dts/mpc8548cds.dtsi b/arch/powerpc/boot/dts/mpc8548cds.dtsi
index c61f525..85da565 100644
--- a/arch/powerpc/boot/dts/mpc8548cds.dtsi
+++ b/arch/powerpc/boot/dts/mpc8548cds.dtsi
@@ -137,7 +137,7 @@
 		phy-handle = <&phy1>;
 	};
 
-	mdio@25520 {
+	mdio@25000 {
 		tbi1: tbi-phy@11 {
 			reg = <0x11>;
 			device_type = "tbi-phy";
@@ -149,7 +149,7 @@
 		phy-handle = <&phy2>;
 	};
 
-	mdio@26520 {
+	mdio@26000 {
 		tbi2: tbi-phy@11 {
 			reg = <0x11>;
 			device_type = "tbi-phy";
@@ -161,7 +161,7 @@
 		phy-handle = <&phy3>;
 	};
 
-	mdio@27520 {
+	mdio@27000 {
 		tbi3: tbi-phy@11 {
 			reg = <0x11>;
 			device_type = "tbi-phy";
diff --git a/arch/powerpc/boot/dts/mpc8568mds.dts b/arch/powerpc/boot/dts/mpc8568mds.dts
index 09598bb..30cd8b8 100644
--- a/arch/powerpc/boot/dts/mpc8568mds.dts
+++ b/arch/powerpc/boot/dts/mpc8568mds.dts
@@ -120,7 +120,7 @@
 			sleep = <&pmc 0x00000040>;
 		};
 
-		mdio@25520 {
+		mdio@25000 {
 			tbi1: tbi-phy@11 {
 				reg = <0x11>;
 				device_type = "tbi-phy";
diff --git a/arch/powerpc/boot/dts/mpc8572ds.dtsi b/arch/powerpc/boot/dts/mpc8572ds.dtsi
index 357490b..c82a800 100644
--- a/arch/powerpc/boot/dts/mpc8572ds.dtsi
+++ b/arch/powerpc/boot/dts/mpc8572ds.dtsi
@@ -208,7 +208,7 @@
 
 	};
 
-	mdio@25520 {
+	mdio@25000 {
 		tbi1: tbi-phy@11 {
 			reg = <0x11>;
 			device_type = "tbi-phy";
@@ -221,7 +221,7 @@
 		phy-connection-type = "rgmii-id";
 
 	};
-	mdio@26520 {
+	mdio@26000 {
 		tbi2: tbi-phy@11 {
 			reg = <0x11>;
 			device_type = "tbi-phy";
@@ -234,7 +234,7 @@
 		phy-connection-type = "rgmii-id";
 	};
 
-	mdio@27520 {
+	mdio@27000 {
 		tbi3: tbi-phy@11 {
 			reg = <0x11>;
 			device_type = "tbi-phy";
diff --git a/arch/powerpc/boot/dts/mpc8572ds_camp_core0.dts b/arch/powerpc/boot/dts/mpc8572ds_camp_core0.dts
index ef9ef56..5dae410 100644
--- a/arch/powerpc/boot/dts/mpc8572ds_camp_core0.dts
+++ b/arch/powerpc/boot/dts/mpc8572ds_camp_core0.dts
@@ -47,13 +47,13 @@
 		ethernet@26000 {
 			status = "disabled";
 		};
-		mdio@26520 {
+		mdio@26000 {
 			status = "disabled";
 		};
 		ethernet@27000 {
 			status = "disabled";
 		};
-		mdio@27520 {
+		mdio@27000 {
 			status = "disabled";
 		};
 		pic@40000 {
diff --git a/arch/powerpc/boot/dts/mpc8572ds_camp_core1.dts b/arch/powerpc/boot/dts/mpc8572ds_camp_core1.dts
index 24564ee..706e782 100644
--- a/arch/powerpc/boot/dts/mpc8572ds_camp_core1.dts
+++ b/arch/powerpc/boot/dts/mpc8572ds_camp_core1.dts
@@ -73,7 +73,7 @@
 		ethernet@25000 {
 			status = "disabled";
 		};
-		mdio@25520 {
+		mdio@25000 {
 			status = "disabled";
 		};
 		crypto@30000 {
diff --git a/arch/powerpc/boot/dts/p2020ds.dtsi b/arch/powerpc/boot/dts/p2020ds.dtsi
index e699cf9..1e9001c 100644
--- a/arch/powerpc/boot/dts/p2020ds.dtsi
+++ b/arch/powerpc/boot/dts/p2020ds.dtsi
@@ -167,14 +167,14 @@
 
 	};
 
-	mdio@25520 {
+	mdio@25000 {
 		tbi1: tbi-phy@11 {
 			reg = <0x11>;
 			device_type = "tbi-phy";
 		};
 	};
 
-	mdio@26520 {
+	mdio@26000 {
 		tbi2: tbi-phy@11 {
 			reg = <0x11>;
 			device_type = "tbi-phy";
diff --git a/arch/powerpc/boot/dts/p2020rdb-pc.dtsi b/arch/powerpc/boot/dts/p2020rdb-pc.dtsi
index c21d1c7..994cb4c 100644
--- a/arch/powerpc/boot/dts/p2020rdb-pc.dtsi
+++ b/arch/powerpc/boot/dts/p2020rdb-pc.dtsi
@@ -203,14 +203,14 @@
 			};
 	};
 
-	mdio@25520 {
+	mdio@25000 {
 		tbi0: tbi-phy@11 {
 			reg = <0x11>;
 			device_type = "tbi-phy";
 		};
 	};
 
-	mdio@26520 {
+	mdio@26000 {
 		status = "disabled";
 	};
 
diff --git a/arch/powerpc/boot/dts/p2020rdb.dts b/arch/powerpc/boot/dts/p2020rdb.dts
index 4d52bce..b6f50e1 100644
--- a/arch/powerpc/boot/dts/p2020rdb.dts
+++ b/arch/powerpc/boot/dts/p2020rdb.dts
@@ -215,14 +215,14 @@
 			};
 		};
 
-		mdio@25520 {
+		mdio@25000 {
 			tbi0: tbi-phy@11 {
 				reg = <0x11>;
 				device_type = "tbi-phy";
 			};
 		};
 
-		mdio@26520 {
+		mdio@26000 {
 			status = "disabled";
 		};
 
diff --git a/arch/powerpc/boot/dts/ppa8548.dts b/arch/powerpc/boot/dts/ppa8548.dts
index f97ecee..2117927 100644
--- a/arch/powerpc/boot/dts/ppa8548.dts
+++ b/arch/powerpc/boot/dts/ppa8548.dts
@@ -128,7 +128,7 @@
 		phy-handle = <&phy0>;
 	};
 
-	mdio@25520 {
+	mdio@25000 {
 		tbi1: tbi-phy@11 {
 			reg = <0x11>;
 			device_type = "tbi-phy";
@@ -140,7 +140,7 @@
 		phy-handle = <&phy1>;
 	};
 
-	mdio@26520 {
+	mdio@26000 {
 		tbi2: tbi-phy@11 {
 			reg = <0x11>;
 			device_type = "tbi-phy";
@@ -151,7 +151,7 @@
 		status = "disabled";
 	};
 
-	mdio@27520 {
+	mdio@27000 {
 		tbi3: tbi-phy@11 {
 			reg = <0x11>;
 			device_type = "tbi-phy";
diff --git a/drivers/net/ethernet/freescale/fsl_pq_mdio.c b/drivers/net/ethernet/freescale/fsl_pq_mdio.c
index c93a056..4228c4e 100644
--- a/drivers/net/ethernet/freescale/fsl_pq_mdio.c
+++ b/drivers/net/ethernet/freescale/fsl_pq_mdio.c
@@ -288,7 +288,7 @@ static struct of_device_id fsl_pq_mdio_match[] = {
 	{
 		.compatible = "fsl,gianfar-tbi",
 		.data = &(struct fsl_pq_mdio_data) {
-			.mii_offset = 0,
+			.mii_offset = 0x520,
 			.get_tbipa = get_gfar_tbipa,
 		},
 	},
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH] MDIO: FSL_PQ_MDIO: Fix bug on incorrect offset of tbipa register
@ 2013-06-12 12:47 ` Oded Gabbay
  0 siblings, 0 replies; 24+ messages in thread
From: Oded Gabbay @ 2013-06-12 12:47 UTC (permalink / raw)
  To: benh, paulus, leoli, galak, B38951, Dongsheng.Wang, bigeasy,
	stef.van.os, timur, davem, ogabbay
  Cc: netdev, linuxppc-dev, linux-kernel

This patch fixes a bug in the fsl_pq_mdio.c module and in relevant device-tree
files regarding the correct offset of the tbipa register in the eTSEC
controller in some of Freescale's PQ3 and QorIQ SoC.
The bug happens when the mdio in the device tree is configured to be compatible
to "fsl,gianfar-tbi". Because the mdio device in the device tree points to
addresses 25520, 26520 or 27520 (depends on the controller ID), the variable
priv->map at function fsl_pq_mdio_probe, points to that address. However,
later in the function there is a write to register tbipa that is actually
located at 25030, 26030 or 27030. Because the correct address is not io mapped,
the contents are written to a different register in the controller.
The fix sets the address of the mdio device to start at 25000, 26000 or 27000
and changes the mii_offset field to 0x520 in the relevant entry
(fsl,gianfar-tbi) of the fsl_pq_mdio_match array.

Note: This patch may break MDIO functionallity of some old Freescale's SoC
until Freescale will fix their device tree files. Basically, every device tree
which contains an mdio device that is compatible to "fsl,gianfar-tbi" should be
examined.

Signed-off-by: Oded Gabbay <ogabbay@advaoptical.com>
---
 arch/powerpc/boot/dts/fsl/pq3-etsec1-1.dtsi    | 4 ++--
 arch/powerpc/boot/dts/fsl/pq3-etsec1-2.dtsi    | 4 ++--
 arch/powerpc/boot/dts/fsl/pq3-etsec1-3.dtsi    | 4 ++--
 arch/powerpc/boot/dts/ge_imp3a.dts             | 4 ++--
 arch/powerpc/boot/dts/mpc8536ds.dtsi           | 4 ++--
 arch/powerpc/boot/dts/mpc8544ds.dtsi           | 2 +-
 arch/powerpc/boot/dts/mpc8548cds.dtsi          | 6 +++---
 arch/powerpc/boot/dts/mpc8568mds.dts           | 2 +-
 arch/powerpc/boot/dts/mpc8572ds.dtsi           | 6 +++---
 arch/powerpc/boot/dts/mpc8572ds_camp_core0.dts | 4 ++--
 arch/powerpc/boot/dts/mpc8572ds_camp_core1.dts | 2 +-
 arch/powerpc/boot/dts/p2020ds.dtsi             | 4 ++--
 arch/powerpc/boot/dts/p2020rdb-pc.dtsi         | 4 ++--
 arch/powerpc/boot/dts/p2020rdb.dts             | 4 ++--
 arch/powerpc/boot/dts/ppa8548.dts              | 6 +++---
 drivers/net/ethernet/freescale/fsl_pq_mdio.c   | 2 +-
 16 files changed, 31 insertions(+), 31 deletions(-)

diff --git a/arch/powerpc/boot/dts/fsl/pq3-etsec1-1.dtsi b/arch/powerpc/boot/dts/fsl/pq3-etsec1-1.dtsi
index 96693b4..d38bf63 100644
--- a/arch/powerpc/boot/dts/fsl/pq3-etsec1-1.dtsi
+++ b/arch/powerpc/boot/dts/fsl/pq3-etsec1-1.dtsi
@@ -46,9 +46,9 @@ ethernet@25000 {
 	interrupts = <35 2 0 0 36 2 0 0 40 2 0 0>;
 };
 
-mdio@25520 {
+mdio@25000 {
 	#address-cells = <1>;
 	#size-cells = <0>;
 	compatible = "fsl,gianfar-tbi";
-	reg = <0x25520 0x20>;
+	reg = <0x25000 0x1000>;
 };
diff --git a/arch/powerpc/boot/dts/fsl/pq3-etsec1-2.dtsi b/arch/powerpc/boot/dts/fsl/pq3-etsec1-2.dtsi
index 6b3fab1..6290b49 100644
--- a/arch/powerpc/boot/dts/fsl/pq3-etsec1-2.dtsi
+++ b/arch/powerpc/boot/dts/fsl/pq3-etsec1-2.dtsi
@@ -46,9 +46,9 @@ ethernet@26000 {
 	interrupts = <31 2 0 0 32 2 0 0 33 2 0 0>;
 };
 
-mdio@26520 {
+mdio@26000 {
 	#address-cells = <1>;
 	#size-cells = <0>;
 	compatible = "fsl,gianfar-tbi";
-	reg = <0x26520 0x20>;
+	reg = <0x26000 0x1000>;
 };
diff --git a/arch/powerpc/boot/dts/fsl/pq3-etsec1-3.dtsi b/arch/powerpc/boot/dts/fsl/pq3-etsec1-3.dtsi
index 0da592d..5296811 100644
--- a/arch/powerpc/boot/dts/fsl/pq3-etsec1-3.dtsi
+++ b/arch/powerpc/boot/dts/fsl/pq3-etsec1-3.dtsi
@@ -46,9 +46,9 @@ ethernet@27000 {
 	interrupts = <37 2 0 0 38 2 0 0 39 2 0 0>;
 };
 
-mdio@27520 {
+mdio@27000 {
 	#address-cells = <1>;
 	#size-cells = <0>;
 	compatible = "fsl,gianfar-tbi";
-	reg = <0x27520 0x20>;
+	reg = <0x27000 0x1000>;
 };
diff --git a/arch/powerpc/boot/dts/ge_imp3a.dts b/arch/powerpc/boot/dts/ge_imp3a.dts
index fefae41..49d9b4e 100644
--- a/arch/powerpc/boot/dts/ge_imp3a.dts
+++ b/arch/powerpc/boot/dts/ge_imp3a.dts
@@ -174,14 +174,14 @@
 			};
 		};
 
-		mdio@25520 {
+		mdio@25000 {
 			tbi1: tbi-phy@11 {
 				reg = <0x11>;
 				device_type = "tbi-phy";
 			};
 		};
 
-		mdio@26520 {
+		mdio@26000 {
 			status = "disabled";
 		};
 
diff --git a/arch/powerpc/boot/dts/mpc8536ds.dtsi b/arch/powerpc/boot/dts/mpc8536ds.dtsi
index 7c3dde8..c4df5a1 100644
--- a/arch/powerpc/boot/dts/mpc8536ds.dtsi
+++ b/arch/powerpc/boot/dts/mpc8536ds.dtsi
@@ -227,11 +227,11 @@
 		phy-connection-type = "rgmii-id";
 	};
 
-	mdio@26520 {
+	mdio@26000 {
 		#address-cells = <1>;
 		#size-cells = <0>;
 		compatible = "fsl,gianfar-tbi";
-		reg = <0x26520 0x20>;
+		reg = <0x26000 0x1000>;
 
 		tbi1: tbi-phy@11 {
 			reg = <0x11>;
diff --git a/arch/powerpc/boot/dts/mpc8544ds.dtsi b/arch/powerpc/boot/dts/mpc8544ds.dtsi
index b219d03..ba051b2 100644
--- a/arch/powerpc/boot/dts/mpc8544ds.dtsi
+++ b/arch/powerpc/boot/dts/mpc8544ds.dtsi
@@ -111,7 +111,7 @@
 		phy-connection-type = "rgmii-id";
 	};
 
-	mdio@26520 {
+	mdio@26000 {
 		tbi1: tbi-phy@11 {
 			reg = <0x11>;
 			device_type = "tbi-phy";
diff --git a/arch/powerpc/boot/dts/mpc8548cds.dtsi b/arch/powerpc/boot/dts/mpc8548cds.dtsi
index c61f525..85da565 100644
--- a/arch/powerpc/boot/dts/mpc8548cds.dtsi
+++ b/arch/powerpc/boot/dts/mpc8548cds.dtsi
@@ -137,7 +137,7 @@
 		phy-handle = <&phy1>;
 	};
 
-	mdio@25520 {
+	mdio@25000 {
 		tbi1: tbi-phy@11 {
 			reg = <0x11>;
 			device_type = "tbi-phy";
@@ -149,7 +149,7 @@
 		phy-handle = <&phy2>;
 	};
 
-	mdio@26520 {
+	mdio@26000 {
 		tbi2: tbi-phy@11 {
 			reg = <0x11>;
 			device_type = "tbi-phy";
@@ -161,7 +161,7 @@
 		phy-handle = <&phy3>;
 	};
 
-	mdio@27520 {
+	mdio@27000 {
 		tbi3: tbi-phy@11 {
 			reg = <0x11>;
 			device_type = "tbi-phy";
diff --git a/arch/powerpc/boot/dts/mpc8568mds.dts b/arch/powerpc/boot/dts/mpc8568mds.dts
index 09598bb..30cd8b8 100644
--- a/arch/powerpc/boot/dts/mpc8568mds.dts
+++ b/arch/powerpc/boot/dts/mpc8568mds.dts
@@ -120,7 +120,7 @@
 			sleep = <&pmc 0x00000040>;
 		};
 
-		mdio@25520 {
+		mdio@25000 {
 			tbi1: tbi-phy@11 {
 				reg = <0x11>;
 				device_type = "tbi-phy";
diff --git a/arch/powerpc/boot/dts/mpc8572ds.dtsi b/arch/powerpc/boot/dts/mpc8572ds.dtsi
index 357490b..c82a800 100644
--- a/arch/powerpc/boot/dts/mpc8572ds.dtsi
+++ b/arch/powerpc/boot/dts/mpc8572ds.dtsi
@@ -208,7 +208,7 @@
 
 	};
 
-	mdio@25520 {
+	mdio@25000 {
 		tbi1: tbi-phy@11 {
 			reg = <0x11>;
 			device_type = "tbi-phy";
@@ -221,7 +221,7 @@
 		phy-connection-type = "rgmii-id";
 
 	};
-	mdio@26520 {
+	mdio@26000 {
 		tbi2: tbi-phy@11 {
 			reg = <0x11>;
 			device_type = "tbi-phy";
@@ -234,7 +234,7 @@
 		phy-connection-type = "rgmii-id";
 	};
 
-	mdio@27520 {
+	mdio@27000 {
 		tbi3: tbi-phy@11 {
 			reg = <0x11>;
 			device_type = "tbi-phy";
diff --git a/arch/powerpc/boot/dts/mpc8572ds_camp_core0.dts b/arch/powerpc/boot/dts/mpc8572ds_camp_core0.dts
index ef9ef56..5dae410 100644
--- a/arch/powerpc/boot/dts/mpc8572ds_camp_core0.dts
+++ b/arch/powerpc/boot/dts/mpc8572ds_camp_core0.dts
@@ -47,13 +47,13 @@
 		ethernet@26000 {
 			status = "disabled";
 		};
-		mdio@26520 {
+		mdio@26000 {
 			status = "disabled";
 		};
 		ethernet@27000 {
 			status = "disabled";
 		};
-		mdio@27520 {
+		mdio@27000 {
 			status = "disabled";
 		};
 		pic@40000 {
diff --git a/arch/powerpc/boot/dts/mpc8572ds_camp_core1.dts b/arch/powerpc/boot/dts/mpc8572ds_camp_core1.dts
index 24564ee..706e782 100644
--- a/arch/powerpc/boot/dts/mpc8572ds_camp_core1.dts
+++ b/arch/powerpc/boot/dts/mpc8572ds_camp_core1.dts
@@ -73,7 +73,7 @@
 		ethernet@25000 {
 			status = "disabled";
 		};
-		mdio@25520 {
+		mdio@25000 {
 			status = "disabled";
 		};
 		crypto@30000 {
diff --git a/arch/powerpc/boot/dts/p2020ds.dtsi b/arch/powerpc/boot/dts/p2020ds.dtsi
index e699cf9..1e9001c 100644
--- a/arch/powerpc/boot/dts/p2020ds.dtsi
+++ b/arch/powerpc/boot/dts/p2020ds.dtsi
@@ -167,14 +167,14 @@
 
 	};
 
-	mdio@25520 {
+	mdio@25000 {
 		tbi1: tbi-phy@11 {
 			reg = <0x11>;
 			device_type = "tbi-phy";
 		};
 	};
 
-	mdio@26520 {
+	mdio@26000 {
 		tbi2: tbi-phy@11 {
 			reg = <0x11>;
 			device_type = "tbi-phy";
diff --git a/arch/powerpc/boot/dts/p2020rdb-pc.dtsi b/arch/powerpc/boot/dts/p2020rdb-pc.dtsi
index c21d1c7..994cb4c 100644
--- a/arch/powerpc/boot/dts/p2020rdb-pc.dtsi
+++ b/arch/powerpc/boot/dts/p2020rdb-pc.dtsi
@@ -203,14 +203,14 @@
 			};
 	};
 
-	mdio@25520 {
+	mdio@25000 {
 		tbi0: tbi-phy@11 {
 			reg = <0x11>;
 			device_type = "tbi-phy";
 		};
 	};
 
-	mdio@26520 {
+	mdio@26000 {
 		status = "disabled";
 	};
 
diff --git a/arch/powerpc/boot/dts/p2020rdb.dts b/arch/powerpc/boot/dts/p2020rdb.dts
index 4d52bce..b6f50e1 100644
--- a/arch/powerpc/boot/dts/p2020rdb.dts
+++ b/arch/powerpc/boot/dts/p2020rdb.dts
@@ -215,14 +215,14 @@
 			};
 		};
 
-		mdio@25520 {
+		mdio@25000 {
 			tbi0: tbi-phy@11 {
 				reg = <0x11>;
 				device_type = "tbi-phy";
 			};
 		};
 
-		mdio@26520 {
+		mdio@26000 {
 			status = "disabled";
 		};
 
diff --git a/arch/powerpc/boot/dts/ppa8548.dts b/arch/powerpc/boot/dts/ppa8548.dts
index f97ecee..2117927 100644
--- a/arch/powerpc/boot/dts/ppa8548.dts
+++ b/arch/powerpc/boot/dts/ppa8548.dts
@@ -128,7 +128,7 @@
 		phy-handle = <&phy0>;
 	};
 
-	mdio@25520 {
+	mdio@25000 {
 		tbi1: tbi-phy@11 {
 			reg = <0x11>;
 			device_type = "tbi-phy";
@@ -140,7 +140,7 @@
 		phy-handle = <&phy1>;
 	};
 
-	mdio@26520 {
+	mdio@26000 {
 		tbi2: tbi-phy@11 {
 			reg = <0x11>;
 			device_type = "tbi-phy";
@@ -151,7 +151,7 @@
 		status = "disabled";
 	};
 
-	mdio@27520 {
+	mdio@27000 {
 		tbi3: tbi-phy@11 {
 			reg = <0x11>;
 			device_type = "tbi-phy";
diff --git a/drivers/net/ethernet/freescale/fsl_pq_mdio.c b/drivers/net/ethernet/freescale/fsl_pq_mdio.c
index c93a056..4228c4e 100644
--- a/drivers/net/ethernet/freescale/fsl_pq_mdio.c
+++ b/drivers/net/ethernet/freescale/fsl_pq_mdio.c
@@ -288,7 +288,7 @@ static struct of_device_id fsl_pq_mdio_match[] = {
 	{
 		.compatible = "fsl,gianfar-tbi",
 		.data = &(struct fsl_pq_mdio_data) {
-			.mii_offset = 0,
+			.mii_offset = 0x520,
 			.get_tbipa = get_gfar_tbipa,
 		},
 	},
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH] MDIO: FSL_PQ_MDIO: Fix bug on incorrect offset of tbipa register
  2013-06-12 12:47 ` Oded Gabbay
@ 2013-06-12 13:04   ` Timur Tabi
  -1 siblings, 0 replies; 24+ messages in thread
From: Timur Tabi @ 2013-06-12 13:04 UTC (permalink / raw)
  To: Oded Gabbay, benh, paulus, leoli, galak, B38951, Dongsheng.Wang,
	bigeasy, stef.van.os, davem
  Cc: linuxppc-dev, linux-kernel, netdev

Oded Gabbay wrote:
> Note: This patch may break MDIO functionallity of some old Freescale's SoC
> until Freescale will fix their device tree files. Basically, every device tree
> which contains an mdio device that is compatible to "fsl,gianfar-tbi" should be
> examined.

I haven't had a chance to review the patch in detail, but I can tell you 
that breaking compatibility with older device trees is unacceptable.  You 
need to add some code, even if it's an ugly hack, to support those trees.

-- 
Timur Tabi

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] MDIO: FSL_PQ_MDIO: Fix bug on incorrect offset of tbipa register
@ 2013-06-12 13:04   ` Timur Tabi
  0 siblings, 0 replies; 24+ messages in thread
From: Timur Tabi @ 2013-06-12 13:04 UTC (permalink / raw)
  To: Oded Gabbay, benh, paulus, leoli, galak, B38951, Dongsheng.Wang,
	bigeasy, stef.van.os, davem
  Cc: netdev, linuxppc-dev, linux-kernel

Oded Gabbay wrote:
> Note: This patch may break MDIO functionallity of some old Freescale's SoC
> until Freescale will fix their device tree files. Basically, every device tree
> which contains an mdio device that is compatible to "fsl,gianfar-tbi" should be
> examined.

I haven't had a chance to review the patch in detail, but I can tell you 
that breaking compatibility with older device trees is unacceptable.  You 
need to add some code, even if it's an ugly hack, to support those trees.

-- 
Timur Tabi

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] MDIO: FSL_PQ_MDIO: Fix bug on incorrect offset of tbipa register
  2013-06-12 13:04   ` Timur Tabi
@ 2013-06-12 13:09     ` Oded Gabbay
  -1 siblings, 0 replies; 24+ messages in thread
From: Oded Gabbay @ 2013-06-12 13:09 UTC (permalink / raw)
  To: Timur Tabi
  Cc: benh, paulus, leoli, galak, B38951, Dongsheng.Wang, bigeasy,
	stef.van.os, davem, linuxppc-dev, linux-kernel, netdev

Oded Gabbay wrote:
>> Note: This patch may break MDIO functionallity of some old 
>> Freescale's SoC
>> until Freescale will fix their device tree files. Basically, every 
>> device tree
>> which contains an mdio device that is compatible to "fsl,gianfar-tbi" 
>> should be
>> examined.
>
> On 06/12/2013 04:04 PM, Timur Tabi wrote:
> I haven't had a chance to review the patch in detail, but I can tell 
> you that breaking compatibility with older device trees is 
> unacceptable.  You need to add some code, even if it's an ugly hack, 
> to support those trees.
>
I generally agree with this statement except that without this patch, 
almost ALL of Freescale's SoC that uses "fsl,gianfar-tbi" are broken, 
including the older ones. At least this patch fixes some of the device 
trees. Because I'm not working at Freescale, I have a very limited 
access to a few SoC which I could test this patch on. I think it is 
Freescale's responsibility to release a complementary patch to fix the 
rest of the SoC device trees.

Oded

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] MDIO: FSL_PQ_MDIO: Fix bug on incorrect offset of tbipa register
@ 2013-06-12 13:09     ` Oded Gabbay
  0 siblings, 0 replies; 24+ messages in thread
From: Oded Gabbay @ 2013-06-12 13:09 UTC (permalink / raw)
  To: Timur Tabi
  Cc: stef.van.os, bigeasy, Dongsheng.Wang, paulus, netdev,
	linuxppc-dev, davem, linux-kernel, B38951

Oded Gabbay wrote:
>> Note: This patch may break MDIO functionallity of some old 
>> Freescale's SoC
>> until Freescale will fix their device tree files. Basically, every 
>> device tree
>> which contains an mdio device that is compatible to "fsl,gianfar-tbi" 
>> should be
>> examined.
>
> On 06/12/2013 04:04 PM, Timur Tabi wrote:
> I haven't had a chance to review the patch in detail, but I can tell 
> you that breaking compatibility with older device trees is 
> unacceptable.  You need to add some code, even if it's an ugly hack, 
> to support those trees.
>
I generally agree with this statement except that without this patch, 
almost ALL of Freescale's SoC that uses "fsl,gianfar-tbi" are broken, 
including the older ones. At least this patch fixes some of the device 
trees. Because I'm not working at Freescale, I have a very limited 
access to a few SoC which I could test this patch on. I think it is 
Freescale's responsibility to release a complementary patch to fix the 
rest of the SoC device trees.

Oded

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] MDIO: FSL_PQ_MDIO: Fix bug on incorrect offset of tbipa register
  2013-06-12 12:47 ` Oded Gabbay
@ 2013-06-12 15:08   ` Sebastian Andrzej Siewior
  -1 siblings, 0 replies; 24+ messages in thread
From: Sebastian Andrzej Siewior @ 2013-06-12 15:08 UTC (permalink / raw)
  To: Oded Gabbay
  Cc: benh, paulus, leoli, galak, B38951, Dongsheng.Wang, stef.van.os,
	timur, davem, linuxppc-dev, linux-kernel, netdev

On 06/12/2013 02:47 PM, Oded Gabbay wrote:
> This patch fixes a bug in the fsl_pq_mdio.c module and in relevant device-tree
> files regarding the correct offset of the tbipa register in the eTSEC
> controller in some of Freescale's PQ3 and QorIQ SoC.
> The bug happens when the mdio in the device tree is configured to be compatible
> to "fsl,gianfar-tbi". Because the mdio device in the device tree points to
> addresses 25520, 26520 or 27520 (depends on the controller ID), the variable
> priv->map at function fsl_pq_mdio_probe, points to that address. However,
> later in the function there is a write to register tbipa that is actually
> located at 25030, 26030 or 27030. Because the correct address is not io mapped,
> the contents are written to a different register in the controller.
> The fix sets the address of the mdio device to start at 25000, 26000 or 27000
> and changes the mii_offset field to 0x520 in the relevant entry
> (fsl,gianfar-tbi) of the fsl_pq_mdio_match array.
> 
> Note: This patch may break MDIO functionallity of some old Freescale's SoC
> until Freescale will fix their device tree files. Basically, every device tree
> which contains an mdio device that is compatible to "fsl,gianfar-tbi" should be
> examined.

Not as is.
Please add a check for the original address. If it has 0x520 at the end
print a warning and fix it up. Please add to the patch description
which register is modified instead if this patch is not applied.
Depending on how critical this it might has to go stable.

Sebastian

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] MDIO: FSL_PQ_MDIO: Fix bug on incorrect offset of tbipa register
@ 2013-06-12 15:08   ` Sebastian Andrzej Siewior
  0 siblings, 0 replies; 24+ messages in thread
From: Sebastian Andrzej Siewior @ 2013-06-12 15:08 UTC (permalink / raw)
  To: Oded Gabbay
  Cc: stef.van.os, timur, Dongsheng.Wang, paulus, netdev, linuxppc-dev,
	davem, linux-kernel, B38951

On 06/12/2013 02:47 PM, Oded Gabbay wrote:
> This patch fixes a bug in the fsl_pq_mdio.c module and in relevant device-tree
> files regarding the correct offset of the tbipa register in the eTSEC
> controller in some of Freescale's PQ3 and QorIQ SoC.
> The bug happens when the mdio in the device tree is configured to be compatible
> to "fsl,gianfar-tbi". Because the mdio device in the device tree points to
> addresses 25520, 26520 or 27520 (depends on the controller ID), the variable
> priv->map at function fsl_pq_mdio_probe, points to that address. However,
> later in the function there is a write to register tbipa that is actually
> located at 25030, 26030 or 27030. Because the correct address is not io mapped,
> the contents are written to a different register in the controller.
> The fix sets the address of the mdio device to start at 25000, 26000 or 27000
> and changes the mii_offset field to 0x520 in the relevant entry
> (fsl,gianfar-tbi) of the fsl_pq_mdio_match array.
> 
> Note: This patch may break MDIO functionallity of some old Freescale's SoC
> until Freescale will fix their device tree files. Basically, every device tree
> which contains an mdio device that is compatible to "fsl,gianfar-tbi" should be
> examined.

Not as is.
Please add a check for the original address. If it has 0x520 at the end
print a warning and fix it up. Please add to the patch description
which register is modified instead if this patch is not applied.
Depending on how critical this it might has to go stable.

Sebastian

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] MDIO: FSL_PQ_MDIO: Fix bug on incorrect offset of tbipa register
  2013-06-12 15:08   ` Sebastian Andrzej Siewior
@ 2013-06-12 18:31     ` Scott Wood
  -1 siblings, 0 replies; 24+ messages in thread
From: Scott Wood @ 2013-06-12 18:31 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: Oded Gabbay, stef.van.os, timur, Dongsheng.Wang, paulus, netdev,
	linuxppc-dev, davem, linux-kernel, B38951

On 06/12/2013 10:08:29 AM, Sebastian Andrzej Siewior wrote:
> On 06/12/2013 02:47 PM, Oded Gabbay wrote:
> > This patch fixes a bug in the fsl_pq_mdio.c module and in relevant  
> device-tree
> > files regarding the correct offset of the tbipa register in the  
> eTSEC
> > controller in some of Freescale's PQ3 and QorIQ SoC.
> > The bug happens when the mdio in the device tree is configured to  
> be compatible
> > to "fsl,gianfar-tbi". Because the mdio device in the device tree  
> points to
> > addresses 25520, 26520 or 27520 (depends on the controller ID), the  
> variable
> > priv->map at function fsl_pq_mdio_probe, points to that address.  
> However,
> > later in the function there is a write to register tbipa that is  
> actually
> > located at 25030, 26030 or 27030. Because the correct address is  
> not io mapped,
> > the contents are written to a different register in the controller.
> > The fix sets the address of the mdio device to start at 25000,  
> 26000 or 27000
> > and changes the mii_offset field to 0x520 in the relevant entry
> > (fsl,gianfar-tbi) of the fsl_pq_mdio_match array.
> >
> > Note: This patch may break MDIO functionallity of some old  
> Freescale's SoC
> > until Freescale will fix their device tree files. Basically, every  
> device tree
> > which contains an mdio device that is compatible to  
> "fsl,gianfar-tbi" should be
> > examined.
> 
> Not as is.
> Please add a check for the original address. If it has 0x520 at the  
> end
> print a warning and fix it up. Please add to the patch description
> which register is modified instead if this patch is not applied.
> Depending on how critical this it might has to go stable.

I'm not sure it's stable material if this is something that has never  
worked...

The device tree binding will also need to be fixed to note the  
difference in "reg" between "fsl,gianfar-mdio" and "fsl-gianfar-tbi" --  
and should give an example of the latter.

-Scott

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] MDIO: FSL_PQ_MDIO: Fix bug on incorrect offset of tbipa register
@ 2013-06-12 18:31     ` Scott Wood
  0 siblings, 0 replies; 24+ messages in thread
From: Scott Wood @ 2013-06-12 18:31 UTC (permalink / raw)
  To: Sebastian Andrzej Siewior
  Cc: stef.van.os, Oded Gabbay, netdev, Dongsheng.Wang, timur, paulus,
	linuxppc-dev, davem, linux-kernel, B38951

On 06/12/2013 10:08:29 AM, Sebastian Andrzej Siewior wrote:
> On 06/12/2013 02:47 PM, Oded Gabbay wrote:
> > This patch fixes a bug in the fsl_pq_mdio.c module and in relevant =20
> device-tree
> > files regarding the correct offset of the tbipa register in the =20
> eTSEC
> > controller in some of Freescale's PQ3 and QorIQ SoC.
> > The bug happens when the mdio in the device tree is configured to =20
> be compatible
> > to "fsl,gianfar-tbi". Because the mdio device in the device tree =20
> points to
> > addresses 25520, 26520 or 27520 (depends on the controller ID), the =20
> variable
> > priv->map at function fsl_pq_mdio_probe, points to that address. =20
> However,
> > later in the function there is a write to register tbipa that is =20
> actually
> > located at 25030, 26030 or 27030. Because the correct address is =20
> not io mapped,
> > the contents are written to a different register in the controller.
> > The fix sets the address of the mdio device to start at 25000, =20
> 26000 or 27000
> > and changes the mii_offset field to 0x520 in the relevant entry
> > (fsl,gianfar-tbi) of the fsl_pq_mdio_match array.
> >
> > Note: This patch may break MDIO functionallity of some old =20
> Freescale's SoC
> > until Freescale will fix their device tree files. Basically, every =20
> device tree
> > which contains an mdio device that is compatible to =20
> "fsl,gianfar-tbi" should be
> > examined.
>=20
> Not as is.
> Please add a check for the original address. If it has 0x520 at the =20
> end
> print a warning and fix it up. Please add to the patch description
> which register is modified instead if this patch is not applied.
> Depending on how critical this it might has to go stable.

I'm not sure it's stable material if this is something that has never =20
worked...

The device tree binding will also need to be fixed to note the =20
difference in "reg" between "fsl,gianfar-mdio" and "fsl-gianfar-tbi" -- =20
and should give an example of the latter.

-Scott=

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] MDIO: FSL_PQ_MDIO: Fix bug on incorrect offset of tbipa register
  2013-06-12 18:31     ` Scott Wood
@ 2013-06-13  6:44       ` Oded Gabbay
  -1 siblings, 0 replies; 24+ messages in thread
From: Oded Gabbay @ 2013-06-13  6:44 UTC (permalink / raw)
  To: Scott Wood, Sebastian Andrzej Siewior
  Cc: stef.van.os, timur, Dongsheng.Wang, paulus, netdev, linuxppc-dev,
	davem, linux-kernel, B38951

On 06/12/2013 09:31 PM, Scott Wood wrote:
> On 06/12/2013 10:08:29 AM, Sebastian Andrzej Siewior wrote:
>> On 06/12/2013 02:47 PM, Oded Gabbay wrote:
>> > This patch fixes a bug in the fsl_pq_mdio.c module and in relevant 
>> device-tree
>> > files regarding the correct offset of the tbipa register in the eTSEC
>> > controller in some of Freescale's PQ3 and QorIQ SoC.
>> > The bug happens when the mdio in the device tree is configured to 
>> be compatible
>> > to "fsl,gianfar-tbi". Because the mdio device in the device tree 
>> points to
>> > addresses 25520, 26520 or 27520 (depends on the controller ID), the 
>> variable
>> > priv->map at function fsl_pq_mdio_probe, points to that address. 
>> However,
>> > later in the function there is a write to register tbipa that is 
>> actually
>> > located at 25030, 26030 or 27030. Because the correct address is 
>> not io mapped,
>> > the contents are written to a different register in the controller.
>> > The fix sets the address of the mdio device to start at 25000, 
>> 26000 or 27000
>> > and changes the mii_offset field to 0x520 in the relevant entry
>> > (fsl,gianfar-tbi) of the fsl_pq_mdio_match array.
>> >
>> > Note: This patch may break MDIO functionallity of some old 
>> Freescale's SoC
>> > until Freescale will fix their device tree files. Basically, every 
>> device tree
>> > which contains an mdio device that is compatible to 
>> "fsl,gianfar-tbi" should be
>> > examined.
>>
>> Not as is.
>> Please add a check for the original address. If it has 0x520 at the end
>> print a warning and fix it up. Please add to the patch description
>> which register is modified instead if this patch is not applied.
>> Depending on how critical this it might has to go stable.
>
> I'm not sure it's stable material if this is something that has never 
> worked...
>
> The device tree binding will also need to be fixed to note the 
> difference in "reg" between "fsl,gianfar-mdio" and "fsl-gianfar-tbi" 
> -- and should give an example of the latter.
>
> -Scott
I read the 2 comments and I'm not sure what should be the best way to 
move ahead.
I would like to describe what is the impact of not accepting this patch:
When you connect any eTSEC, except the first one, using SGMII, you must 
configure the TBIPA register because
the MII management configuration uses the TBIPA address as part of the 
SGMII initialization sequence,
as described in the P2020 Reference manual.
So, if that register is not initialized, the sequence is broken the and 
eTSEC is not functioning (can not send/receive
packets).
I still think the best way to fix it is what I did:
1. Point the priv->map to the start of the whole registers range of the 
eTSEC
2. Set mii_offset to 0x520 in the "gianfar-tbi" entry of the 
"fsl_pq_mdio_match" array.
3. Fix all the usages of the "gianfar-tbi" in the device tree files - 
change the starting address and reg range

I think this is the best way because it is stated in "fsl_pq_mdio_probe" 
function that:
     /*
      * Some device tree nodes represent only the MII registers, and
      * others represent the MAC and MII registers.  The 'mii_offset' field
      * contains the offset of the MII registers inside the mapped register
      * space.
      */
and that's why we have priv->map and priv->regs. So my fix goes 
according to the current design of the driver.

-Oded

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] MDIO: FSL_PQ_MDIO: Fix bug on incorrect offset of tbipa register
@ 2013-06-13  6:44       ` Oded Gabbay
  0 siblings, 0 replies; 24+ messages in thread
From: Oded Gabbay @ 2013-06-13  6:44 UTC (permalink / raw)
  To: Scott Wood, Sebastian Andrzej Siewior
  Cc: stef.van.os, netdev, timur, Dongsheng.Wang, paulus, linuxppc-dev,
	davem, linux-kernel, B38951

On 06/12/2013 09:31 PM, Scott Wood wrote:
> On 06/12/2013 10:08:29 AM, Sebastian Andrzej Siewior wrote:
>> On 06/12/2013 02:47 PM, Oded Gabbay wrote:
>> > This patch fixes a bug in the fsl_pq_mdio.c module and in relevant 
>> device-tree
>> > files regarding the correct offset of the tbipa register in the eTSEC
>> > controller in some of Freescale's PQ3 and QorIQ SoC.
>> > The bug happens when the mdio in the device tree is configured to 
>> be compatible
>> > to "fsl,gianfar-tbi". Because the mdio device in the device tree 
>> points to
>> > addresses 25520, 26520 or 27520 (depends on the controller ID), the 
>> variable
>> > priv->map at function fsl_pq_mdio_probe, points to that address. 
>> However,
>> > later in the function there is a write to register tbipa that is 
>> actually
>> > located at 25030, 26030 or 27030. Because the correct address is 
>> not io mapped,
>> > the contents are written to a different register in the controller.
>> > The fix sets the address of the mdio device to start at 25000, 
>> 26000 or 27000
>> > and changes the mii_offset field to 0x520 in the relevant entry
>> > (fsl,gianfar-tbi) of the fsl_pq_mdio_match array.
>> >
>> > Note: This patch may break MDIO functionallity of some old 
>> Freescale's SoC
>> > until Freescale will fix their device tree files. Basically, every 
>> device tree
>> > which contains an mdio device that is compatible to 
>> "fsl,gianfar-tbi" should be
>> > examined.
>>
>> Not as is.
>> Please add a check for the original address. If it has 0x520 at the end
>> print a warning and fix it up. Please add to the patch description
>> which register is modified instead if this patch is not applied.
>> Depending on how critical this it might has to go stable.
>
> I'm not sure it's stable material if this is something that has never 
> worked...
>
> The device tree binding will also need to be fixed to note the 
> difference in "reg" between "fsl,gianfar-mdio" and "fsl-gianfar-tbi" 
> -- and should give an example of the latter.
>
> -Scott
I read the 2 comments and I'm not sure what should be the best way to 
move ahead.
I would like to describe what is the impact of not accepting this patch:
When you connect any eTSEC, except the first one, using SGMII, you must 
configure the TBIPA register because
the MII management configuration uses the TBIPA address as part of the 
SGMII initialization sequence,
as described in the P2020 Reference manual.
So, if that register is not initialized, the sequence is broken the and 
eTSEC is not functioning (can not send/receive
packets).
I still think the best way to fix it is what I did:
1. Point the priv->map to the start of the whole registers range of the 
eTSEC
2. Set mii_offset to 0x520 in the "gianfar-tbi" entry of the 
"fsl_pq_mdio_match" array.
3. Fix all the usages of the "gianfar-tbi" in the device tree files - 
change the starting address and reg range

I think this is the best way because it is stated in "fsl_pq_mdio_probe" 
function that:
     /*
      * Some device tree nodes represent only the MII registers, and
      * others represent the MAC and MII registers.  The 'mii_offset' field
      * contains the offset of the MII registers inside the mapped register
      * space.
      */
and that's why we have priv->map and priv->regs. So my fix goes 
according to the current design of the driver.

-Oded

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] MDIO: FSL_PQ_MDIO: Fix bug on incorrect offset of tbipa register
  2013-06-12 18:31     ` Scott Wood
@ 2013-06-15 22:57       ` Timur Tabi
  -1 siblings, 0 replies; 24+ messages in thread
From: Timur Tabi @ 2013-06-15 22:57 UTC (permalink / raw)
  To: Scott Wood
  Cc: Sebastian Andrzej Siewior, Oded Gabbay, Stef van Os, Timur Tabi,
	Dongsheng.Wang, Paul Mackerras, netdev, linuxppc-dev,
	David Miller, lkml, B38951

On Wed, Jun 12, 2013 at 1:31 PM, Scott Wood <scottwood@freescale.com> wrote:
>
> I'm not sure it's stable material if this is something that has never
> worked...
>
> The device tree binding will also need to be fixed to note the difference in
> "reg" between "fsl,gianfar-mdio" and "fsl-gianfar-tbi" -- and should give an
> example of the latter.

I don't remember how much I tested it, but I'm pretty sure that at
some of the suspect devices did work for me.  My goal was to maintain
compatibility with existing device trees, and just refactor the code
so that it's easier to read.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH] MDIO: FSL_PQ_MDIO: Fix bug on incorrect offset of tbipa register
@ 2013-06-15 22:57       ` Timur Tabi
  0 siblings, 0 replies; 24+ messages in thread
From: Timur Tabi @ 2013-06-15 22:57 UTC (permalink / raw)
  To: Scott Wood
  Cc: Stef van Os, Oded Gabbay, netdev, Sebastian Andrzej Siewior,
	Dongsheng.Wang, Timur Tabi, Paul Mackerras, linuxppc-dev,
	David Miller, lkml, B38951

On Wed, Jun 12, 2013 at 1:31 PM, Scott Wood <scottwood@freescale.com> wrote:
>
> I'm not sure it's stable material if this is something that has never
> worked...
>
> The device tree binding will also need to be fixed to note the difference in
> "reg" between "fsl,gianfar-mdio" and "fsl-gianfar-tbi" -- and should give an
> example of the latter.

I don't remember how much I tested it, but I'm pretty sure that at
some of the suspect devices did work for me.  My goal was to maintain
compatibility with existing device trees, and just refactor the code
so that it's easier to read.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH 1/2] net/fsl_pq_mdio: check TBI address for consistency with mapped range
  2013-06-12 12:47 ` Oded Gabbay
                   ` (2 preceding siblings ...)
  (?)
@ 2015-10-09 15:15 ` Gerlando Falauto
  2015-10-09 15:15   ` [PATCH 2/2] net/fsl_pq_mdio: fix computed address for the TBI register Gerlando Falauto
                     ` (2 more replies)
  -1 siblings, 3 replies; 24+ messages in thread
From: Gerlando Falauto @ 2015-10-09 15:15 UTC (permalink / raw)
  To: netdev, timur, linuxppc-dev, linux-kernel
  Cc: ogabbay, bigeasy, Gerlando Falauto, Timur Tabi, David S. Miller,
	Andy Fleming, Kumar Gala

When configuring the MDIO subsystem it is also necessary to configure
the TBI register. Make sure the TBI is contained within the mapped
register range in order to:
a) make sure the address is computed correctly
b) make users aware that we're actually accessing that register

In case of error, print a message but continue anyway.

Change-Id: If1e7d8931f440ea9259726c36d3df797dda016fb
Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com>
Cc: Timur Tabi <timur@freescale.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Andy Fleming <afleming@freescale.com>
Cc: Kumar Gala <galak@kernel.crashing.org>
---
 drivers/net/ethernet/freescale/fsl_pq_mdio.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/ethernet/freescale/fsl_pq_mdio.c b/drivers/net/ethernet/freescale/fsl_pq_mdio.c
index 3c40f6b..4618011 100644
--- a/drivers/net/ethernet/freescale/fsl_pq_mdio.c
+++ b/drivers/net/ethernet/freescale/fsl_pq_mdio.c
@@ -445,6 +445,16 @@ static int fsl_pq_mdio_probe(struct platform_device *pdev)
 
 			tbipa = data->get_tbipa(priv->map);
 
+			/*
+			 * Add consistency check to make sure TBI is contained
+			 * within the mapped range (not because we would get a
+			 * segfault, rather to catch bugs in computing TBI
+			 * address). Print error message but continue anyway.
+			 */
+			if (tbipa > priv->map + resource_size(&res))
+				dev_err(&pdev->dev, "invalid register map (should be at least 0x%04x to contain TBI address)\n",
+					((void *)tbipa - priv->map) + 4);
+
 			iowrite32be(be32_to_cpup(prop), tbipa);
 		}
 	}
-- 
1.8.0.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH 2/2] net/fsl_pq_mdio: fix computed address for the TBI register
  2015-10-09 15:15 ` [PATCH 1/2] net/fsl_pq_mdio: check TBI address for consistency with mapped range Gerlando Falauto
@ 2015-10-09 15:15   ` Gerlando Falauto
  2015-10-09 17:39   ` [PATCH 1/2] net/fsl_pq_mdio: check TBI address for consistency with mapped range kbuild test robot
  2015-10-11 12:41   ` Timur Tabi
  2 siblings, 0 replies; 24+ messages in thread
From: Gerlando Falauto @ 2015-10-09 15:15 UTC (permalink / raw)
  To: netdev, timur, linuxppc-dev, linux-kernel
  Cc: ogabbay, bigeasy, Gerlando Falauto, Timur Tabi, David S. Miller,
	Andy Fleming, Kumar Gala

commit afae5ad78b342f401c28b0bb1adb3cd494cb125a
  "net/fsl_pq_mdio: streamline probing of MDIO nodes"

added support for different types of MDIO devices:
1) Gianfar MDIO nodes that only map the MII registers
2) Gianfar MDIO nodes that map the full MDIO register set
3) eTSEC2 MDIO nodes (which map the full MDIO register set)
4) QE MDIO nodes (which map only the MII registers)

However, the implementation for types 1 and 4 would mistakenly assume
a mapping of the full MDIO register set, thereby computing the address
for the TBI register starting from the containing structure.
The TBI register would therefore be accessed at a wrong (much bigger)
address, not giving the expected result at all.
This patch restores the correct behavior we had prior to the above one.

The consequences of this bug are apparent when trying to access a PHY
with the same address as the value contained in the initial value of
the TBI register (normally 0); in that case you'll get answers from the
internal TBI device (even though MDIO/MDC pins are actually *also*
toggling on the physical bus!).
Beware that you also need to add a fake tbi node to your device tree
with an unused address.

Notice how this fix is related to commit
220669495bf8b68130a8218607147c7b74c28d2b
  "powerpc: Add TBI PHY node to first MDIO bus"

which fixed the behavior in kernel 3.3, which was later broken by the
above commit on kernel 3.7.

Change-Id: If78651268435aaed1f07ebdef374c46c0a528429
Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com>
Cc: Timur Tabi <timur@freescale.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Andy Fleming <afleming@freescale.com>
Cc: Kumar Gala <galak@kernel.crashing.org>
---
 drivers/net/ethernet/freescale/fsl_pq_mdio.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fsl_pq_mdio.c b/drivers/net/ethernet/freescale/fsl_pq_mdio.c
index 4618011..9e12e02 100644
--- a/drivers/net/ethernet/freescale/fsl_pq_mdio.c
+++ b/drivers/net/ethernet/freescale/fsl_pq_mdio.c
@@ -198,11 +198,13 @@ static int fsl_pq_mdio_reset(struct mii_bus *bus)
 
 #if defined(CONFIG_GIANFAR) || defined(CONFIG_GIANFAR_MODULE)
 /*
+ * Return the TBIPA address, starting from the address
+ * of the mapped GFAR MDIO registers (struct gfar)
  * This is mildly evil, but so is our hardware for doing this.
  * Also, we have to cast back to struct gfar because of
  * definition weirdness done in gianfar.h.
  */
-static uint32_t __iomem *get_gfar_tbipa(void __iomem *p)
+static uint32_t __iomem *get_gfar_tbipa_from_mdio(void __iomem *p)
 {
 	struct gfar __iomem *enet_regs = p;
 
@@ -210,6 +212,15 @@ static uint32_t __iomem *get_gfar_tbipa(void __iomem *p)
 }
 
 /*
+ * Return the TBIPA address, starting from the address
+ * of the mapped GFAR MII registers (gfar_mii_regs[] within struct gfar)
+ */
+static uint32_t __iomem *get_gfar_tbipa_from_mii(void __iomem *p)
+{
+	return get_gfar_tbipa_from_mdio(container_of(p, struct gfar, gfar_mii_regs));
+}
+
+/*
  * Return the TBIPAR address for an eTSEC2 node
  */
 static uint32_t __iomem *get_etsec_tbipa(void __iomem *p)
@@ -220,11 +231,12 @@ static uint32_t __iomem *get_etsec_tbipa(void __iomem *p)
 
 #if defined(CONFIG_UCC_GETH) || defined(CONFIG_UCC_GETH_MODULE)
 /*
- * Return the TBIPAR address for a QE MDIO node
+ * Return the TBIPAR address for a QE MDIO node, starting from the address
+ * of the mapped MII registers (struct fsl_pq_mii)
  */
 static uint32_t __iomem *get_ucc_tbipa(void __iomem *p)
 {
-	struct fsl_pq_mdio __iomem *mdio = p;
+	struct fsl_pq_mdio __iomem *mdio = container_of(p, struct fsl_pq_mdio, mii);
 
 	return &mdio->utbipar;
 }
@@ -300,14 +312,14 @@ static const struct of_device_id fsl_pq_mdio_match[] = {
 		.compatible = "fsl,gianfar-tbi",
 		.data = &(struct fsl_pq_mdio_data) {
 			.mii_offset = 0,
-			.get_tbipa = get_gfar_tbipa,
+			.get_tbipa = get_gfar_tbipa_from_mii,
 		},
 	},
 	{
 		.compatible = "fsl,gianfar-mdio",
 		.data = &(struct fsl_pq_mdio_data) {
 			.mii_offset = 0,
-			.get_tbipa = get_gfar_tbipa,
+			.get_tbipa = get_gfar_tbipa_from_mii,
 		},
 	},
 	{
@@ -315,7 +327,7 @@ static const struct of_device_id fsl_pq_mdio_match[] = {
 		.compatible = "gianfar",
 		.data = &(struct fsl_pq_mdio_data) {
 			.mii_offset = offsetof(struct fsl_pq_mdio, mii),
-			.get_tbipa = get_gfar_tbipa,
+			.get_tbipa = get_gfar_tbipa_from_mdio,
 		},
 	},
 	{
-- 
1.8.0.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] net/fsl_pq_mdio: check TBI address for consistency with mapped range
  2015-10-09 15:15 ` [PATCH 1/2] net/fsl_pq_mdio: check TBI address for consistency with mapped range Gerlando Falauto
  2015-10-09 15:15   ` [PATCH 2/2] net/fsl_pq_mdio: fix computed address for the TBI register Gerlando Falauto
@ 2015-10-09 17:39   ` kbuild test robot
  2015-10-11 12:41   ` Timur Tabi
  2 siblings, 0 replies; 24+ messages in thread
From: kbuild test robot @ 2015-10-09 17:39 UTC (permalink / raw)
  To: Gerlando Falauto
  Cc: kbuild-all, netdev, timur, linuxppc-dev, linux-kernel, ogabbay,
	bigeasy, Gerlando Falauto, Timur Tabi, David S. Miller,
	Andy Fleming, Kumar Gala

[-- Attachment #1: Type: text/plain, Size: 1802 bytes --]

Hi Gerlando,

[auto build test WARNING on net/master -- if it's inappropriate base, please ignore]

config: powerpc-tqm8541_defconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=powerpc 

All warnings (new ones prefixed by >>):

   drivers/net/ethernet/freescale/fsl_pq_mdio.c: In function 'fsl_pq_mdio_probe':
>> drivers/net/ethernet/freescale/fsl_pq_mdio.c:454:14: warning: comparison of distinct pointer types lacks a cast
       if (tbipa > priv->map + resource_size(&res))
                 ^

vim +454 drivers/net/ethernet/freescale/fsl_pq_mdio.c

   438				if (!prop) {
   439					dev_err(&pdev->dev,
   440						"missing 'reg' property in node %s\n",
   441						tbi->full_name);
   442					err = -EBUSY;
   443					goto error;
   444				}
   445	
   446				tbipa = data->get_tbipa(priv->map);
   447	
   448				/*
   449				 * Add consistency check to make sure TBI is contained
   450				 * within the mapped range (not because we would get a
   451				 * segfault, rather to catch bugs in computing TBI
   452				 * address). Print error message but continue anyway.
   453				 */
 > 454				if (tbipa > priv->map + resource_size(&res))
   455					dev_err(&pdev->dev, "invalid register map (should be at least 0x%04x to contain TBI address)\n",
   456						((void *)tbipa - priv->map) + 4);
   457	
   458				iowrite32be(be32_to_cpup(prop), tbipa);
   459			}
   460		}
   461	
   462		if (data->ucc_configure)

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 13495 bytes --]

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH 1/2] net/fsl_pq_mdio: check TBI address for consistency with mapped range
  2015-10-09 15:15 ` [PATCH 1/2] net/fsl_pq_mdio: check TBI address for consistency with mapped range Gerlando Falauto
  2015-10-09 15:15   ` [PATCH 2/2] net/fsl_pq_mdio: fix computed address for the TBI register Gerlando Falauto
  2015-10-09 17:39   ` [PATCH 1/2] net/fsl_pq_mdio: check TBI address for consistency with mapped range kbuild test robot
@ 2015-10-11 12:41   ` Timur Tabi
  2 siblings, 0 replies; 24+ messages in thread
From: Timur Tabi @ 2015-10-11 12:41 UTC (permalink / raw)
  To: Gerlando Falauto, netdev, linuxppc-dev, linux-kernel
  Cc: ogabbay, bigeasy, David S. Miller, Andy Fleming, Kumar Gala

Gerlando Falauto wrote:
>
> Change-Id: If1e7d8931f440ea9259726c36d3df797dda016fb

You need to remove these from patches that are emailed, and fix the 
pointer type comparison.

Otherwise,

Acked-by: Timur Tabi <timur@tabi.org>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v2 1/2] net/fsl_pq_mdio: check TBI address for consistency with mapped range
  2013-06-12 12:47 ` Oded Gabbay
@ 2015-10-12  7:18   ` Gerlando Falauto
  -1 siblings, 0 replies; 24+ messages in thread
From: Gerlando Falauto @ 2015-10-12  7:18 UTC (permalink / raw)
  To: netdev, timur, linuxppc-dev, linux-kernel
  Cc: ogabbay, bigeasy, Gerlando Falauto, David S. Miller, Kumar Gala

When configuring the MDIO subsystem it is also necessary to configure
the TBI register. Make sure the TBI is contained within the mapped
register range in order to:
a) make sure the address is computed correctly
b) make users aware that we're actually accessing that register

In case of error, print a message but continue anyway.

Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com>
Cc: Timur Tabi <timur@tabi.org>
Cc: David S. Miller <davem@davemloft.net>
Cc: Kumar Gala <galak@kernel.crashing.org>
---
Changes from v1:
- Added type cast & fixed range
- removed freescale recipients

 drivers/net/ethernet/freescale/fsl_pq_mdio.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/ethernet/freescale/fsl_pq_mdio.c b/drivers/net/ethernet/freescale/fsl_pq_mdio.c
index 3c40f6b..5333d0a 100644
--- a/drivers/net/ethernet/freescale/fsl_pq_mdio.c
+++ b/drivers/net/ethernet/freescale/fsl_pq_mdio.c
@@ -445,6 +445,16 @@ static int fsl_pq_mdio_probe(struct platform_device *pdev)
 
 			tbipa = data->get_tbipa(priv->map);
 
+			/*
+			 * Add consistency check to make sure TBI is contained
+			 * within the mapped range (not because we would get a
+			 * segfault, rather to catch bugs in computing TBI
+			 * address). Print error message but continue anyway.
+			 */
+			if ((void *)tbipa > priv->map + resource_size(&res) - 4)
+				dev_err(&pdev->dev, "invalid register map (should be at least 0x%04x to contain TBI address)\n",
+					((void *)tbipa - priv->map) + 4);
+
 			iowrite32be(be32_to_cpup(prop), tbipa);
 		}
 	}
-- 
1.8.0.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2 1/2] net/fsl_pq_mdio: check TBI address for consistency with mapped range
@ 2015-10-12  7:18   ` Gerlando Falauto
  0 siblings, 0 replies; 24+ messages in thread
From: Gerlando Falauto @ 2015-10-12  7:18 UTC (permalink / raw)
  To: netdev, timur, linuxppc-dev, linux-kernel
  Cc: bigeasy, David S. Miller, Gerlando Falauto, ogabbay

When configuring the MDIO subsystem it is also necessary to configure
the TBI register. Make sure the TBI is contained within the mapped
register range in order to:
a) make sure the address is computed correctly
b) make users aware that we're actually accessing that register

In case of error, print a message but continue anyway.

Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com>
Cc: Timur Tabi <timur@tabi.org>
Cc: David S. Miller <davem@davemloft.net>
Cc: Kumar Gala <galak@kernel.crashing.org>
---
Changes from v1:
- Added type cast & fixed range
- removed freescale recipients

 drivers/net/ethernet/freescale/fsl_pq_mdio.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/ethernet/freescale/fsl_pq_mdio.c b/drivers/net/ethernet/freescale/fsl_pq_mdio.c
index 3c40f6b..5333d0a 100644
--- a/drivers/net/ethernet/freescale/fsl_pq_mdio.c
+++ b/drivers/net/ethernet/freescale/fsl_pq_mdio.c
@@ -445,6 +445,16 @@ static int fsl_pq_mdio_probe(struct platform_device *pdev)
 
 			tbipa = data->get_tbipa(priv->map);
 
+			/*
+			 * Add consistency check to make sure TBI is contained
+			 * within the mapped range (not because we would get a
+			 * segfault, rather to catch bugs in computing TBI
+			 * address). Print error message but continue anyway.
+			 */
+			if ((void *)tbipa > priv->map + resource_size(&res) - 4)
+				dev_err(&pdev->dev, "invalid register map (should be at least 0x%04x to contain TBI address)\n",
+					((void *)tbipa - priv->map) + 4);
+
 			iowrite32be(be32_to_cpup(prop), tbipa);
 		}
 	}
-- 
1.8.0.1

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2 2/2] net/fsl_pq_mdio: fix computed address for the TBI register
  2015-10-12  7:18   ` Gerlando Falauto
@ 2015-10-12  7:18     ` Gerlando Falauto
  -1 siblings, 0 replies; 24+ messages in thread
From: Gerlando Falauto @ 2015-10-12  7:18 UTC (permalink / raw)
  To: netdev, timur, linuxppc-dev, linux-kernel
  Cc: ogabbay, bigeasy, Gerlando Falauto, David S. Miller, Kumar Gala

commit afae5ad78b342f401c28b0bb1adb3cd494cb125a
  "net/fsl_pq_mdio: streamline probing of MDIO nodes"

added support for different types of MDIO devices:
1) Gianfar MDIO nodes that only map the MII registers
2) Gianfar MDIO nodes that map the full MDIO register set
3) eTSEC2 MDIO nodes (which map the full MDIO register set)
4) QE MDIO nodes (which map only the MII registers)

However, the implementation for types 1 and 4 would mistakenly assume
a mapping of the full MDIO register set, thereby computing the address
for the TBI register starting from the containing structure.
The TBI register would therefore be accessed at a wrong (much bigger)
address, not giving the expected result at all.
This patch restores the correct behavior we had prior to the above one.

The consequences of this bug are apparent when trying to access a PHY
with the same address as the value contained in the initial value of
the TBI register (normally 0); in that case you'll get answers from the
internal TBI device (even though MDIO/MDC pins are actually *also*
toggling on the physical bus!).
Beware that you also need to add a fake tbi node to your device tree
with an unused address.

Notice how this fix is related to commit
220669495bf8b68130a8218607147c7b74c28d2b
  "powerpc: Add TBI PHY node to first MDIO bus"

which fixed the behavior in kernel 3.3, which was later broken by the
above commit on kernel 3.7.

Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com>
Cc: Timur Tabi <timur@tabi.org>
Cc: David S. Miller <davem@davemloft.net>
Cc: Kumar Gala <galak@kernel.crashing.org>
---
 drivers/net/ethernet/freescale/fsl_pq_mdio.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fsl_pq_mdio.c b/drivers/net/ethernet/freescale/fsl_pq_mdio.c
index 5333d0a..55c3623 100644
--- a/drivers/net/ethernet/freescale/fsl_pq_mdio.c
+++ b/drivers/net/ethernet/freescale/fsl_pq_mdio.c
@@ -198,11 +198,13 @@ static int fsl_pq_mdio_reset(struct mii_bus *bus)
 
 #if defined(CONFIG_GIANFAR) || defined(CONFIG_GIANFAR_MODULE)
 /*
+ * Return the TBIPA address, starting from the address
+ * of the mapped GFAR MDIO registers (struct gfar)
  * This is mildly evil, but so is our hardware for doing this.
  * Also, we have to cast back to struct gfar because of
  * definition weirdness done in gianfar.h.
  */
-static uint32_t __iomem *get_gfar_tbipa(void __iomem *p)
+static uint32_t __iomem *get_gfar_tbipa_from_mdio(void __iomem *p)
 {
 	struct gfar __iomem *enet_regs = p;
 
@@ -210,6 +212,15 @@ static uint32_t __iomem *get_gfar_tbipa(void __iomem *p)
 }
 
 /*
+ * Return the TBIPA address, starting from the address
+ * of the mapped GFAR MII registers (gfar_mii_regs[] within struct gfar)
+ */
+static uint32_t __iomem *get_gfar_tbipa_from_mii(void __iomem *p)
+{
+	return get_gfar_tbipa_from_mdio(container_of(p, struct gfar, gfar_mii_regs));
+}
+
+/*
  * Return the TBIPAR address for an eTSEC2 node
  */
 static uint32_t __iomem *get_etsec_tbipa(void __iomem *p)
@@ -220,11 +231,12 @@ static uint32_t __iomem *get_etsec_tbipa(void __iomem *p)
 
 #if defined(CONFIG_UCC_GETH) || defined(CONFIG_UCC_GETH_MODULE)
 /*
- * Return the TBIPAR address for a QE MDIO node
+ * Return the TBIPAR address for a QE MDIO node, starting from the address
+ * of the mapped MII registers (struct fsl_pq_mii)
  */
 static uint32_t __iomem *get_ucc_tbipa(void __iomem *p)
 {
-	struct fsl_pq_mdio __iomem *mdio = p;
+	struct fsl_pq_mdio __iomem *mdio = container_of(p, struct fsl_pq_mdio, mii);
 
 	return &mdio->utbipar;
 }
@@ -300,14 +312,14 @@ static const struct of_device_id fsl_pq_mdio_match[] = {
 		.compatible = "fsl,gianfar-tbi",
 		.data = &(struct fsl_pq_mdio_data) {
 			.mii_offset = 0,
-			.get_tbipa = get_gfar_tbipa,
+			.get_tbipa = get_gfar_tbipa_from_mii,
 		},
 	},
 	{
 		.compatible = "fsl,gianfar-mdio",
 		.data = &(struct fsl_pq_mdio_data) {
 			.mii_offset = 0,
-			.get_tbipa = get_gfar_tbipa,
+			.get_tbipa = get_gfar_tbipa_from_mii,
 		},
 	},
 	{
@@ -315,7 +327,7 @@ static const struct of_device_id fsl_pq_mdio_match[] = {
 		.compatible = "gianfar",
 		.data = &(struct fsl_pq_mdio_data) {
 			.mii_offset = offsetof(struct fsl_pq_mdio, mii),
-			.get_tbipa = get_gfar_tbipa,
+			.get_tbipa = get_gfar_tbipa_from_mdio,
 		},
 	},
 	{
-- 
1.8.0.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v2 2/2] net/fsl_pq_mdio: fix computed address for the TBI register
@ 2015-10-12  7:18     ` Gerlando Falauto
  0 siblings, 0 replies; 24+ messages in thread
From: Gerlando Falauto @ 2015-10-12  7:18 UTC (permalink / raw)
  To: netdev, timur, linuxppc-dev, linux-kernel
  Cc: bigeasy, David S. Miller, Gerlando Falauto, ogabbay

commit afae5ad78b342f401c28b0bb1adb3cd494cb125a
  "net/fsl_pq_mdio: streamline probing of MDIO nodes"

added support for different types of MDIO devices:
1) Gianfar MDIO nodes that only map the MII registers
2) Gianfar MDIO nodes that map the full MDIO register set
3) eTSEC2 MDIO nodes (which map the full MDIO register set)
4) QE MDIO nodes (which map only the MII registers)

However, the implementation for types 1 and 4 would mistakenly assume
a mapping of the full MDIO register set, thereby computing the address
for the TBI register starting from the containing structure.
The TBI register would therefore be accessed at a wrong (much bigger)
address, not giving the expected result at all.
This patch restores the correct behavior we had prior to the above one.

The consequences of this bug are apparent when trying to access a PHY
with the same address as the value contained in the initial value of
the TBI register (normally 0); in that case you'll get answers from the
internal TBI device (even though MDIO/MDC pins are actually *also*
toggling on the physical bus!).
Beware that you also need to add a fake tbi node to your device tree
with an unused address.

Notice how this fix is related to commit
220669495bf8b68130a8218607147c7b74c28d2b
  "powerpc: Add TBI PHY node to first MDIO bus"

which fixed the behavior in kernel 3.3, which was later broken by the
above commit on kernel 3.7.

Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com>
Cc: Timur Tabi <timur@tabi.org>
Cc: David S. Miller <davem@davemloft.net>
Cc: Kumar Gala <galak@kernel.crashing.org>
---
 drivers/net/ethernet/freescale/fsl_pq_mdio.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fsl_pq_mdio.c b/drivers/net/ethernet/freescale/fsl_pq_mdio.c
index 5333d0a..55c3623 100644
--- a/drivers/net/ethernet/freescale/fsl_pq_mdio.c
+++ b/drivers/net/ethernet/freescale/fsl_pq_mdio.c
@@ -198,11 +198,13 @@ static int fsl_pq_mdio_reset(struct mii_bus *bus)
 
 #if defined(CONFIG_GIANFAR) || defined(CONFIG_GIANFAR_MODULE)
 /*
+ * Return the TBIPA address, starting from the address
+ * of the mapped GFAR MDIO registers (struct gfar)
  * This is mildly evil, but so is our hardware for doing this.
  * Also, we have to cast back to struct gfar because of
  * definition weirdness done in gianfar.h.
  */
-static uint32_t __iomem *get_gfar_tbipa(void __iomem *p)
+static uint32_t __iomem *get_gfar_tbipa_from_mdio(void __iomem *p)
 {
 	struct gfar __iomem *enet_regs = p;
 
@@ -210,6 +212,15 @@ static uint32_t __iomem *get_gfar_tbipa(void __iomem *p)
 }
 
 /*
+ * Return the TBIPA address, starting from the address
+ * of the mapped GFAR MII registers (gfar_mii_regs[] within struct gfar)
+ */
+static uint32_t __iomem *get_gfar_tbipa_from_mii(void __iomem *p)
+{
+	return get_gfar_tbipa_from_mdio(container_of(p, struct gfar, gfar_mii_regs));
+}
+
+/*
  * Return the TBIPAR address for an eTSEC2 node
  */
 static uint32_t __iomem *get_etsec_tbipa(void __iomem *p)
@@ -220,11 +231,12 @@ static uint32_t __iomem *get_etsec_tbipa(void __iomem *p)
 
 #if defined(CONFIG_UCC_GETH) || defined(CONFIG_UCC_GETH_MODULE)
 /*
- * Return the TBIPAR address for a QE MDIO node
+ * Return the TBIPAR address for a QE MDIO node, starting from the address
+ * of the mapped MII registers (struct fsl_pq_mii)
  */
 static uint32_t __iomem *get_ucc_tbipa(void __iomem *p)
 {
-	struct fsl_pq_mdio __iomem *mdio = p;
+	struct fsl_pq_mdio __iomem *mdio = container_of(p, struct fsl_pq_mdio, mii);
 
 	return &mdio->utbipar;
 }
@@ -300,14 +312,14 @@ static const struct of_device_id fsl_pq_mdio_match[] = {
 		.compatible = "fsl,gianfar-tbi",
 		.data = &(struct fsl_pq_mdio_data) {
 			.mii_offset = 0,
-			.get_tbipa = get_gfar_tbipa,
+			.get_tbipa = get_gfar_tbipa_from_mii,
 		},
 	},
 	{
 		.compatible = "fsl,gianfar-mdio",
 		.data = &(struct fsl_pq_mdio_data) {
 			.mii_offset = 0,
-			.get_tbipa = get_gfar_tbipa,
+			.get_tbipa = get_gfar_tbipa_from_mii,
 		},
 	},
 	{
@@ -315,7 +327,7 @@ static const struct of_device_id fsl_pq_mdio_match[] = {
 		.compatible = "gianfar",
 		.data = &(struct fsl_pq_mdio_data) {
 			.mii_offset = offsetof(struct fsl_pq_mdio, mii),
-			.get_tbipa = get_gfar_tbipa,
+			.get_tbipa = get_gfar_tbipa_from_mdio,
 		},
 	},
 	{
-- 
1.8.0.1

_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 1/2] net/fsl_pq_mdio: check TBI address for consistency with mapped range
  2015-10-12  7:18   ` Gerlando Falauto
  (?)
  (?)
@ 2015-10-13 11:30   ` David Miller
  -1 siblings, 0 replies; 24+ messages in thread
From: David Miller @ 2015-10-13 11:30 UTC (permalink / raw)
  To: gerlando.falauto
  Cc: netdev, timur, linuxppc-dev, linux-kernel, ogabbay, bigeasy, galak

From: Gerlando Falauto <gerlando.falauto@keymile.com>
Date: Mon, 12 Oct 2015 09:18:40 +0200

> When configuring the MDIO subsystem it is also necessary to configure
> the TBI register. Make sure the TBI is contained within the mapped
> register range in order to:
> a) make sure the address is computed correctly
> b) make users aware that we're actually accessing that register
> 
> In case of error, print a message but continue anyway.
> 
> Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com>
> Cc: Timur Tabi <timur@tabi.org>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Kumar Gala <galak@kernel.crashing.org>
> ---
> Changes from v1:
> - Added type cast & fixed range
> - removed freescale recipients

Applied.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v2 2/2] net/fsl_pq_mdio: fix computed address for the TBI register
  2015-10-12  7:18     ` Gerlando Falauto
  (?)
@ 2015-10-13 11:30     ` David Miller
  -1 siblings, 0 replies; 24+ messages in thread
From: David Miller @ 2015-10-13 11:30 UTC (permalink / raw)
  To: gerlando.falauto
  Cc: netdev, timur, linuxppc-dev, linux-kernel, ogabbay, bigeasy, galak

From: Gerlando Falauto <gerlando.falauto@keymile.com>
Date: Mon, 12 Oct 2015 09:18:41 +0200

> commit afae5ad78b342f401c28b0bb1adb3cd494cb125a
>   "net/fsl_pq_mdio: streamline probing of MDIO nodes"
> 
> added support for different types of MDIO devices:
> 1) Gianfar MDIO nodes that only map the MII registers
> 2) Gianfar MDIO nodes that map the full MDIO register set
> 3) eTSEC2 MDIO nodes (which map the full MDIO register set)
> 4) QE MDIO nodes (which map only the MII registers)
> 
> However, the implementation for types 1 and 4 would mistakenly assume
> a mapping of the full MDIO register set, thereby computing the address
> for the TBI register starting from the containing structure.
> The TBI register would therefore be accessed at a wrong (much bigger)
> address, not giving the expected result at all.
> This patch restores the correct behavior we had prior to the above one.
> 
> The consequences of this bug are apparent when trying to access a PHY
> with the same address as the value contained in the initial value of
> the TBI register (normally 0); in that case you'll get answers from the
> internal TBI device (even though MDIO/MDC pins are actually *also*
> toggling on the physical bus!).
> Beware that you also need to add a fake tbi node to your device tree
> with an unused address.
> 
> Notice how this fix is related to commit
> 220669495bf8b68130a8218607147c7b74c28d2b
>   "powerpc: Add TBI PHY node to first MDIO bus"
> 
> which fixed the behavior in kernel 3.3, which was later broken by the
> above commit on kernel 3.7.
> 
> Signed-off-by: Gerlando Falauto <gerlando.falauto@keymile.com>

Applied.

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2015-10-13 11:14 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-06-12 12:47 [PATCH] MDIO: FSL_PQ_MDIO: Fix bug on incorrect offset of tbipa register Oded Gabbay
2013-06-12 12:47 ` Oded Gabbay
2013-06-12 13:04 ` Timur Tabi
2013-06-12 13:04   ` Timur Tabi
2013-06-12 13:09   ` Oded Gabbay
2013-06-12 13:09     ` Oded Gabbay
2013-06-12 15:08 ` Sebastian Andrzej Siewior
2013-06-12 15:08   ` Sebastian Andrzej Siewior
2013-06-12 18:31   ` Scott Wood
2013-06-12 18:31     ` Scott Wood
2013-06-13  6:44     ` Oded Gabbay
2013-06-13  6:44       ` Oded Gabbay
2013-06-15 22:57     ` Timur Tabi
2013-06-15 22:57       ` Timur Tabi
2015-10-09 15:15 ` [PATCH 1/2] net/fsl_pq_mdio: check TBI address for consistency with mapped range Gerlando Falauto
2015-10-09 15:15   ` [PATCH 2/2] net/fsl_pq_mdio: fix computed address for the TBI register Gerlando Falauto
2015-10-09 17:39   ` [PATCH 1/2] net/fsl_pq_mdio: check TBI address for consistency with mapped range kbuild test robot
2015-10-11 12:41   ` Timur Tabi
2015-10-12  7:18 ` [PATCH v2 " Gerlando Falauto
2015-10-12  7:18   ` Gerlando Falauto
2015-10-12  7:18   ` [PATCH v2 2/2] net/fsl_pq_mdio: fix computed address for the TBI register Gerlando Falauto
2015-10-12  7:18     ` Gerlando Falauto
2015-10-13 11:30     ` David Miller
2015-10-13 11:30   ` [PATCH v2 1/2] net/fsl_pq_mdio: check TBI address for consistency with mapped range David Miller

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.