| Message ID | 20211025172405.211164-1-sean.anderson@seco.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v4] net: macb: Fix several edge cases in validate | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Single patches do not need cover letters |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 25 this patch: 25 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 91 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 25 this patch: 25 |
| netdev/header_inline | success | No static functions without inline keyword in header files |
On Mon, Oct 25, 2021 at 01:24:05PM -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. > > 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 <sean.anderson@seco.com> > --- > > 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(-) > > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c > index 309371abfe23..40bd5a069368 100644 > --- a/drivers/net/ethernet/cadence/macb_main.c > +++ b/drivers/net/ethernet/cadence/macb_main.c > @@ -510,11 +510,16 @@ static void macb_validate(struct phylink_config *config, > unsigned long *supported, > struct phylink_link_state *state) > { > + bool have_1g = true, have_10g = true; > struct net_device *ndev = to_net_dev(config->dev); > __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, }; I think DaveM would ask for this to be reverse-christmas-tree, so the new bool should be here. > 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.
On 10/25/21 5:19 PM, Russell King (Oracle) wrote: > On Mon, Oct 25, 2021 at 01:24:05PM -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. >> >> 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 <sean.anderson@seco.com> >> --- >> >> 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(-) >> >> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c >> index 309371abfe23..40bd5a069368 100644 >> --- a/drivers/net/ethernet/cadence/macb_main.c >> +++ b/drivers/net/ethernet/cadence/macb_main.c >> @@ -510,11 +510,16 @@ static void macb_validate(struct phylink_config *config, >> unsigned long *supported, >> struct phylink_link_state *state) >> { >> + bool have_1g = true, have_10g = true; >> struct net_device *ndev = to_net_dev(config->dev); >> __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, }; > > 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. >> 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
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 :)
Hi Jakub, On 10/25/21 8:44 PM, Jakub Kicinski wrote: > 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 :) 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/
On Tue, 26 Oct 2021 11:30:08 -0400 Sean Anderson wrote: > Hi Jakub, > > On 10/25/21 8:44 PM, Jakub Kicinski wrote: > > 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 :) > > 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/
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.
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: >> Hi Jakub, >> >> On 10/25/21 8:44 PM, Jakub Kicinski wrote: >>> 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 :) >> >> 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 >> 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/ >
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: >> On Mon, Oct 25, 2021 at 01:24:05PM -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. >>> >>> 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 <sean.anderson@seco.com> >>> --- >>> >>> 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(-) >>> >>> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c >>> index 309371abfe23..40bd5a069368 100644 >>> --- a/drivers/net/ethernet/cadence/macb_main.c >>> +++ b/drivers/net/ethernet/cadence/macb_main.c >>> @@ -510,11 +510,16 @@ static void macb_validate(struct phylink_config *config, >>> unsigned long *supported, >>> struct phylink_link_state *state) >>> { >>> + bool have_1g = true, have_10g = true; >>> struct net_device *ndev = to_net_dev(config->dev); >>> __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, }; >> >> 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. > >>> 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
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);
}
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);
}
On 10/26/21 1:04 PM, Russell King (Oracle) wrote: > 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; > } 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 > > 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); > } >
On Tue, Oct 26, 2021 at 01:28:15PM -0400, Sean Anderson wrote: > 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 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?
On 10/26/21 1:46 PM, Russell King (Oracle) wrote: > On Tue, Oct 26, 2021 at 01:28:15PM -0400, Sean Anderson wrote: >> 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 > > 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
On Tue, Oct 26, 2021 at 01:49:03PM -0400, Sean Anderson wrote: > On 10/26/21 1:46 PM, Russell King (Oracle) wrote: > > On Tue, Oct 26, 2021 at 01:28:15PM -0400, Sean Anderson wrote: > > > 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 > > > > 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. 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...
On 10/26/21 2:28 PM, Russell King (Oracle) wrote: > On Tue, Oct 26, 2021 at 01:49:03PM -0400, Sean Anderson wrote: >> On 10/26/21 1:46 PM, Russell King (Oracle) wrote: >> > On Tue, Oct 26, 2021 at 01:28:15PM -0400, Sean Anderson wrote: >> > > 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 >> > >> > 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. > > 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. The autonegotiation registers [2, 3] both mention SGMII "non SGMII". The mux to turn on SGMII is in the network_config register [3]. [1] https://www.xilinx.com/html_docs/registers/ug1087/gem___pcs_an_adv.html [2] https://www.xilinx.com/html_docs/registers/ug1087/gem___pcs_an_lp_base.html [3] https://www.xilinx.com/html_docs/registers/ug1087/gem___network_config.html > 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: >> >> 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. Claudiu and myself can certainly help in reviewing and testing on Microchip devices. The limitation that I see is that we only have 10/100 and 1000 speeds implemented in our SoC with limited number of link types covered (MII, RMII, GMII and RGMII). Our datasheets including different variants of MACB and GEM can give some information: MACB example in: http://ww1.microchip.com/downloads/en/DeviceDoc/SAM9X60-Data-Sheet-DS60001579A.pdf GEM example at 10/100 in: https://www.microchip.com/content/dam/mchp/documents/MPU32/ProductDocuments/DataSheets/SAMA5D2-Series-Data-sheet-ds60001476G.pdf GEM example with 10/100/1000 in: https://www.microchip.com/content/dam/mchp/documents/MPU32/ProductDocuments/DataSheets/SAMA5D3-Series-Data-sheet-DS60001609b.pdf We can also explore some of the design and configuration registers (DCFGn). > Perhaps someone from cadence could comment on what is actually supported > by gem/macb? That could be good as well. Best regards, Nicolas
diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c index 309371abfe23..40bd5a069368 100644 --- a/drivers/net/ethernet/cadence/macb_main.c +++ b/drivers/net/ethernet/cadence/macb_main.c @@ -510,11 +510,16 @@ static void macb_validate(struct phylink_config *config, unsigned long *supported, struct phylink_link_state *state) { + bool have_1g = true, have_10g = true; struct net_device *ndev = to_net_dev(config->dev); __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, }; 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; + } } - 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; + } else if (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; + } else if (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) @@ -558,11 +584,10 @@ static void macb_validate(struct phylink_config *config, phylink_set(mask, 100baseT_Half); phylink_set(mask, 100baseT_Full); - if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE && - (state->interface == PHY_INTERFACE_MODE_NA || + if ((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))) { + phy_interface_mode_is_rgmii(state->interface)) && have_1g) { phylink_set(mask, 1000baseT_Full); phylink_set(mask, 1000baseX_Full);
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 <sean.anderson@seco.com> --- 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(-)