| Message ID | 20180711174511.15308-1-linus.walleij@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | of: mdio: Support fixed links in of_phy_get_and_connect() | expand |
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
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.
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
> 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
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);
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(-) -- 2.17.1