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;
Actually, according to the Zynq UltraScale+ Devices Register Reference [1], the PCS does not support 10/100. So should SGMII even fall through here?
[1] https://www.xilinx.com/html_docs/registers/ug1087/gem___pcs_control.html
quoted hunk ↗ jump to hunk
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);
}
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 309371abfe23..36fcd5563a71 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -512,30 +512,43 @@ static void macb_validate(struct phylink_config *config,
{
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);
- /* We only support MII, RMII, GMII, RGMII & SGMII. */
- if (state->interface != PHY_INTERFACE_MODE_NA &&
- state->interface != PHY_INTERFACE_MODE_MII &&
- state->interface != PHY_INTERFACE_MODE_RMII &&
- state->interface != PHY_INTERFACE_MODE_GMII &&
- state->interface != PHY_INTERFACE_MODE_SGMII &&
- state->interface != PHY_INTERFACE_MODE_10GBASER &&
- !phy_interface_mode_is_rgmii(state->interface)) {
- linkmode_zero(supported);
- return;
+ 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;
}
- if (!macb_is_gem(bp) &&
- (state->interface == PHY_INTERFACE_MODE_GMII ||
- phy_interface_mode_is_rgmii(state->interface))) {
- linkmode_zero(supported);
- return;
- }
+ 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;
- if (state->interface == PHY_INTERFACE_MODE_10GBASER &&
- !(bp->caps & MACB_CAPS_HIGH_SPEED &&
- bp->caps & MACB_CAPS_PCS)) {
+ default:
linkmode_zero(supported);
return;
}@@ -544,32 +557,40 @@ static void macb_validate(struct phylink_config *config,
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)) {
- phylink_set_10g_modes(mask);
- phylink_set(mask, 10000baseKR_Full);
+ 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)
- goto out;
- }
+ 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;
- phylink_set(mask, 10baseT_Half);
- phylink_set(mask, 10baseT_Full);
- phylink_set(mask, 100baseT_Half);
- phylink_set(mask, 100baseT_Full);
-
- if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE &&
- (state->interface == PHY_INTERFACE_MODE_NA ||
- state->interface == PHY_INTERFACE_MODE_GMII ||
- state->interface == PHY_INTERFACE_MODE_SGMII ||
- phy_interface_mode_is_rgmii(state->interface))) {
- phylink_set(mask, 1000baseT_Full);
- phylink_set(mask, 1000baseX_Full);
-
- if (!(bp->caps & MACB_CAPS_NO_GIGABIT_HALF))
- phylink_set(mask, 1000baseT_Half);
+ default:
+ phylink_set(mask, 10baseT_Half);
+ phylink_set(mask, 10baseT_Full);
+ phylink_set(mask, 100baseT_Half);
+ phylink_set(mask, 100baseT_Full);
+ break;
}
-out:
+
linkmode_and(supported, supported, mask);
linkmode_and(state->advertising, state->advertising, mask);
}