phy/fixed.c: rework to not duplicate PHY layer functionality

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

Signed-off-by Vitaly Bordug <[email protected]>

Signed-off-by Anton Vorontsov <[email protected]>

---

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

Documentation/powerpc/booting-without-of.txt +0 -0

index e9a3cb1..cf25070 100644

Quoted 13 lines · Vitaly Bordug

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

Signed-off-by Vitaly Bordug <[email protected]>

Signed-off-by Anton Vorontsov <[email protected]>

---

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

Documentation/powerpc/booting-without-of.txt platforms are moved over to use the flattened-device-tree model. · line 1259

Quoted 11 lines

Documentation/powerpc/booting-without-of.txt +3 -0

index e9a3cb1..cf25070 100644
@@ -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.

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.

Quoted 14 lines · Joakim Tjernlund

Quoted 13 lines · Vitaly Bordug

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

Signed-off-by Vitaly Bordug <[email protected]>

Signed-off-by Anton Vorontsov <[email protected]>

---

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

Quoted 14 lines

Documentation/powerpc/booting-without-of.txt platforms are moved over to use the flattened-device-tree model. · line 1259

Quoted 11 lines

Documentation/powerpc/booting-without-of.txt +3 -0

index e9a3cb1..cf25070 100644
@@ -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.

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

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 bit we can emulate?..

Quoted 2 lines

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

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

Quoted 30 lines · Anton Vorontsov

Quoted 14 lines · Joakim Tjernlund

Quoted 13 lines · Vitaly Bordug

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

Signed-off-by Vitaly Bordug <[email protected]>

Signed-off-by Anton Vorontsov <[email protected]>

---

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

Quoted 14 lines

Documentation/powerpc/booting-without-of.txt platforms are moved over to use the flattened-device-tree model. · line 1259

Quoted 11 lines

Documentation/powerpc/booting-without-of.txt +3 -0

index e9a3cb1..cf25070 100644
@@ -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.

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

Quoted 3 lines

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 bit we can emulate?..

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.

If you seach for pause in phy_device.c you will find it.

Quoted 36 lines · Joakim Tjernlund

Quoted 33 lines · Anton Vorontsov

Quoted 14 lines · Joakim Tjernlund

Quoted 13 lines · Vitaly Bordug

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

Signed-off-by Vitaly Bordug <[email protected]>

Signed-off-by Anton Vorontsov <[email protected]>

---

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

Quoted 14 lines

Documentation/powerpc/booting-without-of.txt platforms are moved over to use the flattened-device-tree model. · line 1259

Quoted 11 lines

Documentation/powerpc/booting-without-of.txt +3 -0

index e9a3cb1..cf25070 100644
@@ -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.

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

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 bit we can emulate?..

Quoted 5 lines

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.

If you seach for pause in phy_device.c you will find it.

Ah, pause. Sure, we can emulate that.

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.

Hi Vitaly,

Quoted 9 lines

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.

Quoted 3 lines

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

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.

drivers/net/fs_enet/fs_enet-main.c +18 -1

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

Hi Vitaly,

Quoted 9 lines

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.

Quoted 3 lines

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

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.

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

drivers/net/fs_enet/fs_enet-main.c +12 -0

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

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?

Quoted 3 lines

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.

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

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;

	}

^^ 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 nicely and also should be ideologically correct. ;-)

Quoted 3 lines

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?

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

Signature · 4 lines

Anton Vorontsov
email: [email protected]
backup email: [email protected]
irc://irc.freenode.net/bd2

[SNIP]

Quoted 19 lines

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

Quoted 3 lines

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

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.

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 <[email protected]>

Signed-off-by Vitaly Bordug <[email protected]>

Quoted 9 lines · Vitaly Bordug

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.

Quoted 5 lines

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 <[email protected]>

Signed-off-by Vitaly Bordug <[email protected]>

what's the context of this patchset? 2.6.25?

it's late for 2.6.24-rc, IMO.

Do I have the latest version (sent Nov 26 @ 9:29am)?

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 <[email protected]>

Signed-off-by Vitaly Bordug <[email protected]>

Quoted 9 lines · Vitaly Bordug

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.

Quoted 5 lines

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 <[email protected]>

Signed-off-by Vitaly Bordug <[email protected]>

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