| Message ID | 559EB1A6.1090008@list.ru |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
(there is no such thing as linux-net@vger.kernel.org, please remove it from your future submissions). On 09/07/15 10:38, Stas Sergeev wrote: > > Currently for fixed-link the link state is always set to UP. Not quite true, this is always a driver decision to make. > This patch introduces the new property 'link' that accepts the > following string arguments: "up", "down" and "auto". > "down" may be needed if the link is physically unconnected. In which case you probably do not even care about inserting such a property in the first place, do you? What would be the value of forcibly having a link permanently down (not counting loopback)? > "auto" is needed to enable the link paramaters auto-negotiation, > that is built into some MII protocols, namely SGMII. RGMII also has an in-band status FWIW. > The appropriate documentation is added and explicitly states that > "auto" is very specific (protocol, HW and driver-specific), and > is therefore should be used with care. And therefore probably be made a device (and driver) specific decision whether this is the right thing to do. I do no think this addition to the "fixed-link" property is desirable the way you have defined it. More comments below. > > Signed-off-by: Stas Sergeev <stsp@users.sourceforge.net> > > CC: Rob Herring <robh+dt@kernel.org> > CC: Pawel Moll <pawel.moll@arm.com> > CC: Mark Rutland <mark.rutland@arm.com> > CC: Ian Campbell <ijc+devicetree@hellion.org.uk> > CC: Kumar Gala <galak@codeaurora.org> > CC: Florian Fainelli <f.fainelli@gmail.com> > CC: Grant Likely <grant.likely@linaro.org> > CC: devicetree@vger.kernel.org > CC: linux-kernel@vger.kernel.org > CC: netdev@vger.kernel.org > --- > .../devicetree/bindings/net/fixed-link.txt | 8 +++- > drivers/of/of_mdio.c | 48 ++++++++++++++++++++-- > include/linux/of_mdio.h | 5 +++ > 3 files changed, 57 insertions(+), 4 deletions(-) > > diff --git a/Documentation/devicetree/bindings/net/fixed-link.txt b/Documentation/devicetree/bindings/net/fixed-link.txt > index 82bf7e0..070f554 100644 > --- a/Documentation/devicetree/bindings/net/fixed-link.txt > +++ b/Documentation/devicetree/bindings/net/fixed-link.txt > @@ -9,8 +9,14 @@ Such a fixed link situation is described by creating a 'fixed-link' > sub-node of the Ethernet MAC device node, with the following > properties: > > +* 'link' (string, optional), to indicate the link state. Accepted > + values are "up", "down" and "auto". "auto" means auto-negotiation of > + link parameters. Auto-negotiation is MII protocol, HW and driver-specific > + and is not supported in many cases, so use it only when you know what > + you do. > * 'speed' (integer, mandatory), to indicate the link speed. Accepted > - values are 10, 100 and 1000 > + values are 10, 100 and 1000. If the 'link' property is set to 'auto', > + 'speed' may not be set. It will then be auto-negotiated, if possible. > * '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 > diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c > index 1bd4305..2152cf8 100644 > --- a/drivers/of/of_mdio.c > +++ b/drivers/of/of_mdio.c > @@ -280,6 +280,26 @@ bool of_phy_is_fixed_link(struct device_node *np) > } > EXPORT_SYMBOL(of_phy_is_fixed_link); > > +bool of_phy_is_autoneg_link(struct device_node *np) > +{ > + struct device_node *dn; > + const char *link_str; > + int rc; > + bool ret = false; > + > + dn = of_get_child_by_name(np, "fixed-link"); > + if (!dn) > + return false; > + > + rc = of_property_read_string(dn, "link", &link_str); > + if (rc == 0 && strcmp(link_str, "auto") == 0) > + ret = true; > + > + of_node_put(dn); > + return ret; > +} > +EXPORT_SYMBOL(of_phy_is_autoneg_link); > + > int of_phy_register_fixed_link(struct device_node *np) > { > struct fixed_phy_status status = {}; > @@ -291,11 +311,33 @@ int of_phy_register_fixed_link(struct device_node *np) > /* New binding */ > fixed_link_node = of_get_child_by_name(np, "fixed-link"); > if (fixed_link_node) { > - status.link = 1; > + const char *link_str; > + int ret; > + bool link_auto = false; > + > + ret = of_property_read_string(fixed_link_node, "link", > + &link_str); > + if (ret == 0) { > + if (strcmp(link_str, "up") == 0) > + status.link = 1; > + else > + status.link = 0; > + if (strcmp(link_str, "auto") == 0) > + link_auto = true; > + } else { > + status.link = 1; > + } > status.duplex = of_property_read_bool(fixed_link_node, > "full-duplex"); > - if (of_property_read_u32(fixed_link_node, "speed", &status.speed)) > - return -EINVAL; > + if (of_property_read_u32(fixed_link_node, "speed", > + &status.speed) != 0) { > + /* in auto mode just set to some sane value: > + * it will be changed by MAC later */ > + if (link_auto) > + status.speed = 1000; This is a completely arbitrary speed, that does not more or less sense than defaulting to 100 or anything else, a driver should be able to set the speed it wants, based on the parsing of a 'phy-mode' property for instance. 1000 does not make sense on e.g: MII links.
09.07.2015 21:24, Florian Fainelli пишет: > (there is no such thing as linux-net@vger.kernel.org, please remove it > from your future submissions). > > On 09/07/15 10:38, Stas Sergeev wrote: >> Currently for fixed-link the link state is always set to UP. > Not quite true, this is always a driver decision to make. But what about this part of of_mdio.c:of_phy_register_fixed_link(): --- fixed_link_node = of_get_child_by_name(np, "fixed-link"); if (fixed_link_node) { status.link = 1 --- >> This patch introduces the new property 'link' that accepts the >> following string arguments: "up", "down" and "auto". >> "down" may be needed if the link is physically unconnected. > In which case you probably do not even care about inserting such a > property in the first place, do you? What would be the value of forcibly > having a link permanently down (not counting loopback)? The DTs have a common parts that are included by other parts. So if you include the definition of your SoC that have all ethernets defined, and you only set up the external things like PHYs, then I would see a potential use for "down". Other than that, it is probably not a big deal. Please note that I haven't even hard-coded it anywhere: whatever is not "up" or "auto", is down. I can remove it from the description if you think that way, but I'd rather leave it for consistency and for a small but possible use. Eg my board has 4 ethernets and only 2 are connected. I feel its right to include the SoC definition and set the unconnected ones to "down", but other approaches are possible too. Should I remove it? >> "auto" is needed to enable the link paramaters auto-negotiation, >> that is built into some MII protocols, namely SGMII. > RGMII also has an in-band status FWIW. Thanks, will take that into account in v2. >> The appropriate documentation is added and explicitly states that >> "auto" is very specific (protocol, HW and driver-specific), and >> is therefore should be used with care. > And therefore probably be made a device (and driver) specific decision > whether this is the right thing to do. This doesn't work. It appears even if the driver supports it and wants to use it, the PHY HW may simply not generate the inband status. This is actually the whole point why we have a regression now. It is _currently_ a driver decision, and that doesn't work for some people. The point of this patch set is to make it a DT decision instead. >> - return -EINVAL; >> + if (of_property_read_u32(fixed_link_node, "speed", >> + &status.speed) != 0) { >> + /* in auto mode just set to some sane value: >> + * it will be changed by MAC later */ >> + if (link_auto) >> + status.speed = 1000; > This is a completely arbitrary speed, that does not more or less sense > than defaulting to 100 or anything else, Exactly. But if I leave it to 0, then fixed-phy driver will return an error, so I took an arbitrary value. But if it obscures the code, I'll hack fixed-phy to accept 0 instead, to get something cleaner. So in v2. > a driver should be able to set > the speed it wants, based on the parsing of a 'phy-mode' property for > instance. It actually does, that value is just to "cheat" fixed-phy. I'll make things more obvious next time. -- 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 09/07/15 13:43, Stas Sergeev wrote: > 09.07.2015 21:24, Florian Fainelli пишет: >> (there is no such thing as linux-net@vger.kernel.org, please remove it >> from your future submissions). >> >> On 09/07/15 10:38, Stas Sergeev wrote: >>> Currently for fixed-link the link state is always set to UP. >> Not quite true, this is always a driver decision to make. > But what about this part of of_mdio.c:of_phy_register_fixed_link(): > --- > > fixed_link_node = of_get_child_by_name(np, "fixed-link"); > if (fixed_link_node) { > status.link = 1 > > --- This seems like a logical consequence of finding a "fixed-link" property for the DT node of interest. If no such property exist, then we do not set anything. > >>> This patch introduces the new property 'link' that accepts the >>> following string arguments: "up", "down" and "auto". >>> "down" may be needed if the link is physically unconnected. >> In which case you probably do not even care about inserting such a >> property in the first place, do you? What would be the value of forcibly >> having a link permanently down (not counting loopback)? > The DTs have a common parts that are included by other > parts. So if you include the definition of your SoC that have > all ethernets defined, and you only set up the external things > like PHYs, then I would see a potential use for "down". "down" is equivalent to using a status = "disabled", in fact the latter is much better since you can even conserve energy and resources by not enabling something which is not usable. > Other than that, it is probably not a big deal. > Please note that I haven't even hard-coded it anywhere: > whatever is not "up" or "auto", is down. > I can remove it from the description if you think that way, > but I'd rather leave it for consistency and for a small but > possible use. Eg my board has 4 ethernets and only 2 are > connected. I feel its right to include the SoC definition and > set the unconnected ones to "down", but other approaches > are possible too. > Should I remove it? What you describe about your board is the perfect example of how a "status" property should be used. > >>> "auto" is needed to enable the link paramaters auto-negotiation, >>> that is built into some MII protocols, namely SGMII. >> RGMII also has an in-band status FWIW. > Thanks, will take that into account in v2. > >>> The appropriate documentation is added and explicitly states that >>> "auto" is very specific (protocol, HW and driver-specific), and >>> is therefore should be used with care. >> And therefore probably be made a device (and driver) specific decision >> whether this is the right thing to do. > This doesn't work. > It appears even if the driver supports it and wants to use it, the > PHY HW may simply not generate the inband status. This is actually > the whole point why we have a regression now. It is _currently_ > a driver decision, and that doesn't work for some people. > The point of this patch set is to make it a DT decision instead. Then, if the in-band status indication is not reliable (which really should be completely understood), you can just ignore the in-band status and use all the parameter in a 'fixed-link' property, should not we? If in-band status can be used, then you can decide this with a separate property which is not in 'fixed-link', would that seem reasonable? > >>> - return -EINVAL; >>> + if (of_property_read_u32(fixed_link_node, "speed", >>> + &status.speed) != 0) { >>> + /* in auto mode just set to some sane value: >>> + * it will be changed by MAC later */ >>> + if (link_auto) >>> + status.speed = 1000; >> This is a completely arbitrary speed, that does not more or less sense >> than defaulting to 100 or anything else, > Exactly. > But if I leave it to 0, then fixed-phy driver will return an error, > so I took an arbitrary value. > But if it obscures the code, I'll hack fixed-phy to accept 0 instead, > to get something cleaner. So in v2. > >> a driver should be able to set >> the speed it wants, based on the parsing of a 'phy-mode' property for >> instance. > It actually does, that value is just to "cheat" fixed-phy. > I'll make things more obvious next time.
10.07.2015 00:15, Florian Fainelli пишет: > On 09/07/15 13:43, Stas Sergeev wrote: >> 09.07.2015 21:24, Florian Fainelli пишет: >>> (there is no such thing as linux-net@vger.kernel.org, please remove it >>> from your future submissions). >>> >>> On 09/07/15 10:38, Stas Sergeev wrote: >>>> Currently for fixed-link the link state is always set to UP. >>> Not quite true, this is always a driver decision to make. >> But what about this part of of_mdio.c:of_phy_register_fixed_link(): >> --- >> >> fixed_link_node = of_get_child_by_name(np, "fixed-link"); >> if (fixed_link_node) { >> status.link = 1 >> >> --- > This seems like a logical consequence of finding a "fixed-link" property > for the DT node of interest. If no such property exist, then we do not > set anything. > >>>> This patch introduces the new property 'link' that accepts the >>>> following string arguments: "up", "down" and "auto". >>>> "down" may be needed if the link is physically unconnected. >>> In which case you probably do not even care about inserting such a >>> property in the first place, do you? What would be the value of forcibly >>> having a link permanently down (not counting loopback)? >> The DTs have a common parts that are included by other >> parts. So if you include the definition of your SoC that have >> all ethernets defined, and you only set up the external things >> like PHYs, then I would see a potential use for "down". > "down" is equivalent to using a status = "disabled", in fact the latter > is much better since you can even conserve energy and resources by not > enabling something which is not usable. OK, agree. So I'll probably go for something like autoneg = 1 | 0; >> This doesn't work. >> It appears even if the driver supports it and wants to use it, the >> PHY HW may simply not generate the inband status. This is actually >> the whole point why we have a regression now. It is _currently_ >> a driver decision, and that doesn't work for some people. >> The point of this patch set is to make it a DT decision instead. > Then, if the in-band status indication is not reliable (which really > should be completely understood), Agree! But this is not something I can help with. Sebastien Rannou reports the problem, please ask him whatever you see fits to get a better understanding of a problem. The fact that his HW does not generate the inband status, is _my own guess_. > you can just ignore the in-band status > and use all the parameter in a 'fixed-link' property, should not we? I don't think there is any way at all to find out if the inband stat is usable or not. I think only the user (or manufacturer) can decide on that. If there is any way to do a guess-work, that would be an entirely different story. -- 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, 10 Jul 2015, Stas Sergeev wrote: > 10.07.2015 00:15, Florian Fainelli пишет: > > Then, if the in-band status indication is not reliable (which really > > should be completely understood), > Agree! > But this is not something I can help with. > Sebastien Rannou reports the problem, please ask him whatever > you see fits to get a better understanding of a problem. > The fact that his HW does not generate the inband status, is > _my own guess_. Yes, I confirm that my HW does not generate an in-band status. AFAIK, it's a PHY that aggregates 4xSGMIIs to 1xQSGMII ; the MAC side of the PHY (with inband status) is connected to the switch through QSGMII, and in this context we are on the media side of the PHY.
10.07.2015 11:46, Sebastien Rannou пишет: > On Fri, 10 Jul 2015, Stas Sergeev wrote: > >> 10.07.2015 00:15, Florian Fainelli пишет: >>> Then, if the in-band status indication is not reliable (which really >>> should be completely understood), >> Agree! >> But this is not something I can help with. >> Sebastien Rannou reports the problem, please ask him whatever >> you see fits to get a better understanding of a problem. >> The fact that his HW does not generate the inband status, is >> _my own guess_. > > Yes, I confirm that my HW does not generate an in-band status. AFAIK, it's > a PHY that aggregates 4xSGMIIs to 1xQSGMII ; the MAC side of the PHY (with > inband status) is connected to the switch through QSGMII, and in this context > we are on the media side of the PHY. Hmm, interesting. So if I parse the above correctly, you have something like 88E1340S set up into a mode when SGMII is used as media interface and QSGMII as system interface (terms are from datasheet page 5), then you connect the media interface to armada-xp and system interface to the switch. I wonder if it is the right thing to do. AFAIK you could as well set up armada-xp into QSGMII mode and connect that to switch. The driver would then disable the use of inband status and everything would be fine. Either way, your use-case proves that only DT can decide the use of an inband status. -- 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 10/07/15 04:20, Stas Sergeev wrote: > 10.07.2015 11:46, Sebastien Rannou пишет: >> On Fri, 10 Jul 2015, Stas Sergeev wrote: >> >>> 10.07.2015 00:15, Florian Fainelli пишет: >>>> Then, if the in-band status indication is not reliable (which really >>>> should be completely understood), >>> Agree! >>> But this is not something I can help with. >>> Sebastien Rannou reports the problem, please ask him whatever >>> you see fits to get a better understanding of a problem. >>> The fact that his HW does not generate the inband status, is >>> _my own guess_. >> >> Yes, I confirm that my HW does not generate an in-band status. AFAIK, it's >> a PHY that aggregates 4xSGMIIs to 1xQSGMII ; the MAC side of the PHY (with >> inband status) is connected to the switch through QSGMII, and in this context >> we are on the media side of the PHY. > Hmm, interesting. > So if I parse the above correctly, you have something like 88E1340S set > up into a mode when SGMII is used as media interface and QSGMII as system > interface (terms are from datasheet page 5), then you connect the media > interface to armada-xp and system interface to the switch. > > I wonder if it is the right thing to do. > AFAIK you could as well set up armada-xp into QSGMII mode and connect > that to switch. The driver would then disable the use of inband status > and everything would be fine. > Either way, your use-case proves that only DT can decide the use of an > inband status. I do not think there is any debate around the need for a property that defines whether in-band-status is both reliable and usable, the debate is about *where* to put it. I still think this does not belong in the fixed-link property, but now that you have explained a bit more in the other patch what your understanding of "fixed-link" is, I can see the confusion. Instead of having a link = "auto", property, how about just something like this: fixed-link { speed = <1000>; full-duplex; use-in-band-status; }; or event this: fixed-link { use-in-band-status; }; If you parse the 'use-in-band-status' which means that it is reliable information, then you can override whatever was defined in the DT under the 'fixed-link' property?
diff --git a/Documentation/devicetree/bindings/net/fixed-link.txt b/Documentation/devicetree/bindings/net/fixed-link.txt index 82bf7e0..070f554 100644 --- a/Documentation/devicetree/bindings/net/fixed-link.txt +++ b/Documentation/devicetree/bindings/net/fixed-link.txt @@ -9,8 +9,14 @@ Such a fixed link situation is described by creating a 'fixed-link' sub-node of the Ethernet MAC device node, with the following properties: +* 'link' (string, optional), to indicate the link state. Accepted + values are "up", "down" and "auto". "auto" means auto-negotiation of + link parameters. Auto-negotiation is MII protocol, HW and driver-specific + and is not supported in many cases, so use it only when you know what + you do. * 'speed' (integer, mandatory), to indicate the link speed. Accepted - values are 10, 100 and 1000 + values are 10, 100 and 1000. If the 'link' property is set to 'auto', + 'speed' may not be set. It will then be auto-negotiated, if possible. * '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 diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c index 1bd4305..2152cf8 100644 --- a/drivers/of/of_mdio.c +++ b/drivers/of/of_mdio.c @@ -280,6 +280,26 @@ bool of_phy_is_fixed_link(struct device_node *np) } EXPORT_SYMBOL(of_phy_is_fixed_link); +bool of_phy_is_autoneg_link(struct device_node *np) +{ + struct device_node *dn; + const char *link_str; + int rc; + bool ret = false; + + dn = of_get_child_by_name(np, "fixed-link"); + if (!dn) + return false; + + rc = of_property_read_string(dn, "link", &link_str); + if (rc == 0 && strcmp(link_str, "auto") == 0) + ret = true; + + of_node_put(dn); + return ret; +} +EXPORT_SYMBOL(of_phy_is_autoneg_link); + int of_phy_register_fixed_link(struct device_node *np) { struct fixed_phy_status status = {}; @@ -291,11 +311,33 @@ int of_phy_register_fixed_link(struct device_node *np) /* New binding */ fixed_link_node = of_get_child_by_name(np, "fixed-link"); if (fixed_link_node) { - status.link = 1; + const char *link_str; + int ret; + bool link_auto = false; + + ret = of_property_read_string(fixed_link_node, "link", + &link_str); + if (ret == 0) { + if (strcmp(link_str, "up") == 0) + status.link = 1; + else + status.link = 0; + if (strcmp(link_str, "auto") == 0) + link_auto = true; + } else { + status.link = 1; + } status.duplex = of_property_read_bool(fixed_link_node, "full-duplex"); - if (of_property_read_u32(fixed_link_node, "speed", &status.speed)) - return -EINVAL; + if (of_property_read_u32(fixed_link_node, "speed", + &status.speed) != 0) { + /* in auto mode just set to some sane value: + * it will be changed by MAC later */ + if (link_auto) + status.speed = 1000; + else + return -EINVAL; + } status.pause = of_property_read_bool(fixed_link_node, "pause"); status.asym_pause = of_property_read_bool(fixed_link_node, "asym-pause"); diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h index d449018..647f348 100644 --- a/include/linux/of_mdio.h +++ b/include/linux/of_mdio.h @@ -65,6 +65,7 @@ static inline struct mii_bus *of_mdio_find_bus(struct device_node *mdio_np) #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); +extern bool of_phy_is_autoneg_link(struct device_node *np); #else static inline int of_phy_register_fixed_link(struct device_node *np) { @@ -74,6 +75,10 @@ static inline bool of_phy_is_fixed_link(struct device_node *np) { return false; } +static inline bool of_phy_is_autoneg_link(struct device_node *np) +{ + return false; +} #endif
Currently for fixed-link the link state is always set to UP. This patch introduces the new property 'link' that accepts the following string arguments: "up", "down" and "auto". "down" may be needed if the link is physically unconnected. "auto" is needed to enable the link paramaters auto-negotiation, that is built into some MII protocols, namely SGMII. The appropriate documentation is added and explicitly states that "auto" is very specific (protocol, HW and driver-specific), and is therefore should be used with care. Signed-off-by: Stas Sergeev <stsp@users.sourceforge.net> CC: Rob Herring <robh+dt@kernel.org> CC: Pawel Moll <pawel.moll@arm.com> CC: Mark Rutland <mark.rutland@arm.com> CC: Ian Campbell <ijc+devicetree@hellion.org.uk> CC: Kumar Gala <galak@codeaurora.org> CC: Florian Fainelli <f.fainelli@gmail.com> CC: Grant Likely <grant.likely@linaro.org> CC: devicetree@vger.kernel.org CC: linux-kernel@vger.kernel.org CC: netdev@vger.kernel.org --- .../devicetree/bindings/net/fixed-link.txt | 8 +++- drivers/of/of_mdio.c | 48 ++++++++++++++++++++-- include/linux/of_mdio.h | 5 +++ 3 files changed, 57 insertions(+), 4 deletions(-)