* [PATCH v3 0/3] net: enable inband link state negotiation only when explicitly requested @ 2015-07-14 17:09 Stas Sergeev 2015-07-14 17:11 ` [PATCH 1/3] fixed_phy: handle link-down case Stas Sergeev ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Stas Sergeev @ 2015-07-14 17:09 UTC (permalink / raw) To: netdev Cc: Linux kernel, Sebastien Rannou, Arnaud Ebalard, Stas Sergeev, Florian Fainelli Hello. Currently the link status auto-negotiation is enabled for any SGMII link with fixed-link DT binding. The regression was reported: https://lkml.org/lkml/2015/7/8/865 Apparently not all HW that implements SGMII protocol, generates the inband status for the auto-negotiation to work. More details here: https://lkml.org/lkml/2015/7/10/206 The following patches reverts to the old behavior by default, which is to not enable the auto-negotiation for fixed-link. The new DT property is added that allows to explicitly request the auto-negotiation. Those who were affected by the change, please send your Tested-by, Thanks! ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/3] fixed_phy: handle link-down case 2015-07-14 17:09 [PATCH v3 0/3] net: enable inband link state negotiation only when explicitly requested Stas Sergeev @ 2015-07-14 17:11 ` Stas Sergeev 2015-07-14 18:28 ` Florian Fainelli 2015-07-14 17:13 ` Stas Sergeev 2015-07-14 17:14 ` [PATCH 3/3] mvneta: use inband status only when explicitly enabled Stas Sergeev 2 siblings, 1 reply; 12+ messages in thread From: Stas Sergeev @ 2015-07-14 17:11 UTC (permalink / raw) To: netdev Cc: Linux kernel, Sebastien Rannou, Arnaud Ebalard, Stas Sergeev, Florian Fainelli Currently fixed_phy driver recognizes only the link-up state. This simple patch adds an implementation of link-down state. It fixes the status registers when link is down, and also allows to register the fixed-phy with link down without specifying the speed. Signed-off-by: Stas Sergeev <stsp@users.sourceforge.net> CC: Florian Fainelli <f.fainelli@gmail.com> CC: netdev@vger.kernel.org CC: linux-kernel@vger.kernel.org --- drivers/net/phy/fixed_phy.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c index 1960b46..479b93f 100644 --- a/drivers/net/phy/fixed_phy.c +++ b/drivers/net/phy/fixed_phy.c @@ -52,6 +52,10 @@ static int fixed_phy_update_regs(struct fixed_phy *fp) u16 lpagb = 0; u16 lpa = 0; + if (!fp->status.link) + goto done; + bmsr |= BMSR_LSTATUS | BMSR_ANEGCOMPLETE; + if (fp->status.duplex) { bmcr |= BMCR_FULLDPLX; @@ -96,15 +100,13 @@ static int fixed_phy_update_regs(struct fixed_phy *fp) } } - if (fp->status.link) - bmsr |= BMSR_LSTATUS | BMSR_ANEGCOMPLETE; - if (fp->status.pause) lpa |= LPA_PAUSE_CAP; if (fp->status.asym_pause) lpa |= LPA_PAUSE_ASYM; +done: fp->regs[MII_PHYSID1] = 0; fp->regs[MII_PHYSID2] = 0; -- 1.9.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3] fixed_phy: handle link-down case 2015-07-14 17:11 ` [PATCH 1/3] fixed_phy: handle link-down case Stas Sergeev @ 2015-07-14 18:28 ` Florian Fainelli [not found] ` <55A56D4D.5090004@list.ru> 0 siblings, 1 reply; 12+ messages in thread From: Florian Fainelli @ 2015-07-14 18:28 UTC (permalink / raw) To: Stas Sergeev, netdev Cc: Linux kernel, Sebastien Rannou, Arnaud Ebalard, Stas Sergeev On 14/07/15 10:11, Stas Sergeev wrote: > > Currently fixed_phy driver recognizes only the link-up state. > This simple patch adds an implementation of link-down state. > It fixes the status registers when link is down, and also allows > to register the fixed-phy with link down without specifying the speed. > > Signed-off-by: Stas Sergeev <stsp@users.sourceforge.net> This does not quite seem to work for me here on two different setups that use fixed PHYs: Before patch link up: # ethtool moca Settings for moca: Supported ports: [ TP AUI BNC MII FIBRE ] Supported link modes: 1000baseT/Half 1000baseT/Full Supported pause frame use: No Supports auto-negotiation: Yes Advertised link modes: 1000baseT/Half 1000baseT/Full Advertised pause frame use: No Advertised auto-negotiation: Yes Link partner advertised link modes: 1000baseT/Full Link partner advertised pause frame use: No Link partner advertised auto-negotiation: No Speed: 1000Mb/s Duplex: Full Port: BNC PHYAD: 2 Transceiver: external Auto-negotiation: on Supports Wake-on: gs Wake-on: d SecureOn password: 00:00:00:00:00:00 Link detected: yes # Before patch link down: # ethtool moca Settings for moca: Supported ports: [ TP AUI BNC MII FIBRE ] Supported link modes: 1000baseT/Half 1000baseT/Full Supported pause frame use: No Supports auto-negotiation: Yes Advertised link modes: 1000baseT/Half 1000baseT/Full Advertised pause frame use: No Advertised auto-negotiation: No Speed: 1000Mb/s Duplex: Full Port: BNC PHYAD: 2 Transceiver: external Auto-negotiation: off Supports Wake-on: gs Wake-on: d SecureOn password: 00:00:00:00:00:00 Link detected: no # After patch link up: # ethtool moca Settings for moca: Supported ports: [ TP AUI BNC MII FIBRE ] Supported link modes: 1000baseT/Half 1000baseT/Full Supported pause frame use: No Supports auto-negotiation: Yes Advertised link modes: 1000baseT/Half 1000baseT/Full Advertised pause frame use: No Advertised auto-negotiation: Yes Link partner advertised link modes: 10baseT/Full Link partner advertised pause frame use: No Link partner advertised auto-negotiation: No Speed: 10Mb/s <---- this is not quite the speed we want Duplex: Full Port: BNC PHYAD: 2 Transceiver: external Auto-negotiation: on Supports Wake-on: gs Wake-on: d SecureOn password: 00:00:00:00:00:00 Link detected: yes # After patch link down: # ethtool moca Settings for moca: Supported ports: [ TP AUI BNC MII FIBRE ] Supported link modes: 1000baseT/Half 1000baseT/Full Supported pause frame use: No Supports auto-negotiation: Yes Advertised link modes: 1000baseT/Half 1000baseT/Full Advertised pause frame use: No Advertised auto-negotiation: No Speed: 10Mb/s Duplex: Half Port: BNC PHYAD: 2 Transceiver: external Auto-negotiation: off Supports Wake-on: gs Wake-on: d SecureOn password: 00:00:00:00:00:00 Link detected: no # Does it behave properly for you? > > CC: Florian Fainelli <f.fainelli@gmail.com> > CC: netdev@vger.kernel.org > CC: linux-kernel@vger.kernel.org > --- > drivers/net/phy/fixed_phy.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c > index 1960b46..479b93f 100644 > --- a/drivers/net/phy/fixed_phy.c > +++ b/drivers/net/phy/fixed_phy.c > @@ -52,6 +52,10 @@ static int fixed_phy_update_regs(struct fixed_phy *fp) > u16 lpagb = 0; > u16 lpa = 0; > > + if (!fp->status.link) > + goto done; > + bmsr |= BMSR_LSTATUS | BMSR_ANEGCOMPLETE; > + > if (fp->status.duplex) { > bmcr |= BMCR_FULLDPLX; > > @@ -96,15 +100,13 @@ static int fixed_phy_update_regs(struct fixed_phy *fp) > } > } > > - if (fp->status.link) > - bmsr |= BMSR_LSTATUS | BMSR_ANEGCOMPLETE; > - > if (fp->status.pause) > lpa |= LPA_PAUSE_CAP; > > if (fp->status.asym_pause) > lpa |= LPA_PAUSE_ASYM; > > +done: > fp->regs[MII_PHYSID1] = 0; > fp->regs[MII_PHYSID2] = 0; > -- Florian ^ permalink raw reply [flat|nested] 12+ messages in thread
[parent not found: <55A56D4D.5090004@list.ru>]
* Re: Fwd: Re: [PATCH 1/3] fixed_phy: handle link-down case [not found] ` <55A56D4D.5090004@list.ru> @ 2015-07-15 15:33 ` Stas Sergeev 0 siblings, 0 replies; 12+ messages in thread From: Stas Sergeev @ 2015-07-15 15:33 UTC (permalink / raw) To: Florian Fainelli Cc: Linux kernel, Sebastien Rannou, Arnaud Ebalard, Stas Sergeev Florian Fainelli <f.fainelli@gmail.com> : > > Does it behave properly for you? Yes, I've just checked and can't reproduce the problem you mentioned. And I can't think of the possible reason: fixed_phy.c keeps the private copy of the status, which should have link speed kept unchanged, and used as long as link is up again. Could you please help with this? ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/3] of_mdio: add new DT property 'managed' to specify the PHY management type 2015-07-14 17:09 [PATCH v3 0/3] net: enable inband link state negotiation only when explicitly requested Stas Sergeev @ 2015-07-14 17:13 ` Stas Sergeev 2015-07-14 17:13 ` Stas Sergeev 2015-07-14 17:14 ` [PATCH 3/3] mvneta: use inband status only when explicitly enabled Stas Sergeev 2 siblings, 0 replies; 12+ messages in thread From: Stas Sergeev @ 2015-07-14 17:13 UTC (permalink / raw) To: netdev Cc: Linux kernel, Sebastien Rannou, Arnaud Ebalard, Stas Sergeev, Florian Fainelli, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Florian Fainelli, Grant Likely, devicetree Currently the PHY management type is selected by the MAC driver arbitrary. The decision is based on the presence of the "fixed-link" node and on a will of the driver's authors. This caused a regression recently, when mvneta driver suddenly started to use the in-band status for auto-negotiation on fixed links. It appears the auto-negotiation may not work when expected by the MAC driver. Sebastien Rannou explains: << 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. >> https://lkml.org/lkml/2015/7/10/206 This patch introduces the new string property 'managed' that allows the user to set the management type explicitly. The supported values are: "auto" - default. Uses either MDIO or nothing, depending on the presence of the fixed-link node "mdio" - use MDIO "in-band-status" - use in-band status "none" - use fixed-link description 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 --- Documentation/devicetree/bindings/net/ethernet.txt | 5 ++++ drivers/of/of_mdio.c | 28 ++++++++++++++++++++-- 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/net/ethernet.txt b/Documentation/devicetree/bindings/net/ethernet.txt index 3fc3605..23743e9 100644 --- a/Documentation/devicetree/bindings/net/ethernet.txt +++ b/Documentation/devicetree/bindings/net/ethernet.txt @@ -19,7 +19,12 @@ The following properties are common to the Ethernet controllers: - phy: the same as "phy-handle" property, not recommended for new bindings. - phy-device: the same as "phy-handle" property, not recommended for new bindings. +- managed: string, specifies the PHY management type. Supported values are: + "auto", "mdio", "in-band-status", "none". "auto" is the default, and it + sets the management type to either "mdio" or "none", depending on the + presence of the "fixed-link" child node. Child nodes of the Ethernet controller are typically the individual PHY devices connected via the MDIO bus (sometimes the MDIO bus controller is separate). They are described in the phy.txt file in this same directory. +For non-MDIO PHY management see fixed-link.txt. diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c index 1bd4305..a460812 100644 --- a/drivers/of/of_mdio.c +++ b/drivers/of/of_mdio.c @@ -262,7 +262,16 @@ EXPORT_SYMBOL(of_phy_attach); bool of_phy_is_fixed_link(struct device_node *np) { struct device_node *dn; - int len; + int len, m_err; + const char *managed; + + m_err = of_property_read_string(np, "managed", &managed); + if (m_err == 0) { + if (strcmp(managed, "none") == 0) + return true; + if (strcmp(managed, "mdio") == 0) + return false; + } /* New binding */ dn = of_get_child_by_name(np, "fixed-link"); @@ -271,6 +280,9 @@ bool of_phy_is_fixed_link(struct device_node *np) return true; } + if (m_err == 0 && strcmp(managed, "auto") != 0) + return true; + /* Old binding */ if (of_get_property(np, "fixed-link", &len) && len == (5 * sizeof(__be32))) @@ -285,8 +297,20 @@ int of_phy_register_fixed_link(struct device_node *np) struct fixed_phy_status status = {}; struct device_node *fixed_link_node; const __be32 *fixed_link_prop; - int len; + int len, err; struct phy_device *phy; + const char *managed; + + err = of_property_read_string(np, "managed", &managed); + if (err == 0) { + if (strcmp(managed, "mdio") == 0) + return -EINVAL; + if (strcmp(managed, "in-band-status") == 0) { + status.link = 0; + phy = fixed_phy_register(PHY_POLL, &status, np); + return IS_ERR(phy) ? PTR_ERR(phy) : 0; + } + } /* New binding */ fixed_link_node = of_get_child_by_name(np, "fixed-link"); -- 1.9.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/3] of_mdio: add new DT property 'managed' to specify the PHY management type @ 2015-07-14 17:13 ` Stas Sergeev 0 siblings, 0 replies; 12+ messages in thread From: Stas Sergeev @ 2015-07-14 17:13 UTC (permalink / raw) To: netdev Cc: Linux kernel, Sebastien Rannou, Arnaud Ebalard, Stas Sergeev, Florian Fainelli, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala Currently the PHY management type is selected by the MAC driver arbitrary. The decision is based on the presence of the "fixed-link" node and on a will of the driver's authors. This caused a regression recently, when mvneta driver suddenly started to use the in-band status for auto-negotiation on fixed links. It appears the auto-negotiation may not work when expected by the MAC driver. Sebastien Rannou explains: << 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. >> https://lkml.org/lkml/2015/7/10/206 This patch introduces the new string property 'managed' that allows the user to set the management type explicitly. The supported values are: "auto" - default. Uses either MDIO or nothing, depending on the presence of the fixed-link node "mdio" - use MDIO "in-band-status" - use in-band status "none" - use fixed-link description 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 --- Documentation/devicetree/bindings/net/ethernet.txt | 5 ++++ drivers/of/of_mdio.c | 28 ++++++++++++++++++++-- 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/Documentation/devicetree/bindings/net/ethernet.txt b/Documentation/devicetree/bindings/net/ethernet.txt index 3fc3605..23743e9 100644 --- a/Documentation/devicetree/bindings/net/ethernet.txt +++ b/Documentation/devicetree/bindings/net/ethernet.txt @@ -19,7 +19,12 @@ The following properties are common to the Ethernet controllers: - phy: the same as "phy-handle" property, not recommended for new bindings. - phy-device: the same as "phy-handle" property, not recommended for new bindings. +- managed: string, specifies the PHY management type. Supported values are: + "auto", "mdio", "in-band-status", "none". "auto" is the default, and it + sets the management type to either "mdio" or "none", depending on the + presence of the "fixed-link" child node. Child nodes of the Ethernet controller are typically the individual PHY devices connected via the MDIO bus (sometimes the MDIO bus controller is separate). They are described in the phy.txt file in this same directory. +For non-MDIO PHY management see fixed-link.txt. diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c index 1bd4305..a460812 100644 --- a/drivers/of/of_mdio.c +++ b/drivers/of/of_mdio.c @@ -262,7 +262,16 @@ EXPORT_SYMBOL(of_phy_attach); bool of_phy_is_fixed_link(struct device_node *np) { struct device_node *dn; - int len; + int len, m_err; + const char *managed; + + m_err = of_property_read_string(np, "managed", &managed); + if (m_err == 0) { + if (strcmp(managed, "none") == 0) + return true; + if (strcmp(managed, "mdio") == 0) + return false; + } /* New binding */ dn = of_get_child_by_name(np, "fixed-link"); @@ -271,6 +280,9 @@ bool of_phy_is_fixed_link(struct device_node *np) return true; } + if (m_err == 0 && strcmp(managed, "auto") != 0) + return true; + /* Old binding */ if (of_get_property(np, "fixed-link", &len) && len == (5 * sizeof(__be32))) @@ -285,8 +297,20 @@ int of_phy_register_fixed_link(struct device_node *np) struct fixed_phy_status status = {}; struct device_node *fixed_link_node; const __be32 *fixed_link_prop; - int len; + int len, err; struct phy_device *phy; + const char *managed; + + err = of_property_read_string(np, "managed", &managed); + if (err == 0) { + if (strcmp(managed, "mdio") == 0) + return -EINVAL; + if (strcmp(managed, "in-band-status") == 0) { + status.link = 0; + phy = fixed_phy_register(PHY_POLL, &status, np); + return IS_ERR(phy) ? PTR_ERR(phy) : 0; + } + } /* New binding */ fixed_link_node = of_get_child_by_name(np, "fixed-link"); -- 1.9.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] of_mdio: add new DT property 'managed' to specify the PHY management type @ 2015-07-14 17:51 ` Florian Fainelli 0 siblings, 0 replies; 12+ messages in thread From: Florian Fainelli @ 2015-07-14 17:51 UTC (permalink / raw) To: Stas Sergeev, netdev Cc: Linux kernel, Sebastien Rannou, Arnaud Ebalard, Stas Sergeev, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Grant Likely, devicetree On 14/07/15 10:13, Stas Sergeev wrote: > > Currently the PHY management type is selected by the MAC driver arbitrary. > The decision is based on the presence of the "fixed-link" node and on a > will of the driver's authors. > This caused a regression recently, when mvneta driver suddenly started > to use the in-band status for auto-negotiation on fixed links. > It appears the auto-negotiation may not work when expected by the MAC driver. > Sebastien Rannou explains: > << 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. >> > https://lkml.org/lkml/2015/7/10/206 > > This patch introduces the new string property 'managed' that allows > the user to set the management type explicitly. > The supported values are: > "auto" - default. Uses either MDIO or nothing, depending on the presence > of the fixed-link node > "mdio" - use MDIO > "in-band-status" - use in-band status > "none" - use fixed-link description I thought we agreed in the last thread that "mdio" was implied whenever a proper PHY phandle to a device sitting on MDIO bus is used and that "auto" did not make much sense unless we were also describing how to do this auto-negotiation completely? The way I see it, the only thing that is needed at this point is an "in-band-status" property which you rightfully placed at the Ethernet MAC DT node level, this is fine, however, I disagree with how the values are enforced, see 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 > --- > Documentation/devicetree/bindings/net/ethernet.txt | 5 ++++ > drivers/of/of_mdio.c | 28 ++++++++++++++++++++-- > 2 files changed, 31 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/net/ethernet.txt b/Documentation/devicetree/bindings/net/ethernet.txt > index 3fc3605..23743e9 100644 > --- a/Documentation/devicetree/bindings/net/ethernet.txt > +++ b/Documentation/devicetree/bindings/net/ethernet.txt > @@ -19,7 +19,12 @@ The following properties are common to the Ethernet controllers: > - phy: the same as "phy-handle" property, not recommended for new bindings. > - phy-device: the same as "phy-handle" property, not recommended for new > bindings. > +- managed: string, specifies the PHY management type. Supported values are: > + "auto", "mdio", "in-band-status", "none". "auto" is the default, and it > + sets the management type to either "mdio" or "none", depending on the > + presence of the "fixed-link" child node. As mentioned below, "mdio" is redundant with finding a "phy-handle", and "auto" is not specific enough to be useful to a consumer of this information. > > Child nodes of the Ethernet controller are typically the individual PHY devices > connected via the MDIO bus (sometimes the MDIO bus controller is separate). > They are described in the phy.txt file in this same directory. > +For non-MDIO PHY management see fixed-link.txt. > diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c > index 1bd4305..a460812 100644 > --- a/drivers/of/of_mdio.c > +++ b/drivers/of/of_mdio.c > @@ -262,7 +262,16 @@ EXPORT_SYMBOL(of_phy_attach); > bool of_phy_is_fixed_link(struct device_node *np) > { > struct device_node *dn; > - int len; > + int len, m_err; > + const char *managed; > + > + m_err = of_property_read_string(np, "managed", &managed); > + if (m_err == 0) { > + if (strcmp(managed, "none") == 0) > + return true; > + if (strcmp(managed, "mdio") == 0) > + return false; > + } managed = "mdio" on a fixed link is by definition not something remotely correct, there should be a proper PHY driver for this, and therefore a different representation: a PHY phandle to a PHY node on a MDIO bus. I do not think enforcing this has been incorrect provides much value, since you ought to write correct DT in the first place. > > /* New binding */ > dn = of_get_child_by_name(np, "fixed-link"); > @@ -271,6 +280,9 @@ bool of_phy_is_fixed_link(struct device_node *np) > return true; > } > > + if (m_err == 0 && strcmp(managed, "auto") != 0) > + return true; > + > /* Old binding */ > if (of_get_property(np, "fixed-link", &len) && > len == (5 * sizeof(__be32))) > @@ -285,8 +297,20 @@ int of_phy_register_fixed_link(struct device_node *np) > struct fixed_phy_status status = {}; > struct device_node *fixed_link_node; > const __be32 *fixed_link_prop; > - int len; > + int len, err; > struct phy_device *phy; > + const char *managed; > + > + err = of_property_read_string(np, "managed", &managed); > + if (err == 0) { > + if (strcmp(managed, "mdio") == 0) > + return -EINVAL; > + if (strcmp(managed, "in-band-status") == 0) { > + status.link = 0; > + phy = fixed_phy_register(PHY_POLL, &status, np); > + return IS_ERR(phy) ? PTR_ERR(phy) : 0; > + } And that is the only hunk of this patch that is really needed and useful to solving the problem here. -- Florian ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] of_mdio: add new DT property 'managed' to specify the PHY management type @ 2015-07-14 17:51 ` Florian Fainelli 0 siblings, 0 replies; 12+ messages in thread From: Florian Fainelli @ 2015-07-14 17:51 UTC (permalink / raw) To: Stas Sergeev, netdev Cc: Linux kernel, Sebastien Rannou, Arnaud Ebalard, Stas Sergeev, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Grant Likely, devicetree-u79uwXL29TY76Z2rM5mHXA On 14/07/15 10:13, Stas Sergeev wrote: > > Currently the PHY management type is selected by the MAC driver arbitrary. > The decision is based on the presence of the "fixed-link" node and on a > will of the driver's authors. > This caused a regression recently, when mvneta driver suddenly started > to use the in-band status for auto-negotiation on fixed links. > It appears the auto-negotiation may not work when expected by the MAC driver. > Sebastien Rannou explains: > << 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. >> > https://lkml.org/lkml/2015/7/10/206 > > This patch introduces the new string property 'managed' that allows > the user to set the management type explicitly. > The supported values are: > "auto" - default. Uses either MDIO or nothing, depending on the presence > of the fixed-link node > "mdio" - use MDIO > "in-band-status" - use in-band status > "none" - use fixed-link description I thought we agreed in the last thread that "mdio" was implied whenever a proper PHY phandle to a device sitting on MDIO bus is used and that "auto" did not make much sense unless we were also describing how to do this auto-negotiation completely? The way I see it, the only thing that is needed at this point is an "in-band-status" property which you rightfully placed at the Ethernet MAC DT node level, this is fine, however, I disagree with how the values are enforced, see below: > > Signed-off-by: Stas Sergeev <stsp-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org> > > CC: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> > CC: Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org> > CC: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org> > CC: Ian Campbell <ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org> > CC: Kumar Gala <galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> > CC: Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> > CC: Grant Likely <grant.likely-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> > CC: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > CC: linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > CC: netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > --- > Documentation/devicetree/bindings/net/ethernet.txt | 5 ++++ > drivers/of/of_mdio.c | 28 ++++++++++++++++++++-- > 2 files changed, 31 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/net/ethernet.txt b/Documentation/devicetree/bindings/net/ethernet.txt > index 3fc3605..23743e9 100644 > --- a/Documentation/devicetree/bindings/net/ethernet.txt > +++ b/Documentation/devicetree/bindings/net/ethernet.txt > @@ -19,7 +19,12 @@ The following properties are common to the Ethernet controllers: > - phy: the same as "phy-handle" property, not recommended for new bindings. > - phy-device: the same as "phy-handle" property, not recommended for new > bindings. > +- managed: string, specifies the PHY management type. Supported values are: > + "auto", "mdio", "in-band-status", "none". "auto" is the default, and it > + sets the management type to either "mdio" or "none", depending on the > + presence of the "fixed-link" child node. As mentioned below, "mdio" is redundant with finding a "phy-handle", and "auto" is not specific enough to be useful to a consumer of this information. > > Child nodes of the Ethernet controller are typically the individual PHY devices > connected via the MDIO bus (sometimes the MDIO bus controller is separate). > They are described in the phy.txt file in this same directory. > +For non-MDIO PHY management see fixed-link.txt. > diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c > index 1bd4305..a460812 100644 > --- a/drivers/of/of_mdio.c > +++ b/drivers/of/of_mdio.c > @@ -262,7 +262,16 @@ EXPORT_SYMBOL(of_phy_attach); > bool of_phy_is_fixed_link(struct device_node *np) > { > struct device_node *dn; > - int len; > + int len, m_err; > + const char *managed; > + > + m_err = of_property_read_string(np, "managed", &managed); > + if (m_err == 0) { > + if (strcmp(managed, "none") == 0) > + return true; > + if (strcmp(managed, "mdio") == 0) > + return false; > + } managed = "mdio" on a fixed link is by definition not something remotely correct, there should be a proper PHY driver for this, and therefore a different representation: a PHY phandle to a PHY node on a MDIO bus. I do not think enforcing this has been incorrect provides much value, since you ought to write correct DT in the first place. > > /* New binding */ > dn = of_get_child_by_name(np, "fixed-link"); > @@ -271,6 +280,9 @@ bool of_phy_is_fixed_link(struct device_node *np) > return true; > } > > + if (m_err == 0 && strcmp(managed, "auto") != 0) > + return true; > + > /* Old binding */ > if (of_get_property(np, "fixed-link", &len) && > len == (5 * sizeof(__be32))) > @@ -285,8 +297,20 @@ int of_phy_register_fixed_link(struct device_node *np) > struct fixed_phy_status status = {}; > struct device_node *fixed_link_node; > const __be32 *fixed_link_prop; > - int len; > + int len, err; > struct phy_device *phy; > + const char *managed; > + > + err = of_property_read_string(np, "managed", &managed); > + if (err == 0) { > + if (strcmp(managed, "mdio") == 0) > + return -EINVAL; > + if (strcmp(managed, "in-band-status") == 0) { > + status.link = 0; > + phy = fixed_phy_register(PHY_POLL, &status, np); > + return IS_ERR(phy) ? PTR_ERR(phy) : 0; > + } And that is the only hunk of this patch that is really needed and useful to solving the problem here. -- Florian -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3] of_mdio: add new DT property 'managed' to specify the PHY management type 2015-07-14 17:51 ` Florian Fainelli (?) @ 2015-07-14 20:26 ` Stas Sergeev -1 siblings, 0 replies; 12+ messages in thread From: Stas Sergeev @ 2015-07-14 20:26 UTC (permalink / raw) To: Florian Fainelli, netdev Cc: Linux kernel, Sebastien Rannou, Arnaud Ebalard, Stas Sergeev, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala, Grant Likely, devicetree 14.07.2015 20:51, Florian Fainelli пишет: > On 14/07/15 10:13, Stas Sergeev wrote: >> Currently the PHY management type is selected by the MAC driver arbitrary. >> The decision is based on the presence of the "fixed-link" node and on a >> will of the driver's authors. >> This caused a regression recently, when mvneta driver suddenly started >> to use the in-band status for auto-negotiation on fixed links. >> It appears the auto-negotiation may not work when expected by the MAC driver. >> Sebastien Rannou explains: >> << 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. >> >> https://lkml.org/lkml/2015/7/10/206 >> >> This patch introduces the new string property 'managed' that allows >> the user to set the management type explicitly. >> The supported values are: >> "auto" - default. Uses either MDIO or nothing, depending on the presence >> of the fixed-link node >> "mdio" - use MDIO >> "in-band-status" - use in-band status >> "none" - use fixed-link description > I thought we agreed in the last thread that "mdio" was implied whenever > a proper PHY phandle to a device sitting on MDIO bus is used and that > "auto" did not make much sense unless we were also describing how to do > this auto-negotiation completely? Exactly not: --- > If we would implement autoneg outside of the fixed-link, > then its semantic would likely be > autoneg = "mdio" | "in-band" | "off" Right, if auto-negotiation was defined outside of fixed-link, that is indeed how I would also specify this. --- That's why I implemented it the way you see. But as you changed your mind, I'll remove "mdio". > As mentioned below, "mdio" is redundant with finding a "phy-handle", > and "auto" is not specific enough to be useful to a consumer of this > information. I prefer to keep "auto", as otherwise we'll have the default value (when the option is not specified) never achievable when the option _is_ specified, which is probably not very good. But I can remove "none" instead, leaving only "in-band-status" and "auto". Ok? Of course one can propose to completely ignore that option in presence of MDIO, but I wonder why not to allow for example to opt for in-band status even if you have MDIO? So if we want this option to play nicely with MDIO, as opposed to being entirely overridden, then "auto" still makes sense IMO. ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] mvneta: use inband status only when explicitly enabled 2015-07-14 17:09 [PATCH v3 0/3] net: enable inband link state negotiation only when explicitly requested Stas Sergeev 2015-07-14 17:11 ` [PATCH 1/3] fixed_phy: handle link-down case Stas Sergeev 2015-07-14 17:13 ` Stas Sergeev @ 2015-07-14 17:14 ` Stas Sergeev 2 siblings, 0 replies; 12+ messages in thread From: Stas Sergeev @ 2015-07-14 17:14 UTC (permalink / raw) To: netdev Cc: Linux kernel, Sebastien Rannou, Arnaud Ebalard, Stas Sergeev, Florian Fainelli, Thomas Petazzoni, stable The commit 898b2970e2c9 ("mvneta: implement SGMII-based in-band link state signaling") implemented the link parameters auto-negotiation unconditionally. Unfortunately it appears that some HW that implements SGMII protocol, doesn't generate the inband status, so it is not possible to auto-negotiate anything with such HW. This patch enables the auto-negotiation only if explicitly requested with the 'managed' DT property. This patch fixes the following regression: https://lkml.org/lkml/2015/7/8/865 and is therefore CCed to stable. Signed-off-by: Stas Sergeev <stsp@users.sourceforge.net> CC: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> CC: netdev@vger.kernel.org CC: linux-kernel@vger.kernel.org CC: stable@vger.kernel.org --- drivers/net/ethernet/marvell/mvneta.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c index 74176ec..7a1deee 100644 --- a/drivers/net/ethernet/marvell/mvneta.c +++ b/drivers/net/ethernet/marvell/mvneta.c @@ -3008,8 +3008,8 @@ static int mvneta_probe(struct platform_device *pdev) const char *dt_mac_addr; char hw_mac_addr[ETH_ALEN]; const char *mac_from; + const char *managed; int phy_mode; - int fixed_phy = 0; int err; /* Our multiqueue support is not complete, so for now, only @@ -3043,7 +3043,6 @@ static int mvneta_probe(struct platform_device *pdev) dev_err(&pdev->dev, "cannot register fixed PHY\n"); goto err_free_irq; } - fixed_phy = 1; /* In the case of a fixed PHY, the DT node associated * to the PHY is the Ethernet MAC DT node. @@ -3067,8 +3066,10 @@ static int mvneta_probe(struct platform_device *pdev) pp = netdev_priv(dev); pp->phy_node = phy_node; pp->phy_interface = phy_mode; - pp->use_inband_status = (phy_mode == PHY_INTERFACE_MODE_SGMII) && - fixed_phy; + + err = of_property_read_string(dn, "managed", &managed); + pp->use_inband_status = (err == 0 && + strcmp(managed, "in-band-status") == 0); pp->clk = devm_clk_get(&pdev->dev, NULL); if (IS_ERR(pp->clk)) { -- 1.9.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v4 0/3] net: enable inband link state negotiation only when explicitly requested @ 2015-07-16 14:49 Stas Sergeev 2015-07-16 14:53 ` [PATCH 3/3] mvneta: use inband status only when explicitly enabled Stas Sergeev 0 siblings, 1 reply; 12+ messages in thread From: Stas Sergeev @ 2015-07-16 14:49 UTC (permalink / raw) To: netdev Cc: Linux kernel, Sebastien Rannou, Arnaud Ebalard, Stas Sergeev, Florian Fainelli Hello. Currently the link status auto-negotiation is enabled for any SGMII link with fixed-link DT binding. The regression was reported: https://lkml.org/lkml/2015/7/8/865 Apparently not all HW that implements SGMII protocol, generates the inband status for the auto-negotiation to work. More details here: https://lkml.org/lkml/2015/7/10/206 The following patches reverts to the old behavior by default, which is to not enable the auto-negotiation for fixed-link. The new DT property is added that allows to explicitly request the auto-negotiation. Those who were affected by the change, please send your Tested-by, Thanks! ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] mvneta: use inband status only when explicitly enabled 2015-07-16 14:49 [PATCH v4 0/3] net: enable inband link state negotiation only when explicitly requested Stas Sergeev @ 2015-07-16 14:53 ` Stas Sergeev 0 siblings, 0 replies; 12+ messages in thread From: Stas Sergeev @ 2015-07-16 14:53 UTC (permalink / raw) To: netdev Cc: Linux kernel, Sebastien Rannou, Arnaud Ebalard, Stas Sergeev, Florian Fainelli, Thomas Petazzoni The commit 898b2970e2c9 ("mvneta: implement SGMII-based in-band link state signaling") implemented the link parameters auto-negotiation unconditionally. Unfortunately it appears that some HW that implements SGMII protocol, doesn't generate the inband status, so it is not possible to auto-negotiate anything with such HW. This patch enables the auto-negotiation only if explicitly requested with the 'managed' DT property. This patch fixes the following regression: https://lkml.org/lkml/2015/7/8/865 Signed-off-by: Stas Sergeev <stsp@users.sourceforge.net> CC: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> CC: netdev@vger.kernel.org CC: linux-kernel@vger.kernel.org --- drivers/net/ethernet/marvell/mvneta.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c index 74176ec..7a1deee 100644 --- a/drivers/net/ethernet/marvell/mvneta.c +++ b/drivers/net/ethernet/marvell/mvneta.c @@ -3008,8 +3008,8 @@ static int mvneta_probe(struct platform_device *pdev) const char *dt_mac_addr; char hw_mac_addr[ETH_ALEN]; const char *mac_from; + const char *managed; int phy_mode; - int fixed_phy = 0; int err; /* Our multiqueue support is not complete, so for now, only @@ -3043,7 +3043,6 @@ static int mvneta_probe(struct platform_device *pdev) dev_err(&pdev->dev, "cannot register fixed PHY\n"); goto err_free_irq; } - fixed_phy = 1; /* In the case of a fixed PHY, the DT node associated * to the PHY is the Ethernet MAC DT node. @@ -3067,8 +3066,10 @@ static int mvneta_probe(struct platform_device *pdev) pp = netdev_priv(dev); pp->phy_node = phy_node; pp->phy_interface = phy_mode; - pp->use_inband_status = (phy_mode == PHY_INTERFACE_MODE_SGMII) && - fixed_phy; + + err = of_property_read_string(dn, "managed", &managed); + pp->use_inband_status = (err == 0 && + strcmp(managed, "in-band-status") == 0); pp->clk = devm_clk_get(&pdev->dev, NULL); if (IS_ERR(pp->clk)) { -- 1.9.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 0/2] net: enable inband link state negotiation only when explicitly requested @ 2015-07-10 16:38 Stas Sergeev 2015-07-10 16:45 ` [PATCH 3/3] mvneta: use inband status only when explicitly enabled Stas Sergeev 0 siblings, 1 reply; 12+ messages in thread From: Stas Sergeev @ 2015-07-10 16:38 UTC (permalink / raw) To: netdev; +Cc: Linux kernel, Sebastien Rannou, Arnaud Ebalard, Stas Sergeev Hello. Currently the link status auto-negotiation is enabled for any SGMII link with fixed-link DT binding. The regression was reported: https://lkml.org/lkml/2015/7/8/865 Apparently not all HW that implements SGMII protocol, generates the inband status for the auto-negotiation to work. More details here: https://lkml.org/lkml/2015/7/10/206 The following patches reverts to the old behavior by default, which is to not enable the auto-negotiation for fixed-link. The new DT property is added that allows to explicitly request the auto-negotiation. Those who were affected by the change, please send your Tested-by, Thanks! ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/3] mvneta: use inband status only when explicitly enabled 2015-07-10 16:38 [PATCH v2 0/2] net: enable inband link state negotiation only when explicitly requested Stas Sergeev @ 2015-07-10 16:45 ` Stas Sergeev 0 siblings, 0 replies; 12+ messages in thread From: Stas Sergeev @ 2015-07-10 16:45 UTC (permalink / raw) To: netdev Cc: Linux kernel, Sebastien Rannou, Arnaud Ebalard, Stas Sergeev, Thomas Petazzoni, stable The commit 898b2970e2c9 ("mvneta: implement SGMII-based in-band link state signaling") implemented the link parameters auto-negotiation unconditionally. Unfortunately it appears that some HW that implements SGMII protocol, doesn't generate the inband status, so it is not possible to auto-negotiate anything with such HW. This patch enables the auto-negotiation only if the 'autoneg' DT property is set to 1. For old configurations where the 'autoneg' property is not specified, the default is to not use auto-negotiation. This patch fixes the following regression: https://lkml.org/lkml/2015/7/8/865 and is therefore CCed to stable. Signed-off-by: Stas Sergeev <stsp@users.sourceforge.net> CC: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> CC: netdev@vger.kernel.org CC: linux-kernel@vger.kernel.org CC: stable@vger.kernel.org --- drivers/net/ethernet/marvell/mvneta.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c index 74176ec..d4c29a3 100644 --- a/drivers/net/ethernet/marvell/mvneta.c +++ b/drivers/net/ethernet/marvell/mvneta.c @@ -3009,7 +3009,7 @@ static int mvneta_probe(struct platform_device *pdev) char hw_mac_addr[ETH_ALEN]; const char *mac_from; int phy_mode; - int fixed_phy = 0; + int autoneg_link = 0; int err; /* Our multiqueue support is not complete, so for now, only @@ -3043,7 +3043,7 @@ static int mvneta_probe(struct platform_device *pdev) dev_err(&pdev->dev, "cannot register fixed PHY\n"); goto err_free_irq; } - fixed_phy = 1; + autoneg_link = of_phy_is_autoneg_link(dn); /* In the case of a fixed PHY, the DT node associated * to the PHY is the Ethernet MAC DT node. @@ -3067,8 +3067,7 @@ static int mvneta_probe(struct platform_device *pdev) pp = netdev_priv(dev); pp->phy_node = phy_node; pp->phy_interface = phy_mode; - pp->use_inband_status = (phy_mode == PHY_INTERFACE_MODE_SGMII) && - fixed_phy; + pp->use_inband_status = autoneg_link; pp->clk = devm_clk_get(&pdev->dev, NULL); if (IS_ERR(pp->clk)) { -- 1.9.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
end of thread, other threads:[~2015-07-16 14:53 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-07-14 17:09 [PATCH v3 0/3] net: enable inband link state negotiation only when explicitly requested Stas Sergeev 2015-07-14 17:11 ` [PATCH 1/3] fixed_phy: handle link-down case Stas Sergeev 2015-07-14 18:28 ` Florian Fainelli [not found] ` <55A56D4D.5090004@list.ru> 2015-07-15 15:33 ` Fwd: " Stas Sergeev 2015-07-14 17:13 ` [PATCH 2/3] of_mdio: add new DT property 'managed' to specify the PHY management type Stas Sergeev 2015-07-14 17:13 ` Stas Sergeev 2015-07-14 17:51 ` Florian Fainelli 2015-07-14 17:51 ` Florian Fainelli 2015-07-14 20:26 ` Stas Sergeev 2015-07-14 17:14 ` [PATCH 3/3] mvneta: use inband status only when explicitly enabled Stas Sergeev -- strict thread matches above, loose matches on Subject: below -- 2015-07-16 14:49 [PATCH v4 0/3] net: enable inband link state negotiation only when explicitly requested Stas Sergeev 2015-07-16 14:53 ` [PATCH 3/3] mvneta: use inband status only when explicitly enabled Stas Sergeev 2015-07-10 16:38 [PATCH v2 0/2] net: enable inband link state negotiation only when explicitly requested Stas Sergeev 2015-07-10 16:45 ` [PATCH 3/3] mvneta: use inband status only when explicitly enabled Stas Sergeev
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.