enable inband link state negotiation only when explicitly requested
Subsystems (7)
cover enable inband link state negotiation only when explicitly requested
Hello.
Currently the link status auto-negotiation is enabled for any SGMII link with fixed-link DT binding.
The regression was reported:
https://lkml.org/lkml/2015/7/8/865
Apparently not all HW that implements SGMII protocol, generates the inband status for the auto-negotiation to work.
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
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.
--- .../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(-)
Documentation/devicetree/bindings/net/fixed-link.txt +7 -1
@@ -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
drivers/of/of_mdio.c +45 -3
62 lines not on this page · raw message
include/linux/of_mdio.h +5 -0
@@ -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
Signature · 1 line
1.9.1
Re: [PATCH 1/2] of_mdio: add new DT property 'link' for fixed-link
(there is no such thing as [email protected], please remove it from your future submissions).
Quoted 2 lines · Stas Sergeev
Currently for fixed-link the link state is always set to UP.
Not quite true, this is always a driver decision to make.
Quoted 3 lines
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)?
Quoted 2 lines
"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.
Quoted 3 lines
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.
Quoted 91 lines
Signed-off-by Stas Sergeev <[email protected]>CC Rob Herring <[email protected]>CC Pawel Moll <[email protected]>CC Mark Rutland <[email protected]>CC Ian Campbell <[email protected]>CC Kumar Gala <[email protected]>CC Florian Fainelli <[email protected]>CC Grant Likely <[email protected]>--- .../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(-)Documentation/devicetree/bindings/net/fixed-link.txt +7
-1@@ -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 bedrivers/of/of_mdio.c +36
-1@@ -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 ; + }
drivers/of/of_mdio.c int of_phy_register_fixed_link(struct device_node *np) · line 337
Quoted 10 lines
drivers/of/of_mdio.c +6
-2status.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.
Signature · 1 line
Florian
Re: [PATCH 1/2] of_mdio: add new DT property 'link' for fixed-link
09.07.2015 21:24, Florian Fainelli пишет:
Quoted 6 lines
(there is no such thing as [email protected], please remove it from your future submissions).
Quoted 1 line · Stas Sergeev
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
---
Quoted 6 lines
Quoted 3 lines
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?
Quoted 3 lines
Quoted 2 lines
"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.
Quoted 5 lines
Quoted 3 lines
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.
Quoted 9 lines
drivers/of/of_mdio.c int of_phy_register_fixed_link(struct device_node *np) · line 337
Quoted 7 lines
drivers/of/of_mdio.c +6
-1-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.
Quoted 3 lines
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 1/2] of_mdio: add new DT property 'link' for fixed-link
Quoted 7 lines · Stas Sergeev
09.07.2015 21:24, Florian Fainelli пишет:
Quoted 6 lines
(there is no such thing as [email protected], please remove it from your future submissions).
Quoted 1 line · Stas Sergeev
Currently for fixed-link the link state is always set to UP.
Not quite true, this is always a driver decision to make.
Quoted 8 lines
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.
Quoted 11 lines
Quoted 7 lines
Quoted 4 lines
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.
Quoted 10 lines
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.
Quoted 9 lines
Quoted 4 lines
Quoted 3 lines
"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.
Quoted 3 lines
Quoted 3 lines
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.
Quoted 8 lines
Quoted 2 lines
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?
Quoted 21 lines
Quoted 10 lines
drivers/of/of_mdio.c int of_phy_register_fixed_link(struct device_node *np) · line 337
Quoted 8 lines
drivers/of/of_mdio.c +6
-1-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.Quoted 3 lines
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.
Signature · 1 line
Florian
Re: [PATCH 1/2] of_mdio: add new DT property 'link' for fixed-link
10.07.2015 00:15, Florian Fainelli пишет:
Quoted 26 lines · Stas Sergeev
Quoted 7 lines
09.07.2015 21:24, Florian Fainelli пишет:
Quoted 6 lines
(there is no such thing as [email protected], please remove it from your future submissions).
Quoted 1 line · Stas Sergeev
Currently for fixed-link the link state is always set to UP.
Not quite true, this is always a driver decision to make.
Quoted 8 lines
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.
Quoted 6 lines
Quoted 6 lines
Quoted 3 lines
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)?
Quoted 7 lines
Quoted 4 lines
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;
Quoted 8 lines
Quoted 6 lines
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_.
Quoted 2 lines
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
Quoted 9 lines · Stas Sergeev
10.07.2015 00:15, Florian Fainelli пишет:
Quoted 2 lines
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_myownguess_.
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.
Signature · 1 line
Sébastien
Re: [PATCH 1/2] of_mdio: add new DT property 'link' for fixed-link
10.07.2015 11:46, Sebastien Rannou пишет:
Quoted 12 lines · Stas Sergeev
Quoted 10 lines
10.07.2015 00:15, Florian Fainelli пишет:
Quoted 2 lines
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_myownguess_.
Quoted 4 lines
Yes, I confirm that my HW does not generate an in-band status. AFAIK, it's a PHY that aggregates 4xSGMIIs to 1xQSGMII ; the MAC side of the PHY (with inband status) is connected to the switch through QSGMII, and in this context we are on the media side of the PHY.
Hmm, interesting.
So if I parse the above correctly, you have something like 88E1340S set up into a mode when SGMII is used as media interface and QSGMII as system interface (terms are from datasheet page 5), then you connect the media interface to armada-xp and system interface to the switch.
I wonder if it is the right thing to do.
AFAIK you could as well set up armada-xp into QSGMII mode and connect that to switch. The driver would then disable the use of inband status and everything would be fine.
Either way, your use-case proves that only DT can decide the use of an inband status.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] of_mdio: add new DT property 'link' for fixed-link
Quoted 23 lines · Stas Sergeev
10.07.2015 11:46, Sebastien Rannou пишет:
Quoted 12 lines · Stas Sergeev
Quoted 10 lines
10.07.2015 00:15, Florian Fainelli пишет:
Quoted 2 lines
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_myownguess_.Quoted 4 lines
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.
Quoted 6 lines
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?
Signature · 1 line
Florian
PATCH 2/2 mvneta: use inband status only when link type is "auto"
In reply to Stas SergeevThe 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.
--- drivers/net/ethernet/marvell/mvneta.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/net/ethernet/marvell/mvneta.c +3 -3
@@ -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)) {
Signature · 1 line
1.9.1
Re: [PATCH 2/2] mvneta: use inband status only when link type is "auto"
Quoted 6 lines · 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.
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...?
Quoted 52 lines
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 <[email protected]>--- drivers/net/ethernet/marvell/mvneta.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)drivers/net/ethernet/marvell/mvneta.c +3
-3@@ -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)) {
Signature · 1 line
Florian
Re: [PATCH 2/2] mvneta: use inband status only when link type is "auto"
09.07.2015 21:18, Florian Fainelli пишет:
Quoted 9 lines · Stas Sergeev
Quoted 5 lines
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 2/2] mvneta: use inband status only when link type is "auto"
Quoted 16 lines · Stas Sergeev
09.07.2015 21:18, Florian Fainelli пишет:
Quoted 9 lines · Stas Sergeev
Quoted 8 lines
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.Quoted 3 lines
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:
Quoted 6 lines
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.
Signature · 1 line
Florian
Re: [PATCH 2/2] mvneta: use inband status only when link type is "auto"
10.07.2015 00:14, Florian Fainelli пишет:
Quoted 21 lines · Stas Sergeev
Quoted 20 lines
09.07.2015 21:18, Florian Fainelli пишет:
Quoted 9 lines · Stas Sergeev
Quoted 8 lines
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.Quoted 3 lines
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"; };
Quoted 6 lines
Quoted 2 lines
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.
Hi Stas,
Quoted 2 lines · Stas Sergeev
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).
Thanks!
Signature · 1 line
Sébastien