| Message ID | 1378480701-12908-4-git-send-email-thomas.petazzoni@free-electrons.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, 6 Sep 2013 17:18:20 +0200, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Some Ethernet MACs have a "fixed link", and are not connected to a > normal MDIO-managed PHY device. For those situations, a Device Tree > binding allows to describe a "fixed link" using a special PHY node. > > This patch adds: > > * A documentation for the fixed PHY Device Tree binding. > > * An of_phy_is_fixed_link() function that an Ethernet driver can call > on its PHY phandle to find out whether it's a fixed link PHY or > not. It should typically be used to know if > of_phy_register_fixed_link() should be called. > > * An of_phy_register_fixed_link() function that instantiates the > fixed PHY into the PHY subsystem, so that when the driver calls > of_phy_connect(), the PHY device associated to the OF node will be > found. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> Hi Thomas, The implemenation in this series looks like it is in good shape, so I'll restrict my comments to be binding... > --- > .../devicetree/bindings/net/fixed-link.txt | 34 ++++++++++++++++++++++ > drivers/of/of_mdio.c | 24 +++++++++++++++ > include/linux/of_mdio.h | 15 ++++++++++ > 3 files changed, 73 insertions(+) > create mode 100644 Documentation/devicetree/bindings/net/fixed-link.txt > > diff --git a/Documentation/devicetree/bindings/net/fixed-link.txt b/Documentation/devicetree/bindings/net/fixed-link.txt > new file mode 100644 > index 0000000..9f2a1a50 > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/fixed-link.txt > @@ -0,0 +1,34 @@ > +Fixed link Device Tree binding > +------------------------------ > + > +Some Ethernet MACs have a "fixed link", and are not connected to a > +normal MDIO-managed PHY device. For those situations, a Device Tree > +binding allows to describe a "fixed link". > + > +Such a fixed link situation is described by creating a PHY node as a > +sub-node of an Ethernet device, with the following properties: > + > +* 'fixed-link' (boolean, mandatory), to indicate that this PHY is a > + fixed link PHY. > +* 'speed' (integer, mandatory), to indicate the link speed. Accepted > + values are 10, 100 and 1000 > +* 'full-duplex' (boolean, optional), to indicate that full duplex is > + used. When absent, half duplex is assumed. > +* 'pause' (boolean, optional), to indicate that pause should be > + enabled. > +* 'asym-pause' (boolean, optional), to indicate that asym_pause should > + be enabled. I understand what you're trying to do here, but it causes a troublesome leakage of implementation detail into the binding, making the whole thing look very odd. This binding tries to make a fixed link look exactly like a real PHY even to the point of including a phandle to the phy. But having a phandle to a node which is *always* a direct child of the MAC node is redundant and a rather looney. Yes, doing it that way makes it easy for of_phy_find_device() to be transparent for fixed link, but that should *not* drive bindings, especially when that makes the binding really rather weird. Second, this new binding doesn't provide anything over and above the existing fixed-link binding. It may not be pretty, but it is estabilshed. That said, I do agree that the current Linux implementation is not good because it cannot handle a fixed-link property transparently. That's a deficiency in the Linux implementation and it should be fixed. of_phy_connect() currently requires the phy phandle to be passed in. Part of the reason it was done this way is that some drivers connect to multiple 'phys'. A soulition could be to make the phy handle optional. If it is empty then go looking for either a phy-device or fixed-link property. Otherwise use the provided node. g. > + > +Example: > + > +ethernet@0 { > + ... > + phy = <&phy0>; > + phy0: phy@0 { > + fixed-link; > + speed = <1000>; > + full-duplex; > + }; > + ... > +}; > + > diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c > index d5a57a9..0507f8f 100644 > --- a/drivers/of/of_mdio.c > +++ b/drivers/of/of_mdio.c > @@ -14,6 +14,7 @@ > #include <linux/netdevice.h> > #include <linux/err.h> > #include <linux/phy.h> > +#include <linux/phy_fixed.h> > #include <linux/of.h> > #include <linux/of_irq.h> > #include <linux/of_mdio.h> > @@ -247,3 +248,26 @@ struct phy_device *of_phy_connect_fixed_link(struct net_device *dev, > return IS_ERR(phy) ? NULL : phy; > } > EXPORT_SYMBOL(of_phy_connect_fixed_link); > + > +#if defined(CONFIG_FIXED_PHY) > +bool of_phy_is_fixed_link(struct device_node *np) > +{ > + return of_property_read_bool(np, "fixed-link"); > +} > +EXPORT_SYMBOL(of_phy_is_fixed_link); > + > +int of_phy_register_fixed_link(struct device_node *np) > +{ > + struct fixed_phy_status status = {}; > + > + status.link = 1; > + status.duplex = of_property_read_bool(np, "full-duplex"); > + if (of_property_read_u32(np, "speed", &status.speed)) > + return -EINVAL; > + status.pause = of_property_read_bool(np, "pause"); > + status.asym_pause = of_property_read_bool(np, "asym-pause"); > + > + return fixed_phy_register(PHY_POLL, &status, np); > +} > +EXPORT_SYMBOL(of_phy_register_fixed_link); > +#endif > diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h > index 8163107..2f535ee 100644 > --- a/include/linux/of_mdio.h > +++ b/include/linux/of_mdio.h > @@ -57,4 +57,19 @@ static inline struct mii_bus *of_mdio_find_bus(struct device_node *mdio_np) > } > #endif /* CONFIG_OF */ > > +#if defined(CONFIG_OF) && defined(CONFIG_FIXED_PHY) > +extern int of_phy_register_fixed_link(struct device_node *np); > +extern bool of_phy_is_fixed_link(struct device_node *np); > +#else > +static inline int of_phy_register_fixed_link(struct device_node *np) > +{ > + return -ENOSYS; > +} > +static inline bool of_phy_is_fixed_link(struct device_node *np) > +{ > + return false; > +} > +#endif > + > + > #endif /* __LINUX_OF_MDIO_H */ > -- > 1.8.1.2 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, 2013/9/18 Grant Likely <grant.likely@secretlab.ca>: > On Fri, 6 Sep 2013 17:18:20 +0200, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: >> Some Ethernet MACs have a "fixed link", and are not connected to a >> normal MDIO-managed PHY device. For those situations, a Device Tree >> binding allows to describe a "fixed link" using a special PHY node. >> >> This patch adds: >> >> * A documentation for the fixed PHY Device Tree binding. >> >> * An of_phy_is_fixed_link() function that an Ethernet driver can call >> on its PHY phandle to find out whether it's a fixed link PHY or >> not. It should typically be used to know if >> of_phy_register_fixed_link() should be called. >> >> * An of_phy_register_fixed_link() function that instantiates the >> fixed PHY into the PHY subsystem, so that when the driver calls >> of_phy_connect(), the PHY device associated to the OF node will be >> found. >> >> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > > Hi Thomas, > > The implemenation in this series looks like it is in good shape, so I'll > restrict my comments to be binding... > >> --- >> .../devicetree/bindings/net/fixed-link.txt | 34 ++++++++++++++++++++++ >> drivers/of/of_mdio.c | 24 +++++++++++++++ >> include/linux/of_mdio.h | 15 ++++++++++ >> 3 files changed, 73 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/net/fixed-link.txt >> >> diff --git a/Documentation/devicetree/bindings/net/fixed-link.txt b/Documentation/devicetree/bindings/net/fixed-link.txt >> new file mode 100644 >> index 0000000..9f2a1a50 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/net/fixed-link.txt >> @@ -0,0 +1,34 @@ >> +Fixed link Device Tree binding >> +------------------------------ >> + >> +Some Ethernet MACs have a "fixed link", and are not connected to a >> +normal MDIO-managed PHY device. For those situations, a Device Tree >> +binding allows to describe a "fixed link". >> + >> +Such a fixed link situation is described by creating a PHY node as a >> +sub-node of an Ethernet device, with the following properties: >> + >> +* 'fixed-link' (boolean, mandatory), to indicate that this PHY is a >> + fixed link PHY. >> +* 'speed' (integer, mandatory), to indicate the link speed. Accepted >> + values are 10, 100 and 1000 >> +* 'full-duplex' (boolean, optional), to indicate that full duplex is >> + used. When absent, half duplex is assumed. >> +* 'pause' (boolean, optional), to indicate that pause should be >> + enabled. >> +* 'asym-pause' (boolean, optional), to indicate that asym_pause should >> + be enabled. > > I understand what you're trying to do here, but it causes a troublesome > leakage of implementation detail into the binding, making the whole > thing look very odd. This binding tries to make a fixed link look > exactly like a real PHY even to the point of including a phandle to the > phy. But having a phandle to a node which is *always* a direct child of > the MAC node is redundant and a rather looney. Yes, doing it that way > makes it easy for of_phy_find_device() to be transparent for fixed link, > but that should *not* drive bindings, especially when that makes the > binding really rather weird. This is not exactly true in the sense that the "new" binding just re-shuffles the properties representation into something that is clearer and more extendible but there is not much difference in the semantics. > > Second, this new binding doesn't provide anything over and above the > existing fixed-link binding. It may not be pretty, but it is > estabilshed. In fact it does, the old one is obscure and not easily extendable because we rely on an integer array to represent the various properties, so at least this new one makes it easy to extend the binding to support a possibly new fixed-link property. Being able to deprecate a fundamentaly badly designed binding should still be a prerogative, software is flexible and can deal with both with little cost. > > That said, I do agree that the current Linux implementation is not good > because it cannot handle a fixed-link property transparently. That's a > deficiency in the Linux implementation and it should be fixed. > of_phy_connect() currently requires the phy phandle to be passed in. > Part of the reason it was done this way is that some drivers connect to > multiple 'phys'. A soulition could be to make the phy handle optional. > If it is empty then go looking for either a phy-device or fixed-link > property. Otherwise use the provided node. I do not quite follow you on this one, and I fear we might be leaking some Linux probing heuristic into Device Tree bindings by implicitely saying "not including a PHY phandle means connecting to a fixed-PHY link." This would also dramatically change the current behavior for most drivers where they might refuse probing if no corresponding PHY device node is present and they are not designed to connect to a fixed PHY one.
On Wed, 18 Sep 2013 10:21:11 +0100, Florian Fainelli <f.fainelli@gmail.com> wrote: > Hello, > > 2013/9/18 Grant Likely <grant.likely@secretlab.ca>: > > On Fri, 6 Sep 2013 17:18:20 +0200, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > >> Some Ethernet MACs have a "fixed link", and are not connected to a > >> normal MDIO-managed PHY device. For those situations, a Device Tree > >> binding allows to describe a "fixed link" using a special PHY node. > >> > >> This patch adds: > >> > >> * A documentation for the fixed PHY Device Tree binding. > >> > >> * An of_phy_is_fixed_link() function that an Ethernet driver can call > >> on its PHY phandle to find out whether it's a fixed link PHY or > >> not. It should typically be used to know if > >> of_phy_register_fixed_link() should be called. > >> > >> * An of_phy_register_fixed_link() function that instantiates the > >> fixed PHY into the PHY subsystem, so that when the driver calls > >> of_phy_connect(), the PHY device associated to the OF node will be > >> found. > >> > >> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> > > > > Hi Thomas, > > > > The implemenation in this series looks like it is in good shape, so I'll > > restrict my comments to be binding... > > > >> --- > >> .../devicetree/bindings/net/fixed-link.txt | 34 ++++++++++++++++++++++ > >> drivers/of/of_mdio.c | 24 +++++++++++++++ > >> include/linux/of_mdio.h | 15 ++++++++++ > >> 3 files changed, 73 insertions(+) > >> create mode 100644 Documentation/devicetree/bindings/net/fixed-link.txt > >> > >> diff --git a/Documentation/devicetree/bindings/net/fixed-link.txt b/Documentation/devicetree/bindings/net/fixed-link.txt > >> new file mode 100644 > >> index 0000000..9f2a1a50 > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/net/fixed-link.txt > >> @@ -0,0 +1,34 @@ > >> +Fixed link Device Tree binding > >> +------------------------------ > >> + > >> +Some Ethernet MACs have a "fixed link", and are not connected to a > >> +normal MDIO-managed PHY device. For those situations, a Device Tree > >> +binding allows to describe a "fixed link". > >> + > >> +Such a fixed link situation is described by creating a PHY node as a > >> +sub-node of an Ethernet device, with the following properties: > >> + > >> +* 'fixed-link' (boolean, mandatory), to indicate that this PHY is a > >> + fixed link PHY. > >> +* 'speed' (integer, mandatory), to indicate the link speed. Accepted > >> + values are 10, 100 and 1000 > >> +* 'full-duplex' (boolean, optional), to indicate that full duplex is > >> + used. When absent, half duplex is assumed. > >> +* 'pause' (boolean, optional), to indicate that pause should be > >> + enabled. > >> +* 'asym-pause' (boolean, optional), to indicate that asym_pause should > >> + be enabled. > > > > I understand what you're trying to do here, but it causes a troublesome > > leakage of implementation detail into the binding, making the whole > > thing look very odd. This binding tries to make a fixed link look > > exactly like a real PHY even to the point of including a phandle to the > > phy. But having a phandle to a node which is *always* a direct child of > > the MAC node is redundant and a rather looney. Yes, doing it that way > > makes it easy for of_phy_find_device() to be transparent for fixed link, > > but that should *not* drive bindings, especially when that makes the > > binding really rather weird. > > This is not exactly true in the sense that the "new" binding just > re-shuffles the properties representation into something that is > clearer and more extendible but there is not much difference in the > semantics. That's not my point in the above paragraph. My point is a binding that consists of a phandle to a node that is always a direct child is goofy and wrong. > > Second, this new binding doesn't provide anything over and above the > > existing fixed-link binding. It may not be pretty, but it is > > estabilshed. > > In fact it does, the old one is obscure and not easily extendable > because we rely on an integer array to represent the various > properties, so at least this new one makes it easy to extend the > binding to support a possibly new fixed-link property. Being able to > deprecate a fundamentaly badly designed binding should still be a > prerogative, software is flexible and can deal with both with little > cost. I understand that, but consistency is also important. I don't see a proposal for a new feature for the binding in this patch. Without a really compelling reason to change a binding that works (even if it is opaque) I cannot agree to changing it. > > That said, I do agree that the current Linux implementation is not good > > because it cannot handle a fixed-link property transparently. That's a > > deficiency in the Linux implementation and it should be fixed. > > of_phy_connect() currently requires the phy phandle to be passed in. > > Part of the reason it was done this way is that some drivers connect to > > multiple 'phys'. A soulition could be to make the phy handle optional. > > If it is empty then go looking for either a phy-device or fixed-link > > property. Otherwise use the provided node. > > I do not quite follow you on this one, and I fear we might be leaking > some Linux probing heuristic into Device Tree bindings by implicitely > saying "not including a PHY phandle means connecting to a fixed-PHY > link." This would also dramatically change the current behavior for > most drivers where they might refuse probing if no corresponding PHY > device node is present and they are not designed to connect to a fixed > PHY one. Drivers we can change and fix. There aren't very may call sites affected so I'm not overly worried about it. Also, I was making a suggestion on how to fix it, but a suggestion is not a fully formed patch. The issue you raise would of course need to be addressed. g. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Dear Grant Likely, On Tue, 17 Sep 2013 23:29:23 -0500, Grant Likely wrote: > I understand what you're trying to do here, but it causes a troublesome > leakage of implementation detail into the binding, making the whole > thing look very odd. This binding tries to make a fixed link look > exactly like a real PHY even to the point of including a phandle to the > phy. But having a phandle to a node which is *always* a direct child of > the MAC node is redundant and a rather looney. Yes, doing it that way > makes it easy for of_phy_find_device() to be transparent for fixed link, > but that should *not* drive bindings, especially when that makes the > binding really rather weird. > > Second, this new binding doesn't provide anything over and above the > existing fixed-link binding. It may not be pretty, but it is > estabilshed. Have you followed the past discussions about this patch set? Basically the *only* feedback I received on RFCv1 is that the fixed-link property sucks, and everybody (including the known Device Tree binding maintainer Mark Rutland) suggested to not use the fixed-link mechanism. See http://article.gmane.org/gmane.linux.network/276932, where Mark said: "" I'm not sure grouping these values together is the best way of handling this. It's rather opaque, and inflexible for future extension. "" So, please DT maintainers, tell me what you want. I honestly don't care whether fixed-link or a separate node is chosen. However, I do care about being dragged around between two solutions just because the former DT maintainer and the new DT maintainers do not agree. Thanks, Thomas
On Wed, Sep 18, 2013 at 10:00:31AM -0500, Grant Likely wrote: > On Wed, 18 Sep 2013 10:21:11 +0100, Florian Fainelli <f.fainelli@gmail.com> wrote: > > > > > > I understand what you're trying to do here, but it causes a troublesome > > > leakage of implementation detail into the binding, making the whole > > > thing look very odd. This binding tries to make a fixed link look > > > exactly like a real PHY even to the point of including a phandle to the > > > phy. But having a phandle to a node which is *always* a direct child of > > > the MAC node is redundant and a rather looney. Yes, doing it that way > > > makes it easy for of_phy_find_device() to be transparent for fixed link, > > > but that should *not* drive bindings, especially when that makes the > > > binding really rather weird. > > > > This is not exactly true in the sense that the "new" binding just > > re-shuffles the properties representation into something that is > > clearer and more extendible but there is not much difference in the > > semantics. > > That's not my point in the above paragraph. My point is a binding that > consists of a phandle to a node that is always a direct child is goofy > and wrong. It's not necessarily a direct child. Most of these fixed links are really ethernet switches. These are (mostly) i2c devices which are under their corresponding i2c bus node. Using a phandle from the ethernet MAC to the port of a switch not only provides the link information, but also the information to which port of the switch the MAC is connected. Another situation is that some SoCs have a MDIO bus external to the MAC and possibly shared for multiple ethernet MACs. This also requires a phandle from the MAC to the MDIO bus. So we have the situation that we need a phandle from the MAC to something that provides a link. For consistency it would be good to always use a phandle instead of having an inflexible 'fixed-link' property. You're right, the binding doesn't provide anything new, but I think it straightens things up for the future. Sascha
Le mercredi 18 septembre 2013, 18:11:12 Thomas Petazzoni a écrit : > Dear Grant Likely, > > On Tue, 17 Sep 2013 23:29:23 -0500, Grant Likely wrote: > > I understand what you're trying to do here, but it causes a troublesome > > leakage of implementation detail into the binding, making the whole > > thing look very odd. This binding tries to make a fixed link look > > exactly like a real PHY even to the point of including a phandle to the > > phy. But having a phandle to a node which is *always* a direct child of > > the MAC node is redundant and a rather looney. Yes, doing it that way > > makes it easy for of_phy_find_device() to be transparent for fixed link, > > but that should *not* drive bindings, especially when that makes the > > binding really rather weird. > > > > Second, this new binding doesn't provide anything over and above the > > existing fixed-link binding. It may not be pretty, but it is > > estabilshed. > > Have you followed the past discussions about this patch set? Basically > the *only* feedback I received on RFCv1 is that the fixed-link property > sucks, and everybody (including the known Device Tree binding > maintainer Mark Rutland) suggested to not use the fixed-link mechanism. > See http://article.gmane.org/gmane.linux.network/276932, where Mark > said: > > "" > I'm not sure grouping these values together is the best way of handling > this. It's rather opaque, and inflexible for future extension. > "" > > So, please DT maintainers, tell me what you want. I honestly don't care > whether fixed-link or a separate node is chosen. However, I do care > about being dragged around between two solutions just because the > former DT maintainer and the new DT maintainers do not agree. Since I would like to move forward so I can one day use that binding in a real-life product, I am going to go for Mark's side which happens to be how I want the binding to look like. Do we all agree that the new binding is just way better than the old one? In light of the recent unstable DT ABI discussion, we can still continue parsing the old one for the sake of compatibility.
Hello Thomas, 2013/9/6 Thomas Petazzoni <thomas.petazzoni@free-electrons.com>: > +Some Ethernet MACs have a "fixed link", and are not connected to a > +normal MDIO-managed PHY device. For those situations, a Device Tree > +binding allows to describe a "fixed link". > + > +Such a fixed link situation is described by creating a PHY node as a > +sub-node of an Ethernet device, with the following properties: > + > +* 'fixed-link' (boolean, mandatory), to indicate that this PHY is a > + fixed link PHY. > +* 'speed' (integer, mandatory), to indicate the link speed. Accepted > + values are 10, 100 and 1000 'max-speed' might be better here to match ePAPR v1.1 (if we do care, 'speed') works for me too. > +* 'full-duplex' (boolean, optional), to indicate that full duplex is > + used. When absent, half duplex is assumed. > +* 'pause' (boolean, optional), to indicate that pause should be > + enabled. > +* 'asym-pause' (boolean, optional), to indicate that asym_pause should > + be enabled. We also need to add a property: 'connection-type' which can be any of 'mii', 'rgmii' etc... When operating Ethernet devices with Ethernet devices connected back to back, it might be required to configure the Ethernet MAC with an appropriate connection type. Note that I picked 'connection-type' here because this the ePAPR v1.1 terminology. Now the good thing is that it is a new "feature" wrt. the old binding. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 25 Oct 2013 05:40:57 +0100, Florian Fainelli <florian@openwrt.org> wrote: > Le mercredi 18 septembre 2013, 18:11:12 Thomas Petazzoni a écrit : > > Dear Grant Likely, > > > > On Tue, 17 Sep 2013 23:29:23 -0500, Grant Likely wrote: > > > I understand what you're trying to do here, but it causes a troublesome > > > leakage of implementation detail into the binding, making the whole > > > thing look very odd. This binding tries to make a fixed link look > > > exactly like a real PHY even to the point of including a phandle to the > > > phy. But having a phandle to a node which is *always* a direct child of > > > the MAC node is redundant and a rather looney. Yes, doing it that way > > > makes it easy for of_phy_find_device() to be transparent for fixed link, > > > but that should *not* drive bindings, especially when that makes the > > > binding really rather weird. > > > > > > Second, this new binding doesn't provide anything over and above the > > > existing fixed-link binding. It may not be pretty, but it is > > > estabilshed. > > > > Have you followed the past discussions about this patch set? Basically > > the *only* feedback I received on RFCv1 is that the fixed-link property > > sucks, and everybody (including the known Device Tree binding > > maintainer Mark Rutland) suggested to not use the fixed-link mechanism. > > See http://article.gmane.org/gmane.linux.network/276932, where Mark > > said: > > > > "" > > I'm not sure grouping these values together is the best way of handling > > this. It's rather opaque, and inflexible for future extension. > > "" > > > > So, please DT maintainers, tell me what you want. I honestly don't care > > whether fixed-link or a separate node is chosen. However, I do care > > about being dragged around between two solutions just because the > > former DT maintainer and the new DT maintainers do not agree. I've been sleepy on this issue, which limits how much I can push. I'll say one more thing on the issue (below) and then leave the decision up to Mark. I trust him and he knows what he is doing. > Since I would like to move forward so I can one day use that binding in a > real-life product, I am going to go for Mark's side which happens to be how I > want the binding to look like. > > Do we all agree that the new binding is just way better than the old one? In > light of the recent unstable DT ABI discussion, we can still continue parsing > the old one for the sake of compatibility. Regardless of what you want it to look like, does the old binding work for your purposes? If yes then use it. The only valid reason for creating a new binding is if the old one doesn't work for a specific (not theoretical) use case. g. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Florian, Grant, On Tue, Nov 12, 2013 at 12:37:46PM +0000, Grant Likely wrote: > On Fri, 25 Oct 2013 05:40:57 +0100, Florian Fainelli <florian@openwrt.org> wrote: > > Le mercredi 18 septembre 2013, 18:11:12 Thomas Petazzoni a écrit : > > > Dear Grant Likely, > > > > > > On Tue, 17 Sep 2013 23:29:23 -0500, Grant Likely wrote: > > > > I understand what you're trying to do here, but it causes a troublesome > > > > leakage of implementation detail into the binding, making the whole > > > > thing look very odd. This binding tries to make a fixed link look > > > > exactly like a real PHY even to the point of including a phandle to the > > > > phy. But having a phandle to a node which is *always* a direct child of > > > > the MAC node is redundant and a rather looney. Yes, doing it that way > > > > makes it easy for of_phy_find_device() to be transparent for fixed link, > > > > but that should *not* drive bindings, especially when that makes the > > > > binding really rather weird. > > > > > > > > Second, this new binding doesn't provide anything over and above the > > > > existing fixed-link binding. It may not be pretty, but it is > > > > estabilshed. > > > > > > Have you followed the past discussions about this patch set? Basically > > > the *only* feedback I received on RFCv1 is that the fixed-link property > > > sucks, and everybody (including the known Device Tree binding > > > maintainer Mark Rutland) suggested to not use the fixed-link mechanism. > > > See http://article.gmane.org/gmane.linux.network/276932, where Mark > > > said: > > > > > > "" > > > I'm not sure grouping these values together is the best way of handling > > > this. It's rather opaque, and inflexible for future extension. > > > "" > > > > > > So, please DT maintainers, tell me what you want. I honestly don't care > > > whether fixed-link or a separate node is chosen. However, I do care > > > about being dragged around between two solutions just because the > > > former DT maintainer and the new DT maintainers do not agree. > > I've been sleepy on this issue, which limits how much I can push. I'll > say one more thing on the issue (below) and then leave the decision up > to Mark. I trust him and he knows what he is doing. > > > Since I would like to move forward so I can one day use that binding in a > > real-life product, I am going to go for Mark's side which happens to be how I > > want the binding to look like. > > > > Do we all agree that the new binding is just way better than the old one? In > > light of the recent unstable DT ABI discussion, we can still continue parsing > > the old one for the sake of compatibility. > > Regardless of what you want it to look like, does the old binding work > for your purposes? If yes then use it. The only valid reason for > creating a new binding is if the old one doesn't work for a specific > (not theoretical) use case. I think the issue here was that I am not versed in the history of all of the existing bindings. While I'm not keen on the existing fixed-link property and I think it should be done differently were it being created from scratch today, as Grant has pointed out we're already supporting it today, and adding a new binding is going to make the code handling it more complex. If fixed-link works for your use case today, then use fixed-link. If we have a valid reason to create a new binding, we should. At the moment I think the only egregious portion of the binding is the globally unique fake PHY id, and if that causes issues we should be able to assign IDs within Linux ignoring the values in the DT, or reorganise things such that the arbitrary ID doesn't matter. If there are configurations we need to support that the fixed-link property cannot encode, then I think we should go ahead with the binding style that you are proposing today. However, we don't need to go with it right away, and we can continue to support fixed-link regardless. I apologise for the lack of consistency here, and I'm sorry that I've delayed this series for so long. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2013/11/12 Mark Rutland <mark.rutland@arm.com>: > Hi Florian, Grant, > > On Tue, Nov 12, 2013 at 12:37:46PM +0000, Grant Likely wrote: >> On Fri, 25 Oct 2013 05:40:57 +0100, Florian Fainelli <florian@openwrt.org> wrote: >> > Le mercredi 18 septembre 2013, 18:11:12 Thomas Petazzoni a écrit : >> > > Dear Grant Likely, >> > > >> > > On Tue, 17 Sep 2013 23:29:23 -0500, Grant Likely wrote: >> > > > I understand what you're trying to do here, but it causes a troublesome >> > > > leakage of implementation detail into the binding, making the whole >> > > > thing look very odd. This binding tries to make a fixed link look >> > > > exactly like a real PHY even to the point of including a phandle to the >> > > > phy. But having a phandle to a node which is *always* a direct child of >> > > > the MAC node is redundant and a rather looney. Yes, doing it that way >> > > > makes it easy for of_phy_find_device() to be transparent for fixed link, >> > > > but that should *not* drive bindings, especially when that makes the >> > > > binding really rather weird. >> > > > >> > > > Second, this new binding doesn't provide anything over and above the >> > > > existing fixed-link binding. It may not be pretty, but it is >> > > > estabilshed. >> > > >> > > Have you followed the past discussions about this patch set? Basically >> > > the *only* feedback I received on RFCv1 is that the fixed-link property >> > > sucks, and everybody (including the known Device Tree binding >> > > maintainer Mark Rutland) suggested to not use the fixed-link mechanism. >> > > See http://article.gmane.org/gmane.linux.network/276932, where Mark >> > > said: >> > > >> > > "" >> > > I'm not sure grouping these values together is the best way of handling >> > > this. It's rather opaque, and inflexible for future extension. >> > > "" >> > > >> > > So, please DT maintainers, tell me what you want. I honestly don't care >> > > whether fixed-link or a separate node is chosen. However, I do care >> > > about being dragged around between two solutions just because the >> > > former DT maintainer and the new DT maintainers do not agree. >> >> I've been sleepy on this issue, which limits how much I can push. I'll >> say one more thing on the issue (below) and then leave the decision up >> to Mark. I trust him and he knows what he is doing. >> >> > Since I would like to move forward so I can one day use that binding in a >> > real-life product, I am going to go for Mark's side which happens to be how I >> > want the binding to look like. >> > >> > Do we all agree that the new binding is just way better than the old one? In >> > light of the recent unstable DT ABI discussion, we can still continue parsing >> > the old one for the sake of compatibility. >> >> Regardless of what you want it to look like, does the old binding work >> for your purposes? If yes then use it. The only valid reason for >> creating a new binding is if the old one doesn't work for a specific >> (not theoretical) use case. > > I think the issue here was that I am not versed in the history of all of > the existing bindings. While I'm not keen on the existing fixed-link > property and I think it should be done differently were it being created > from scratch today, as Grant has pointed out we're already supporting it > today, and adding a new binding is going to make the code handling it > more complex. > > If fixed-link works for your use case today, then use fixed-link. Like I said in an earlier response to Thomas yesterday, it does not cover everything, most importantly, the existing "fixed-link" property does not allow the representation of direct MAC to MAC connections where you still need to specify the MII connection type for it to work properly. This is something that can be easily provided by the new fixed PHY binding, but less easily by the existing "fixed-link" property. We cannot extend "fixed-link" to contain a 6th integer because "phy-mode" or "phy-connection-type" is a string. My options here are pretty simple: - use the existing "fixed-link" property even though it is not nice, it does the job - add a supplemental "connection-type" property and parse it in my driver, which is where it is going to be used - go on with the new proposed Device Tree binding > > If we have a valid reason to create a new binding, we should. At the > moment I think the only egregious portion of the binding is the globally > unique fake PHY id, and if that causes issues we should be able to > assign IDs within Linux ignoring the values in the DT, or reorganise > things such that the arbitrary ID doesn't matter. I think we all agreed that the PHY ID (the term should be clarified as it tends to either mean PHY address or PHY OUI) had to be gone, and this is what Thomas did in this serie. Whichever binding we choose, it should be fairly easy to keep the "PHY ID" integer in the "fixed-link" property but make the code ignore it (with possibly a big fat warning for a transition period). > > If there are configurations we need to support that the fixed-link > property cannot encode, then I think we should go ahead with the binding > style that you are proposing today. However, we don't need to go with it > right away, and we can continue to support fixed-link regardless. Well, yes, the lack of "connection-type" representation is a blocker for some hardware configurations where two Ethernet MACs are connected one to each other. Whether this deserves a new incarnation of the "fixed PHY" Device Tree binding is left for discussion. > > I apologise for the lack of consistency here, and I'm sorry that I've > delayed this series for so long.
diff --git a/Documentation/devicetree/bindings/net/fixed-link.txt b/Documentation/devicetree/bindings/net/fixed-link.txt new file mode 100644 index 0000000..9f2a1a50 --- /dev/null +++ b/Documentation/devicetree/bindings/net/fixed-link.txt @@ -0,0 +1,34 @@ +Fixed link Device Tree binding +------------------------------ + +Some Ethernet MACs have a "fixed link", and are not connected to a +normal MDIO-managed PHY device. For those situations, a Device Tree +binding allows to describe a "fixed link". + +Such a fixed link situation is described by creating a PHY node as a +sub-node of an Ethernet device, with the following properties: + +* 'fixed-link' (boolean, mandatory), to indicate that this PHY is a + fixed link PHY. +* 'speed' (integer, mandatory), to indicate the link speed. Accepted + values are 10, 100 and 1000 +* 'full-duplex' (boolean, optional), to indicate that full duplex is + used. When absent, half duplex is assumed. +* 'pause' (boolean, optional), to indicate that pause should be + enabled. +* 'asym-pause' (boolean, optional), to indicate that asym_pause should + be enabled. + +Example: + +ethernet@0 { + ... + phy = <&phy0>; + phy0: phy@0 { + fixed-link; + speed = <1000>; + full-duplex; + }; + ... +}; + diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c index d5a57a9..0507f8f 100644 --- a/drivers/of/of_mdio.c +++ b/drivers/of/of_mdio.c @@ -14,6 +14,7 @@ #include <linux/netdevice.h> #include <linux/err.h> #include <linux/phy.h> +#include <linux/phy_fixed.h> #include <linux/of.h> #include <linux/of_irq.h> #include <linux/of_mdio.h> @@ -247,3 +248,26 @@ struct phy_device *of_phy_connect_fixed_link(struct net_device *dev, return IS_ERR(phy) ? NULL : phy; } EXPORT_SYMBOL(of_phy_connect_fixed_link); + +#if defined(CONFIG_FIXED_PHY) +bool of_phy_is_fixed_link(struct device_node *np) +{ + return of_property_read_bool(np, "fixed-link"); +} +EXPORT_SYMBOL(of_phy_is_fixed_link); + +int of_phy_register_fixed_link(struct device_node *np) +{ + struct fixed_phy_status status = {}; + + status.link = 1; + status.duplex = of_property_read_bool(np, "full-duplex"); + if (of_property_read_u32(np, "speed", &status.speed)) + return -EINVAL; + status.pause = of_property_read_bool(np, "pause"); + status.asym_pause = of_property_read_bool(np, "asym-pause"); + + return fixed_phy_register(PHY_POLL, &status, np); +} +EXPORT_SYMBOL(of_phy_register_fixed_link); +#endif diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h index 8163107..2f535ee 100644 --- a/include/linux/of_mdio.h +++ b/include/linux/of_mdio.h @@ -57,4 +57,19 @@ static inline struct mii_bus *of_mdio_find_bus(struct device_node *mdio_np) } #endif /* CONFIG_OF */ +#if defined(CONFIG_OF) && defined(CONFIG_FIXED_PHY) +extern int of_phy_register_fixed_link(struct device_node *np); +extern bool of_phy_is_fixed_link(struct device_node *np); +#else +static inline int of_phy_register_fixed_link(struct device_node *np) +{ + return -ENOSYS; +} +static inline bool of_phy_is_fixed_link(struct device_node *np) +{ + return false; +} +#endif + + #endif /* __LINUX_OF_MDIO_H */
Some Ethernet MACs have a "fixed link", and are not connected to a normal MDIO-managed PHY device. For those situations, a Device Tree binding allows to describe a "fixed link" using a special PHY node. This patch adds: * A documentation for the fixed PHY Device Tree binding. * An of_phy_is_fixed_link() function that an Ethernet driver can call on its PHY phandle to find out whether it's a fixed link PHY or not. It should typically be used to know if of_phy_register_fixed_link() should be called. * An of_phy_register_fixed_link() function that instantiates the fixed PHY into the PHY subsystem, so that when the driver calls of_phy_connect(), the PHY device associated to the OF node will be found. Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> --- .../devicetree/bindings/net/fixed-link.txt | 34 ++++++++++++++++++++++ drivers/of/of_mdio.c | 24 +++++++++++++++ include/linux/of_mdio.h | 15 ++++++++++ 3 files changed, 73 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/fixed-link.txt