[PATCH 1/3] [NET] phy/fixed.c: rework to not duplicate PHY layer functionality

Previous thread: [PATCH 1/3] fix setsid() for sub-namespace /sbin/init by Oleg Nesterov on Monday, November 26, 2007 - 7:25 am. (7 messages)

Next thread: [PATCH v2] Add the word 'Warning' in check_nmi_watchdog() output by Don Zickus on Monday, November 26, 2007 - 8:20 am. (2 messages)
From: Vitaly Bordug
Date: Monday, November 26, 2007 - 7:29 am

With that patch fixed.c now fully emulates MDIO bus, thus no need
to duplicate PHY layer functionality. That, in turn, drastically
simplifies the code, and drops down line count.

As an additional bonus, now there is no need to register MDIO bus
for each PHY, all emulated PHYs placed on the platform fixed MDIO bus.
There is also no more need to pre-allocate PHYs via .config option,
this is all now handled dynamically.

p.s. Don't even try to understand patch content! Better: apply patch
and look into resulting drivers/net/phy/fixed.c.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
Signed-off-by: Vitaly Bordug <vitb@kernel.crashing.org>

---

 drivers/net/phy/Kconfig   |   32 +--
 drivers/net/phy/fixed.c   |  427 ++++++++++++++++-----------------------------
 include/linux/phy_fixed.h |   49 ++---
 3 files changed, 176 insertions(+), 332 deletions(-)


diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 54b2ba9..a05c614 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -61,34 +61,12 @@ config ICPLUS_PHY
 	  Currently supports the IP175C PHY.
 
 config FIXED_PHY
-	tristate "Drivers for PHY emulation on fixed speed/link"
+	tristate "Drivers for MDIO Bus/PHY emulation on fixed speed/link"
 	---help---
-	  Adds the driver to PHY layer to cover the boards that do not have any PHY bound,
-	  but with the ability to manipulate the speed/link in software. The relevant MII
-	  speed/duplex parameters could be effectively handled in a user-specified function.
-	  Currently tested with mpc866ads.
-
-config FIXED_MII_10_FDX
-	bool "Emulation for 10M Fdx fixed PHY behavior"
-	depends on FIXED_PHY
-
-config FIXED_MII_100_FDX
-	bool "Emulation for 100M Fdx fixed PHY behavior"
-	depends on FIXED_PHY
-
-config FIXED_MII_1000_FDX
-	bool "Emulation for 1000M Fdx fixed PHY behavior"
-	depends on FIXED_PHY
-
-config FIXED_MII_AMNT
-        int "Number of emulated PHYs to allocate "
-        depends on FIXED_PHY
-        default ...
From: Vitaly Bordug
Date: Monday, November 26, 2007 - 7:29 am

fixed-link says: register new "Fixed/emulated PHY", i.e. PHY that
not connected to the real MDIO bus.

Signed-off-by: Vitaly Bordug <vitb@kernel.crashing.org>
Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>

---

 Documentation/powerpc/booting-without-of.txt |    3 +
 arch/powerpc/sysdev/fsl_soc.c                |   56 ++++++++++++++++++--------
 2 files changed, 42 insertions(+), 17 deletions(-)


diff --git a/Documentation/powerpc/booting-without-of.txt b/Documentation/powerpc/booting-without-of.txt
index e9a3cb1..cf25070 100644
--- a/Documentation/powerpc/booting-without-of.txt
+++ b/Documentation/powerpc/booting-without-of.txt
@@ -1254,6 +1254,9 @@ platforms are moved over to use the flattened-device-tree model.
       services interrupts for this device.
     - phy-handle : The phandle for the PHY connected to this ethernet
       controller.
+    - fixed-link : <a b c> where a is emulated phy id - choose any,
+      but unique to the all specified fixed-links, b is duplex - 0 half,
+      1 full, c is link speed - d#10/d#100/d#1000.
 
   Recommended properties:
 
diff --git a/arch/powerpc/sysdev/fsl_soc.c b/arch/powerpc/sysdev/fsl_soc.c
index 3ace747..e06a5c9 100644
--- a/arch/powerpc/sysdev/fsl_soc.c
+++ b/arch/powerpc/sysdev/fsl_soc.c
@@ -24,6 +24,7 @@
 #include <linux/platform_device.h>
 #include <linux/of_platform.h>
 #include <linux/phy.h>
+#include <linux/phy_fixed.h>
 #include <linux/spi/spi.h>
 #include <linux/fsl_devices.h>
 #include <linux/fs_enet_pd.h>
@@ -193,7 +194,6 @@ static const char *gfar_tx_intr = "tx";
 static const char *gfar_rx_intr = "rx";
 static const char *gfar_err_intr = "error";
 
-
 static int __init gfar_of_init(void)
 {
 	struct device_node *np;
@@ -277,29 +277,51 @@ static int __init gfar_of_init(void)
 			gfar_data.interface = PHY_INTERFACE_MODE_MII;
 
 		ph = of_get_property(np, "phy-handle", NULL);
-		phy = of_find_node_by_phandle(*ph);
+		if (ph == NULL) {
+			struct fixed_phy_status status = ...
From: Joakim Tjernlund
Date: Monday, November 26, 2007 - 8:04 am

Good work!
May I suggest adding a "d" to <a b c> where d is flow control - 0 no, 1 yes

flow control or not just popped up here today so I got a use for it.

 Jocke 


-

From: Anton Vorontsov
Date: Tuesday, November 27, 2007 - 4:39 am

Well, I see no reference of the "flow" neither in the include/linux/mii.h
nor in the drivers/net/phy/*. :-/ Thus today there is no such register

..and I looked into few drivers (gianfar, ucc), and they seem to use
hard-coded flow control values, thus they don't speak to the PHYs
about that.

Can you give any reference to the driver that needs that parameter?
Does it use PHY subsystem to obtain flow-control on/off information?

Thanks,

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2
-

From: Joakim Tjernlund
Date: Tuesday, November 27, 2007 - 6:17 am

Well, as good as I can recall, flow control(pause) is something that the
PHY negotiates, just like FDX/HDX and should be dealt with in the
adjust_link callback but not many do currently.


-

From: Vitaly Bordug
Date: Monday, November 26, 2007 - 7:29 am

...thus use fixed-link to register proper "Fixed PHY"

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
Signed-off-by: Vitaly Bordug <vitb@kernel.crashing.org>

---

 arch/powerpc/boot/dts/mpc8349emitx.dts |   11 ++---------
 1 files changed, 2 insertions(+), 9 deletions(-)


diff --git a/arch/powerpc/boot/dts/mpc8349emitx.dts b/arch/powerpc/boot/dts/mpc8349emitx.dts
index 5072f6d..e2d00f1 100644
--- a/arch/powerpc/boot/dts/mpc8349emitx.dts
+++ b/arch/powerpc/boot/dts/mpc8349emitx.dts
@@ -115,14 +115,6 @@
 				reg = <1c>;
 				device_type = "ethernet-phy";
 			};
-
-			/* Vitesse 7385 */
-			phy1f: ethernet-phy@1f {
-				interrupt-parent = < &ipic >;
-				interrupts = <12 8>;
-				reg = <1f>;
-				device_type = "ethernet-phy";
-			};
 		};
 
 		ethernet@24000 {
@@ -159,7 +151,8 @@
 			local-mac-address = [ 00 00 00 00 00 00 ];
 			interrupts = <23 8 24 8 25 8>;
 			interrupt-parent = < &ipic >;
-			phy-handle = < &phy1f >;
+			/* Vitesse 7385 isn't on the MDIO bus */
+			fixed-link = <1 1 d#1000>;
 			linux,network-index = <1>;
 		};
 

-

From: Jochen Friedrich
Date: Saturday, December 1, 2007 - 6:48 am

If i understand your code correctly, you seem to rely on the fact 
that fixed_phy_add() is called before the fixed MDIO bus is scanned for 
devices. How is this supposed to work for modules or for the 
PPC_CPM_NEW_BINDING mode where the device tree is no longer scanned 
during fs_soc initialization but during device initialization?

I tried to add fixed-phy support to fs_enet, but the fixed phy is not 
found this way.

--- a/drivers/net/fs_enet/fs_enet-main.c
+++ b/drivers/net/fs_enet/fs_enet-main.c
@@ -36,6 +36,7 @@
 #include <linux/fs.h>
 #include <linux/platform_device.h>
 #include <linux/phy.h>
+#include <linux/phy_fixed.h>
 
 #include <linux/vmalloc.h>
 #include <asm/pgtable.h>
@@ -1174,8 +1175,24 @@ static int __devinit find_phy(struct device_node *np,
        struct device_node *phynode, *mdionode;
        struct resource res;
        int ret = 0, len;
+       const u32 *data;
+       struct fixed_phy_status status = {};
+
+       data  = of_get_property(np, "fixed-link", NULL);
+       if (data) {
+               status.link = 1;
+               status.duplex = data[1];
+               status.speed  = data[2];
+
+               ret = fixed_phy_add(PHY_POLL, data[0], &status);
+               if (ret)
+                       return ret;
+
+               snprintf(fpi->bus_id, 16, PHY_ID_FMT, 0, *data);
+               return 0;
+       }
 
-       const u32 *data = of_get_property(np, "phy-handle", &len);
+       data = of_get_property(np, "phy-handle", &len);
        if (!data || len != 4)
                return -EINVAL;

Thanks,
Jochen
--

From: Vitaly Bordug
Date: Saturday, December 1, 2007 - 12:07 pm

On Sat, 01 Dec 2007 14:48:54 +0100
Well, this is kind of known issue - to work it around for now, place PHY lib after fs_enet in


-- 
Sincerely, Vitaly
--

From: Anton Vorontsov
Date: Saturday, December 1, 2007 - 2:34 pm

Yes, indeed. The other name of "fixed phys" are "platform phys"
or "platform MDIO bus" on which virtual PHYs are placed.

That is, these phys supposed to be created by the platform setup
code (arch/). The rationale here is: we do hardware emulation, thus
to make drivers actually see that "hardware", we have to create it

^^ the correct solution is to implement arch_initcall function
which will create fixed PHYs, and then leave only
snprintf(fpi->bus_id, 16, PHY_ID_FMT, 0, *data); part in the
fs_enet's find_phy().

Try add something like this to the fsl_soc.c (compile untested):

- - - -
static int __init of_add_fixed_phys(void)
{
	struct device_node *np;
	const u32 *prop;
	struct fixed_phy_status status = {};

	while ((np = of_find_node_by_name(NULL, "ethernet"))) {
		data  = of_get_property(np, "fixed-link", NULL);
		if (!data)
			continue;

		status.link = 1;
		status.duplex = data[1];
		status.speed  = data[2];

		ret = fixed_phy_add(PHY_POLL, data[0], &status);
		if (ret)
			return ret;
	}

	return 0;
}
arch_initcall(of_add_fixed_phys);
- - - -

And remove fixed_phy_add() from the fs_enet. This should work

We should mark fixed.c as bool. Fake/virtual/fixed/platform PHYs
creation is architecture code anyway, can't be =m.

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2
--

From: Vitaly Bordug
Date: Saturday, December 1, 2007 - 3:22 pm

On Sun, 2 Dec 2007 00:34:03 +0300

well that was the intention but... The point is - as device is emulated, (nearly) everything is doable,
and the only tradeoff to consider, is how far will we go with that emulation. IOW, PHYlib could be tricked
to "do the right thing", and I thought about adding module flexibility...

But thinking more about it, it seems that BSP-code-phy-creation just sucks less and is clear enough yet flexible.
-- 
Sincerely, Vitaly
--

From: Stephen Rothwell
Date: Saturday, December 1, 2007 - 4:27 pm

Just a little reminder ...


	for_each_node_by_name(np, "ethernet") {
(this probably does what you want instead of finding just the first
		if (ret) {
			of_put_node(np);
			retun ret;

--=20
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
From: Joakim Tjernlund
Date: Sunday, December 2, 2007 - 4:54 am

What about Pause and Asym_Pause? Dunno why so few, if any, eth drivers
impl. it, but the PHY lib supports it.
Even if fixed PHYs doesn't support it directly I think the OF interface
should have it.

    - fixed-link : <a b c d e> where a is emulated phy id - choose any,
      but unique to the all specified fixed-links, b is duplex - 0 half,
      1 full, c is link speed - d#10/d#100/d#1000, d is pause - 0 no pause,
      1 pause, d asym_pause - 0 no asym_pause, 1 asym_pause.


--

From: Anton Vorontsov
Date: Sunday, December 2, 2007 - 5:13 am

Will be addressed in the next respin of these patches. Let's

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2
--

From: Vitaly Bordug
Date: Saturday, December 1, 2007 - 3:16 pm

On Sat, 01 Dec 2007 16:59:52 -0500
yes, that's it.

-- 
Sincerely, Vitaly
--

From: Jeff Garzik
Date: Tuesday, December 4, 2007 - 1:07 pm

ACK, I presume this will go via the ppc tree?


--

From: Vitaly Bordug
Date: Tuesday, December 4, 2007 - 6:22 pm

On Tue, 04 Dec 2007 15:07:49 -0500

Yes, I'll add your ack in next respin and will ask Paul to consider it,
if you don't mind.

-- 
Sincerely, Vitaly
--

Previous thread: [PATCH 1/3] fix setsid() for sub-namespace /sbin/init by Oleg Nesterov on Monday, November 26, 2007 - 7:25 am. (7 messages)

Next thread: [PATCH v2] Add the word 'Warning' in check_nmi_watchdog() output by Don Zickus on Monday, November 26, 2007 - 8:20 am. (2 messages)