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

[PATCH 0/2] 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.

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/2] of_mdio: add new DT property 'link' for fixed-link

[Stas Sergeev]

Currently for fixed-link the link state is always set to UP.
This patch introduces the new property 'link' that accepts the
following string arguments: "up", "down" and "auto".
"down" may be needed if the link is physically unconnected.
"auto" is needed to enable the link paramaters auto-negotiation,
that is built into some MII protocols, namely SGMII.
The appropriate documentation is added and explicitly states that
"auto" is very specific (protocol, HW and driver-specific), and
is therefore should be used with care.

Signed-off-by: Stas Sergeev <stsp@users.sourceforge.net>

CC: Rob Herring <robh+dt@kernel.org>
CC: Pawel Moll <pawel.moll@arm.com>
CC: Mark Rutland <mark.rutland@arm.com>
CC: Ian Campbell <ijc+devicetree@hellion.org.uk>
CC: Kumar Gala <galak@codeaurora.org>
CC: Florian Fainelli <f.fainelli@gmail.com>
CC: Grant Likely <grant.likely@linaro.org>
CC: devicetree@vger.kernel.org
CC: linux-kernel@vger.kernel.org
CC: netdev@vger.kernel.org
---
 .../devicetree/bindings/net/fixed-link.txt         |  8 +++-
 drivers/of/of_mdio.c                               | 48 ++++++++++++++++++++--
 include/linux/of_mdio.h                            |  5 +++
 3 files changed, 57 insertions(+), 4 deletions(-)

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

+* 'link' (string, optional), to indicate the link state. Accepted
+  values are "up", "down" and "auto". "auto" means auto-negotiation of
+  link parameters. Auto-negotiation is MII protocol, HW and driver-specific
+  and is not supported in many cases, so use it only when you know what
+  you do.
 * 'speed' (integer, mandatory), to indicate the link speed. Accepted
-  values are 10, 100 and 1000
+  values are 10, 100 and 1000. If the 'link' property is set to 'auto',
+  'speed' may not be set. It will then be auto-negotiated, if possible.
 * 'full-duplex' (boolean, optional), to indicate that full duplex is
   used. When absent, half duplex is assumed.
 * 'pause' (boolean, optional), to indicate that pause should be
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index 1bd4305..2152cf8 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -280,6 +280,26 @@ bool of_phy_is_fixed_link(struct device_node *np)
 }
 EXPORT_SYMBOL(of_phy_is_fixed_link);

+bool of_phy_is_autoneg_link(struct device_node *np)
+{
+	struct device_node *dn;
+	const char *link_str;
+	int rc;
+	bool ret = false;
+
+	dn = of_get_child_by_name(np, "fixed-link");
+	if (!dn)
+		return false;
+
+	rc = of_property_read_string(dn, "link", &link_str);
+	if (rc == 0 && strcmp(link_str, "auto") == 0)
+		ret = true;
+
+	of_node_put(dn);
+	return ret;
+}
+EXPORT_SYMBOL(of_phy_is_autoneg_link);
+
 int of_phy_register_fixed_link(struct device_node *np)
 {
 	struct fixed_phy_status status = {};
@@ -291,11 +311,33 @@ int of_phy_register_fixed_link(struct device_node *np)
 	/* New binding */
 	fixed_link_node = of_get_child_by_name(np, "fixed-link");
 	if (fixed_link_node) {
-		status.link = 1;
+		const char *link_str;
+		int ret;
+		bool link_auto = false;
+
+		ret = of_property_read_string(fixed_link_node, "link",
+					      &link_str);
+		if (ret == 0) {
+			if (strcmp(link_str, "up") == 0)
+				status.link = 1;
+			else
+				status.link = 0;
+			if (strcmp(link_str, "auto") == 0)
+				link_auto = true;
+		} else {
+			status.link = 1;
+		}
 		status.duplex = of_property_read_bool(fixed_link_node,
 						      "full-duplex");
-		if (of_property_read_u32(fixed_link_node, "speed", &status.speed))
-			return -EINVAL;
+		if (of_property_read_u32(fixed_link_node, "speed",
+					 &status.speed) != 0) {
+			/* in auto mode just set to some sane value:
+			 * it will be changed by MAC later */
+			if (link_auto)
+				status.speed = 1000;
+			else
+				return -EINVAL;
+		}
 		status.pause = of_property_read_bool(fixed_link_node, "pause");
 		status.asym_pause = of_property_read_bool(fixed_link_node,
 							  "asym-pause");
diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h
index d449018..647f348 100644
--- a/include/linux/of_mdio.h
+++ b/include/linux/of_mdio.h
@@ -65,6 +65,7 @@ static inline struct mii_bus *of_mdio_find_bus(struct device_node *mdio_np)
 #if defined(CONFIG_OF) && defined(CONFIG_FIXED_PHY)
 extern int of_phy_register_fixed_link(struct device_node *np);
 extern bool of_phy_is_fixed_link(struct device_node *np);
+extern bool of_phy_is_autoneg_link(struct device_node *np);
 #else
 static inline int of_phy_register_fixed_link(struct device_node *np)
 {
@@ -74,6 +75,10 @@ static inline bool of_phy_is_fixed_link(struct device_node *np)
 {
 	return false;
 }
+static inline bool of_phy_is_autoneg_link(struct device_node *np)
+{
+	return false;
+}
 #endif


-- 
1.9.1

[PATCH 2/2] mvneta: use inband status only when link type is "auto"

[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 'link' DT property
is set to "auto".
For old configurations where the 'link' 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 | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 74176ec..b7f4d7f 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.
@@ -3068,7 +3068,7 @@ static int mvneta_probe(struct platform_device *pdev)
 	pp->phy_node = phy_node;
 	pp->phy_interface = phy_mode;
 	pp->use_inband_status = (phy_mode == PHY_INTERFACE_MODE_SGMII) &&
-				fixed_phy;
+				autoneg_link;

 	pp->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(pp->clk)) {
-- 
1.9.1

Re: [PATCH 2/2] mvneta: use inband status only when link type is "auto"

[Florian Fainelli]

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

What is the purpose of using the in-band status in the first place if
you end-up having to specify a 'fixed-link' property which contains most
of the link parameters: speed, duplex etc...?

> 
> This patch enables the auto-negotiation only if the 'link' DT property
> is set to "auto".
> For old configurations where the 'link' 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 | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
> index 74176ec..b7f4d7f 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.
> @@ -3068,7 +3068,7 @@ static int mvneta_probe(struct platform_device *pdev)
>  	pp->phy_node = phy_node;
>  	pp->phy_interface = phy_mode;
>  	pp->use_inband_status = (phy_mode == PHY_INTERFACE_MODE_SGMII) &&
> -				fixed_phy;
> +				autoneg_link;
> 
>  	pp->clk = devm_clk_get(&pdev->dev, NULL);
>  	if (IS_ERR(pp->clk)) {
> 


-- 
Florian

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

[Florian Fainelli]

(there is no such thing as linux-net@vger.kernel.org, please remove it
from your future submissions).

On 09/07/15 10:38, Stas Sergeev wrote:
> 
> Currently for fixed-link the link state is always set to UP.

Not quite true, this is always a driver decision to make.

> This patch introduces the new property 'link' that accepts the
> following string arguments: "up", "down" and "auto".
> "down" may be needed if the link is physically unconnected.

In which case you probably do not even care about inserting such a
property in the first place, do you? What would be the value of forcibly
having a link permanently down (not counting loopback)?

> "auto" is needed to enable the link paramaters auto-negotiation,
> that is built into some MII protocols, namely SGMII.

RGMII also has an in-band status FWIW.

> The appropriate documentation is added and explicitly states that
> "auto" is very specific (protocol, HW and driver-specific), and
> is therefore should be used with care.

And therefore probably be made a device (and driver) specific decision
whether this is the right thing to do.

I do no think this addition to the "fixed-link" property is desirable
the way you have defined it.

More comments below.

> 
> Signed-off-by: Stas Sergeev <stsp@users.sourceforge.net>
> 
> CC: Rob Herring <robh+dt@kernel.org>
> CC: Pawel Moll <pawel.moll@arm.com>
> CC: Mark Rutland <mark.rutland@arm.com>
> CC: Ian Campbell <ijc+devicetree@hellion.org.uk>
> CC: Kumar Gala <galak@codeaurora.org>
> CC: Florian Fainelli <f.fainelli@gmail.com>
> CC: Grant Likely <grant.likely@linaro.org>
> CC: devicetree@vger.kernel.org
> CC: linux-kernel@vger.kernel.org
> CC: netdev@vger.kernel.org
> ---
>  .../devicetree/bindings/net/fixed-link.txt         |  8 +++-
>  drivers/of/of_mdio.c                               | 48 ++++++++++++++++++++--
>  include/linux/of_mdio.h                            |  5 +++
>  3 files changed, 57 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/fixed-link.txt b/Documentation/devicetree/bindings/net/fixed-link.txt
> index 82bf7e0..070f554 100644
> --- a/Documentation/devicetree/bindings/net/fixed-link.txt
> +++ b/Documentation/devicetree/bindings/net/fixed-link.txt
> @@ -9,8 +9,14 @@ Such a fixed link situation is described by creating a 'fixed-link'
>  sub-node of the Ethernet MAC device node, with the following
>  properties:
> 
> +* 'link' (string, optional), to indicate the link state. Accepted
> +  values are "up", "down" and "auto". "auto" means auto-negotiation of
> +  link parameters. Auto-negotiation is MII protocol, HW and driver-specific
> +  and is not supported in many cases, so use it only when you know what
> +  you do.
>  * 'speed' (integer, mandatory), to indicate the link speed. Accepted
> -  values are 10, 100 and 1000
> +  values are 10, 100 and 1000. If the 'link' property is set to 'auto',
> +  'speed' may not be set. It will then be auto-negotiated, if possible.
>  * 'full-duplex' (boolean, optional), to indicate that full duplex is
>    used. When absent, half duplex is assumed.
>  * 'pause' (boolean, optional), to indicate that pause should be
> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> index 1bd4305..2152cf8 100644
> --- a/drivers/of/of_mdio.c
> +++ b/drivers/of/of_mdio.c
> @@ -280,6 +280,26 @@ bool of_phy_is_fixed_link(struct device_node *np)
>  }
>  EXPORT_SYMBOL(of_phy_is_fixed_link);
> 
> +bool of_phy_is_autoneg_link(struct device_node *np)
> +{
> +	struct device_node *dn;
> +	const char *link_str;
> +	int rc;
> +	bool ret = false;
> +
> +	dn = of_get_child_by_name(np, "fixed-link");
> +	if (!dn)
> +		return false;
> +
> +	rc = of_property_read_string(dn, "link", &link_str);
> +	if (rc == 0 && strcmp(link_str, "auto") == 0)
> +		ret = true;
> +
> +	of_node_put(dn);
> +	return ret;
> +}
> +EXPORT_SYMBOL(of_phy_is_autoneg_link);
> +
>  int of_phy_register_fixed_link(struct device_node *np)
>  {
>  	struct fixed_phy_status status = {};
> @@ -291,11 +311,33 @@ int of_phy_register_fixed_link(struct device_node *np)
>  	/* New binding */
>  	fixed_link_node = of_get_child_by_name(np, "fixed-link");
>  	if (fixed_link_node) {
> -		status.link = 1;
> +		const char *link_str;
> +		int ret;
> +		bool link_auto = false;
> +
> +		ret = of_property_read_string(fixed_link_node, "link",
> +					      &link_str);
> +		if (ret == 0) {
> +			if (strcmp(link_str, "up") == 0)
> +				status.link = 1;
> +			else
> +				status.link = 0;
> +			if (strcmp(link_str, "auto") == 0)
> +				link_auto = true;
> +		} else {
> +			status.link = 1;
> +		}
>  		status.duplex = of_property_read_bool(fixed_link_node,
>  						      "full-duplex");
> -		if (of_property_read_u32(fixed_link_node, "speed", &status.speed))
> -			return -EINVAL;
> +		if (of_property_read_u32(fixed_link_node, "speed",
> +					 &status.speed) != 0) {
> +			/* in auto mode just set to some sane value:
> +			 * it will be changed by MAC later */
> +			if (link_auto)
> +				status.speed = 1000;

This is a completely arbitrary speed, that does not more or less sense
than defaulting to 100 or anything else, a driver should be able to set
the speed it wants, based on the parsing of a 'phy-mode' property for
instance. 1000 does not make sense on e.g: MII links.
-- 
Florian

Re: [PATCH 2/2] mvneta: use inband status only when link type is "auto"

[Stas Sergeev]

09.07.2015 21:18, Florian Fainelli пишет:
> On 09/07/15 10:41, Stas Sergeev wrote:
>> 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.
> What is the purpose of using the in-band status in the first place if
> you end-up having to specify a 'fixed-link' property which contains most
> of the link parameters: speed, duplex etc...?
You don't have to.
My config from today is as simple as:

fixed-link {
   link = "auto";
};

and that's all.
Without my today's patch, only 'speed' is a mandatory - not too much.

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

[Stas Sergeev]

09.07.2015 21:24, Florian Fainelli пишет:
> (there is no such thing as linux-net@vger.kernel.org, please remove it
> from your future submissions).
>
> On 09/07/15 10:38, Stas Sergeev wrote:
>> Currently for fixed-link the link state is always set to UP.
> Not quite true, this is always a driver decision to make.
But what about this part of of_mdio.c:of_phy_register_fixed_link():
---

  	fixed_link_node = of_get_child_by_name(np, "fixed-link");
  	if (fixed_link_node) {
		status.link = 1

---

>> This patch introduces the new property 'link' that accepts the
>> following string arguments: "up", "down" and "auto".
>> "down" may be needed if the link is physically unconnected.
> In which case you probably do not even care about inserting such a
> property in the first place, do you? What would be the value of forcibly
> having a link permanently down (not counting loopback)?
The DTs have a common parts that are included by other
parts. So if you include the definition of your SoC that have
all ethernets defined, and you only set up the external things
like PHYs, then I would see a potential use for "down".
Other than that, it is probably not a big deal.
Please note that I haven't even hard-coded it anywhere:
whatever is not "up" or "auto", is down.
I can remove it from the description if you think that way,
but I'd rather leave it for consistency and for a small but
possible use. Eg my board has 4 ethernets and only 2 are
connected. I feel its right to include the SoC definition and
set the unconnected ones to "down", but other approaches
are possible too.
Should I remove it?

>> "auto" is needed to enable the link paramaters auto-negotiation,
>> that is built into some MII protocols, namely SGMII.
> RGMII also has an in-band status FWIW.
Thanks, will take that into account in v2.

>> The appropriate documentation is added and explicitly states that
>> "auto" is very specific (protocol, HW and driver-specific), and
>> is therefore should be used with care.
> And therefore probably be made a device (and driver) specific decision
> whether this is the right thing to do.
This doesn't work.
It appears even if the driver supports it and wants to use it, the
PHY HW may simply not generate the inband status. This is actually
the whole point why we have a regression now. It is _currently_
a driver decision, and that doesn't work for some people.
The point of this patch set is to make it a DT decision instead.

>> -			return -EINVAL;
>> +		if (of_property_read_u32(fixed_link_node, "speed",
>> +					 &status.speed) != 0) {
>> +			/* in auto mode just set to some sane value:
>> +			 * it will be changed by MAC later */
>> +			if (link_auto)
>> +				status.speed = 1000;
> This is a completely arbitrary speed, that does not more or less sense
> than defaulting to 100 or anything else,
Exactly.
But if I leave it to 0, then fixed-phy driver will return an error,
so I took an arbitrary value.
But if it obscures the code, I'll hack fixed-phy to accept 0 instead,
to get something cleaner. So in v2.

>   a driver should be able to set
> the speed it wants, based on the parsing of a 'phy-mode' property for
> instance.
It actually does, that value is just to "cheat" fixed-phy.
I'll make things more obvious next time.

Re: [PATCH 2/2] mvneta: use inband status only when link type is "auto"

[Florian Fainelli]

On 09/07/15 13:26, Stas Sergeev wrote:
> 09.07.2015 21:18, Florian Fainelli пишет:
>> On 09/07/15 10:41, Stas Sergeev wrote:
>>> 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.
>> What is the purpose of using the in-band status in the first place if
>> you end-up having to specify a 'fixed-link' property which contains most
>> of the link parameters: speed, duplex etc...?
> You don't have to.
> My config from today is as simple as:
> 
> fixed-link {
>   link = "auto";
> };
> 
> and that's all.
> Without my today's patch, only 'speed' is a mandatory - not too much.

That makes me think that 'fixed-link' is not exactly what you want then,
you would probably want something like "marvell,use-in-band-status" or
something like this. It could be a more generic property that is not
Marvell specific after all, that would be fine.
-- 
Florian

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

[Florian Fainelli]

On 09/07/15 13:43, Stas Sergeev wrote:
> 09.07.2015 21:24, Florian Fainelli пишет:
>> (there is no such thing as linux-net@vger.kernel.org, please remove it
>> from your future submissions).
>>
>> On 09/07/15 10:38, Stas Sergeev wrote:
>>> Currently for fixed-link the link state is always set to UP.
>> Not quite true, this is always a driver decision to make.
> But what about this part of of_mdio.c:of_phy_register_fixed_link():
> ---
> 
>      fixed_link_node = of_get_child_by_name(np, "fixed-link");
>      if (fixed_link_node) {
>         status.link = 1
> 
> ---

This seems like a logical consequence of finding a "fixed-link" property
for the DT node of interest. If no such property exist, then we do not
set anything.

> 
>>> This patch introduces the new property 'link' that accepts the
>>> following string arguments: "up", "down" and "auto".
>>> "down" may be needed if the link is physically unconnected.
>> In which case you probably do not even care about inserting such a
>> property in the first place, do you? What would be the value of forcibly
>> having a link permanently down (not counting loopback)?
> The DTs have a common parts that are included by other
> parts. So if you include the definition of your SoC that have
> all ethernets defined, and you only set up the external things
> like PHYs, then I would see a potential use for "down".

"down" is equivalent to using a status = "disabled", in fact the latter
is much better since you can even conserve energy and resources by not
enabling something which is not usable.

> Other than that, it is probably not a big deal.
> Please note that I haven't even hard-coded it anywhere:
> whatever is not "up" or "auto", is down.
> I can remove it from the description if you think that way,
> but I'd rather leave it for consistency and for a small but
> possible use. Eg my board has 4 ethernets and only 2 are
> connected. I feel its right to include the SoC definition and
> set the unconnected ones to "down", but other approaches
> are possible too.
> Should I remove it?

What you describe about your board is the perfect example of how a
"status" property should be used.

> 
>>> "auto" is needed to enable the link paramaters auto-negotiation,
>>> that is built into some MII protocols, namely SGMII.
>> RGMII also has an in-band status FWIW.
> Thanks, will take that into account in v2.
> 
>>> The appropriate documentation is added and explicitly states that
>>> "auto" is very specific (protocol, HW and driver-specific), and
>>> is therefore should be used with care.
>> And therefore probably be made a device (and driver) specific decision
>> whether this is the right thing to do.
> This doesn't work.
> It appears even if the driver supports it and wants to use it, the
> PHY HW may simply not generate the inband status. This is actually
> the whole point why we have a regression now. It is _currently_
> a driver decision, and that doesn't work for some people.
> The point of this patch set is to make it a DT decision instead.

Then, if the in-band status indication is not reliable (which really
should be completely understood), you can just ignore the in-band status
and use all the parameter in a 'fixed-link' property, should not we?

If in-band status can be used, then you can decide this with a separate
property which is not in 'fixed-link', would that seem reasonable?

> 
>>> -            return -EINVAL;
>>> +        if (of_property_read_u32(fixed_link_node, "speed",
>>> +                     &status.speed) != 0) {
>>> +            /* in auto mode just set to some sane value:
>>> +             * it will be changed by MAC later */
>>> +            if (link_auto)
>>> +                status.speed = 1000;
>> This is a completely arbitrary speed, that does not more or less sense
>> than defaulting to 100 or anything else,
> Exactly.
> But if I leave it to 0, then fixed-phy driver will return an error,
> so I took an arbitrary value.
> But if it obscures the code, I'll hack fixed-phy to accept 0 instead,
> to get something cleaner. So in v2.
> 
>>   a driver should be able to set
>> the speed it wants, based on the parsing of a 'phy-mode' property for
>> instance.
> It actually does, that value is just to "cheat" fixed-phy.
> I'll make things more obvious next time.


-- 
Florian

Re: [PATCH 2/2] mvneta: use inband status only when link type is "auto"

[Stas Sergeev]

10.07.2015 00:14, Florian Fainelli пишет:
> On 09/07/15 13:26, Stas Sergeev wrote:
>> 09.07.2015 21:18, Florian Fainelli пишет:
>>> On 09/07/15 10:41, Stas Sergeev wrote:
>>>> 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.
>>> What is the purpose of using the in-band status in the first place if
>>> you end-up having to specify a 'fixed-link' property which contains most
>>> of the link parameters: speed, duplex etc...?
>> You don't have to.
>> My config from today is as simple as:
>>
>> fixed-link {
>>    link = "auto";
>> };
>>
>> and that's all.
>> Without my today's patch, only 'speed' is a mandatory - not too much.
> That makes me think that 'fixed-link' is not exactly what you want then,
> you would probably want something like "marvell,use-in-band-status" or
> something like this. It could be a more generic property that is not
> Marvell specific after all, that would be fine.
I think there is some confusion around fixed-link, because
of its name. This is what fixed-link is:
---
Some Ethernet MACs have a "fixed link", and are not connected to a
normal MDIO-managed PHY device.
---
A bit vague, but to me it means "non-MDIO", and that's all.
If we make it like "marvell,use-in-band-status", then it will
suddenly cancel everything in a fixed-link definition, which
is non obvious. Or, if we make it so that fixed-link def is
not needed in presence of "marvell,use-in-band-status", then
this "marvell,use-in-band-status" will have to silently enable
the fixed-phy driver the way fixed-link does.
If we just view fixed-link as non-MDIO link, then everything
fits, IMHO.

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

[Stas Sergeev]

10.07.2015 00:15, Florian Fainelli пишет:
> On 09/07/15 13:43, Stas Sergeev wrote:
>> 09.07.2015 21:24, Florian Fainelli пишет:
>>> (there is no such thing as linux-net@vger.kernel.org, please remove it
>>> from your future submissions).
>>>
>>> On 09/07/15 10:38, Stas Sergeev wrote:
>>>> Currently for fixed-link the link state is always set to UP.
>>> Not quite true, this is always a driver decision to make.
>> But what about this part of of_mdio.c:of_phy_register_fixed_link():
>> ---
>>
>>       fixed_link_node = of_get_child_by_name(np, "fixed-link");
>>       if (fixed_link_node) {
>>          status.link = 1
>>
>> ---
> This seems like a logical consequence of finding a "fixed-link" property
> for the DT node of interest. If no such property exist, then we do not
> set anything.
>
>>>> This patch introduces the new property 'link' that accepts the
>>>> following string arguments: "up", "down" and "auto".
>>>> "down" may be needed if the link is physically unconnected.
>>> In which case you probably do not even care about inserting such a
>>> property in the first place, do you? What would be the value of forcibly
>>> having a link permanently down (not counting loopback)?
>> The DTs have a common parts that are included by other
>> parts. So if you include the definition of your SoC that have
>> all ethernets defined, and you only set up the external things
>> like PHYs, then I would see a potential use for "down".
> "down" is equivalent to using a status = "disabled", in fact the latter
> is much better since you can even conserve energy and resources by not
> enabling something which is not usable.
OK, agree.
So I'll probably go for something like
autoneg = 1 | 0;

>> This doesn't work.
>> It appears even if the driver supports it and wants to use it, the
>> PHY HW may simply not generate the inband status. This is actually
>> the whole point why we have a regression now. It is _currently_
>> a driver decision, and that doesn't work for some people.
>> The point of this patch set is to make it a DT decision instead.
> Then, if the in-band status indication is not reliable (which really
> should be completely understood),
Agree!
But this is not something I can help with.
Sebastien Rannou reports the problem, please ask him whatever
you see fits to get a better understanding of a problem.
The fact that his HW does not generate the inband status, is
_my own guess_.

>   you can just ignore the in-band status
> and use all the parameter in a 'fixed-link' property, should not we?
I don't think there is any way at all to find out if the inband stat is
usable or not. I think only the user (or manufacturer) can decide
on that. If there is any way to do a guess-work, that would be an
entirely different story.

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

[Sebastien Rannou]

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

On Fri, 10 Jul 2015, Stas Sergeev wrote:

> 10.07.2015 00:15, Florian Fainelli пишет:
> > Then, if the in-band status indication is not reliable (which really
> > should be completely understood),
> Agree!
> But this is not something I can help with.
> Sebastien Rannou reports the problem, please ask him whatever
> you see fits to get a better understanding of a problem.
> The fact that his HW does not generate the inband status, is
> _my own guess_.

Yes, I confirm that my HW does not generate an in-band status. AFAIK, it's
a PHY that aggregates 4xSGMIIs to 1xQSGMII ; the MAC side of the PHY (with
inband status) is connected to the switch through QSGMII, and in this context
we are on the media side of the PHY.

-- 
Sébastien

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

[Stas Sergeev]

10.07.2015 11:46, Sebastien Rannou пишет:
> On Fri, 10 Jul 2015, Stas Sergeev wrote:
> 
>> 10.07.2015 00:15, Florian Fainelli пишет:
>>> Then, if the in-band status indication is not reliable (which really
>>> should be completely understood),
>> Agree!
>> But this is not something I can help with.
>> Sebastien Rannou reports the problem, please ask him whatever
>> you see fits to get a better understanding of a problem.
>> The fact that his HW does not generate the inband status, is
>> _my own guess_.
> 
> Yes, I confirm that my HW does not generate an in-band status. AFAIK, it's
> a PHY that aggregates 4xSGMIIs to 1xQSGMII ; the MAC side of the PHY (with
> inband status) is connected to the switch through QSGMII, and in this context
> we are on the media side of the PHY.
Hmm, interesting.
So if I parse the above correctly, you have something like 88E1340S set
up into a mode when SGMII is used as media interface and QSGMII as system
interface (terms are from datasheet page 5), then you connect the media
interface to armada-xp and system interface to the switch.

I wonder if it is the right thing to do.
AFAIK you could as well set up armada-xp into QSGMII mode and connect
that to switch. The driver would then disable the use of inband status
and everything would be fine.
Either way, your use-case proves that only DT can decide the use of an
inband status.

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

[Sebastien Rannou]

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

Hi Stas,

On Thu, 9 Jul 2015, Stas Sergeev wrote:

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

I confirm that it fixes the issue I had (without modification of the DT on
my side).

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

Thanks!

-- 
Sébastien

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

[Florian Fainelli]

On 10/07/15 04:20, Stas Sergeev wrote:
> 10.07.2015 11:46, Sebastien Rannou пишет:
>> On Fri, 10 Jul 2015, Stas Sergeev wrote:
>>
>>> 10.07.2015 00:15, Florian Fainelli пишет:
>>>> Then, if the in-band status indication is not reliable (which really
>>>> should be completely understood),
>>> Agree!
>>> But this is not something I can help with.
>>> Sebastien Rannou reports the problem, please ask him whatever
>>> you see fits to get a better understanding of a problem.
>>> The fact that his HW does not generate the inband status, is
>>> _my own guess_.
>>
>> Yes, I confirm that my HW does not generate an in-band status. AFAIK, it's
>> a PHY that aggregates 4xSGMIIs to 1xQSGMII ; the MAC side of the PHY (with
>> inband status) is connected to the switch through QSGMII, and in this context
>> we are on the media side of the PHY.
> Hmm, interesting.
> So if I parse the above correctly, you have something like 88E1340S set
> up into a mode when SGMII is used as media interface and QSGMII as system
> interface (terms are from datasheet page 5), then you connect the media
> interface to armada-xp and system interface to the switch.
> 
> I wonder if it is the right thing to do.
> AFAIK you could as well set up armada-xp into QSGMII mode and connect
> that to switch. The driver would then disable the use of inband status
> and everything would be fine.
> Either way, your use-case proves that only DT can decide the use of an
> inband status.

I do not think there is any debate around the need for a property that
defines whether in-band-status is both reliable and usable, the debate
is about *where* to put it.

I still think this does not belong in the fixed-link property, but now
that you have explained a bit more in the other patch what your
understanding of "fixed-link" is, I can see the confusion.

Instead of having a link = "auto", property, how about just something
like this:

fixed-link {
	speed = <1000>;
	full-duplex;
	use-in-band-status;
};

or event this:

fixed-link {
	use-in-band-status;
};

If you parse the 'use-in-band-status' which means that it is reliable
information, then you can override whatever was defined in the DT under
the 'fixed-link' property?
-- 
Florian