[PATCH] of: mdio: Support fixed links in of_phy_get_and_connect()

[PATCH] of: mdio: Support fixed links in of_phy_get_and_connect()

[Linus Walleij]

By a simple extension of of_phy_get_and_connect() drivers
that have a fixed link on e.g. RGMII can support also
fixed links, so in addition to:

ethernet-port {
	phy-mode = "rgmii";
	phy-handle = <&foo>;
};

This setup with a fixed-link node and no phy-handle will
now also work just fine:

ethernet-port {
	phy-mode = "rgmii";
	fixed-link {
		speed = <1000>;
		full-duplex;
		pause;
	};
};

This is very helpful for connecting random ethernet ports
to e.g. DSA switches that typically reside on fixed links.

The phy-mode is still there as the fixes link in this case
is still an RGMII link.

Tested on the Cortina Gemini driver with the Vitesse DSA
router chip on a fixed 1Gbit link.

Suggested-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/of/of_mdio.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index d963baf8e53a..e92391d6d1bd 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -367,14 +367,23 @@ struct phy_device *of_phy_get_and_connect(struct net_device *dev,
 	phy_interface_t iface;
 	struct device_node *phy_np;
 	struct phy_device *phy;
+	int ret;
 
 	iface = of_get_phy_mode(np);
 	if (iface < 0)
 		return NULL;
-
-	phy_np = of_parse_phandle(np, "phy-handle", 0);
-	if (!phy_np)
-		return NULL;
+	if (of_phy_is_fixed_link(np)) {
+		ret = of_phy_register_fixed_link(np);
+		if (ret < 0) {
+			netdev_err(dev, "broken fixed-link specification\n");
+			return NULL;
+		}
+		phy_np = of_node_get(np);
+	} else {
+		phy_np = of_parse_phandle(np, "phy-handle", 0);
+		if (!phy_np)
+			return NULL;
+	}
 
 	phy = of_phy_connect(dev, phy_np, hndlr, 0, iface);
 
-- 
2.17.1

Re: [PATCH] of: mdio: Support fixed links in of_phy_get_and_connect()

[Andrew Lunn]

On Wed, Jul 11, 2018 at 07:45:11PM +0200, Linus Walleij wrote:
> By a simple extension of of_phy_get_and_connect() drivers
> that have a fixed link on e.g. RGMII can support also
> fixed links, so in addition to:
> 
> ethernet-port {
> 	phy-mode = "rgmii";
> 	phy-handle = <&foo>;
> };
> 
> This setup with a fixed-link node and no phy-handle will
> now also work just fine:
> 
> ethernet-port {
> 	phy-mode = "rgmii";
> 	fixed-link {
> 		speed = <1000>;
> 		full-duplex;
> 		pause;
> 	};
> };
> 
> This is very helpful for connecting random ethernet ports
> to e.g. DSA switches that typically reside on fixed links.
> 
> The phy-mode is still there as the fixes link in this case
> is still an RGMII link.
> 
> Tested on the Cortina Gemini driver with the Vitesse DSA
> router chip on a fixed 1Gbit link.
> 
> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

What probably make sense as a followup is add a
of_phy_disconnect_and_put(). When the module is unloaded, you leak a
fixed link, because of_phy_deregister_fixed_link() is not being
called. You also hold a reference to np which does not appear to be
released.

    Andrew

Re: [PATCH] of: mdio: Support fixed links in of_phy_get_and_connect()

[Linus Walleij]

On Wed, Jul 11, 2018 at 10:04 PM Andrew Lunn <andrew@lunn.ch> wrote:

> What probably make sense as a followup is add a
> of_phy_disconnect_and_put(). When the module is unloaded, you leak a
> fixed link, because of_phy_deregister_fixed_link() is not being
> called.

I looked at this but I get a bit confused. How to handle
cleanup of the fixed link is pretty straight forward, I'm more
concerned with the proper (today non-existing) way to clean
up after of_phy_connect() that is called for all instances.

This calls phy_connect_direct() that then does this:

/* refcount is held by phy_connect_direct() on success */
put_device(&phy->mdio.dev);

Which seems like wrong - it should keep holding that until
we do of_phy_disconnect_and_put() in that case.
The above seems like some hack, i.e. we are using the
MDIO without holding a reference or something, so it
can go away cleanly later.

Or do I have it wrong?

It's confusing, I guess these PHY's don't come and go
very much so the plug/play part isn't really exercised.

> You also hold a reference to np which does not appear to be
> released.

That seems to be covered as there is a of_node_put(phy_np);
at the end of this function already.

Balancing of_node_get()/put() is another area where there
is not (AFAICT) much stringency in the kernel. I loosely
believe this is mostly for dynamic device trees (so you do not
delete a node that is in use e.g.) and people don't use that
very much (or at all). I think most systems shut down with
a bunch of OF nodes held. :/

(Yeah another universe of cleanups the day we need it
to work.)

Yours,
Linus Walleij

Re: [PATCH] of: mdio: Support fixed links in of_phy_get_and_connect()

[Andrew Lunn]

> It's confusing, I guess these PHY's don't come and go
> very much so the plug/play part isn't really exercised.

Very true. We sometimes need to handle -EPROBE_DEFER, which is not too
different from hot-plugging PHYs. Also, SFP modules are hot-pluggable,
and can contain a copper PHY. However, SFP modules don't have a
traditional MIDO bus. MDIO is tunnelled over i2c. So when a copper SFP
module is hotplugged, the MDIO bus is hot plugged as well.

If you do decided to make any changes here, please ask Russell King to
test, otherwise you might break this use case.

      Andrew

Re: [PATCH] of: mdio: Support fixed links in of_phy_get_and_connect()

[David Miller]

From: Linus Walleij <linus.walleij@linaro.org>
Date: Wed, 11 Jul 2018 19:45:11 +0200

> By a simple extension of of_phy_get_and_connect() drivers
> that have a fixed link on e.g. RGMII can support also
> fixed links, so in addition to:
> 
> ethernet-port {
> 	phy-mode = "rgmii";
> 	phy-handle = <&foo>;
> };
> 
> This setup with a fixed-link node and no phy-handle will
> now also work just fine:
> 
> ethernet-port {
> 	phy-mode = "rgmii";
> 	fixed-link {
> 		speed = <1000>;
> 		full-duplex;
> 		pause;
> 	};
> };
> 
> This is very helpful for connecting random ethernet ports
> to e.g. DSA switches that typically reside on fixed links.
> 
> The phy-mode is still there as the fixes link in this case
> is still an RGMII link.
> 
> Tested on the Cortina Gemini driver with the Vitesse DSA
> router chip on a fixed 1Gbit link.
> 
> Suggested-by: Andrew Lunn <andrew@lunn.ch>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Applied.