Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1181733 > unrolled thread

[PATCH v2 0/2] net: enable inband link state negotiation only when explicitly requested

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 0/2] net: enable inband link state negotiation only when  explicitly requested Stas Sergeev <stsp@list.ru> - 2015-07-10 18:40 +0200
    [PATCH 3/3] mvneta: use inband status only when explicitly enabled Stas Sergeev <stsp@list.ru> - 2015-07-10 18:50 +0200
    [PATCH 1/3] fixed_phy: handle link-down case Stas Sergeev <stsp@list.ru> - 2015-07-10 18:50 +0200
      Re: [PATCH 1/3] fixed_phy: handle link-down case Stas Sergeev <stsp@list.ru> - 2015-07-11 11:00 +0200
    [PATCH 2/3] of_mdio: add new DT property 'autoneg' for fixed-link Stas Sergeev <stsp@list.ru> - 2015-07-10 18:50 +0200
      Re: [PATCH 2/3] of_mdio: add new DT property 'autoneg' for fixed-link Stas Sergeev <stsp@list.ru> - 2015-07-11 11:20 +0200
    Re: [PATCH v2 0/2] net: enable inband link state negotiation only  when explicitly requested Sebastien Rannou <mxs@sbrk.org> - 2015-07-13 12:00 +0200
      Re: [PATCH v2 0/2] net: enable inband link state negotiation only  when explicitly requested Stas Sergeev <stsp@list.ru> - 2015-07-13 12:00 +0200

#1181733 — [PATCH v2 0/2] net: enable inband link state negotiation only when explicitly requested

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!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1181739 — [PATCH 3/3] mvneta: use inband status only when explicitly enabled

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1181740 — [PATCH 1/3] fixed_phy: handle link-down case

Currently fixed_phy driver recognizes only the link-up state.
This simple patch adds an implementation of link-down state.
The actual change is 1-line, the rest is an indentation.

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 | 99 +++++++++++++++++++++++----------------------
 1 file changed, 50 insertions(+), 49 deletions(-)

diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c
index 1960b46..a5d93cf 100644
--- a/drivers/net/phy/fixed_phy.c
+++ b/drivers/net/phy/fixed_phy.c
@@ -52,58 +52,59 @@ static int fixed_phy_update_regs(struct fixed_phy *fp)
 	u16 lpagb = 0;
 	u16 lpa = 0;

-	if (fp->status.duplex) {
-		bmcr |= BMCR_FULLDPLX;
-
-		switch (fp->status.speed) {
-		case 1000:
-			bmsr |= BMSR_ESTATEN;
-			bmcr |= BMCR_SPEED1000;
-			lpagb |= LPA_1000FULL;
-			break;
-		case 100:
-			bmsr |= BMSR_100FULL;
-			bmcr |= BMCR_SPEED100;
-			lpa |= LPA_100FULL;
-			break;
-		case 10:
-			bmsr |= BMSR_10FULL;
-			lpa |= LPA_10FULL;
-			break;
-		default:
-			pr_warn("fixed phy: unknown speed\n");
-			return -EINVAL;
-		}
-	} else {
-		switch (fp->status.speed) {
-		case 1000:
-			bmsr |= BMSR_ESTATEN;
-			bmcr |= BMCR_SPEED1000;
-			lpagb |= LPA_1000HALF;
-			break;
-		case 100:
-			bmsr |= BMSR_100HALF;
-			bmcr |= BMCR_SPEED100;
-			lpa |= LPA_100HALF;
-			break;
-		case 10:
-			bmsr |= BMSR_10HALF;
-			lpa |= LPA_10HALF;
-			break;
-		default:
-			pr_warn("fixed phy: unknown speed\n");
-			return -EINVAL;
-		}
-	}
-
-	if (fp->status.link)
+	if (fp->status.link) {
 		bmsr |= BMSR_LSTATUS | BMSR_ANEGCOMPLETE;

-	if (fp->status.pause)
-		lpa |= LPA_PAUSE_CAP;
+		if (fp->status.duplex) {
+			bmcr |= BMCR_FULLDPLX;
+
+			switch (fp->status.speed) {
+			case 1000:
+				bmsr |= BMSR_ESTATEN;
+				bmcr |= BMCR_SPEED1000;
+				lpagb |= LPA_1000FULL;
+				break;
+			case 100:
+				bmsr |= BMSR_100FULL;
+				bmcr |= BMCR_SPEED100;
+				lpa |= LPA_100FULL;
+				break;
+			case 10:
+				bmsr |= BMSR_10FULL;
+				lpa |= LPA_10FULL;
+				break;
+			default:
+				pr_warn("fixed phy: unknown speed\n");
+				return -EINVAL;
+			}
+		} else {
+			switch (fp->status.speed) {
+			case 1000:
+				bmsr |= BMSR_ESTATEN;
+				bmcr |= BMCR_SPEED1000;
+				lpagb |= LPA_1000HALF;
+				break;
+			case 100:
+				bmsr |= BMSR_100HALF;
+				bmcr |= BMCR_SPEED100;
+				lpa |= LPA_100HALF;
+				break;
+			case 10:
+				bmsr |= BMSR_10HALF;
+				lpa |= LPA_10HALF;
+				break;
+			default:
+				pr_warn("fixed phy: unknown speed\n");
+				return -EINVAL;
+			}
+		}

-	if (fp->status.asym_pause)
-		lpa |= LPA_PAUSE_ASYM;
+		if (fp->status.pause)
+			lpa |= LPA_PAUSE_CAP;
+
+		if (fp->status.asym_pause)
+			lpa |= LPA_PAUSE_ASYM;
+	}

 	fp->regs[MII_PHYSID1] = 0;
 	fp->regs[MII_PHYSID2] = 0;
-- 
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1182112 — Re: [PATCH 1/3] fixed_phy: handle link-down case

11.07.2015 03:15, Florian Fainelli пишет:
> On 10/07/15 14:14, Stas Sergeev wrote:
>> 10.07.2015 23:44, Florian Fainelli пишет:
>>> On 10/07/15 09:41, Stas Sergeev wrote:
>>>> Currently fixed_phy driver recognizes only the link-up state.
>>>> This simple patch adds an implementation of link-down state.
>>>> The actual change is 1-line, the rest is an indentation.
>>> It is not clear to me how this is useful, if you have a link_update
>>> callback manipulating the link state, the fixed PHY driver returns
>>> appropriate MII_BMSR values and always re-initializes everything.
>> It returns the appropriate values only for link status (when its down),
>> but it still returns speed, duplex etc as if the link is up. I had hard
>> times finding the relevant specs, but from what I have googled,
>> when link is down, the speed/duplex/etc status fields should _also_
>> be zero, which is what my patch does.
>> What is more important is that fixed_phy_add() would return
>> -EINVAL if you didn't specify speed while the link is down.
>> This is an absolute must-fix, or I will have to add an arbitrary
>> speed value again, on which you already objected.
> Ok, but that does not seems to be a code path that you can hit, unless
> you are already modifying
> drivers/of/of_mdio.c::of_fixed_phy_register_link() and overriding how
> status.link is defined, am I missing something?
I think you can.
The drivers that do autonegotiation (eg mvneta) should take a
special care to not reset speed when link is down.
Or to nor read the speed when link is down (this is discouraged
anyway of course, but better to follow the real MDIO hw in that).
So while the work-arounds are simple, you can nevertheless hit
the bug if you try to.

>>> Is this meant to be some sort of optimization? If so, you could just
>>> avoid the re-intendation completely and do a goto instead?
>> Oh, c'mon... Adding goto just to keep the _patch_ smaller?
> Well, yes, so it's easy to audit the changes?
So you don't trust me that I only indented the code? OKey. :)

>> (not smaller code, just a smaller patch)
>> Well, this is certainly something that can be done, feel free
>> to request that explicitly and I'll release v3 next week.
> I hereby explicitly request that you make this a new iteration using a goto.
OKey, will do in v3.
Of course if you point me to the coding guidelines that explain
this part, I'll be more comfortable. But this is purely optional, I
simply don't like to add gotos where unneeded, but its not a big
deal at all.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1181743 — [PATCH 2/3] of_mdio: add new DT property 'autoneg' for fixed-link

Currently for fixed-link the MAC driver decides whether to use the
link status auto-negotiation or not.
Unfortunately 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 boolean property 'autoneg' that allows
the user to request the auto-negotiation explicitly.

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         |  6 +++++-
 drivers/of/of_mdio.c                               | 23 ++++++++++++++++++++--
 include/linux/of_mdio.h                            |  5 +++++
 3 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/fixed-link.txt b/Documentation/devicetree/bindings/net/fixed-link.txt
index 82bf7e0..e2959a8 100644
--- a/Documentation/devicetree/bindings/net/fixed-link.txt
+++ b/Documentation/devicetree/bindings/net/fixed-link.txt
@@ -9,8 +9,12 @@ Such a fixed link situation is described by creating a 'fixed-link'
 sub-node of the Ethernet MAC device node, with the following
 properties:

+* 'autoneg' (boolean, optional), to enable the auto-negotiation of link
+  state. 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 auto-negotiation is enabled,
+  '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..12b2ede 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -280,6 +280,22 @@ 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;
+	bool ret;
+
+	dn = of_get_child_by_name(np, "fixed-link");
+	if (!dn)
+		return false;
+
+	ret = of_property_read_bool(dn, "autoneg");
+
+	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,10 +307,13 @@ 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;
+		bool autoneg = of_property_read_bool(fixed_link_node,
+						     "autoneg");
+		status.link = !autoneg;
 		status.duplex = of_property_read_bool(fixed_link_node,
 						      "full-duplex");
-		if (of_property_read_u32(fixed_link_node, "speed", &status.speed))
+		if (of_property_read_u32(fixed_link_node, "speed",
+					 &status.speed) != 0 && !autoneg)
 			return -EINVAL;
 		status.pause = of_property_read_bool(fixed_link_node, "pause");
 		status.asym_pause = of_property_read_bool(fixed_link_node,
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


-- 
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1182114 — Re: [PATCH 2/3] of_mdio: add new DT property 'autoneg' for fixed-link

11.07.2015 03:22, Florian Fainelli пишет:
> On 10/07/15 14:02, Stas Sergeev wrote:
>> 10.07.2015 23:39, Florian Fainelli пишет:
>>>> - in-band status is an implementation delail, and it is
>>>> specific to a particular protocols. If you request the
>>>> in-band status for some protocol that doesn't support
>>>> it, perhaps you should get -EINVAL, because such a
>>>> config makes no sense. With autonegotiation, the rules
>>>> are not that strict: it can be "unimplemented", which doesn't
>>>> necessary mean nonsense in the config.
>>> So by specifying "autoneg", you are not specific about the kind of
>>> auto-negotiation protocol available, which is precisely my point: you
>>> need to go down to that level of detail for this to be useful. So maybe
>>> something like:
>>>
>>> autoneg = "in-band-status" would actually be a better thing in terms of
>>> description because then you would tell what can be made
>>> available/working?
>> I would agree with this if your argument below is true (see below).
>>
>>>> - autonegotiation is a wider term, and may be implemented
>>>> by some other means than the in-band status (which is
>>>> probably impossible for a fixed-link though).
>>>>
>>>> - In the terms that the driver uses, it is autonegotiation, eg
>>>> MVNETA_GMAC_AUTONEG_CONFIG. And when you go down
>>>> the implementation details, you see MVNETA_GMAC_INBAND_AN_ENABLE,
>>>> which is just one AN bit of many.
>>> But arguably, there could be another auto-negotiation method, which is
>>> not in-band status related, which means that you would need a way to
>>> distinguish between using in-band status, or using something else or
>>> nothing, would not you?
>> "something else" is a big question here.
>> Can you think of _any_ other way that is both not an MDIO
>> (suits to fixed-link) and not an in-band?
> Yes, I could think about I2C or SPI PHYs that you could use alongside an
> Ethernet controller that would qualify for out-of-band, not in-band, yet
> could still provide auto-negotiation. You may have special hardware with
> such a SPI or I2C controller which provides automatic decoding of the
> auto-neg registers. Have not looked at e.g: SFP form factors or fiber
> links, but they could also have additional out-of-band type of
> auto-negotiation available.
>
>> If the answer is yes (even theoretically), then
>> autoneg = "in-band" | "off"
>> may make sense. Otherwise boolean just looks enough.
> I think the answer is yes.
>
>> If we would implement autoneg outside of the fixed-link,
>> then its semantic would likely be
>> autoneg = "mdio" | "in-band" | "off"
>> But the fact that we put it under fixed-link where only a
>> single AN possibility exist, may probably be underlined by
>> a semantic specific to fixed-link.
> Right, if auto-negotiation was defined outside of fixed-link, that is
> indeed how I would also specify this.
Hmm, okey.
But then this all doesn't fit into a fixed-link. The inband autoneg
is a very small xtension, it only allows to notify MAC about some
changes on the other end, but never control such changes, so
from some POV it is still pretty much fixed. And it also built into
the protocols that fixed-link already use, so that looked like a
natural xtension to me. But if there are so many possible ways
to abuse fixed-link making it _fully managed_, I am really starting
to think about the possibility of defining the autoneg outside of it,
and leave the poor fixed-link alone.
The patch will be bigger, but... what do you think?
This will of course first require defining the fixed-link in the
docs more strictly, as currently it is (vaguely) defined as
"non-MDIO", which leaves a lot to speculate and abuse.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1182666 — Re: [PATCH v2 0/2] net: enable inband link state negotiation only when explicitly requested

[Multipart message — attachments visible in raw view] — view raw

Hi Stas,

On Fri, 10 Jul 2015, Stas Sergeev wrote:

> Those who were affected by the change, please send your Tested-by,
> Thanks!

I also confirm that this version of the patch solves the issue:

Tested-by: Sebastien Rannou <mxs@sbrk.org>

-- 
Sébastien

[toc] | [prev] | [next] | [standalone]

#1182676 — Re: [PATCH v2 0/2] net: enable inband link state negotiation only when explicitly requested

13.07.2015 12:54, Sebastien Rannou пишет:
> Hi Stas,
> 
> On Fri, 10 Jul 2015, Stas Sergeev wrote:
> 
>> Those who were affected by the change, please send your Tested-by,
>> Thanks!
> 
> I also confirm that this version of the patch solves the issue:
> 
> Tested-by: Sebastien Rannou <mxs@sbrk.org>
Thanks Sebastien!
Unfortunately, there will be v3 in a few days.
Perhaps you should not rush with the tests until the
things are settled, or who knows how many iterations
you'll have to test...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web