| Message ID | E1pxWXz-002Qs5-0N@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: pcs: xpcs: cleanups for clause 73 support | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 98 this patch: 98 |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/build_clang | success | Errors and warnings before: 13 this patch: 13 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 98 this patch: 98 |
| netdev/checkpatch | warning | WARNING: Missing a blank line after declarations |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
> +void phylink_resolve_c73(struct phylink_link_state *state) > +{ > + int i; > + > + for (i = 0; i < ARRAY_SIZE(phylink_c73_priority_resolution); i++) { > + int bit = phylink_c73_priority_resolution[i].bit; > + if (linkmode_test_bit(bit, state->advertising) && > + linkmode_test_bit(bit, state->lp_advertising)) > + break; > + } > + > + if (i < ARRAY_SIZE(phylink_c73_priority_resolution)) { > + state->speed = phylink_c73_priority_resolution[i].speed; > + state->duplex = DUPLEX_FULL; > + } else { > + /* negotiation failure */ > + state->link = false; > + } Hi Russell This looks asymmetric in that state->link is not set true if a resolution is found. Can that set be moved here? Or are there other conditions which also need to be fulfilled before it is set? Andrew
On Sat, May 13, 2023 at 01:57:46AM +0200, Andrew Lunn wrote: > > +void phylink_resolve_c73(struct phylink_link_state *state) > > +{ > > + int i; > > + > > + for (i = 0; i < ARRAY_SIZE(phylink_c73_priority_resolution); i++) { > > + int bit = phylink_c73_priority_resolution[i].bit; > > + if (linkmode_test_bit(bit, state->advertising) && > > + linkmode_test_bit(bit, state->lp_advertising)) > > + break; > > + } > > + > > + if (i < ARRAY_SIZE(phylink_c73_priority_resolution)) { > > + state->speed = phylink_c73_priority_resolution[i].speed; > > + state->duplex = DUPLEX_FULL; > > + } else { > > + /* negotiation failure */ > > + state->link = false; > > + } > > Hi Russell > > This looks asymmetric in that state->link is not set true if a > resolution is found. > > Can that set be moved here? Or are there other conditions which also > need to be fulfilled before it is set? It's intentionally so because it's a failure case. In theory, the PHY shouldn't report link-up if the C73 priority resolution doesn't give a valid result, but given that there are C73 advertisements that we don't support, and that the future may add further advertisements, if our software resolution fails to find a speed, we need to stop the link coming up. Also... PHYs... hardware bugs...
> > Hi Russell > > > > This looks asymmetric in that state->link is not set true if a > > resolution is found. > > > > Can that set be moved here? Or are there other conditions which also > > need to be fulfilled before it is set? > > It's intentionally so because it's a failure case. In theory, the > PHY shouldn't report link-up if the C73 priority resolution doesn't > give a valid result, but given that there are C73 advertisements > that we don't support, and that the future may add further > advertisements, if our software resolution fails to find a speed, > we need to stop the link coming up. Also... PHYs... hardware > bugs... Thanks for the explanation. Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c index 783ab4a66111..268e0c3dfb1d 100644 --- a/drivers/net/phy/phylink.c +++ b/drivers/net/phy/phylink.c @@ -3276,6 +3276,45 @@ static const struct sfp_upstream_ops sfp_phylink_ops = { /* Helpers for MAC drivers */ +static struct { + int bit; + int speed; +} phylink_c73_priority_resolution[] = { + { ETHTOOL_LINK_MODE_100000baseCR4_Full_BIT, SPEED_100000 }, + { ETHTOOL_LINK_MODE_100000baseKR4_Full_BIT, SPEED_100000 }, + /* 100GBASE-KP4 and 100GBASE-CR10 not supported */ + { ETHTOOL_LINK_MODE_40000baseCR4_Full_BIT, SPEED_40000 }, + { ETHTOOL_LINK_MODE_40000baseKR4_Full_BIT, SPEED_40000 }, + { ETHTOOL_LINK_MODE_10000baseKR_Full_BIT, SPEED_10000 }, + { ETHTOOL_LINK_MODE_10000baseKX4_Full_BIT, SPEED_10000 }, + /* 5GBASE-KR not supported */ + { ETHTOOL_LINK_MODE_2500baseX_Full_BIT, SPEED_2500 }, + { ETHTOOL_LINK_MODE_1000baseKX_Full_BIT, SPEED_1000 }, +}; + +void phylink_resolve_c73(struct phylink_link_state *state) +{ + int i; + + for (i = 0; i < ARRAY_SIZE(phylink_c73_priority_resolution); i++) { + int bit = phylink_c73_priority_resolution[i].bit; + if (linkmode_test_bit(bit, state->advertising) && + linkmode_test_bit(bit, state->lp_advertising)) + break; + } + + if (i < ARRAY_SIZE(phylink_c73_priority_resolution)) { + state->speed = phylink_c73_priority_resolution[i].speed; + state->duplex = DUPLEX_FULL; + } else { + /* negotiation failure */ + state->link = false; + } + + phylink_resolve_an_pause(state); +} +EXPORT_SYMBOL_GPL(phylink_resolve_c73); + static void phylink_decode_c37_word(struct phylink_link_state *state, uint16_t config_reg, int speed) { diff --git a/include/linux/phylink.h b/include/linux/phylink.h index 2375ccf75403..86a9249ae5b2 100644 --- a/include/linux/phylink.h +++ b/include/linux/phylink.h @@ -657,6 +657,8 @@ int phylink_mii_c22_pcs_config(struct mdio_device *pcs, unsigned int mode, const unsigned long *advertising); void phylink_mii_c22_pcs_an_restart(struct mdio_device *pcs); +void phylink_resolve_c73(struct phylink_link_state *state); + void phylink_mii_c45_pcs_get_state(struct mdio_device *pcs, struct phylink_link_state *state);
Add a function to resolve clause 73 negotiation according to the priority resolution function described in clause 73.3.6. Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> --- drivers/net/phy/phylink.c | 39 +++++++++++++++++++++++++++++++++++++++ include/linux/phylink.h | 2 ++ 2 files changed, 41 insertions(+)