From: Sean Anderson <hidden> Date: 2021-10-25 17:24:25
There were several cases where validate() would return bogus supported
modes with unusual combinations of interfaces and capabilities. For
example, if state->interface was 10GBASER and the macb had HIGH_SPEED
and PCS but not GIGABIT MODE, then 10/100 modes would be set anyway. In
another case, SGMII could be enabled even if the mac was not a GEM
(despite this being checked for later on in mac_config()). These
inconsistencies make it difficult to refactor this function cleanly.
This attempts to address these by reusing the same conditions used to
decide whether to return early when setting mode bits. The logic is
pretty messy, but this preserves the existing logic where possible.
Signed-off-by: Sean Anderson <redacted>
---
Changes in v4:
- Drop cleanup patch
Changes in v3:
- Order bugfix patch first
Changes in v2:
- New
drivers/net/ethernet/cadence/macb_main.c | 59 +++++++++++++++++-------
1 file changed, 42 insertions(+), 17 deletions(-)
@@ -510,11 +510,16 @@ static void macb_validate(struct phylink_config *config,unsignedlong*supported,structphylink_link_state*state){+boolhave_1g=true,have_10g=true;structnet_device*ndev=to_net_dev(config->dev);__ETHTOOL_DECLARE_LINK_MODE_MASK(mask)={0,};structmacb*bp=netdev_priv(ndev);-/* We only support MII, RMII, GMII, RGMII & SGMII. */+/* There are three major types of interfaces we support:+*-(R)MIIsupporting10/100Mbit/s+*-GMII,RGMII,andSGMIIsupporting10/100/1000Mbit/s+*-10GBASERsupporting10Gbit/sonly+*/if(state->interface!=PHY_INTERFACE_MODE_NA&&state->interface!=PHY_INTERFACE_MODE_MII&&state->interface!=PHY_INTERFACE_MODE_RMII&&
@@ -526,27 +531,48 @@ static void macb_validate(struct phylink_config *config,return;}-if(!macb_is_gem(bp)&&-(state->interface==PHY_INTERFACE_MODE_GMII||-phy_interface_mode_is_rgmii(state->interface))){-linkmode_zero(supported);-return;+/* For 1G and up we must have both have a GEM and GIGABIT_MODE */+if(!macb_is_gem(bp)||+(bp->caps&MACB_CAPS_GIGABIT_MODE_AVAILABLE)){+if(state->interface==PHY_INTERFACE_MODE_GMII||+phy_interface_mode_is_rgmii(state->interface)||+state->interface==PHY_INTERFACE_MODE_SGMII||+state->interface==PHY_INTERFACE_MODE_10GBASER){+linkmode_zero(supported);+return;+}elseif(state->interface==PHY_INTERFACE_MODE_NA){+have_1g=false;+have_10g=false;+}}-if(state->interface==PHY_INTERFACE_MODE_10GBASER&&-!(bp->caps&MACB_CAPS_HIGH_SPEED&&-bp->caps&MACB_CAPS_PCS)){-linkmode_zero(supported);-return;+/* We need a PCS for SGMII and 10GBASER */+if(!(bp->caps&MACB_CAPS_PCS)){+if(state->interface==PHY_INTERFACE_MODE_SGMII||+state->interface==PHY_INTERFACE_MODE_10GBASER){+linkmode_zero(supported);+return;+}elseif(state->interface==PHY_INTERFACE_MODE_NA){+have_10g=false;+}+}++/* And for 10G that PCS must be high-speed */+if(!(bp->caps&MACB_CAPS_HIGH_SPEED)){+if(state->interface==PHY_INTERFACE_MODE_10GBASER){+linkmode_zero(supported);+return;+}elseif(state->interface==PHY_INTERFACE_MODE_NA){+have_10g=false;+}}phylink_set_port_modes(mask);phylink_set(mask,Autoneg);phylink_set(mask,Asym_Pause);-if(bp->caps&MACB_CAPS_GIGABIT_MODE_AVAILABLE&&-(state->interface==PHY_INTERFACE_MODE_NA||-state->interface==PHY_INTERFACE_MODE_10GBASER)){+if((state->interface==PHY_INTERFACE_MODE_NA||+state->interface==PHY_INTERFACE_MODE_10GBASER)&&have_10g){phylink_set_10g_modes(mask);phylink_set(mask,10000baseKR_Full);if(state->interface!=PHY_INTERFACE_MODE_NA)
From: "Russell King (Oracle)" <linux@armlinux.org.uk> Date: 2021-10-25 21:19:11
On Mon, Oct 25, 2021 at 01:24:05PM -0400, Sean Anderson wrote:
quoted hunk
There were several cases where validate() would return bogus supported
modes with unusual combinations of interfaces and capabilities. For
example, if state->interface was 10GBASER and the macb had HIGH_SPEED
and PCS but not GIGABIT MODE, then 10/100 modes would be set anyway. In
another case, SGMII could be enabled even if the mac was not a GEM
(despite this being checked for later on in mac_config()). These
inconsistencies make it difficult to refactor this function cleanly.
This attempts to address these by reusing the same conditions used to
decide whether to return early when setting mode bits. The logic is
pretty messy, but this preserves the existing logic where possible.
Signed-off-by: Sean Anderson <redacted>
---
Changes in v4:
- Drop cleanup patch
Changes in v3:
- Order bugfix patch first
Changes in v2:
- New
drivers/net/ethernet/cadence/macb_main.c | 59 +++++++++++++++++-------
1 file changed, 42 insertions(+), 17 deletions(-)
I think DaveM would ask for this to be reverse-christmas-tree, so the
new bool should be here.
quoted hunk
struct macb *bp = netdev_priv(ndev);
- /* We only support MII, RMII, GMII, RGMII & SGMII. */
+ /* There are three major types of interfaces we support:
+ * - (R)MII supporting 10/100 Mbit/s
+ * - GMII, RGMII, and SGMII supporting 10/100/1000 Mbit/s
+ * - 10GBASER supporting 10 Gbit/s only
+ */
if (state->interface != PHY_INTERFACE_MODE_NA &&
state->interface != PHY_INTERFACE_MODE_MII &&
state->interface != PHY_INTERFACE_MODE_RMII &&
@@ -526,27 +531,48 @@ static void macb_validate(struct phylink_config *config, return; }- if (!macb_is_gem(bp) &&- (state->interface == PHY_INTERFACE_MODE_GMII ||- phy_interface_mode_is_rgmii(state->interface))) {- linkmode_zero(supported);- return;+ /* For 1G and up we must have both have a GEM and GIGABIT_MODE */+ if (!macb_is_gem(bp) ||+ (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)) {+ if (state->interface == PHY_INTERFACE_MODE_GMII ||+ phy_interface_mode_is_rgmii(state->interface) ||+ state->interface == PHY_INTERFACE_MODE_SGMII ||+ state->interface == PHY_INTERFACE_MODE_10GBASER) {+ linkmode_zero(supported);+ return;+ } else if (state->interface == PHY_INTERFACE_MODE_NA) {+ have_1g = false;+ have_10g = false;+ } }
Would it make more sense to do:
bool have_1g = false, have_10g = false;
if (macb_is_gem(bp) &&
(bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)) {
if (bp->caps & MACB_CAPS_PCS)
have_1g = true;
if (bp->caps & MACB_CAPS_HIGH_SPEED)
have_10g = true;
}
switch (state->interface) {
case PHY_INTERFACE_MODE_NA:
case PHY_INTERFACE_MODE_MII:
case PHY_INTERFACE_MODE_RMII:
break;
case PHY_INTERFACE_MODE_GMII:
case PHY_INTERFACE_MODE_RGMII:
case PHY_INTERFACE_MODE_RGMII_ID:
case PHY_INTERFACE_MODE_RGMII_RXID:
case PHY_INTERFACE_MODE_RGMII_TXID:
case PHY_INTERFACE_MODE_SGMII:
if (!have_1g) {
linkmode_zero(supported);
return;
}
break;
case PHY_INTERFACE_MODE_10GBASER:
if (!have_10g) {
linkmode_zero(supported);
return;
}
break;
default:
linkmode_zero(supported);
return;
}
This uses positive logic to derive have_1g and have_10g, and then uses
the switch statement to validate against those. Would the above result
in more understandable code?
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
From: Sean Anderson <hidden> Date: 2021-10-25 21:35:48
On 10/25/21 5:19 PM, Russell King (Oracle) wrote:
On Mon, Oct 25, 2021 at 01:24:05PM -0400, Sean Anderson wrote:
quoted
There were several cases where validate() would return bogus supported
modes with unusual combinations of interfaces and capabilities. For
example, if state->interface was 10GBASER and the macb had HIGH_SPEED
and PCS but not GIGABIT MODE, then 10/100 modes would be set anyway. In
another case, SGMII could be enabled even if the mac was not a GEM
(despite this being checked for later on in mac_config()). These
inconsistencies make it difficult to refactor this function cleanly.
This attempts to address these by reusing the same conditions used to
decide whether to return early when setting mode bits. The logic is
pretty messy, but this preserves the existing logic where possible.
Signed-off-by: Sean Anderson <redacted>
---
Changes in v4:
- Drop cleanup patch
Changes in v3:
- Order bugfix patch first
Changes in v2:
- New
drivers/net/ethernet/cadence/macb_main.c | 59 +++++++++++++++++-------
1 file changed, 42 insertions(+), 17 deletions(-)
I think DaveM would ask for this to be reverse-christmas-tree, so the
new bool should be here.
Ah, I wasn't aware that there was another variable-ordering style in use for net.
quoted
struct macb *bp = netdev_priv(ndev);
- /* We only support MII, RMII, GMII, RGMII & SGMII. */
+ /* There are three major types of interfaces we support:
+ * - (R)MII supporting 10/100 Mbit/s
+ * - GMII, RGMII, and SGMII supporting 10/100/1000 Mbit/s
+ * - 10GBASER supporting 10 Gbit/s only
+ */
if (state->interface != PHY_INTERFACE_MODE_NA &&
state->interface != PHY_INTERFACE_MODE_MII &&
state->interface != PHY_INTERFACE_MODE_RMII &&
@@ -526,27 +531,48 @@ static void macb_validate(struct phylink_config *config, return; }- if (!macb_is_gem(bp) &&- (state->interface == PHY_INTERFACE_MODE_GMII ||- phy_interface_mode_is_rgmii(state->interface))) {- linkmode_zero(supported);- return;+ /* For 1G and up we must have both have a GEM and GIGABIT_MODE */+ if (!macb_is_gem(bp) ||+ (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)) {+ if (state->interface == PHY_INTERFACE_MODE_GMII ||+ phy_interface_mode_is_rgmii(state->interface) ||+ state->interface == PHY_INTERFACE_MODE_SGMII ||+ state->interface == PHY_INTERFACE_MODE_10GBASER) {+ linkmode_zero(supported);+ return;+ } else if (state->interface == PHY_INTERFACE_MODE_NA) {+ have_1g = false;+ have_10g = false;+ } }
Would it make more sense to do:
bool have_1g = false, have_10g = false;
if (macb_is_gem(bp) &&
(bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)) {
if (bp->caps & MACB_CAPS_PCS)
have_1g = true;
if (bp->caps & MACB_CAPS_HIGH_SPEED)
have_10g = true;
}
switch (state->interface) {
case PHY_INTERFACE_MODE_NA:
case PHY_INTERFACE_MODE_MII:
case PHY_INTERFACE_MODE_RMII:
break;
case PHY_INTERFACE_MODE_GMII:
case PHY_INTERFACE_MODE_RGMII:
case PHY_INTERFACE_MODE_RGMII_ID:
case PHY_INTERFACE_MODE_RGMII_RXID:
case PHY_INTERFACE_MODE_RGMII_TXID:
case PHY_INTERFACE_MODE_SGMII:
if (!have_1g) {
linkmode_zero(supported);
return;
}
break;
case PHY_INTERFACE_MODE_10GBASER:
if (!have_10g) {
linkmode_zero(supported);
return;
}
break;
default:
linkmode_zero(supported);
return;
}
This uses positive logic to derive have_1g and have_10g, and then uses
the switch statement to validate against those. Would the above result
in more understandable code?
I experimented with something like the above, but I wasn't able to
express it cleanly. I think what you have would work nicely.
--Sean
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-10-26 00:44:04
On Mon, 25 Oct 2021 13:24:05 -0400 Sean Anderson wrote:
There were several cases where validate() would return bogus supported
modes with unusual combinations of interfaces and capabilities. For
example, if state->interface was 10GBASER and the macb had HIGH_SPEED
and PCS but not GIGABIT MODE, then 10/100 modes would be set anyway. In
another case, SGMII could be enabled even if the mac was not a GEM
(despite this being checked for later on in mac_config()). These
inconsistencies make it difficult to refactor this function cleanly.
Since you're respinning anyway (AFAIU) would you mind clarifying
the fix vs refactoring question? Sounds like it could be a fix for
the right (wrong?) PHY/MAC combination, but I don't think you're
intending it to be treated as a fix.
If it's a fix it needs [PATCH net] in the subject and a Fixes tag,
if it's not a fix it needs [PATCH net-next] in the subject.
This will make the lifes of maintainers and backporters easier,
thanks :)
From: Sean Anderson <hidden> Date: 2021-10-26 15:30:21
Hi Jakub,
On 10/25/21 8:44 PM, Jakub Kicinski wrote:
On Mon, 25 Oct 2021 13:24:05 -0400 Sean Anderson wrote:
quoted
There were several cases where validate() would return bogus supported
modes with unusual combinations of interfaces and capabilities. For
example, if state->interface was 10GBASER and the macb had HIGH_SPEED
and PCS but not GIGABIT MODE, then 10/100 modes would be set anyway. In
another case, SGMII could be enabled even if the mac was not a GEM
(despite this being checked for later on in mac_config()). These
inconsistencies make it difficult to refactor this function cleanly.
Since you're respinning anyway (AFAIU) would you mind clarifying
the fix vs refactoring question? Sounds like it could be a fix for
the right (wrong?) PHY/MAC combination, but I don't think you're
intending it to be treated as a fix.
If it's a fix it needs [PATCH net] in the subject and a Fixes tag,
if it's not a fix it needs [PATCH net-next] in the subject.
This will make the lifes of maintainers and backporters easier,
thanks :)
I don't know if it's a "fix" per se. The current logic isn't wrong,
since I believe that the configurations where the above patch would make
a difference do not exist. However, as noted in the commit message, this
makes refactoring difficult. For example, one might want to implement
supported_interfaces like
if (bp->caps & MACB_CAPS_HIGH_SPEED &&
bp->caps & MACB_CAPS_PCS)
__set_bit(PHY_INTERFACE_MODE_10GBASER, supported);
if (macb_is_gem(bp) && bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
__set_bit(PHY_INTERFACE_MODE_GMII, supported);
phy_interface_set_rgmii(supported);
if (bp->caps & MACB_CAPS_PCS)
__set_bit(PHY_INTERFACE_MODE_SGMII, supported);
}
__set_bit(PHY_INTERFACE_MODE_MII, supported);
__set_bit(PHY_INTERFACE_MODE_RMII, supported);
but then you still need to check for GIGABIT_MODE in validate to
determine whether 10GBASER should "support" 10/100. See [1] for more
discussion.
If you think this fixes a bug, then the appropriate tag is
Fixes: 7897b071ac3b ("net: macb: convert to phylink")
--Sean
[1] https://lore.kernel.org/netdev/YXaIWFB8Kx9rm%2Fj9@shell.armlinux.org.uk/
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-10-26 15:39:44
On Tue, 26 Oct 2021 11:30:08 -0400 Sean Anderson wrote:
Hi Jakub,
On 10/25/21 8:44 PM, Jakub Kicinski wrote:
quoted
On Mon, 25 Oct 2021 13:24:05 -0400 Sean Anderson wrote:
quoted
There were several cases where validate() would return bogus supported
modes with unusual combinations of interfaces and capabilities. For
example, if state->interface was 10GBASER and the macb had HIGH_SPEED
and PCS but not GIGABIT MODE, then 10/100 modes would be set anyway. In
another case, SGMII could be enabled even if the mac was not a GEM
(despite this being checked for later on in mac_config()). These
inconsistencies make it difficult to refactor this function cleanly.
Since you're respinning anyway (AFAIU) would you mind clarifying
the fix vs refactoring question? Sounds like it could be a fix for
the right (wrong?) PHY/MAC combination, but I don't think you're
intending it to be treated as a fix.
If it's a fix it needs [PATCH net] in the subject and a Fixes tag,
if it's not a fix it needs [PATCH net-next] in the subject.
This will make the lifes of maintainers and backporters easier,
thanks :)
I don't know if it's a "fix" per se. The current logic isn't wrong,
since I believe that the configurations where the above patch would make
a difference do not exist. However, as noted in the commit message, this
makes refactoring difficult.
Ok, unless one of the PHY experts can help us make a call let's go
for net-next and no Fixes tag.
For example, one might want to implement supported_interfaces like
if (bp->caps & MACB_CAPS_HIGH_SPEED &&
bp->caps & MACB_CAPS_PCS)
__set_bit(PHY_INTERFACE_MODE_10GBASER, supported);
if (macb_is_gem(bp) && bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
__set_bit(PHY_INTERFACE_MODE_GMII, supported);
phy_interface_set_rgmii(supported);
if (bp->caps & MACB_CAPS_PCS)
__set_bit(PHY_INTERFACE_MODE_SGMII, supported);
}
__set_bit(PHY_INTERFACE_MODE_MII, supported);
__set_bit(PHY_INTERFACE_MODE_RMII, supported);
but then you still need to check for GIGABIT_MODE in validate to
determine whether 10GBASER should "support" 10/100. See [1] for more
discussion.
If you think this fixes a bug, then the appropriate tag is
Fixes: 7897b071ac3b ("net: macb: convert to phylink")
--Sean
[1] https://lore.kernel.org/netdev/YXaIWFB8Kx9rm%2Fj9@shell.armlinux.org.uk/
From: "Russell King (Oracle)" <linux@armlinux.org.uk> Date: 2021-10-26 15:51:49
On Tue, Oct 26, 2021 at 11:30:08AM -0400, Sean Anderson wrote:
I don't know if it's a "fix" per se. The current logic isn't wrong,
since I believe that the configurations where the above patch would make
a difference do not exist. However, as noted in the commit message, this
makes refactoring difficult. For example, one might want to implement
supported_interfaces like
if (bp->caps & MACB_CAPS_HIGH_SPEED &&
bp->caps & MACB_CAPS_PCS)
__set_bit(PHY_INTERFACE_MODE_10GBASER, supported);
if (macb_is_gem(bp) && bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
__set_bit(PHY_INTERFACE_MODE_GMII, supported);
phy_interface_set_rgmii(supported);
if (bp->caps & MACB_CAPS_PCS)
__set_bit(PHY_INTERFACE_MODE_SGMII, supported);
}
__set_bit(PHY_INTERFACE_MODE_MII, supported);
__set_bit(PHY_INTERFACE_MODE_RMII, supported);
but then you still need to check for GIGABIT_MODE in validate to
determine whether 10GBASER should "support" 10/100. See [1] for more
discussion.
However, 10GBASE-R doesn't support anything except 10G speeds, except
if the PHY itself does rate matching to achieve the slower speeds.
Then you need pause frames between the MAC and PHY to control the MAC
sending rate - which isn't something that the phylib model supports
particularly well.
The current code and your patched code conforms to this when
state->interface is 10GBASE-R _and_ MACB_CAPS_GIGABIT_MODE_AVAILABLE
is set, then we only end up with 10G linkmodes being allowed.
The problem case occurs in current code when
MACB_CAPS_GIGABIT_MODE_AVAILABLE is _not_ set, but state->interface
is 10GBASE-R. Current code ends up saying that 10GBASE-R supports
10/100 link modes, which is wrong.
The existing code:
if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE &&
(state->interface == PHY_INTERFACE_MODE_NA ||
state->interface == PHY_INTERFACE_MODE_10GBASER)) {
phylink_set(mask, 10000baseCR_Full);
phylink_set(mask, 10000baseER_Full);
phylink_set(mask, 10000baseKR_Full);
phylink_set(mask, 10000baseLR_Full);
phylink_set(mask, 10000baseLRM_Full);
phylink_set(mask, 10000baseSR_Full);
phylink_set(mask, 10000baseT_Full);
if (state->interface != PHY_INTERFACE_MODE_NA)
goto out;
}
phylink_set(mask, 10baseT_Half);
phylink_set(mask, 10baseT_Full);
phylink_set(mask, 100baseT_Half);
phylink_set(mask, 100baseT_Full);
Would have been better written as:
if (state->interface == PHY_INTERFACE_MODE_NA ||
state->interface == PHY_INTERFACE_MODE_10GBASER) {
if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
phylink_set(mask, 10000baseCR_Full);
phylink_set(mask, 10000baseER_Full);
phylink_set(mask, 10000baseKR_Full);
phylink_set(mask, 10000baseLR_Full);
phylink_set(mask, 10000baseLRM_Full);
phylink_set(mask, 10000baseSR_Full);
phylink_set(mask, 10000baseT_Full);
}
if (state->interface != PHY_INTERFACE_MODE_NA)
goto out;
}
phylink_set(mask, 10baseT_Half);
phylink_set(mask, 10baseT_Full);
phylink_set(mask, 100baseT_Half);
phylink_set(mask, 100baseT_Full);
which would have avoided getting to the code that sets 10/100 link
modes when state->interface is 10GBASE-R.
The same transformation is probably applicable to the next if ()
block as well.
If I truely understood exactly what MACB_CAPS_GIGABIT_MODE_AVAILABLE,
MACB_CAPS_HIGH_SPEED, and MACB_CAPS_PCS were indicating and how they
relate to what is supported, I might be tempted to come up with a
better implementation myself. However, every time I look at the
existing code, it just confuses me - it seems to me that the use of
those capability bits is entirely inconsistent in the current
macb_validate() implementation.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
From: Nicolas Ferre <nicolas.ferre@microchip.com> Date: 2021-10-26 16:32:44
On 26/10/2021 at 17:39, Jakub Kicinski wrote:
EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
On Tue, 26 Oct 2021 11:30:08 -0400 Sean Anderson wrote:
quoted
Hi Jakub,
On 10/25/21 8:44 PM, Jakub Kicinski wrote:
quoted
On Mon, 25 Oct 2021 13:24:05 -0400 Sean Anderson wrote:
quoted
There were several cases where validate() would return bogus supported
modes with unusual combinations of interfaces and capabilities. For
example, if state->interface was 10GBASER and the macb had HIGH_SPEED
and PCS but not GIGABIT MODE, then 10/100 modes would be set anyway. In
another case, SGMII could be enabled even if the mac was not a GEM
(despite this being checked for later on in mac_config()). These
inconsistencies make it difficult to refactor this function cleanly.
Since you're respinning anyway (AFAIU) would you mind clarifying
the fix vs refactoring question? Sounds like it could be a fix for
the right (wrong?) PHY/MAC combination, but I don't think you're
intending it to be treated as a fix.
If it's a fix it needs [PATCH net] in the subject and a Fixes tag,
if it's not a fix it needs [PATCH net-next] in the subject.
This will make the lifes of maintainers and backporters easier,
thanks :)
I don't know if it's a "fix" per se. The current logic isn't wrong,
since I believe that the configurations where the above patch would make
a difference do not exist. However, as noted in the commit message, this
makes refactoring difficult.
Ok, unless one of the PHY experts can help us make a call let's go
for net-next and no Fixes tag.
Agreed.
Regards,
Nicolas
quoted
For example, one might want to implement supported_interfaces like
if (bp->caps & MACB_CAPS_HIGH_SPEED &&
bp->caps & MACB_CAPS_PCS)
__set_bit(PHY_INTERFACE_MODE_10GBASER, supported);
if (macb_is_gem(bp) && bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
__set_bit(PHY_INTERFACE_MODE_GMII, supported);
phy_interface_set_rgmii(supported);
if (bp->caps & MACB_CAPS_PCS)
__set_bit(PHY_INTERFACE_MODE_SGMII, supported);
}
__set_bit(PHY_INTERFACE_MODE_MII, supported);
__set_bit(PHY_INTERFACE_MODE_RMII, supported);
but then you still need to check for GIGABIT_MODE in validate to
determine whether 10GBASER should "support" 10/100. See [1] for more
discussion.
If you think this fixes a bug, then the appropriate tag is
Fixes: 7897b071ac3b ("net: macb: convert to phylink")
--Sean
[1] https://lore.kernel.org/netdev/YXaIWFB8Kx9rm%2Fj9@shell.armlinux.org.uk/
From: Nicolas Ferre <nicolas.ferre@microchip.com> Date: 2021-10-26 16:37:25
On 25/10/2021 at 23:35, Sean Anderson wrote:
EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
On 10/25/21 5:19 PM, Russell King (Oracle) wrote:
quoted
On Mon, Oct 25, 2021 at 01:24:05PM -0400, Sean Anderson wrote:
quoted
There were several cases where validate() would return bogus supported
modes with unusual combinations of interfaces and capabilities. For
example, if state->interface was 10GBASER and the macb had HIGH_SPEED
and PCS but not GIGABIT MODE, then 10/100 modes would be set anyway. In
another case, SGMII could be enabled even if the mac was not a GEM
(despite this being checked for later on in mac_config()). These
inconsistencies make it difficult to refactor this function cleanly.
This attempts to address these by reusing the same conditions used to
decide whether to return early when setting mode bits. The logic is
pretty messy, but this preserves the existing logic where possible.
Signed-off-by: Sean Anderson <redacted>
---
Changes in v4:
- Drop cleanup patch
Changes in v3:
- Order bugfix patch first
Changes in v2:
- New
drivers/net/ethernet/cadence/macb_main.c | 59 +++++++++++++++++-------
1 file changed, 42 insertions(+), 17 deletions(-)
I think DaveM would ask for this to be reverse-christmas-tree, so the
new bool should be here.
Ah, I wasn't aware that there was another variable-ordering style in use for net.
quoted
quoted
struct macb *bp = netdev_priv(ndev);
- /* We only support MII, RMII, GMII, RGMII & SGMII. */
+ /* There are three major types of interfaces we support:
+ * - (R)MII supporting 10/100 Mbit/s
+ * - GMII, RGMII, and SGMII supporting 10/100/1000 Mbit/s
+ * - 10GBASER supporting 10 Gbit/s only
+ */
if (state->interface != PHY_INTERFACE_MODE_NA &&
state->interface != PHY_INTERFACE_MODE_MII &&
state->interface != PHY_INTERFACE_MODE_RMII &&
@@ -526,27 +531,48 @@ static void macb_validate(struct phylink_config *config, return; }- if (!macb_is_gem(bp) &&- (state->interface == PHY_INTERFACE_MODE_GMII ||- phy_interface_mode_is_rgmii(state->interface))) {- linkmode_zero(supported);- return;+ /* For 1G and up we must have both have a GEM and GIGABIT_MODE */+ if (!macb_is_gem(bp) ||+ (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)) {+ if (state->interface == PHY_INTERFACE_MODE_GMII ||+ phy_interface_mode_is_rgmii(state->interface) ||+ state->interface == PHY_INTERFACE_MODE_SGMII ||+ state->interface == PHY_INTERFACE_MODE_10GBASER) {+ linkmode_zero(supported);+ return;+ } else if (state->interface == PHY_INTERFACE_MODE_NA) {+ have_1g = false;+ have_10g = false;+ } }
Would it make more sense to do:
bool have_1g = false, have_10g = false;
if (macb_is_gem(bp) &&
(bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)) {
if (bp->caps & MACB_CAPS_PCS)
have_1g = true;
if (bp->caps & MACB_CAPS_HIGH_SPEED)
have_10g = true;
}
switch (state->interface) {
case PHY_INTERFACE_MODE_NA:
case PHY_INTERFACE_MODE_MII:
case PHY_INTERFACE_MODE_RMII:
break;
case PHY_INTERFACE_MODE_GMII:
case PHY_INTERFACE_MODE_RGMII:
case PHY_INTERFACE_MODE_RGMII_ID:
case PHY_INTERFACE_MODE_RGMII_RXID:
case PHY_INTERFACE_MODE_RGMII_TXID:
case PHY_INTERFACE_MODE_SGMII:
if (!have_1g) {
linkmode_zero(supported);
return;
}
break;
case PHY_INTERFACE_MODE_10GBASER:
if (!have_10g) {
linkmode_zero(supported);
return;
}
break;
default:
linkmode_zero(supported);
return;
}
This uses positive logic to derive have_1g and have_10g, and then uses
the switch statement to validate against those. Would the above result
in more understandable code?
I experimented with something like the above, but I wasn't able to
express it cleanly. I think what you have would work nicely.
I like it as well. Thanks a lot for these enhancements.
Regards,
Nicolas
--
Nicolas Ferre
From: "Russell King (Oracle)" <linux@armlinux.org.uk> Date: 2021-10-26 17:04:41
On Tue, Oct 26, 2021 at 06:37:18PM +0200, Nicolas Ferre wrote:
I like it as well. Thanks a lot for these enhancements.
So, would this work - have I understood all these capability flags
correctly? To aid this process, I've included the resulting code
below as well as the diff.
Should we only be setting have_10g if _all_ of MACB_CAPS_HIGH_SPEED,
MACB_CAPS_PCS, and MACB_CAPS_GIGABIT_MODE_AVAILABLE are set and we
have gem, or is what I have below sufficient (does it matter if
MACB_CAPS_PCS is clear?)
static void macb_validate(struct phylink_config *config,
unsigned long *supported,
struct phylink_link_state *state)
{
struct net_device *ndev = to_net_dev(config->dev);
__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
bool have_1g = false, have_10g = false;
struct macb *bp = netdev_priv(ndev);
if (macb_is_gem(bp) &&
(bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)) {
if (bp->caps & MACB_CAPS_PCS)
have_1g = true;
if (bp->caps & MACB_CAPS_HIGH_SPEED)
have_10g = true;
}
switch (state->interface) {
case PHY_INTERFACE_MODE_NA:
case PHY_INTERFACE_MODE_MII:
case PHY_INTERFACE_MODE_RMII:
break;
case PHY_INTERFACE_MODE_GMII:
case PHY_INTERFACE_MODE_RGMII:
case PHY_INTERFACE_MODE_RGMII_ID:
case PHY_INTERFACE_MODE_RGMII_RXID:
case PHY_INTERFACE_MODE_RGMII_TXID:
case PHY_INTERFACE_MODE_SGMII:
if (!have_1g) {
linkmode_zero(supported);
return;
}
break;
case PHY_INTERFACE_MODE_10GBASER:
if (!have_10g) {
linkmode_zero(supported);
return;
}
break;
default:
linkmode_zero(supported);
return;
}
phylink_set_port_modes(mask);
phylink_set(mask, Autoneg);
phylink_set(mask, Asym_Pause);
switch (state->interface) {
case PHY_INTERFACE_MODE_NA:
case PHY_INTERFACE_MODE_10GBASER:
if (have_10g) {
phylink_set_10g_modes(mask);
phylink_set(mask, 10000baseKR_Full);
}
if (state->interface != PHY_INTERFACE_MODE_NA)
break;
fallthrough;
case PHY_INTERFACE_MODE_GMII:
case PHY_INTERFACE_MODE_RGMII:
case PHY_INTERFACE_MODE_RGMII_ID:
case PHY_INTERFACE_MODE_RGMII_RXID:
case PHY_INTERFACE_MODE_RGMII_TXID:
case PHY_INTERFACE_MODE_SGMII:
if (have_1g) {
phylink_set(mask, 1000baseT_Full);
phylink_set(mask, 1000baseX_Full);
if (!(bp->caps & MACB_CAPS_NO_GIGABIT_HALF))
phylink_set(mask, 1000baseT_Half);
}
fallthrough;
default:
phylink_set(mask, 10baseT_Half);
phylink_set(mask, 10baseT_Full);
phylink_set(mask, 100baseT_Half);
phylink_set(mask, 100baseT_Full);
break;
}
linkmode_and(supported, supported, mask);
linkmode_and(state->advertising, state->advertising, mask);
}
From: Sean Anderson <hidden> Date: 2021-10-26 17:28:35
On 10/26/21 1:04 PM, Russell King (Oracle) wrote:
On Tue, Oct 26, 2021 at 06:37:18PM +0200, Nicolas Ferre wrote:
quoted
I like it as well. Thanks a lot for these enhancements.
So, would this work - have I understood all these capability flags
correctly? To aid this process, I've included the resulting code
below as well as the diff.
Should we only be setting have_10g if _all_ of MACB_CAPS_HIGH_SPEED,
MACB_CAPS_PCS, and MACB_CAPS_GIGABIT_MODE_AVAILABLE are set and we
have gem, or is what I have below sufficient (does it matter if
MACB_CAPS_PCS is clear?)
static void macb_validate(struct phylink_config *config,
unsigned long *supported,
struct phylink_link_state *state)
{
struct net_device *ndev = to_net_dev(config->dev);
__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
bool have_1g = false, have_10g = false;
struct macb *bp = netdev_priv(ndev);
if (macb_is_gem(bp) &&
(bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)) {
if (bp->caps & MACB_CAPS_PCS)
have_1g = true;
if (bp->caps & MACB_CAPS_HIGH_SPEED)
have_10g = true;
}
This is incorrect. (R)GMII does not need a PCS. Only SGMII needs one. So it really should be
if (macb_is_gem(bp) &&
(bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)) {
have_1g = true;
if (bp->caps & MACB_CAPS_PCS)
have_sgmii = true;
if (bp->caps & MACB_CAPS_HIGH_SPEED)
have_10g = true;
}
switch (state->interface) {
case PHY_INTERFACE_MODE_NA:
case PHY_INTERFACE_MODE_MII:
case PHY_INTERFACE_MODE_RMII:
break;
case PHY_INTERFACE_MODE_GMII:
case PHY_INTERFACE_MODE_RGMII:
case PHY_INTERFACE_MODE_RGMII_ID:
case PHY_INTERFACE_MODE_RGMII_RXID:
case PHY_INTERFACE_MODE_RGMII_TXID:
case PHY_INTERFACE_MODE_SGMII:
And then SGMII needs its own case here.
if (!have_1g) {
linkmode_zero(supported);
return;
}
break;
case PHY_INTERFACE_MODE_10GBASER:
if (!have_10g) {
linkmode_zero(supported);
return;
}
break;
default:
linkmode_zero(supported);
return;
}
phylink_set_port_modes(mask);
phylink_set(mask, Autoneg);
phylink_set(mask, Asym_Pause);
switch (state->interface) {
case PHY_INTERFACE_MODE_NA:
case PHY_INTERFACE_MODE_10GBASER:
if (have_10g) {
phylink_set_10g_modes(mask);
phylink_set(mask, 10000baseKR_Full);
}
if (state->interface != PHY_INTERFACE_MODE_NA)
break;
fallthrough;
case PHY_INTERFACE_MODE_GMII:
case PHY_INTERFACE_MODE_RGMII:
case PHY_INTERFACE_MODE_RGMII_ID:
case PHY_INTERFACE_MODE_RGMII_RXID:
case PHY_INTERFACE_MODE_RGMII_TXID:
case PHY_INTERFACE_MODE_SGMII:
if (have_1g) {
phylink_set(mask, 1000baseT_Full);
phylink_set(mask, 1000baseX_Full);
if (!(bp->caps & MACB_CAPS_NO_GIGABIT_HALF))
phylink_set(mask, 1000baseT_Half);
}
fallthrough;
Hmm. That brings with it fundamental question: if the PCS supports 1G
only, does it _actually_ support Cisco SGMII, or does it only support
1000base-X?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Hmm. That brings with it fundamental question: if the PCS supports 1G
only, does it _actually_ support Cisco SGMII, or does it only support
1000base-X?
Of course, in the technical reference manual [1], they say
> The line rate is 1 Gb/s as SGMII hardwired to function at 1 Gb/s only.
> However, the data transfer rate can be forced down to 100 Mb/s or 10
> Mb/s if the link partner is not capable.
which sounds like the normal byte-repetition of SGMII...
And they also talk about how the autonegotiation timeout and control words are different.
--Sean
[1] https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf
Hmm. That brings with it fundamental question: if the PCS supports 1G
only, does it _actually_ support Cisco SGMII, or does it only support
1000base-X?
Of course, in the technical reference manual [1], they say
quoted
The line rate is 1 Gb/s as SGMII hardwired to function at 1 Gb/s only.
However, the data transfer rate can be forced down to 100 Mb/s or 10
Mb/s if the link partner is not capable.
which sounds like the normal byte-repetition of SGMII...
And they also talk about how the autonegotiation timeout and control words are different.
This document looks a little comical. "GEM supports the serial gigabit
media-independent interface (SGMII, 1000BASE-SX and 1000BASE-LX) at
1000 Gb/s using the PS-GTR interface."
So:
a) it supports terabyte speeds?
b) it provides an optical connection direct from the SoC?
(the L and S in 1000BASE-X refer to the laser wavelength!)
They really should just be saying "1000BASE-X" rather than specifying
an optical standard, but lets ignore that fundamental mistake for now.
In the section "SGMII, 1000BASE-SX, or 1000BASE-LX" it says:
"When bit [27] (SGMII mode) in the network configuration register
(GEM(0:3).network_config[sgmii_mode_enable]) is set, it changes the
behavior of the auto-negotiation advertisement and link partner ability
registers to meet the requirements of SGMII. Additionally, the time
duration of the link timer is reduced from 10ms to 1.6ms."
That bodes well for Cisco SGMII support, but it says nothing about how
that affects the PCS registers themselves.
As you say above, it goes on to say:
"The line rate is 1 Gb/s as SGMII hardwired to function at 1 GB/s
only."
That statement is confused. Cisco SGMII and 1000Base-X actually operate
at 1.25Gbaud line rate due to the 4B5B encoding on the Serdes. However,
the underlying data rate is 1Gbps, with 100 and 10Mbps achieved by
symbol replication of only the data portions of the packet transfer.
This replication does not, however, apply to non-data symbols though.
I suppose they _could_ have implemented Cisco SGMII by having the PCS
fixed in 1G mode, and then replicate the data prior to the PCS. That
would be rather peculiar though, and I'm not sure whether that could
work for the non-data symbols. I suppose it depends whether they just
slow down the transmission rate into the PCS, or do only data portion
replication before the PCS.
I've also just found the register listing in HTML form (so less
searchable than a PDF), and the PCS register listing only shows
1000base-X layout for the advertisement and link partner registers.
It seems to make no mention of "SGMII" mode.
So we have an open question: do 10 and 100M speeds actually work on
GEM, and if they do, how does one program it to operate at these
speeds. Currently, the driver seems to change bits in NCFGR to change
speed, and also reconfigure the transmit clock rate.
Going back to the first point I mentioned above, how much should we
take from these documents as actually being correct? Should we not
assume anything, but instead just experiment with the hardware and
see what works.
For example, are the two speed bits in the PCS control register
really read-only when in Cisco SGMII mode, or can they be changed -
and if they can be changed, does that have an effect on the ethernet
link?
Hmm, this seems to have uncovered more questions...
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Hmm. That brings with it fundamental question: if the PCS supports 1G
only, does it _actually_ support Cisco SGMII, or does it only support
1000base-X?
Of course, in the technical reference manual [1], they say
quoted
The line rate is 1 Gb/s as SGMII hardwired to function at 1 Gb/s only.
However, the data transfer rate can be forced down to 100 Mb/s or 10
Mb/s if the link partner is not capable.
which sounds like the normal byte-repetition of SGMII...
And they also talk about how the autonegotiation timeout and control words are different.
This document looks a little comical. "GEM supports the serial gigabit
media-independent interface (SGMII, 1000BASE-SX and 1000BASE-LX) at
1000 Gb/s using the PS-GTR interface."
So:
a) it supports terabyte speeds?
b) it provides an optical connection direct from the SoC?
(the L and S in 1000BASE-X refer to the laser wavelength!)
They really should just be saying "1000BASE-X" rather than specifying
an optical standard, but lets ignore that fundamental mistake for now.
In the section "SGMII, 1000BASE-SX, or 1000BASE-LX" it says:
"When bit [27] (SGMII mode) in the network configuration register
(GEM(0:3).network_config[sgmii_mode_enable]) is set, it changes the
behavior of the auto-negotiation advertisement and link partner ability
registers to meet the requirements of SGMII. Additionally, the time
duration of the link timer is reduced from 10ms to 1.6ms."
That bodes well for Cisco SGMII support, but it says nothing about how
that affects the PCS registers themselves.
As you say above, it goes on to say:
"The line rate is 1 Gb/s as SGMII hardwired to function at 1 GB/s
only."
That statement is confused. Cisco SGMII and 1000Base-X actually operate
at 1.25Gbaud line rate due to the 4B5B encoding on the Serdes. However,
the underlying data rate is 1Gbps, with 100 and 10Mbps achieved by
symbol replication of only the data portions of the packet transfer.
This replication does not, however, apply to non-data symbols though.
I suppose they _could_ have implemented Cisco SGMII by having the PCS
fixed in 1G mode, and then replicate the data prior to the PCS. That
would be rather peculiar though, and I'm not sure whether that could
work for the non-data symbols. I suppose it depends whether they just
slow down the transmission rate into the PCS, or do only data portion
replication before the PCS.
I've also just found the register listing in HTML form (so less
searchable than a PDF),
Unfortunately, AFAIK this is the only listing they have :l
(OTOH it is easy to link to)
and the PCS register listing only shows
1000base-X layout for the advertisement and link partner registers.
It seems to make no mention of "SGMII" mode.
So we have an open question: do 10 and 100M speeds actually work on
GEM, and if they do, how does one program it to operate at these
speeds. Currently, the driver seems to change bits in NCFGR to change
speed, and also reconfigure the transmit clock rate.
Going back to the first point I mentioned above, how much should we
take from these documents as actually being correct? Should we not
assume anything, but instead just experiment with the hardware and
see what works.
For example, are the two speed bits in the PCS control register
really read-only when in Cisco SGMII mode, or can they be changed -
and if they can be changed, does that have an effect on the ethernet
link?
Keep in mind that it is not only Zynq(MP) parts with have GEMs, but
several other SoCs as well. I have not reviewed their datasheets (except
for SiFive's which just say "go read the Linux driver"). It is possible
that other SoCs may not have these limitations. So any experimental
program will need to also experiment with e.g. sama.
Perhaps someone from cadence could comment on what is actually supported
by gem/macb?
--Sean
Sean, Russell,
On 26/10/2021 at 20:52, Sean Anderson wrote:
quoted
Going back to the first point I mentioned above, how much should we
take from these documents as actually being correct? Should we not
assume anything, but instead just experiment with the hardware and
see what works.
For example, are the two speed bits in the PCS control register
really read-only when in Cisco SGMII mode, or can they be changed -
and if they can be changed, does that have an effect on the ethernet
link?
Keep in mind that it is not only Zynq(MP) parts with have GEMs, but
several other SoCs as well. I have not reviewed their datasheets (except
for SiFive's which just say "go read the Linux driver"). It is possible
that other SoCs may not have these limitations. So any experimental
program will need to also experiment with e.g. sama.