| Message ID | E1ifSV8-0000b1-NW@rmk-PC.armlinux.org.uk |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | improve clause 45 support in phylink | expand |
Hello Russell, On Thu, Dec 12, 2019 at 05:43:46PM +0000, Russell King wrote: > > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > index 111b3b8239e1..fddd856338b4 100644 > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > @@ -4786,6 +4786,8 @@ static void mvpp2_phylink_validate(struct phylink_config *config, > phylink_set(mask, 10000baseER_Full); > phylink_set(mask, 10000baseKR_Full); > } > + if (state->interface != PHY_INTERFACE_MODE_NA) > + break; > /* Fall-through */ > case PHY_INTERFACE_MODE_RGMII: > case PHY_INTERFACE_MODE_RGMII_ID: > @@ -4796,13 +4798,21 @@ static void mvpp2_phylink_validate(struct phylink_config *config, > phylink_set(mask, 10baseT_Full); > phylink_set(mask, 100baseT_Half); > phylink_set(mask, 100baseT_Full); > + if (state->interface != PHY_INTERFACE_MODE_NA) > + break; The two checks above will break the 10G/1G interfaces on the mcbin (eth0/eth1) as they can support both 10gbase-kr and 10/100/1000baseT modes depending on what's connected. With this patch only the modes related to the one defined in the device tree would be valid, breaking run-time reconfiguration of the link. Thanks, Antoine
On Fri, Dec 13, 2019 at 05:04:20PM +0100, Antoine Tenart wrote: > Hello Russell, > > On Thu, Dec 12, 2019 at 05:43:46PM +0000, Russell King wrote: > > > > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > > index 111b3b8239e1..fddd856338b4 100644 > > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > > @@ -4786,6 +4786,8 @@ static void mvpp2_phylink_validate(struct phylink_config *config, > > phylink_set(mask, 10000baseER_Full); > > phylink_set(mask, 10000baseKR_Full); > > } > > + if (state->interface != PHY_INTERFACE_MODE_NA) > > + break; > > > /* Fall-through */ > > case PHY_INTERFACE_MODE_RGMII: > > case PHY_INTERFACE_MODE_RGMII_ID: > > @@ -4796,13 +4798,21 @@ static void mvpp2_phylink_validate(struct phylink_config *config, > > phylink_set(mask, 10baseT_Full); > > phylink_set(mask, 100baseT_Half); > > phylink_set(mask, 100baseT_Full); > > + if (state->interface != PHY_INTERFACE_MODE_NA) > > + break; > > The two checks above will break the 10G/1G interfaces on the mcbin > (eth0/eth1) as they can support both 10gbase-kr and 10/100/1000baseT > modes depending on what's connected. With this patch only the modes > related to the one defined in the device tree would be valid, breaking > run-time reconfiguration of the link. Exactly which scenario are you talking about? The mcbin doubleshot setup, or the singleshot setup? This patch (when combined with the others) has no effect on the doubleshot, and should have no effect on the SFP cages on the single shot.
On Fri, Dec 13, 2019 at 04:11:44PM +0000, Russell King - ARM Linux admin wrote: > On Fri, Dec 13, 2019 at 05:04:20PM +0100, Antoine Tenart wrote: > > On Thu, Dec 12, 2019 at 05:43:46PM +0000, Russell King wrote: > > > > > > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > > > index 111b3b8239e1..fddd856338b4 100644 > > > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > > > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > > > @@ -4786,6 +4786,8 @@ static void mvpp2_phylink_validate(struct phylink_config *config, > > > phylink_set(mask, 10000baseER_Full); > > > phylink_set(mask, 10000baseKR_Full); > > > } > > > + if (state->interface != PHY_INTERFACE_MODE_NA) > > > + break; > > > > > /* Fall-through */ > > > case PHY_INTERFACE_MODE_RGMII: > > > case PHY_INTERFACE_MODE_RGMII_ID: > > > @@ -4796,13 +4798,21 @@ static void mvpp2_phylink_validate(struct phylink_config *config, > > > phylink_set(mask, 10baseT_Full); > > > phylink_set(mask, 100baseT_Half); > > > phylink_set(mask, 100baseT_Full); > > > + if (state->interface != PHY_INTERFACE_MODE_NA) > > > + break; > > > > The two checks above will break the 10G/1G interfaces on the mcbin > > (eth0/eth1) as they can support both 10gbase-kr and 10/100/1000baseT > > modes depending on what's connected. With this patch only the modes > > related to the one defined in the device tree would be valid, breaking > > run-time reconfiguration of the link. > > Exactly which scenario are you talking about? The mcbin doubleshot > setup, or the singleshot setup? I was thinking about the doubleshot. > This patch (when combined with the others) has no effect on the > doubleshot, and should have no effect on the SFP cages on the single > shot. You're right, I just tested the series and it the two 10G/1G ports were able to be reconfigured at runtime, my bad. However it seems cp1_eth1 is coming up at 100Mbps only. (I haven't looked into it more than a quick test so far). # ethtool eth2 Settings for eth2: Supported ports: [ TP MII FIBRE ] Supported link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full Supported pause frame use: Symmetric Receive-only Supports auto-negotiation: Yes Supported FEC modes: Not reported Advertised link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full Advertised pause frame use: Symmetric Receive-only Advertised auto-negotiation: Yes Advertised FEC modes: Not reported Link partner advertised link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full Link partner advertised pause frame use: Symmetric Receive-only Link partner advertised auto-negotiation: Yes Link partner advertised FEC modes: Not reported Speed: 100Mb/s Duplex: Full Port: MII PHYAD: 0 Transceiver: internal Auto-negotiation: on Link detected: yes The link partner however advertises: Advertised link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Full Thanks, Antoine
On Fri, Dec 13, 2019 at 05:28:08PM +0100, Antoine Tenart wrote: > On Fri, Dec 13, 2019 at 04:11:44PM +0000, Russell King - ARM Linux admin wrote: > > On Fri, Dec 13, 2019 at 05:04:20PM +0100, Antoine Tenart wrote: > > > On Thu, Dec 12, 2019 at 05:43:46PM +0000, Russell King wrote: > > > > > > > > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > > > > index 111b3b8239e1..fddd856338b4 100644 > > > > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > > > > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > > > > @@ -4786,6 +4786,8 @@ static void mvpp2_phylink_validate(struct phylink_config *config, > > > > phylink_set(mask, 10000baseER_Full); > > > > phylink_set(mask, 10000baseKR_Full); > > > > } > > > > + if (state->interface != PHY_INTERFACE_MODE_NA) > > > > + break; > > > > > > > /* Fall-through */ > > > > case PHY_INTERFACE_MODE_RGMII: > > > > case PHY_INTERFACE_MODE_RGMII_ID: > > > > @@ -4796,13 +4798,21 @@ static void mvpp2_phylink_validate(struct phylink_config *config, > > > > phylink_set(mask, 10baseT_Full); > > > > phylink_set(mask, 100baseT_Half); > > > > phylink_set(mask, 100baseT_Full); > > > > + if (state->interface != PHY_INTERFACE_MODE_NA) > > > > + break; > > > > > > The two checks above will break the 10G/1G interfaces on the mcbin > > > (eth0/eth1) as they can support both 10gbase-kr and 10/100/1000baseT > > > modes depending on what's connected. With this patch only the modes > > > related to the one defined in the device tree would be valid, breaking > > > run-time reconfiguration of the link. > > > > Exactly which scenario are you talking about? The mcbin doubleshot > > setup, or the singleshot setup? > > I was thinking about the doubleshot. > > > This patch (when combined with the others) has no effect on the > > doubleshot, and should have no effect on the SFP cages on the single > > shot. > > You're right, I just tested the series and it the two 10G/1G ports were > able to be reconfigured at runtime, my bad. > > However it seems cp1_eth1 is coming up at 100Mbps only. (I haven't > looked into it more than a quick test so far). Oh, looks like I made a mistake in the fallthrough for *GMII modes. It's probably easier just to add the two 1G modes there, which should restore eth2 to full functionality. Thanks for spotting. diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c index fddd856338b4..f09fcbe6ea88 100644 --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c @@ -4798,6 +4798,8 @@ static void mvpp2_phylink_validate(struct phylink_config *config, phylink_set(mask, 10baseT_Full); phylink_set(mask, 100baseT_Half); phylink_set(mask, 100baseT_Full); + phylink_set(mask, 1000baseT_Full); + phylink_set(mask, 1000baseX_Full); if (state->interface != PHY_INTERFACE_MODE_NA) break; /* Fall-through */
On Fri, Dec 13, 2019 at 04:40:28PM +0000, Russell King - ARM Linux admin wrote: > On Fri, Dec 13, 2019 at 05:28:08PM +0100, Antoine Tenart wrote: > > On Fri, Dec 13, 2019 at 04:11:44PM +0000, Russell King - ARM Linux admin wrote: > > > On Fri, Dec 13, 2019 at 05:04:20PM +0100, Antoine Tenart wrote: > > > > On Thu, Dec 12, 2019 at 05:43:46PM +0000, Russell King wrote: > > > > > > > > > > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > > > > > index 111b3b8239e1..fddd856338b4 100644 > > > > > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > > > > > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > > > > > @@ -4786,6 +4786,8 @@ static void mvpp2_phylink_validate(struct phylink_config *config, > > > > > phylink_set(mask, 10000baseER_Full); > > > > > phylink_set(mask, 10000baseKR_Full); > > > > > } > > > > > + if (state->interface != PHY_INTERFACE_MODE_NA) > > > > > + break; > > > > > > > > > /* Fall-through */ > > > > > case PHY_INTERFACE_MODE_RGMII: > > > > > case PHY_INTERFACE_MODE_RGMII_ID: > > > > > @@ -4796,13 +4798,21 @@ static void mvpp2_phylink_validate(struct phylink_config *config, > > > > > phylink_set(mask, 10baseT_Full); > > > > > phylink_set(mask, 100baseT_Half); > > > > > phylink_set(mask, 100baseT_Full); > > > > > + if (state->interface != PHY_INTERFACE_MODE_NA) > > > > > + break; > > > > > > > > The two checks above will break the 10G/1G interfaces on the mcbin > > > > (eth0/eth1) as they can support both 10gbase-kr and 10/100/1000baseT > > > > modes depending on what's connected. With this patch only the modes > > > > related to the one defined in the device tree would be valid, breaking > > > > run-time reconfiguration of the link. > > > > > > Exactly which scenario are you talking about? The mcbin doubleshot > > > setup, or the singleshot setup? > > > > I was thinking about the doubleshot. > > > > > This patch (when combined with the others) has no effect on the > > > doubleshot, and should have no effect on the SFP cages on the single > > > shot. > > > > You're right, I just tested the series and it the two 10G/1G ports were > > able to be reconfigured at runtime, my bad. > > > > However it seems cp1_eth1 is coming up at 100Mbps only. (I haven't > > looked into it more than a quick test so far). > > Oh, looks like I made a mistake in the fallthrough for *GMII modes. > It's probably easier just to add the two 1G modes there, which > should restore eth2 to full functionality. Thanks for spotting. > > diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > index fddd856338b4..f09fcbe6ea88 100644 > --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c > @@ -4798,6 +4798,8 @@ static void mvpp2_phylink_validate(struct phylink_config *config, > phylink_set(mask, 10baseT_Full); > phylink_set(mask, 100baseT_Half); > phylink_set(mask, 100baseT_Full); > + phylink_set(mask, 1000baseT_Full); > + phylink_set(mask, 1000baseX_Full); > if (state->interface != PHY_INTERFACE_MODE_NA) > break; > /* Fall-through */ Agreed, adding the two 1G modes here is easier and more readable. Thanks, Antoine
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c index 111b3b8239e1..fddd856338b4 100644 --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c @@ -4786,6 +4786,8 @@ static void mvpp2_phylink_validate(struct phylink_config *config, phylink_set(mask, 10000baseER_Full); phylink_set(mask, 10000baseKR_Full); } + if (state->interface != PHY_INTERFACE_MODE_NA) + break; /* Fall-through */ case PHY_INTERFACE_MODE_RGMII: case PHY_INTERFACE_MODE_RGMII_ID: @@ -4796,13 +4798,21 @@ static void mvpp2_phylink_validate(struct phylink_config *config, phylink_set(mask, 10baseT_Full); phylink_set(mask, 100baseT_Half); phylink_set(mask, 100baseT_Full); + if (state->interface != PHY_INTERFACE_MODE_NA) + break; /* Fall-through */ case PHY_INTERFACE_MODE_1000BASEX: case PHY_INTERFACE_MODE_2500BASEX: - phylink_set(mask, 1000baseT_Full); - phylink_set(mask, 1000baseX_Full); - phylink_set(mask, 2500baseT_Full); - phylink_set(mask, 2500baseX_Full); + if (port->comphy || + state->interface != PHY_INTERFACE_MODE_2500BASEX) { + phylink_set(mask, 1000baseT_Full); + phylink_set(mask, 1000baseX_Full); + } + if (port->comphy || + state->interface == PHY_INTERFACE_MODE_2500BASEX) { + phylink_set(mask, 2500baseT_Full); + phylink_set(mask, 2500baseX_Full); + } break; default: goto empty_set; @@ -4811,6 +4821,8 @@ static void mvpp2_phylink_validate(struct phylink_config *config, bitmap_and(supported, supported, mask, __ETHTOOL_LINK_MODE_MASK_NBITS); bitmap_and(state->advertising, state->advertising, mask, __ETHTOOL_LINK_MODE_MASK_NBITS); + + phylink_helper_basex_speed(state); return; empty_set:
Fix up the mvpp2 validate implementation to adopt the same behaviour as mvneta: - only allow the link modes that the specified PHY interface mode supports with the exception of 1000base-X and 2500base-X. - use the basex helper to deal with SFP modules that can be switched between 1000base-X vs 2500base-X. This gives consistent behaviour between mvneta and mvpp2. This commit depends on "net: phylink: extend clause 45 PHY validation workaround" so is not marked for backporting to stable kernels. Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk> --- .../net/ethernet/marvell/mvpp2/mvpp2_main.c | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)