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

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

[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!

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

[Stas Sergeev]

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

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

[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.

Is this meant to be some sort of optimization? If so, you could just
avoid the re-intendation completely and do a goto instead?

> 
> 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;
> 


-- 
Florian

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

[Stas Sergeev]

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.

> 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?
(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.

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

[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?

> 
>> 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?

> (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.

Thank you.
-- 
Florian

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

[Stas Sergeev]

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.

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

[Stas Sergeev]

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-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
---
 .../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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Florian Fainelli]

On 10/07/15 09:43, Stas Sergeev wrote:
> 
> 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.

The implementation looks better, but the name might still be slightly
controversial. I would go with "use-in-band-status" which is more
strictly defined than "autoneg" which could mean anything and everything.

What do you think?

> 
> 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
> 
> 


-- 
Florian

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

[Stas Sergeev]

10.07.2015 21:37, Florian Fainelli пишет:
> On 10/07/15 09:43, Stas Sergeev wrote:
>> 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.
> The implementation looks better, but the name might still be slightly
> controversial. I would go with "use-in-band-status" which is more
> strictly defined than "autoneg" which could mean anything and everything.
>
> What do you think?
I actually think autoneg is a bit better.

- Autonegotiation is a widely used and known term:
https://en.wikipedia.org/wiki/Autonegotiation
And who knows what in-band status is?
And, more importantly, who knows what is it used for?
Who even knows it is used for autonegotiation?

- When we set autoneg for fixed-link, we basically just
say "no MDIO here, but please do autoneg by any other
means, if possible".

- 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.

- 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.

So I really would prefer to keep things as is.
But if you insist, I can rename, but there will still be no
-EINVAL checks for obviously wrong configs.

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

[Florian Fainelli]

On 10/07/15 13:08, Stas Sergeev wrote:
> 10.07.2015 21:37, Florian Fainelli пишет:
>> On 10/07/15 09:43, Stas Sergeev wrote:
>>> 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.
>> The implementation looks better, but the name might still be slightly
>> controversial. I would go with "use-in-band-status" which is more
>> strictly defined than "autoneg" which could mean anything and everything.
>>
>> What do you think?
> I actually think autoneg is a bit better.
> 
> - Autonegotiation is a widely used and known term:
> https://en.wikipedia.org/wiki/Autonegotiation
> And who knows what in-band status is?

You and I apparently do because otherwise you would not have ran into
this problem and more generally, anyone having some mild exposure to the
(S|R)GMII protocols should at some point, but that is a pointless argument.

> And, more importantly, who knows what is it used for?
> Who even knows it is used for autonegotiation?

It is not about what do people know most, it is about being accurate and
specific.

> 
> - When we set autoneg for fixed-link, we basically just
> say "no MDIO here, but please do autoneg by any other
> means, if possible".

I agree with this.

> 
> - 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?

> 
> - 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?

> 
> So I really would prefer to keep things as is.
> But if you insist, I can rename, but there will still be no
> -EINVAL checks for obviously wrong configs.


-- 
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

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

[Stas Sergeev]

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?
If the answer is yes (even theoretically), then
autoneg = "in-band" | "off"
may make sense. Otherwise boolean just looks enough.
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.

One may also argue that
autoneg = "any-possible-autoneg-that-works" is better than
specifying it explicitly, which is exactly what the boolean does.
--
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

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

[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.

> 
> One may also argue that
> autoneg = "any-possible-autoneg-that-works" is better than
> specifying it explicitly, which is exactly what the boolean does.

I prefer excess of information rather than lack of information, because
you can always choose what to do with it. Especially when it comes to
Device Tree, plan carefully :)
-- 
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

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

[Stas Sergeev]

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.

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

[Stas Sergeev]

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

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

[Florian Fainelli]

On 10/07/15 09:38, Stas Sergeev wrote:
> 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!

For future submissions, would you mind CC'ing everybody for the entire
patch series and not just on a per-patch basis?
-- 
Florian

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

[Stas Sergeev]

10.07.2015 23:31, Florian Fainelli пишет:
> On 10/07/15 09:38, Stas Sergeev wrote:
>> 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!
> For future submissions, would you mind CC'ing everybody for the entire
> patch series and not just on a per-patch basis?
I used get_maintainers.pl on a per-patch basis.
This is what SubmittingPatches.txt seems to suggest.
It doesn't say how to produce the CC lists for patch
series, it seems...
Anyway, I'll try to add more recipients to the announce
e-mail next time.

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

[Sebastien Rannou]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 255 bytes --]

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

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

[Stas Sergeev]

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...

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

[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!

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

[Stas Sergeev]

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

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

[Florian Fainelli]

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

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

[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!

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

[Stas Sergeev]

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

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

[Florian Fainelli]

On 16/07/15 07:50, 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.

This patch still breaks my setups here, e.g: drivers/net/dsa/bcm_sf2.c,
but I will look into it.

Do we really need this for now for your two other patches to work
properly, or is it just nicer to have?

> 
> 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;
> 


-- 
Florian

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

[Stas Sergeev]

17.07.2015 02:25, Florian Fainelli пишет:
> On 16/07/15 07:50, 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.
> 
> This patch still breaks my setups here, e.g: drivers/net/dsa/bcm_sf2.c,
> but I will look into it.
> 
> Do we really need this for now for your two other patches to work
> properly, or is it just nicer to have?
Yes, absolutely.
Otherwise registering fixed phy will return -EINVAL
because of the missing link speed (even though the link
is down).

Please, see what makes a problem. I can't reproduce what you report.

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

[Florian Fainelli]

On 17/07/15 04:26, Stas Sergeev wrote:
> 17.07.2015 02:25, Florian Fainelli пишет:
>> On 16/07/15 07:50, 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.
>>
>> This patch still breaks my setups here, e.g: drivers/net/dsa/bcm_sf2.c,
>> but I will look into it.
>>
>> Do we really need this for now for your two other patches to work
>> properly, or is it just nicer to have?
> Yes, absolutely.
> Otherwise registering fixed phy will return -EINVAL
> because of the missing link speed (even though the link
> is down).

Ok, I see the problem that you have now. Arguably you could say that
according to the fixed-link binding, speed needs to be specified and the
code correctly errors out with such an error if you do not specify it. I
also agree that having to specify speed and duplex for something you
will end-up auto-negotiating has no useful purpose.

> 
> Please, see what makes a problem. I can't reproduce what you report.
> 

So is different is that I use a link_update callback, and so we rely on
at least one call of this function to initialize the hardware in
drivers/net/dsa/bcm_sf2.c for this to work, after that, the hardware
reflects the fixed link parameters we configured, and we feed the
fixed_phy_status information from the hardware directly.

From there I see two different ways to fix this:

- we ignore the fixed_phy_update_regs return value in fixed_phy_add(),
but that will make us avoid doing verification on the speed, which is
not so great, but is essentially what your patch does anyway

- we update the use of the fixed PHY link_update in drivers using it and
convert them to use fixed_phy_update_state instead, which can take some
time and effort to convert

What do you think? I would go with option 1 and eventually introduce a
special switch() case on the speed settings just to validate we know them.

Thanks
-- 
Florian

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

[Stas Sergeev]

17.07.2015 21:50, Florian Fainelli пишет:
> On 17/07/15 04:26, Stas Sergeev wrote:
>> 17.07.2015 02:25, Florian Fainelli пишет:
>>> On 16/07/15 07:50, 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.
>>> This patch still breaks my setups here, e.g: drivers/net/dsa/bcm_sf2.c,
>>> but I will look into it.
>>>
>>> Do we really need this for now for your two other patches to work
>>> properly, or is it just nicer to have?
>> Yes, absolutely.
>> Otherwise registering fixed phy will return -EINVAL
>> because of the missing link speed (even though the link
>> is down).
> Ok, I see the problem that you have now. Arguably you could say that
> according to the fixed-link binding, speed needs to be specified and the
> code correctly errors out with such an error if you do not specify it. I
Aren't you missing the fact that .link=0?
I think what you say is true only for the link-up case, no?
.speed==0 is valid for link-down IMHO: no link - zero speed.

> So is different is that I use a link_update callback, and so we rely on
> at least one call of this function to initialize the hardware in
> drivers/net/dsa/bcm_sf2.c
Do you mean this?:
core_writel(priv, reg, CORE_STS_OVERRIDE_GMIIP_PORT(port));
Maybe just moving the HW initialization bits to some init func
will be a quick fix?

>   for this to work, after that, the hardware
> reflects the fixed link parameters we configured, and we feed the
> fixed_phy_status information from the hardware directly.
>
> >From there I see two different ways to fix this:
>
> - we ignore the fixed_phy_update_regs return value in fixed_phy_add(),
> but that will make us avoid doing verification on the speed, which is
> not so great, but is essentially what your patch does anyway
No, it does not. All it does is to allow no speed _when link is down_,
which is IMHO a very logical fix. The speed checks for the link-up
case are all still there.

> - we update the use of the fixed PHY link_update in drivers using it
IMHO just 2 drivers: bcmii.c and bcm_sf2.c, and the change
is likely trivial, although of course I am not sure in details.

>   and
> convert them to use fixed_phy_update_state instead, which can take some
> time and effort to convert
Maybe just move the initialization bits out of the link_update
callback, but still use the callback for now? Should be simple, no?

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

[Florian Fainelli]

On 17/07/15 13:03, Stas Sergeev wrote:
> 17.07.2015 21:50, Florian Fainelli пишет:
>> On 17/07/15 04:26, Stas Sergeev wrote:
>>> 17.07.2015 02:25, Florian Fainelli пишет:
>>>> On 16/07/15 07:50, 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.
>>>> This patch still breaks my setups here, e.g: drivers/net/dsa/bcm_sf2.c,
>>>> but I will look into it.
>>>>
>>>> Do we really need this for now for your two other patches to work
>>>> properly, or is it just nicer to have?
>>> Yes, absolutely.
>>> Otherwise registering fixed phy will return -EINVAL
>>> because of the missing link speed (even though the link
>>> is down).
>> Ok, I see the problem that you have now. Arguably you could say that
>> according to the fixed-link binding, speed needs to be specified and the
>> code correctly errors out with such an error if you do not specify it. I
> Aren't you missing the fact that .link=0?
> I think what you say is true only for the link-up case, no?
> .speed==0 is valid for link-down IMHO: no link - zero speed.

Pardon me being very dense and stupid here, but your problem is that the
"speed" parameter is not specified in your DT, and we end-up returning
-EINVAL from of_phy_register_fixed_link(), is that what is happening?

And even if we silenced that error, we would end-up calling
fixed_phy_add() which would also return -EINVAL because then, we would
have status.link = 1, but no speed. So I better understand what is it
that you are after here, and that is also a broken Device Tree, is not
it? So this was the reason why in earlier versions of the patchset you
ended-up with a given speed which would make us pass this condition, right?

> 
>> So is different is that I use a link_update callback, and so we rely on
>> at least one call of this function to initialize the hardware in
>> drivers/net/dsa/bcm_sf2.c
> Do you mean this?:
> core_writel(priv, reg, CORE_STS_OVERRIDE_GMIIP_PORT(port));
> Maybe just moving the HW initialization bits to some init func
> will be a quick fix?

Well, the problem with that is that to know how we should be configuring
the hardware in the adjust_link function, we need to run the link_update
function first. By default, there is no auto-negotiation on these fixed
links at all, so we cannot rely on any value being programmed other than
those specified in DT.

> 
>>   for this to work, after that, the hardware
>> reflects the fixed link parameters we configured, and we feed the
>> fixed_phy_status information from the hardware directly.
>>
>> >From there I see two different ways to fix this:
>>
>> - we ignore the fixed_phy_update_regs return value in fixed_phy_add(),
>> but that will make us avoid doing verification on the speed, which is
>> not so great, but is essentially what your patch does anyway
> No, it does not. All it does is to allow no speed _when link is down_,
> which is IMHO a very logical fix. The speed checks for the link-up
> case are all still there.
> 
>> - we update the use of the fixed PHY link_update in drivers using it
> IMHO just 2 drivers: bcmii.c and bcm_sf2.c, and the change
> is likely trivial, although of course I am not sure in details.

The changes are not trivial, it took a while to get that logic done
correctly, and this would increase the number of patches to backport to
-stable, which is not ideal.

> 
>>   and
>> convert them to use fixed_phy_update_state instead, which can take some
>> time and effort to convert
> Maybe just move the initialization bits out of the link_update
> callback, but still use the callback for now? Should be simple, no?

Let me see if I have a smart idea other the weekend on how to do this.
-- 
Florian

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

[Stas Sergeev]

18.07.2015 01:01, Florian Fainelli пишет:
> On 17/07/15 13:03, Stas Sergeev wrote:
>> 17.07.2015 21:50, Florian Fainelli пишет:
>>> On 17/07/15 04:26, Stas Sergeev wrote:
>>>> 17.07.2015 02:25, Florian Fainelli пишет:
>>>>> On 16/07/15 07:50, 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.
>>>>> This patch still breaks my setups here, e.g: drivers/net/dsa/bcm_sf2.c,
>>>>> but I will look into it.
>>>>>
>>>>> Do we really need this for now for your two other patches to work
>>>>> properly, or is it just nicer to have?
>>>> Yes, absolutely.
>>>> Otherwise registering fixed phy will return -EINVAL
>>>> because of the missing link speed (even though the link
>>>> is down).
>>> Ok, I see the problem that you have now. Arguably you could say that
>>> according to the fixed-link binding, speed needs to be specified and the
>>> code correctly errors out with such an error if you do not specify it. I
>> Aren't you missing the fact that .link=0?
>> I think what you say is true only for the link-up case, no?
>> .speed==0 is valid for link-down IMHO: no link - zero speed.
> Pardon me being very dense and stupid here, but your problem is that the
> "speed" parameter is not specified in your DT,
Not even a fixed-link at all, since the latest patches.
I removed fixed-link defs from my DT.

>   and we end-up returning
> -EINVAL from of_phy_register_fixed_link(), is that what is happening?
Yes.

> And even if we silenced that error,
I don't agree with calling it an error silencing.
To me it is a fix. It will also return a more correct status when
link is down.

>   we would end-up calling
> fixed_phy_add() which would also return -EINVAL because then, we would
> have status.link = 1, but no speed.
Why link=1 and no speed? This is not valid, should never
be used. The error checking is still there to prevent it.

>   So I better understand what is it
> that you are after here, and that is also a broken Device Tree, is not
> it?
I don't understand. If you didn't specify the in-band status, you
_must_ set the speed. There is no broken DT in either case.

>   So this was the reason why in earlier versions of the patchset you
> ended-up with a given speed which would make us pass this condition, right?
As explained earlier, yes.


>>> So is different is that I use a link_update callback, and so we rely on
>>> at least one call of this function to initialize the hardware in
>>> drivers/net/dsa/bcm_sf2.c
>> Do you mean this?:
>> core_writel(priv, reg, CORE_STS_OVERRIDE_GMIIP_PORT(port));
>> Maybe just moving the HW initialization bits to some init func
>> will be a quick fix?
> Well, the problem with that is that to know how we should be configuring
> the hardware in the adjust_link function, we need to run the link_update
> function first. By default, there is no auto-negotiation on these fixed
> links at all, so we cannot rely on any value being programmed other than
> those specified in DT.
Ah, so is my understanding correct that in fixed_link_update()
you set .link=0 and as a result get wrong speed in adjust_link(),
which gets then written to init HW?
AFAIK when link is down, you are not allowed to rely on the PHY
status registers to read speed from, or am I wrong? So if my
understanding is correct, this was working by a pure luck.
As for the quick fix - why not to do this pre-init in fixed_link_update()
instead of adjust_link()? In fixed_link_update() you'll get the speed
right from DT, so it will be correct.

> The changes are not trivial, it took a while to get that logic done
For a longer term fix,
how about adding a *status arg to of_phy_register_fixed_link() to
always get the status back to the driver, unless NULL is provided?
Using an update callback for that doesn't look like the best thing
to do. And besides, if we move to my fixed_phy_update_state(),
this will be needed anyway.

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

[Florian Fainelli]

On 17/07/15 16:24, Stas Sergeev wrote:
> 18.07.2015 01:01, Florian Fainelli пишет:
>> On 17/07/15 13:03, Stas Sergeev wrote:
>>> 17.07.2015 21:50, Florian Fainelli пишет:
>>>> On 17/07/15 04:26, Stas Sergeev wrote:
>>>>> 17.07.2015 02:25, Florian Fainelli пишет:
>>>>>> On 16/07/15 07:50, 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.
>>>>>> This patch still breaks my setups here, e.g:
>>>>>> drivers/net/dsa/bcm_sf2.c,
>>>>>> but I will look into it.
>>>>>>
>>>>>> Do we really need this for now for your two other patches to work
>>>>>> properly, or is it just nicer to have?
>>>>> Yes, absolutely.
>>>>> Otherwise registering fixed phy will return -EINVAL
>>>>> because of the missing link speed (even though the link
>>>>> is down).
>>>> Ok, I see the problem that you have now. Arguably you could say that
>>>> according to the fixed-link binding, speed needs to be specified and
>>>> the
>>>> code correctly errors out with such an error if you do not specify
>>>> it. I
>>> Aren't you missing the fact that .link=0?
>>> I think what you say is true only for the link-up case, no?
>>> .speed==0 is valid for link-down IMHO: no link - zero speed.
>> Pardon me being very dense and stupid here, but your problem is that the
>> "speed" parameter is not specified in your DT,
> Not even a fixed-link at all, since the latest patches.
> I removed fixed-link defs from my DT.

Hummm, okay, so you just have the inband-status property and that's it,
not even a fixed-link node anymore, right? How does
mvneta_fixed_link_update() work then since it needs a fixed PHY to be
registered?

> 
>>   and we end-up returning
>> -EINVAL from of_phy_register_fixed_link(), is that what is happening?
> Yes.
> 
>> And even if we silenced that error,
> I don't agree with calling it an error silencing.
> To me it is a fix. It will also return a more correct status when
> link is down.
> 
>>   we would end-up calling
>> fixed_phy_add() which would also return -EINVAL because then, we would
>> have status.link = 1, but no speed.
> Why link=1 and no speed? This is not valid, should never
> be used. The error checking is still there to prevent it.
> 
>>   So I better understand what is it
>> that you are after here, and that is also a broken Device Tree, is not
>> it?
> I don't understand. If you didn't specify the in-band status, you
> _must_ set the speed. There is no broken DT in either case.



> 
>>   So this was the reason why in earlier versions of the patchset you
>> ended-up with a given speed which would make us pass this condition,
>> right?
> As explained earlier, yes.
> 
> 
>>>> So is different is that I use a link_update callback, and so we rely on
>>>> at least one call of this function to initialize the hardware in
>>>> drivers/net/dsa/bcm_sf2.c
>>> Do you mean this?:
>>> core_writel(priv, reg, CORE_STS_OVERRIDE_GMIIP_PORT(port));
>>> Maybe just moving the HW initialization bits to some init func
>>> will be a quick fix?
>> Well, the problem with that is that to know how we should be configuring
>> the hardware in the adjust_link function, we need to run the link_update
>> function first. By default, there is no auto-negotiation on these fixed
>> links at all, so we cannot rely on any value being programmed other than
>> those specified in DT.
> Ah, so is my understanding correct that in fixed_link_update()
> you set .link=0 and as a result get wrong speed in adjust_link(),
> which gets then written to init HW?

Yes, that's what happens.

> AFAIK when link is down, you are not allowed to rely on the PHY
> status registers to read speed from, or am I wrong? So if my
> understanding is correct, this was working by a pure luck.

Well, it's more like it is undefined, and before this patch, the fixed
PHY would update everything except the link status indication.

> As for the quick fix - why not to do this pre-init in fixed_link_update()
> instead of adjust_link()? In fixed_link_update() you'll get the speed
> right from DT, so it will be correct.

fixed_link_update() only gets called once you start your PHY state
machine, unfortunately, not upon fixed PHY device registration, and it
runs before your adjust_link callback does, that's why starting with
correct parameters is kind of important here. Of course, this could be
fixed.

> 
>> The changes are not trivial, it took a while to get that logic done
> For a longer term fix,
> how about adding a *status arg to of_phy_register_fixed_link() to
> always get the status back to the driver, unless NULL is provided?
> Using an update callback for that doesn't look like the best thing
> to do. And besides, if we move to my fixed_phy_update_state(),
> this will be needed anyway.

I agree that the link_update callback is not the best thing, it polls
the hardware and comes with that problem that it may or may not have yet
run to configure your fixed_phy_status appropriately.
-- 
Florian

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

[Stas Sergeev]

18.07.2015 02:35, Florian Fainelli пишет:
> On 17/07/15 16:24, Stas Sergeev wrote:
>> 18.07.2015 01:01, Florian Fainelli пишет:
>>> On 17/07/15 13:03, Stas Sergeev wrote:
>>>> 17.07.2015 21:50, Florian Fainelli пишет:
>>>>> On 17/07/15 04:26, Stas Sergeev wrote:
>>>>>> 17.07.2015 02:25, Florian Fainelli пишет:
>>>>>>> On 16/07/15 07:50, 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.
>>>>>>> This patch still breaks my setups here, e.g:
>>>>>>> drivers/net/dsa/bcm_sf2.c,
>>>>>>> but I will look into it.
>>>>>>>
>>>>>>> Do we really need this for now for your two other patches to work
>>>>>>> properly, or is it just nicer to have?
>>>>>> Yes, absolutely.
>>>>>> Otherwise registering fixed phy will return -EINVAL
>>>>>> because of the missing link speed (even though the link
>>>>>> is down).
>>>>> Ok, I see the problem that you have now. Arguably you could say that
>>>>> according to the fixed-link binding, speed needs to be specified and
>>>>> the
>>>>> code correctly errors out with such an error if you do not specify
>>>>> it. I
>>>> Aren't you missing the fact that .link=0?
>>>> I think what you say is true only for the link-up case, no?
>>>> .speed==0 is valid for link-down IMHO: no link - zero speed.
>>> Pardon me being very dense and stupid here, but your problem is that the
>>> "speed" parameter is not specified in your DT,
>> Not even a fixed-link at all, since the latest patches.
>> I removed fixed-link defs from my DT.
> Hummm, okay, so you just have the inband-status property and that's it,
> not even a fixed-link node anymore, right? How does
> mvneta_fixed_link_update() work then since it needs a fixed PHY to be
> registered?
You can see it from my patch:
---

+	err = of_property_read_string(np, "managed", &managed);
+	if (err == 0) {
+		if (strcmp(managed, "in-band-status") == 0) {
+			/* status is zeroed, namely its .link member */
+			phy = fixed_phy_register(PHY_POLL, &status, np);
+			return IS_ERR(phy) ? PTR_ERR(phy) : 0;
+		}
+	}

---
which is the hunk added to the of_phy_register_fixed_link().
So in that case we register fixed-phy, but do not parse the fixed-link.

>> AFAIK when link is down, you are not allowed to rely on the PHY
>> status registers to read speed from, or am I wrong? So if my
>> understanding is correct, this was working by a pure luck.
> Well, it's more like it is undefined, and before this patch, the fixed
> PHY would update everything except the link status indication.
And what about the real MDIO PHY? Or does it never hit this
"undefined" code path?
Anyway, if you call it undefined, I guess you automatically agree
this needs to be fixed. :)

>> As for the quick fix - why not to do this pre-init in fixed_link_update()
>> instead of adjust_link()? In fixed_link_update() you'll get the speed
>> right from DT, so it will be correct.
> fixed_link_update() only gets called once you start your PHY state
> machine, unfortunately, not upon fixed PHY device registration, and it
> runs before your adjust_link callback does,
So you say fixed_link_update() runs before adjust_link callback does,
which looks logical. Why would you need it to run on device registration,
if it runs earlier than adjust_link (which you use for init) even now?

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

[Florian Fainelli]

Le 07/17/15 16:53, Stas Sergeev a écrit :
> 18.07.2015 02:35, Florian Fainelli пишет:
>> On 17/07/15 16:24, Stas Sergeev wrote:
>>> 18.07.2015 01:01, Florian Fainelli пишет:
>>>> On 17/07/15 13:03, Stas Sergeev wrote:
>>>>> 17.07.2015 21:50, Florian Fainelli пишет:
>>>>>> On 17/07/15 04:26, Stas Sergeev wrote:
>>>>>>> 17.07.2015 02:25, Florian Fainelli пишет:
>>>>>>>> On 16/07/15 07:50, 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.
>>>>>>>> This patch still breaks my setups here, e.g:
>>>>>>>> drivers/net/dsa/bcm_sf2.c,
>>>>>>>> but I will look into it.
>>>>>>>>
>>>>>>>> Do we really need this for now for your two other patches to work
>>>>>>>> properly, or is it just nicer to have?
>>>>>>> Yes, absolutely.
>>>>>>> Otherwise registering fixed phy will return -EINVAL
>>>>>>> because of the missing link speed (even though the link
>>>>>>> is down).
>>>>>> Ok, I see the problem that you have now. Arguably you could say that
>>>>>> according to the fixed-link binding, speed needs to be specified and
>>>>>> the
>>>>>> code correctly errors out with such an error if you do not specify
>>>>>> it. I
>>>>> Aren't you missing the fact that .link=0?
>>>>> I think what you say is true only for the link-up case, no?
>>>>> .speed==0 is valid for link-down IMHO: no link - zero speed.
>>>> Pardon me being very dense and stupid here, but your problem is that
>>>> the
>>>> "speed" parameter is not specified in your DT,
>>> Not even a fixed-link at all, since the latest patches.
>>> I removed fixed-link defs from my DT.
>> Hummm, okay, so you just have the inband-status property and that's it,
>> not even a fixed-link node anymore, right? How does
>> mvneta_fixed_link_update() work then since it needs a fixed PHY to be
>> registered?
> You can see it from my patch:
> ---
> 
> +    err = of_property_read_string(np, "managed", &managed);
> +    if (err == 0) {
> +        if (strcmp(managed, "in-band-status") == 0) {
> +            /* status is zeroed, namely its .link member */
> +            phy = fixed_phy_register(PHY_POLL, &status, np);
> +            return IS_ERR(phy) ? PTR_ERR(phy) : 0;
> +        }
> +    }
> 
> ---
> which is the hunk added to the of_phy_register_fixed_link().
> So in that case we register fixed-phy, but do not parse the fixed-link.

Ok, I missed that part. Could not you just override everything that is
needed here to get past the point where you register your fixed PHY even
with link = 0, this will be discarded anyway once you start in-band
negotiation.

> 
>>> AFAIK when link is down, you are not allowed to rely on the PHY
>>> status registers to read speed from, or am I wrong? So if my
>>> understanding is correct, this was working by a pure luck.
>> Well, it's more like it is undefined, and before this patch, the fixed
>> PHY would update everything except the link status indication.
> And what about the real MDIO PHY? Or does it never hit this
> "undefined" code path?
> Anyway, if you call it undefined, I guess you automatically agree
> this needs to be fixed. :)

I should have been clearer; it is undefined for real PHYs it was not for
fixed PHYs, you can rely on the configuration that was done during
registration. Maybe not the best assumption; but it worked, and with
this patch it no longer works, so we want to find something here.

> 
>>> As for the quick fix - why not to do this pre-init in
>>> fixed_link_update()
>>> instead of adjust_link()? In fixed_link_update() you'll get the speed
>>> right from DT, so it will be correct.
>> fixed_link_update() only gets called once you start your PHY state
>> machine, unfortunately, not upon fixed PHY device registration, and it
>> runs before your adjust_link callback does,
> So you say fixed_link_update() runs before adjust_link callback does,
> which looks logical. Why would you need it to run on device registration,
> if it runs earlier than adjust_link (which you use for init) even now?

There could be multiple reasons:

- device might be clock gated, until you "open" it you cannot
necessarily start making register accesses

- interfaces can be brought up/down separately so you want to stop the
PHY state machine accordingly

I will work on something anyway.
-- 
Florian

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

[Stas Sergeev]

18.07.2015 05:29, Florian Fainelli пишет:
> Le 07/17/15 16:53, Stas Sergeev a écrit :
>> 18.07.2015 02:35, Florian Fainelli пишет:
>>> On 17/07/15 16:24, Stas Sergeev wrote:
>>>> 18.07.2015 01:01, Florian Fainelli пишет:
>>>>> On 17/07/15 13:03, Stas Sergeev wrote:
>>>>>> 17.07.2015 21:50, Florian Fainelli пишет:
>>>>>>> On 17/07/15 04:26, Stas Sergeev wrote:
>>>>>>>> 17.07.2015 02:25, Florian Fainelli пишет:
>>>>>>>>> On 16/07/15 07:50, 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.
>>>>>>>>> This patch still breaks my setups here, e.g:
>>>>>>>>> drivers/net/dsa/bcm_sf2.c,
>>>>>>>>> but I will look into it.
>>>>>>>>>
>>>>>>>>> Do we really need this for now for your two other patches to work
>>>>>>>>> properly, or is it just nicer to have?
>>>>>>>> Yes, absolutely.
>>>>>>>> Otherwise registering fixed phy will return -EINVAL
>>>>>>>> because of the missing link speed (even though the link
>>>>>>>> is down).
>>>>>>> Ok, I see the problem that you have now. Arguably you could say that
>>>>>>> according to the fixed-link binding, speed needs to be specified and
>>>>>>> the
>>>>>>> code correctly errors out with such an error if you do not specify
>>>>>>> it. I
>>>>>> Aren't you missing the fact that .link=0?
>>>>>> I think what you say is true only for the link-up case, no?
>>>>>> .speed==0 is valid for link-down IMHO: no link - zero speed.
>>>>> Pardon me being very dense and stupid here, but your problem is that
>>>>> the
>>>>> "speed" parameter is not specified in your DT,
>>>> Not even a fixed-link at all, since the latest patches.
>>>> I removed fixed-link defs from my DT.
>>> Hummm, okay, so you just have the inband-status property and that's it,
>>> not even a fixed-link node anymore, right? How does
>>> mvneta_fixed_link_update() work then since it needs a fixed PHY to be
>>> registered?
>> You can see it from my patch:
>> ---
>>
>> +    err = of_property_read_string(np, "managed", &managed);
>> +    if (err == 0) {
>> +        if (strcmp(managed, "in-band-status") == 0) {
>> +            /* status is zeroed, namely its .link member */
>> +            phy = fixed_phy_register(PHY_POLL, &status, np);
>> +            return IS_ERR(phy) ? PTR_ERR(phy) : 0;
>> +        }
>> +    }
>>
>> ---
>> which is the hunk added to the of_phy_register_fixed_link().
>> So in that case we register fixed-phy, but do not parse the fixed-link.
> Ok, I missed that part. Could not you just override everything that is
> needed here to get past the point where you register your fixed PHY even
> with link = 0, this will be discarded anyway once you start in-band
> negotiation.
Maybe my English is bad, but I have problems understanding
some of your senteneces. What do you mean?
If you meant to re-use the existing registration code instead
of adding a new hunk, please note that there is no fixed-link
node at all, so we do not even enter the parsing code block.
As such, there is nothing to override.

> I will work on something anyway. 
Thanks, hope to hear from you soon.
This stream of regressions is disturbing. :)
Should finally be fixed for real.