[net-next PATCH v5] net: macb: Fix several edge cases in validate

[net-next PATCH v5] net: macb: Fix several edge cases in validate

[Sean Anderson]

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.

There is still the open question of what exactly the requirements for
SGMII and 10GBASER are, and what SGMII actually supports. If someone
from Cadence (or anyone else with access to the GEM/MACB datasheet)
could comment on this, it would be greatly appreciated. In particular,
what is supported by Cadence vs. vendor extension/limitation?

To address this, the current logic is split into three parts. First, we
determine what we support, then we eliminate unsupported interfaces, and
finally we set the appropriate link modes. There is still some cruft
related to NA, but this can be removed in a future patch.

Signed-off-by: Sean Anderson <sean.anderson@seco.com>
---

Changes in v5:
- Refactor, taking into account Russell's suggestions

Changes in v4:
- Drop cleanup patch
- Refactor to just address logic issues

Changes in v3:
- Order bugfix patch first

Changes in v2:
- New

 drivers/net/ethernet/cadence/macb_main.c | 108 +++++++++++++++--------
 1 file changed, 71 insertions(+), 37 deletions(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index 309371abfe23..cb436f758bfa 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -513,29 +513,47 @@ static void macb_validate(struct phylink_config *config,
 	struct net_device *ndev = to_net_dev(config->dev);
 	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
 	struct macb *bp = netdev_priv(ndev);
+	bool have_1g, have_sgmii, have_10g;
 
-	/* 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)) {
+	/* Determine what modes are supported */
+	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;
+	}
+
+	/* Eliminate unsupported modes */
+	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:
+		if (have_1g)
+			break;
 		linkmode_zero(supported);
 		return;
-	}
 
-	if (!macb_is_gem(bp) &&
-	    (state->interface == PHY_INTERFACE_MODE_GMII ||
-	     phy_interface_mode_is_rgmii(state->interface))) {
+	case PHY_INTERFACE_MODE_SGMII:
+		if (have_sgmii)
+			break;
 		linkmode_zero(supported);
 		return;
-	}
 
-	if (state->interface == PHY_INTERFACE_MODE_10GBASER &&
-	    !(bp->caps & MACB_CAPS_HIGH_SPEED &&
-	      bp->caps & MACB_CAPS_PCS)) {
+	case PHY_INTERFACE_MODE_10GBASER:
+		if (have_10g)
+			break;
+		fallthrough;
+
+	default:
 		linkmode_zero(supported);
 		return;
 	}
@@ -544,32 +562,48 @@ 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);
+	/* And set the appropriate mask */
+	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;
+
+	/* FIXME: Do we actually support 10/100 for SGMII? Half duplex? */
+	case PHY_INTERFACE_MODE_SGMII:
+		if (!have_sgmii && state->interface != PHY_INTERFACE_MODE_NA)
+			break;
+		fallthrough;
 
-	phylink_set(mask, 10baseT_Half);
-	phylink_set(mask, 10baseT_Full);
-	phylink_set(mask, 100baseT_Half);
-	phylink_set(mask, 100baseT_Full);
+	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:
+		if (have_1g) {
+			phylink_set(mask, 1000baseT_Full);
+			phylink_set(mask, 1000baseX_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);
+		} else if (state->interface != PHY_INTERFACE_MODE_NA) {
+			break;
+		}
+		fallthrough;
 
-		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);
 }
-- 
2.25.1

RE: [net-next PATCH v5] net: macb: Fix several edge cases in validate

[Parshuram Raju Thombare]

Hi Sean,

Thanks for this improvement.

>+	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;

As I understand, MACB_CAPS_GIGABIT_MODE_AVAILABLE is used as a quirk in configs
to prevent giga bit operation support, Nicolas should have more information about this.

macb_is_gem() tells whether giga bit operations is supported by HW, MACB_CAPS_PCS indicate
whether PCS is included in the design (needed for SGMII and 10G operation), MACB_CAPS_HIGH_SPEED
indicate if design supports 10G operation.

I believe this 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;

Regards,
Parshuram Thombare

Re: [net-next PATCH v5] net: macb: Fix several edge cases in validate

[Sean Anderson]

On 11/3/21 6:14 AM, Parshuram Raju Thombare wrote:
> Hi Sean,
> 
> Thanks for this improvement.
> 
>>+	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;
> 
> As I understand, MACB_CAPS_GIGABIT_MODE_AVAILABLE is used as a quirk in configs
> to prevent giga bit operation support, Nicolas should have more information about this.
> 
> macb_is_gem() tells whether giga bit operations is supported by HW, MACB_CAPS_PCS indicate
> whether PCS is included in the design (needed for SGMII and 10G operation), MACB_CAPS_HIGH_SPEED
> indicate if design supports 10G operation.
> 
> I believe this 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;

Ah, you are correct. It seems I forgot to invert this condition.

--Sean

Re: [net-next PATCH v5] net: macb: Fix several edge cases in validate

[Nicolas Ferre]

On 03/11/2021 at 11:14, Parshuram Raju Thombare wrote:
> Hi Sean,
> 
> Thanks for this improvement.
> 
>> +      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;
> 
> As I understand, MACB_CAPS_GIGABIT_MODE_AVAILABLE is used as a quirk in configs
> to prevent giga bit operation support, Nicolas should have more information about this.

That's right Parshuram.

> macb_is_gem() tells whether giga bit operations is supported by HW, MACB_CAPS_PCS indicate
> whether PCS is included in the design (needed for SGMII and 10G operation), MACB_CAPS_HIGH_SPEED
> indicate if design supports 10G operation.
> 
> I believe this 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;


Regards,
   Nicolas

-- 
Nicolas Ferre