[PATCH RFC net-next 0/5] net: dsa: MT7530: Convert to PHYLINK and add support for port 5

[PATCH RFC net-next 0/5] net: dsa: MT7530: Convert to PHYLINK and add support for port 5

[René van Dorst]

Here by I am sending my current patches for review.
I want to know if I am on the right track.

1. 0001-net-dsa-mt7530-Convert-to-PHYLINK-API.patch
   This patch converts mt7530 to PHYLINK API.
2. 0002-dt-bindings-net-dsa-mt7530-Add-support-for-port-5.patch
3. 0003-net-dsa-mt7530-Add-support-for-port-5.patch
   These 2 patches adding support for port 5 of the switch.

Optional:
4. 0004-dt-bindings-net-dsa-mt7530-Add-mediatek-ephy-handle-.patch
5. 0005-net-dsa-mt7530-Add-mediatek-ephy-handle-to-isolate-e.patch
   These 2 patches adding property "mediatek,ephy-handle".
   When set, it puts the external phy in isolation mode.
   This allows the switch PHY of port 0/4 to interface with 2nd GMAC of
   the SOC. The external phy, 2nd GMAC and switch port 5 shares the same
   MII bus.

FWIW: Also working on converting the mediatek ethernet driver to PHYLINK.
This need a bit more work duo to the SGMII work and support hardware which 
I don't have.
https://github.com/vDorst/linux-1/commit/54004b807cba0dcec1653c1c290c2e5aae5127c2

René van Dorst (5):
  net: dsa: mt7530: Convert to PHYLINK API
  dt-bindings: net: dsa: mt7530: Add support for port 5
  net: dsa: mt7530: Add support for port 5
  dt-bindings: net: dsa: mt7530: Add mediatek,ephy-handle to isolate
    ext. phy
  net: dsa: mt7530: Add mediatek,ephy-handle to isolate external phy

 .../devicetree/bindings/net/dsa/mt7530.txt    | 329 ++++++++++++++++
 drivers/net/dsa/mt7530.c                      | 366 +++++++++++++++---
 drivers/net/dsa/mt7530.h                      |  39 ++
 3 files changed, 688 insertions(+), 46 deletions(-)

-- 
2.20.1

[PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API

[René van Dorst]

Convert mt7530 to PHYLINK API

Signed-off-by: René van Dorst <opensource@vdorst.com>
---
 drivers/net/dsa/mt7530.c | 237 +++++++++++++++++++++++++++++----------
 drivers/net/dsa/mt7530.h |   9 ++
 2 files changed, 187 insertions(+), 59 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 3181e95586d6..9c5e4dd00826 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -13,7 +13,7 @@
 #include <linux/of_mdio.h>
 #include <linux/of_net.h>
 #include <linux/of_platform.h>
-#include <linux/phy.h>
+#include <linux/phylink.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 #include <linux/reset.h>
@@ -633,63 +633,6 @@ mt7530_get_sset_count(struct dsa_switch *ds, int port, int sset)
 	return ARRAY_SIZE(mt7530_mib);
 }
 
-static void mt7530_adjust_link(struct dsa_switch *ds, int port,
-			       struct phy_device *phydev)
-{
-	struct mt7530_priv *priv = ds->priv;
-
-	if (phy_is_pseudo_fixed_link(phydev)) {
-		dev_dbg(priv->dev, "phy-mode for master device = %x\n",
-			phydev->interface);
-
-		/* Setup TX circuit incluing relevant PAD and driving */
-		mt7530_pad_clk_setup(ds, phydev->interface);
-
-		if (priv->id == ID_MT7530) {
-			/* Setup RX circuit, relevant PAD and driving on the
-			 * host which must be placed after the setup on the
-			 * device side is all finished.
-			 */
-			mt7623_pad_clk_setup(ds);
-		}
-	} else {
-		u16 lcl_adv = 0, rmt_adv = 0;
-		u8 flowctrl;
-		u32 mcr = PMCR_USERP_LINK | PMCR_FORCE_MODE;
-
-		switch (phydev->speed) {
-		case SPEED_1000:
-			mcr |= PMCR_FORCE_SPEED_1000;
-			break;
-		case SPEED_100:
-			mcr |= PMCR_FORCE_SPEED_100;
-			break;
-		}
-
-		if (phydev->link)
-			mcr |= PMCR_FORCE_LNK;
-
-		if (phydev->duplex) {
-			mcr |= PMCR_FORCE_FDX;
-
-			if (phydev->pause)
-				rmt_adv = LPA_PAUSE_CAP;
-			if (phydev->asym_pause)
-				rmt_adv |= LPA_PAUSE_ASYM;
-
-			lcl_adv = linkmode_adv_to_lcl_adv_t(
-				phydev->advertising);
-			flowctrl = mii_resolve_flowctrl_fdx(lcl_adv, rmt_adv);
-
-			if (flowctrl & FLOW_CTRL_TX)
-				mcr |= PMCR_TX_FC_EN;
-			if (flowctrl & FLOW_CTRL_RX)
-				mcr |= PMCR_RX_FC_EN;
-		}
-		mt7530_write(priv, MT7530_PMCR_P(port), mcr);
-	}
-}
-
 static int
 mt7530_cpu_port_enable(struct mt7530_priv *priv,
 		       int port)
@@ -1323,6 +1266,178 @@ mt7530_setup(struct dsa_switch *ds)
 	return 0;
 }
 
+static void mt7530_phylink_mac_config(struct dsa_switch *ds, int port,
+				      unsigned int mode,
+				      const struct phylink_link_state *state)
+{
+	struct mt7530_priv *priv = ds->priv;
+	u32 mcr = PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | PMCR_BACKOFF_EN |
+		  PMCR_BACKPR_EN | PMCR_TX_EN | PMCR_RX_EN;
+
+	switch (port) {
+	case 0: /* Internal phy */
+	case 1:
+	case 2:
+	case 3:
+	case 4:
+		if (state->interface != PHY_INTERFACE_MODE_GMII)
+			goto unsupported;
+		break;
+	/* case 5: Port 5 is not supported! */
+	case 6: /* 1st cpu port */
+		if (state->interface != PHY_INTERFACE_MODE_RGMII &&
+		    state->interface != PHY_INTERFACE_MODE_TRGMII)
+			goto unsupported;
+
+		/* Setup TX circuit incluing relevant PAD and driving */
+		mt7530_pad_clk_setup(ds, state->interface);
+
+		if (priv->id == ID_MT7530) {
+			/* Setup RX circuit, relevant PAD and driving on the
+			 * host which must be placed after the setup on the
+			 * device side is all finished.
+			 */
+			mt7623_pad_clk_setup(ds);
+		}
+		break;
+	default:
+		dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
+		return;
+	}
+
+	if (!state->an_enabled || mode == MLO_AN_FIXED) {
+		mcr |= PMCR_FORCE_MODE;
+
+		if (state->speed == SPEED_1000)
+			mcr |= PMCR_FORCE_SPEED_1000;
+		if (state->speed == SPEED_100)
+			mcr |= PMCR_FORCE_SPEED_100;
+		if (state->duplex == DUPLEX_FULL)
+			mcr |= PMCR_FORCE_FDX;
+		if (state->link || mode == MLO_AN_FIXED)
+			mcr |= PMCR_FORCE_LNK;
+		if (state->pause || phylink_test(state->advertising, Pause))
+			mcr |= PMCR_TX_FC_EN | PMCR_RX_FC_EN;
+		if (state->pause & MLO_PAUSE_TX)
+			mcr |= PMCR_TX_FC_EN;
+		if (state->pause & MLO_PAUSE_RX)
+			mcr |= PMCR_RX_FC_EN;
+	}
+
+	mt7530_write(priv, MT7530_PMCR_P(port), mcr);
+
+	return;
+
+unsupported:
+	dev_err(ds->dev, "%s: P%d: Unsupported phy_interface mode: %d (%s)\n",
+		__func__, port, state->interface, phy_modes(state->interface));
+}
+
+static void mt7530_phylink_mac_link_down(struct dsa_switch *ds, int port,
+					 unsigned int mode,
+					 phy_interface_t interface)
+{
+	/* Do nothing */
+}
+
+static void mt7530_phylink_mac_link_up(struct dsa_switch *ds, int port,
+				       unsigned int mode,
+				       phy_interface_t interface,
+				       struct phy_device *phydev)
+{
+	/* Do nothing */
+}
+
+static void mt7530_phylink_validate(struct dsa_switch *ds, int port,
+				    unsigned long *supported,
+				    struct phylink_link_state *state)
+{
+	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
+
+	switch (port) {
+	case 0: /* Internal phy */
+	case 1:
+	case 2:
+	case 3:
+	case 4:
+		if (state->interface != PHY_INTERFACE_MODE_NA &&
+		    state->interface != PHY_INTERFACE_MODE_GMII)
+			goto unsupported;
+		break;
+	/* case 5: Port 5 not supported! */
+	case 6: /* 1st cpu port */
+		if (state->interface != PHY_INTERFACE_MODE_RGMII &&
+		    state->interface != PHY_INTERFACE_MODE_TRGMII)
+			goto unsupported;
+		break;
+	default:
+		linkmode_zero(supported);
+		dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
+		return;
+	}
+
+	phylink_set(mask, Autoneg);
+	phylink_set(mask, Pause);
+	phylink_set(mask, Asym_Pause);
+	phylink_set(mask, MII);
+
+	phylink_set(mask, 10baseT_Half);
+	phylink_set(mask, 10baseT_Full);
+	phylink_set(mask, 100baseT_Half);
+	phylink_set(mask, 100baseT_Full);
+	phylink_set(mask, 1000baseT_Full);
+	phylink_set(mask, 1000baseT_Half);
+
+	linkmode_and(supported, supported, mask);
+	linkmode_and(state->advertising, state->advertising, mask);
+	return;
+
+unsupported:
+	linkmode_zero(supported);
+	dev_err(ds->dev, "%s: unsupported interface mode: [0x%x] %s\n",
+		__func__, state->interface, phy_modes(state->interface));
+}
+
+static int
+mt7530_phylink_mac_link_state(struct dsa_switch *ds, int port,
+			      struct phylink_link_state *state)
+{
+	struct mt7530_priv *priv = ds->priv;
+	u32 pmsr;
+
+	if (port < 0 || port >= MT7530_NUM_PORTS)
+		return -EINVAL;
+
+	pmsr = mt7530_read(priv, MT7530_PMSR_P(port));
+
+	state->link = (pmsr & PMSR_LINK);
+	state->an_complete = state->link;
+	state->duplex = (pmsr & PMSR_DPX) >> 1;
+
+	switch (pmsr & (PMSR_SPEED_1000 | PMSR_SPEED_100)) {
+	case 0:
+		state->speed = SPEED_10;
+		break;
+	case PMSR_SPEED_100:
+		state->speed = SPEED_100;
+		break;
+	case PMSR_SPEED_1000:
+		state->speed = SPEED_1000;
+		break;
+	default:
+		state->speed = SPEED_UNKNOWN;
+		break;
+	}
+
+	state->pause = 0;
+	if (pmsr & PMSR_RX_FC)
+		state->pause |= MLO_PAUSE_RX;
+	if (pmsr & PMSR_TX_FC)
+		state->pause |= MLO_PAUSE_TX;
+
+	return 1;
+}
+
 static const struct dsa_switch_ops mt7530_switch_ops = {
 	.get_tag_protocol	= mtk_get_tag_protocol,
 	.setup			= mt7530_setup,
@@ -1331,7 +1446,6 @@ static const struct dsa_switch_ops mt7530_switch_ops = {
 	.phy_write		= mt7530_phy_write,
 	.get_ethtool_stats	= mt7530_get_ethtool_stats,
 	.get_sset_count		= mt7530_get_sset_count,
-	.adjust_link		= mt7530_adjust_link,
 	.port_enable		= mt7530_port_enable,
 	.port_disable		= mt7530_port_disable,
 	.port_stp_state_set	= mt7530_stp_state_set,
@@ -1344,6 +1458,11 @@ static const struct dsa_switch_ops mt7530_switch_ops = {
 	.port_vlan_prepare	= mt7530_port_vlan_prepare,
 	.port_vlan_add		= mt7530_port_vlan_add,
 	.port_vlan_del		= mt7530_port_vlan_del,
+	.phylink_validate	= mt7530_phylink_validate,
+	.phylink_mac_link_state = mt7530_phylink_mac_link_state,
+	.phylink_mac_config	= mt7530_phylink_mac_config,
+	.phylink_mac_link_down	= mt7530_phylink_mac_link_down,
+	.phylink_mac_link_up	= mt7530_phylink_mac_link_up,
 };
 
 static const struct of_device_id mt7530_of_match[] = {
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index bfac90f48102..41d9a132ac70 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -198,6 +198,7 @@ enum mt7530_vlan_port_attr {
 #define  PMCR_FORCE_SPEED_100		BIT(2)
 #define  PMCR_FORCE_FDX			BIT(1)
 #define  PMCR_FORCE_LNK			BIT(0)
+#define  PMCR_FORCE_LNK_DOWN		PMCR_FORCE_MODE
 #define  PMCR_COMMON_LINK		(PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | \
 					 PMCR_BACKOFF_EN | PMCR_BACKPR_EN | \
 					 PMCR_TX_EN | PMCR_RX_EN | \
@@ -218,6 +219,14 @@ enum mt7530_vlan_port_attr {
 					 PMCR_TX_FC_EN | PMCR_RX_FC_EN)
 
 #define MT7530_PMSR_P(x)		(0x3008 + (x) * 0x100)
+#define  PMSR_EEE1G			BIT(7)
+#define  PMSR_EEE100M			BIT(6)
+#define  PMSR_RX_FC			BIT(5)
+#define  PMSR_TX_FC			BIT(4)
+#define  PMSR_SPEED_1000		BIT(3)
+#define  PMSR_SPEED_100			BIT(2)
+#define  PMSR_DPX			BIT(1)
+#define  PMSR_LINK			BIT(0)
 
 /* Register for MIB */
 #define MT7530_PORT_MIB_COUNTER(x)	(0x4000 + (x) * 0x100)
-- 
2.20.1

Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API

[Russell King - ARM Linux admin]

Hi,

On Mon, Jun 24, 2019 at 04:52:47PM +0200, René van Dorst wrote:
> Convert mt7530 to PHYLINK API
> 
> Signed-off-by: René van Dorst <opensource@vdorst.com>
> ---
>  drivers/net/dsa/mt7530.c | 237 +++++++++++++++++++++++++++++----------
>  drivers/net/dsa/mt7530.h |   9 ++
>  2 files changed, 187 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 3181e95586d6..9c5e4dd00826 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -13,7 +13,7 @@
>  #include <linux/of_mdio.h>
>  #include <linux/of_net.h>
>  #include <linux/of_platform.h>
> -#include <linux/phy.h>
> +#include <linux/phylink.h>
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/reset.h>
> @@ -633,63 +633,6 @@ mt7530_get_sset_count(struct dsa_switch *ds, int port, int sset)
>  	return ARRAY_SIZE(mt7530_mib);
>  }
>  
> -static void mt7530_adjust_link(struct dsa_switch *ds, int port,
> -			       struct phy_device *phydev)
> -{
> -	struct mt7530_priv *priv = ds->priv;
> -
> -	if (phy_is_pseudo_fixed_link(phydev)) {
> -		dev_dbg(priv->dev, "phy-mode for master device = %x\n",
> -			phydev->interface);
> -
> -		/* Setup TX circuit incluing relevant PAD and driving */
> -		mt7530_pad_clk_setup(ds, phydev->interface);
> -
> -		if (priv->id == ID_MT7530) {
> -			/* Setup RX circuit, relevant PAD and driving on the
> -			 * host which must be placed after the setup on the
> -			 * device side is all finished.
> -			 */
> -			mt7623_pad_clk_setup(ds);
> -		}
> -	} else {
> -		u16 lcl_adv = 0, rmt_adv = 0;
> -		u8 flowctrl;
> -		u32 mcr = PMCR_USERP_LINK | PMCR_FORCE_MODE;
> -
> -		switch (phydev->speed) {
> -		case SPEED_1000:
> -			mcr |= PMCR_FORCE_SPEED_1000;
> -			break;
> -		case SPEED_100:
> -			mcr |= PMCR_FORCE_SPEED_100;
> -			break;
> -		}
> -
> -		if (phydev->link)
> -			mcr |= PMCR_FORCE_LNK;
> -
> -		if (phydev->duplex) {
> -			mcr |= PMCR_FORCE_FDX;
> -
> -			if (phydev->pause)
> -				rmt_adv = LPA_PAUSE_CAP;
> -			if (phydev->asym_pause)
> -				rmt_adv |= LPA_PAUSE_ASYM;
> -
> -			lcl_adv = linkmode_adv_to_lcl_adv_t(
> -				phydev->advertising);
> -			flowctrl = mii_resolve_flowctrl_fdx(lcl_adv, rmt_adv);
> -
> -			if (flowctrl & FLOW_CTRL_TX)
> -				mcr |= PMCR_TX_FC_EN;
> -			if (flowctrl & FLOW_CTRL_RX)
> -				mcr |= PMCR_RX_FC_EN;
> -		}
> -		mt7530_write(priv, MT7530_PMCR_P(port), mcr);
> -	}
> -}
> -
>  static int
>  mt7530_cpu_port_enable(struct mt7530_priv *priv,
>  		       int port)
> @@ -1323,6 +1266,178 @@ mt7530_setup(struct dsa_switch *ds)
>  	return 0;
>  }
>  
> +static void mt7530_phylink_mac_config(struct dsa_switch *ds, int port,
> +				      unsigned int mode,
> +				      const struct phylink_link_state *state)
> +{
> +	struct mt7530_priv *priv = ds->priv;
> +	u32 mcr = PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | PMCR_BACKOFF_EN |
> +		  PMCR_BACKPR_EN | PMCR_TX_EN | PMCR_RX_EN;
> +
> +	switch (port) {
> +	case 0: /* Internal phy */
> +	case 1:
> +	case 2:
> +	case 3:
> +	case 4:
> +		if (state->interface != PHY_INTERFACE_MODE_GMII)
> +			goto unsupported;
> +		break;
> +	/* case 5: Port 5 is not supported! */
> +	case 6: /* 1st cpu port */
> +		if (state->interface != PHY_INTERFACE_MODE_RGMII &&
> +		    state->interface != PHY_INTERFACE_MODE_TRGMII)
> +			goto unsupported;
> +
> +		/* Setup TX circuit incluing relevant PAD and driving */
> +		mt7530_pad_clk_setup(ds, state->interface);
> +
> +		if (priv->id == ID_MT7530) {
> +			/* Setup RX circuit, relevant PAD and driving on the
> +			 * host which must be placed after the setup on the
> +			 * device side is all finished.
> +			 */
> +			mt7623_pad_clk_setup(ds);
> +		}
> +		break;
> +	default:
> +		dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
> +		return;
> +	}
> +
> +	if (!state->an_enabled || mode == MLO_AN_FIXED) {
> +		mcr |= PMCR_FORCE_MODE;
> +
> +		if (state->speed == SPEED_1000)
> +			mcr |= PMCR_FORCE_SPEED_1000;
> +		if (state->speed == SPEED_100)
> +			mcr |= PMCR_FORCE_SPEED_100;
> +		if (state->duplex == DUPLEX_FULL)
> +			mcr |= PMCR_FORCE_FDX;
> +		if (state->link || mode == MLO_AN_FIXED)
> +			mcr |= PMCR_FORCE_LNK;

This should be removed - state->link is not for use in mac_config.
Even in fixed mode, the link can be brought up/down by means of a
gpio, and this should be dealt with via the mac_link_* functions.

> +		if (state->pause || phylink_test(state->advertising, Pause))
> +			mcr |= PMCR_TX_FC_EN | PMCR_RX_FC_EN;
> +		if (state->pause & MLO_PAUSE_TX)
> +			mcr |= PMCR_TX_FC_EN;
> +		if (state->pause & MLO_PAUSE_RX)
> +			mcr |= PMCR_RX_FC_EN;

This is clearly wrong - if any bit in state->pause is set, then we
end up with both PMCR_TX_FC_EN | PMCR_RX_FC_EN set.  If we have Pause
Pause set in the advertising mask, then both are set.  This doesn't
seem right - are these bits setting the advertisement, or are they
telling the MAC to use flow control?

> +	}
> +
> +	mt7530_write(priv, MT7530_PMCR_P(port), mcr);
> +
> +	return;
> +
> +unsupported:
> +	dev_err(ds->dev, "%s: P%d: Unsupported phy_interface mode: %d (%s)\n",
> +		__func__, port, state->interface, phy_modes(state->interface));
> +}
> +
> +static void mt7530_phylink_mac_link_down(struct dsa_switch *ds, int port,
> +					 unsigned int mode,
> +					 phy_interface_t interface)
> +{
> +	/* Do nothing */
> +}
> +
> +static void mt7530_phylink_mac_link_up(struct dsa_switch *ds, int port,
> +				       unsigned int mode,
> +				       phy_interface_t interface,
> +				       struct phy_device *phydev)
> +{
> +	/* Do nothing */
> +}

These two are where you should be forcing the link up or down if
required (basically, inband modes should let the link come up/down
irrespective of these functions, otherwise it should be forced.)

> +
> +static void mt7530_phylink_validate(struct dsa_switch *ds, int port,
> +				    unsigned long *supported,
> +				    struct phylink_link_state *state)
> +{
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> +
> +	switch (port) {
> +	case 0: /* Internal phy */
> +	case 1:
> +	case 2:
> +	case 3:
> +	case 4:
> +		if (state->interface != PHY_INTERFACE_MODE_NA &&
> +		    state->interface != PHY_INTERFACE_MODE_GMII)
> +			goto unsupported;
> +		break;
> +	/* case 5: Port 5 not supported! */
> +	case 6: /* 1st cpu port */
> +		if (state->interface != PHY_INTERFACE_MODE_RGMII &&
> +		    state->interface != PHY_INTERFACE_MODE_TRGMII)

PHY_INTERFACE_MODE_NA ?

> +			goto unsupported;
> +		break;
> +	default:
> +		linkmode_zero(supported);
> +		dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
> +		return;
> +	}
> +
> +	phylink_set(mask, Autoneg);
> +	phylink_set(mask, Pause);
> +	phylink_set(mask, Asym_Pause);
> +	phylink_set(mask, MII);
> +
> +	phylink_set(mask, 10baseT_Half);
> +	phylink_set(mask, 10baseT_Full);
> +	phylink_set(mask, 100baseT_Half);
> +	phylink_set(mask, 100baseT_Full);
> +	phylink_set(mask, 1000baseT_Full);
> +	phylink_set(mask, 1000baseT_Half);

You seem to be missing phylink_set_port_modes() here.

> +
> +	linkmode_and(supported, supported, mask);
> +	linkmode_and(state->advertising, state->advertising, mask);
> +	return;
> +
> +unsupported:
> +	linkmode_zero(supported);
> +	dev_err(ds->dev, "%s: unsupported interface mode: [0x%x] %s\n",
> +		__func__, state->interface, phy_modes(state->interface));

Not a good idea to print this at error level; sometimes we just probe
for support.

Eg, think about a SFP cage, and a SFP is plugged in that uses a PHY
interface mode that the MAC can't support - we detect that by the
validation failing, and printing a more meaningful message in phylink
itself.

> +}
> +
> +static int
> +mt7530_phylink_mac_link_state(struct dsa_switch *ds, int port,
> +			      struct phylink_link_state *state)
> +{
> +	struct mt7530_priv *priv = ds->priv;
> +	u32 pmsr;
> +
> +	if (port < 0 || port >= MT7530_NUM_PORTS)
> +		return -EINVAL;
> +
> +	pmsr = mt7530_read(priv, MT7530_PMSR_P(port));
> +
> +	state->link = (pmsr & PMSR_LINK);
> +	state->an_complete = state->link;
> +	state->duplex = (pmsr & PMSR_DPX) >> 1;
> +
> +	switch (pmsr & (PMSR_SPEED_1000 | PMSR_SPEED_100)) {
> +	case 0:
> +		state->speed = SPEED_10;
> +		break;
> +	case PMSR_SPEED_100:
> +		state->speed = SPEED_100;
> +		break;
> +	case PMSR_SPEED_1000:
> +		state->speed = SPEED_1000;
> +		break;
> +	default:
> +		state->speed = SPEED_UNKNOWN;
> +		break;
> +	}
> +
> +	state->pause = 0;
> +	if (pmsr & PMSR_RX_FC)
> +		state->pause |= MLO_PAUSE_RX;
> +	if (pmsr & PMSR_TX_FC)
> +		state->pause |= MLO_PAUSE_TX;
> +
> +	return 1;
> +}
> +
>  static const struct dsa_switch_ops mt7530_switch_ops = {
>  	.get_tag_protocol	= mtk_get_tag_protocol,
>  	.setup			= mt7530_setup,
> @@ -1331,7 +1446,6 @@ static const struct dsa_switch_ops mt7530_switch_ops = {
>  	.phy_write		= mt7530_phy_write,
>  	.get_ethtool_stats	= mt7530_get_ethtool_stats,
>  	.get_sset_count		= mt7530_get_sset_count,
> -	.adjust_link		= mt7530_adjust_link,
>  	.port_enable		= mt7530_port_enable,
>  	.port_disable		= mt7530_port_disable,
>  	.port_stp_state_set	= mt7530_stp_state_set,
> @@ -1344,6 +1458,11 @@ static const struct dsa_switch_ops mt7530_switch_ops = {
>  	.port_vlan_prepare	= mt7530_port_vlan_prepare,
>  	.port_vlan_add		= mt7530_port_vlan_add,
>  	.port_vlan_del		= mt7530_port_vlan_del,
> +	.phylink_validate	= mt7530_phylink_validate,
> +	.phylink_mac_link_state = mt7530_phylink_mac_link_state,
> +	.phylink_mac_config	= mt7530_phylink_mac_config,
> +	.phylink_mac_link_down	= mt7530_phylink_mac_link_down,
> +	.phylink_mac_link_up	= mt7530_phylink_mac_link_up,
>  };
>  
>  static const struct of_device_id mt7530_of_match[] = {
> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
> index bfac90f48102..41d9a132ac70 100644
> --- a/drivers/net/dsa/mt7530.h
> +++ b/drivers/net/dsa/mt7530.h
> @@ -198,6 +198,7 @@ enum mt7530_vlan_port_attr {
>  #define  PMCR_FORCE_SPEED_100		BIT(2)
>  #define  PMCR_FORCE_FDX			BIT(1)
>  #define  PMCR_FORCE_LNK			BIT(0)
> +#define  PMCR_FORCE_LNK_DOWN		PMCR_FORCE_MODE
>  #define  PMCR_COMMON_LINK		(PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | \
>  					 PMCR_BACKOFF_EN | PMCR_BACKPR_EN | \
>  					 PMCR_TX_EN | PMCR_RX_EN | \
> @@ -218,6 +219,14 @@ enum mt7530_vlan_port_attr {
>  					 PMCR_TX_FC_EN | PMCR_RX_FC_EN)
>  
>  #define MT7530_PMSR_P(x)		(0x3008 + (x) * 0x100)
> +#define  PMSR_EEE1G			BIT(7)
> +#define  PMSR_EEE100M			BIT(6)
> +#define  PMSR_RX_FC			BIT(5)
> +#define  PMSR_TX_FC			BIT(4)
> +#define  PMSR_SPEED_1000		BIT(3)
> +#define  PMSR_SPEED_100			BIT(2)
> +#define  PMSR_DPX			BIT(1)
> +#define  PMSR_LINK			BIT(0)
>  
>  /* Register for MIB */
>  #define MT7530_PORT_MIB_COUNTER(x)	(0x4000 + (x) * 0x100)
> -- 
> 2.20.1
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API

[René van Dorst]

Quoting Russell King - ARM Linux admin <linux@armlinux.org.uk>:

Hi Russel,

Thanks for your review, see also my comments and questions below.

> Hi,
>
> On Mon, Jun 24, 2019 at 04:52:47PM +0200, René van Dorst wrote:
>> Convert mt7530 to PHYLINK API
>>
>> Signed-off-by: René van Dorst <opensource@vdorst.com>
>> ---
>>  drivers/net/dsa/mt7530.c | 237 +++++++++++++++++++++++++++++----------
>>  drivers/net/dsa/mt7530.h |   9 ++
>>  2 files changed, 187 insertions(+), 59 deletions(-)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index 3181e95586d6..9c5e4dd00826 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -13,7 +13,7 @@
>>  #include <linux/of_mdio.h>
>>  #include <linux/of_net.h>
>>  #include <linux/of_platform.h>
>> -#include <linux/phy.h>
>> +#include <linux/phylink.h>
>>  #include <linux/regmap.h>
>>  #include <linux/regulator/consumer.h>
>>  #include <linux/reset.h>
>> @@ -633,63 +633,6 @@ mt7530_get_sset_count(struct dsa_switch *ds,
>> int port, int sset)
>>      return ARRAY_SIZE(mt7530_mib);
>>  }
>>
>> -static void mt7530_adjust_link(struct dsa_switch *ds, int port,
>> -                           struct phy_device *phydev)
>> -{
>> -    struct mt7530_priv *priv = ds->priv;
>> -
>> -    if (phy_is_pseudo_fixed_link(phydev)) {
>> -            dev_dbg(priv->dev, "phy-mode for master device = %x\n",
>> -                    phydev->interface);
>> -
>> -            /* Setup TX circuit incluing relevant PAD and driving */
>> -            mt7530_pad_clk_setup(ds, phydev->interface);
>> -
>> -            if (priv->id == ID_MT7530) {
>> -                    /* Setup RX circuit, relevant PAD and driving on the
>> -                     * host which must be placed after the setup on the
>> -                     * device side is all finished.
>> -                     */
>> -                    mt7623_pad_clk_setup(ds);
>> -            }
>> -    } else {
>> -            u16 lcl_adv = 0, rmt_adv = 0;
>> -            u8 flowctrl;
>> -            u32 mcr = PMCR_USERP_LINK | PMCR_FORCE_MODE;
>> -
>> -            switch (phydev->speed) {
>> -            case SPEED_1000:
>> -                    mcr |= PMCR_FORCE_SPEED_1000;
>> -                    break;
>> -            case SPEED_100:
>> -                    mcr |= PMCR_FORCE_SPEED_100;
>> -                    break;
>> -            }
>> -
>> -            if (phydev->link)
>> -                    mcr |= PMCR_FORCE_LNK;
>> -
>> -            if (phydev->duplex) {
>> -                    mcr |= PMCR_FORCE_FDX;
>> -
>> -                    if (phydev->pause)
>> -                            rmt_adv = LPA_PAUSE_CAP;
>> -                    if (phydev->asym_pause)
>> -                            rmt_adv |= LPA_PAUSE_ASYM;
>> -
>> -                    lcl_adv = linkmode_adv_to_lcl_adv_t(
>> -                            phydev->advertising);
>> -                    flowctrl = mii_resolve_flowctrl_fdx(lcl_adv, rmt_adv);
>> -
>> -                    if (flowctrl & FLOW_CTRL_TX)
>> -                            mcr |= PMCR_TX_FC_EN;
>> -                    if (flowctrl & FLOW_CTRL_RX)
>> -                            mcr |= PMCR_RX_FC_EN;
>> -            }
>> -            mt7530_write(priv, MT7530_PMCR_P(port), mcr);
>> -    }
>> -}
>> -
>>  static int
>>  mt7530_cpu_port_enable(struct mt7530_priv *priv,
>>                     int port)
>> @@ -1323,6 +1266,178 @@ mt7530_setup(struct dsa_switch *ds)
>>      return 0;
>>  }
>>
>> +static void mt7530_phylink_mac_config(struct dsa_switch *ds, int port,
>> +                                  unsigned int mode,
>> +                                  const struct phylink_link_state *state)
>> +{
>> +    struct mt7530_priv *priv = ds->priv;
>> +    u32 mcr = PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | PMCR_BACKOFF_EN |
>> +              PMCR_BACKPR_EN | PMCR_TX_EN | PMCR_RX_EN;
>> +
>> +    switch (port) {
>> +    case 0: /* Internal phy */
>> +    case 1:
>> +    case 2:
>> +    case 3:
>> +    case 4:
>> +            if (state->interface != PHY_INTERFACE_MODE_GMII)
>> +                    goto unsupported;
>> +            break;
>> +    /* case 5: Port 5 is not supported! */
>> +    case 6: /* 1st cpu port */
>> +            if (state->interface != PHY_INTERFACE_MODE_RGMII &&
>> +                state->interface != PHY_INTERFACE_MODE_TRGMII)
>> +                    goto unsupported;
>> +
>> +            /* Setup TX circuit incluing relevant PAD and driving */
>> +            mt7530_pad_clk_setup(ds, state->interface);
>> +
>> +            if (priv->id == ID_MT7530) {
>> +                    /* Setup RX circuit, relevant PAD and driving on the
>> +                     * host which must be placed after the setup on the
>> +                     * device side is all finished.
>> +                     */
>> +                    mt7623_pad_clk_setup(ds);
>> +            }
>> +            break;
>> +    default:
>> +            dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
>> +            return;
>> +    }
>> +
>> +    if (!state->an_enabled || mode == MLO_AN_FIXED) {
>> +            mcr |= PMCR_FORCE_MODE;
>> +
>> +            if (state->speed == SPEED_1000)
>> +                    mcr |= PMCR_FORCE_SPEED_1000;
>> +            if (state->speed == SPEED_100)
>> +                    mcr |= PMCR_FORCE_SPEED_100;
>> +            if (state->duplex == DUPLEX_FULL)
>> +                    mcr |= PMCR_FORCE_FDX;
>> +            if (state->link || mode == MLO_AN_FIXED)
>> +                    mcr |= PMCR_FORCE_LNK;
>
> This should be removed - state->link is not for use in mac_config.
> Even in fixed mode, the link can be brought up/down by means of a
> gpio, and this should be dealt with via the mac_link_* functions.

Maybe I understand it wrong, but is it the intention that in
phylink_mac_config with modes MLO_AN_FIXED and MLO_AN_PHY the MAC is always
forces into a certain speed/mode/interface. So it never auto-negotiate because
phylink select the best configuration for you?

Also the PMCR_FORCE_LNK is only set in phylink_link_up() or can I do it here
and do nothing phylink_link_up()?


Other question:
Where does the MAC enable/disable TX and RX fits best? port_{enable,disable}?
Or only mac_config() and port_disable?

>
>> +            if (state->pause || phylink_test(state->advertising, Pause))
>> +                    mcr |= PMCR_TX_FC_EN | PMCR_RX_FC_EN;
>> +            if (state->pause & MLO_PAUSE_TX)
>> +                    mcr |= PMCR_TX_FC_EN;
>> +            if (state->pause & MLO_PAUSE_RX)
>> +                    mcr |= PMCR_RX_FC_EN;
>
> This is clearly wrong - if any bit in state->pause is set, then we
> end up with both PMCR_TX_FC_EN | PMCR_RX_FC_EN set.  If we have Pause
> Pause set in the advertising mask, then both are set.  This doesn't
> seem right - are these bits setting the advertisement, or are they
> telling the MAC to use flow control?

Last one, tell the MAC to use flow control.

Qoute of the datasheet:
If you want to disable TX and RX flow control disable bit 4 and 5

MAC Control Register:
BIT(4): Force TX FC:
0: Disabled
1: Let the MAC transmit a pause frame when operating in full-duplex
    mode with low internal free memory page count.
BIT(5): Force RX FC:
0: Disabled
1: Let the MA accept a pause frame when operating in full-duplex
    mode.


On the current driver both bits are set in a forced-link situation.

If we always forces the MAC mode I think I always set these bits and don't
anything with the Pause modes? Is that the right way to do it?

>
>> +    }
>> +
>> +    mt7530_write(priv, MT7530_PMCR_P(port), mcr);
>> +
>> +    return;
>> +
>> +unsupported:
>> +    dev_err(ds->dev, "%s: P%d: Unsupported phy_interface mode: %d (%s)\n",
>> +            __func__, port, state->interface, phy_modes(state->interface));
>> +}
>> +
>> +static void mt7530_phylink_mac_link_down(struct dsa_switch *ds, int port,
>> +                                     unsigned int mode,
>> +                                     phy_interface_t interface)
>> +{
>> +    /* Do nothing */
>> +}
>> +
>> +static void mt7530_phylink_mac_link_up(struct dsa_switch *ds, int port,
>> +                                   unsigned int mode,
>> +                                   phy_interface_t interface,
>> +                                   struct phy_device *phydev)
>> +{
>> +    /* Do nothing */
>> +}
>
> These two are where you should be forcing the link up or down if
> required (basically, inband modes should let the link come up/down
> irrespective of these functions, otherwise it should be forced.)
>

OK

>> +
>> +static void mt7530_phylink_validate(struct dsa_switch *ds, int port,
>> +                                unsigned long *supported,
>> +                                struct phylink_link_state *state)
>> +{
>> +    __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
>> +
>> +    switch (port) {
>> +    case 0: /* Internal phy */
>> +    case 1:
>> +    case 2:
>> +    case 3:
>> +    case 4:
>> +            if (state->interface != PHY_INTERFACE_MODE_NA &&
>> +                state->interface != PHY_INTERFACE_MODE_GMII)
>> +                    goto unsupported;
>> +            break;
>> +    /* case 5: Port 5 not supported! */
>> +    case 6: /* 1st cpu port */
>> +            if (state->interface != PHY_INTERFACE_MODE_RGMII &&
>> +                state->interface != PHY_INTERFACE_MODE_TRGMII)
>
> PHY_INTERFACE_MODE_NA ?

You mean PHY_INTERFACE_MODE_NA is missing?
PHY_INTERFACE_MODE_TRGMII is a valid mode for this port.

>
>> +                    goto unsupported;
>> +            break;
>> +    default:
>> +            linkmode_zero(supported);
>> +            dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
>> +            return;
>> +    }
>> +
>> +    phylink_set(mask, Autoneg);
>> +    phylink_set(mask, Pause);
>> +    phylink_set(mask, Asym_Pause);
>> +    phylink_set(mask, MII);
>> +
>> +    phylink_set(mask, 10baseT_Half);
>> +    phylink_set(mask, 10baseT_Full);
>> +    phylink_set(mask, 100baseT_Half);
>> +    phylink_set(mask, 100baseT_Full);
>> +    phylink_set(mask, 1000baseT_Full);
>> +    phylink_set(mask, 1000baseT_Half);
>
> You seem to be missing phylink_set_port_modes() here.

OK

>
>> +
>> +    linkmode_and(supported, supported, mask);
>> +    linkmode_and(state->advertising, state->advertising, mask);
>> +    return;
>> +
>> +unsupported:
>> +    linkmode_zero(supported);
>> +    dev_err(ds->dev, "%s: unsupported interface mode: [0x%x] %s\n",
>> +            __func__, state->interface, phy_modes(state->interface));
>
> Not a good idea to print this at error level; sometimes we just probe
> for support.
>
> Eg, think about a SFP cage, and a SFP is plugged in that uses a PHY
> interface mode that the MAC can't support - we detect that by the
> validation failing, and printing a more meaningful message in phylink
> itself.

OK, It will be removed.

>
>> +}
>> +
>> +static int
>> +mt7530_phylink_mac_link_state(struct dsa_switch *ds, int port,
>> +                          struct phylink_link_state *state)
>> +{
>> +    struct mt7530_priv *priv = ds->priv;
>> +    u32 pmsr;
>> +
>> +    if (port < 0 || port >= MT7530_NUM_PORTS)
>> +            return -EINVAL;
>> +
>> +    pmsr = mt7530_read(priv, MT7530_PMSR_P(port));
>> +
>> +    state->link = (pmsr & PMSR_LINK);
>> +    state->an_complete = state->link;
>> +    state->duplex = (pmsr & PMSR_DPX) >> 1;
>> +
>> +    switch (pmsr & (PMSR_SPEED_1000 | PMSR_SPEED_100)) {
>> +    case 0:
>> +            state->speed = SPEED_10;
>> +            break;
>> +    case PMSR_SPEED_100:
>> +            state->speed = SPEED_100;
>> +            break;
>> +    case PMSR_SPEED_1000:
>> +            state->speed = SPEED_1000;
>> +            break;
>> +    default:
>> +            state->speed = SPEED_UNKNOWN;
>> +            break;
>> +    }
>> +
>> +    state->pause = 0;
>> +    if (pmsr & PMSR_RX_FC)
>> +            state->pause |= MLO_PAUSE_RX;
>> +    if (pmsr & PMSR_TX_FC)
>> +            state->pause |= MLO_PAUSE_TX;
>> +
>> +    return 1;
>> +}
>> +
>>  static const struct dsa_switch_ops mt7530_switch_ops = {
>>      .get_tag_protocol       = mtk_get_tag_protocol,
>>      .setup                  = mt7530_setup,
>> @@ -1331,7 +1446,6 @@ static const struct dsa_switch_ops
>> mt7530_switch_ops = {
>>      .phy_write              = mt7530_phy_write,
>>      .get_ethtool_stats      = mt7530_get_ethtool_stats,
>>      .get_sset_count         = mt7530_get_sset_count,
>> -    .adjust_link            = mt7530_adjust_link,
>>      .port_enable            = mt7530_port_enable,
>>      .port_disable           = mt7530_port_disable,
>>      .port_stp_state_set     = mt7530_stp_state_set,
>> @@ -1344,6 +1458,11 @@ static const struct dsa_switch_ops
>> mt7530_switch_ops = {
>>      .port_vlan_prepare      = mt7530_port_vlan_prepare,
>>      .port_vlan_add          = mt7530_port_vlan_add,
>>      .port_vlan_del          = mt7530_port_vlan_del,
>> +    .phylink_validate       = mt7530_phylink_validate,
>> +    .phylink_mac_link_state = mt7530_phylink_mac_link_state,
>> +    .phylink_mac_config     = mt7530_phylink_mac_config,
>> +    .phylink_mac_link_down  = mt7530_phylink_mac_link_down,
>> +    .phylink_mac_link_up    = mt7530_phylink_mac_link_up,
>>  };
>>
>>  static const struct of_device_id mt7530_of_match[] = {
>> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
>> index bfac90f48102..41d9a132ac70 100644
>> --- a/drivers/net/dsa/mt7530.h
>> +++ b/drivers/net/dsa/mt7530.h
>> @@ -198,6 +198,7 @@ enum mt7530_vlan_port_attr {
>>  #define  PMCR_FORCE_SPEED_100               BIT(2)
>>  #define  PMCR_FORCE_FDX                     BIT(1)
>>  #define  PMCR_FORCE_LNK                     BIT(0)
>> +#define  PMCR_FORCE_LNK_DOWN                PMCR_FORCE_MODE
>>  #define  PMCR_COMMON_LINK           (PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | \
>>                                       PMCR_BACKOFF_EN | PMCR_BACKPR_EN | \
>>                                       PMCR_TX_EN | PMCR_RX_EN | \
>> @@ -218,6 +219,14 @@ enum mt7530_vlan_port_attr {
>>                                       PMCR_TX_FC_EN | PMCR_RX_FC_EN)
>>
>>  #define MT7530_PMSR_P(x)            (0x3008 + (x) * 0x100)
>> +#define  PMSR_EEE1G                 BIT(7)
>> +#define  PMSR_EEE100M                       BIT(6)
>> +#define  PMSR_RX_FC                 BIT(5)
>> +#define  PMSR_TX_FC                 BIT(4)
>> +#define  PMSR_SPEED_1000            BIT(3)
>> +#define  PMSR_SPEED_100                     BIT(2)
>> +#define  PMSR_DPX                   BIT(1)
>> +#define  PMSR_LINK                  BIT(0)
>>
>>  /* Register for MIB */
>>  #define MT7530_PORT_MIB_COUNTER(x)  (0x4000 + (x) * 0x100)
>> --
>> 2.20.1
>>
>>
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up

Greats,

René

Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API

[Russell King - ARM Linux admin]

On Tue, Jun 25, 2019 at 11:31:58AM +0000, René van Dorst wrote:
> > > +            if (state->link || mode == MLO_AN_FIXED)
> > > +                    mcr |= PMCR_FORCE_LNK;
> > 
> > This should be removed - state->link is not for use in mac_config.
> > Even in fixed mode, the link can be brought up/down by means of a
> > gpio, and this should be dealt with via the mac_link_* functions.
> 
> Maybe I understand it wrong, but is it the intention that in
> phylink_mac_config with modes MLO_AN_FIXED and MLO_AN_PHY the MAC is always
> forces into a certain speed/mode/interface. So it never auto-negotiate because
> phylink select the best configuration for you?

You are not the only one who has recently tried to make use of
state->link in mac_config(), so I'm now preparing a set of patches
to split the current mac_config() method into two separate methods:

        void (*mac_config_fixed)(struct net_device *ndev,
                                 phy_interface_t iface, int speed, int duplex,
                                 int pause);
        void (*mac_config_inband)(struct net_device *ndev,
                                  phy_interface_t iface, bool an_enabled,
                                  unsigned long *advertising, int pause);

so that it's not possible to use members that should not be accessed
in various modes.

> Also the PMCR_FORCE_LNK is only set in phylink_link_up() or can I do it here
> and do nothing phylink_link_up()?

When the link comes up/down, mac_link_up() and mac_link_down() will be
called appropriately.  In PHY mode, this is equivalent to phylink doing
this:

	if (link_changed) {
		if (phydev->link)
			mac_link_up();
		else
			mac_link_down();
	}

So the actions you would've done depending on phydev->link should be in
the mac_link_*() methods.

> Other question:
> Where does the MAC enable/disable TX and RX fits best? port_{enable,disable}?
> Or only mac_config() and port_disable?

mac_link_*().

> > > +            if (state->pause || phylink_test(state->advertising, Pause))
> > > +                    mcr |= PMCR_TX_FC_EN | PMCR_RX_FC_EN;
> > > +            if (state->pause & MLO_PAUSE_TX)
> > > +                    mcr |= PMCR_TX_FC_EN;
> > > +            if (state->pause & MLO_PAUSE_RX)
> > > +                    mcr |= PMCR_RX_FC_EN;
> > 
> > This is clearly wrong - if any bit in state->pause is set, then we
> > end up with both PMCR_TX_FC_EN | PMCR_RX_FC_EN set.  If we have Pause
> > Pause set in the advertising mask, then both are set.  This doesn't
> > seem right - are these bits setting the advertisement, or are they
> > telling the MAC to use flow control?
> 
> Last one, tell the MAC to use flow control.

So the first if() statement is incorrect, and should be removed
entirely.  You only want to enable the MAC to use flow control as a
result of the negotiation results.

> On the current driver both bits are set in a forced-link situation.
> 
> If we always forces the MAC mode I think I always set these bits and don't
> anything with the Pause modes? Is that the right way to do it?

So what happens if your link partner (e.g. switch) does not support
flow control?  What if your link partner floods such frames to all
ports?  You end up transmitting flow control frames, which could be
sent to all stations on the network... seems not a good idea.

Implementing stuff properly and not taking short-cuts is always a
good idea for inter-operability.

> > > +
> > > +static void mt7530_phylink_validate(struct dsa_switch *ds, int port,
> > > +                                unsigned long *supported,
> > > +                                struct phylink_link_state *state)
> > > +{
> > > +    __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> > > +
> > > +    switch (port) {
> > > +    case 0: /* Internal phy */
> > > +    case 1:
> > > +    case 2:
> > > +    case 3:
> > > +    case 4:
> > > +            if (state->interface != PHY_INTERFACE_MODE_NA &&
> > > +                state->interface != PHY_INTERFACE_MODE_GMII)
> > > +                    goto unsupported;
> > > +            break;
> > > +    /* case 5: Port 5 not supported! */
> > > +    case 6: /* 1st cpu port */
> > > +            if (state->interface != PHY_INTERFACE_MODE_RGMII &&
> > > +                state->interface != PHY_INTERFACE_MODE_TRGMII)
> > 
> > PHY_INTERFACE_MODE_NA ?
> 
> You mean PHY_INTERFACE_MODE_NA is missing?

Yes.  Please see the updated documentation in the patch I sent this
morning "net: phylink: further documentation clarifications".

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API

[Daniel Santos]

Hello,

Although I'm new to the entire Ethernet / *MII subsystem and I haven't
touched DSA yet, I've recently had to add some of this functionality to
the older OpenWRT drivers for swconfig control over the ports.  René, do
you have an actual datasheet or programming guide for the mt7530?  I
only have one for the mt7620.


On 6/25/19 7:10 AM, Russell King - ARM Linux admin wrote:
> mac_link_*().
>
>>>> +            if (state->pause || phylink_test(state->advertising, Pause))
>>>> +                    mcr |= PMCR_TX_FC_EN | PMCR_RX_FC_EN;
>>>> +            if (state->pause & MLO_PAUSE_TX)
>>>> +                    mcr |= PMCR_TX_FC_EN;
>>>> +            if (state->pause & MLO_PAUSE_RX)
>>>> +                    mcr |= PMCR_RX_FC_EN;
>>> This is clearly wrong - if any bit in state->pause is set, then we
>>> end up with both PMCR_TX_FC_EN | PMCR_RX_FC_EN set.  If we have Pause
>>> Pause set in the advertising mask, then both are set.  This doesn't
>>> seem right - are these bits setting the advertisement, or are they
>>> telling the MAC to use flow control?
>> Last one, tell the MAC to use flow control.
> So the first if() statement is incorrect, and should be removed
> entirely.  You only want to enable the MAC to use flow control as a
> result of the negotiation results.

René,
iiuc, this is what's documented in table 28B-3 of the 802.3 spec on page
598.  pdf of section 2 here:
http://www.ismlab.usf.edu/dcom/Ch3_802.3-2005_section2.pdf

>> On the current driver both bits are set in a forced-link situation.
>>
>> If we always forces the MAC mode I think I always set these bits and don't
>> anything with the Pause modes? Is that the right way to do it?
> So what happens if your link partner (e.g. switch) does not support
> flow control?  What if your link partner floods such frames to all
> ports?  You end up transmitting flow control frames, which could be
> sent to all stations on the network... seems not a good idea.
>
> Implementing stuff properly and not taking short-cuts is always a
> good idea for inter-operability.

But will there still be a mechanism to ignore link partner's advertising
and force these parameters?  I've run into what appears to be quirks on
two separate NICs or their drivers, a tp-link tg-3468 (r8169) and an
Aquantia AQC107 802.3bz (atlantic) where these link partners aren't
auto-negotiating correctly after I switch the mt7530 out of
auto-negotiation mode.  Of course, it could be a mistake I've made (and
should thus be discussed elsewhere), but iirc, I had to force enable
flow control and then also disable auto-negotiation on the link partner
and force the mode I wanted.

Cheers,
Daniel

Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API

[Andrew Lunn]

> But will there still be a mechanism to ignore link partner's advertising
> and force these parameters?

From man 1 ethtool:

       -a --show-pause
              Queries the specified Ethernet device for pause parameter information.

       -A --pause
              Changes the pause parameters of the specified Ethernet device.

           autoneg on|off
                  Specifies whether pause autonegotiation should be enabled.

           rx on|off
                  Specifies whether RX pause should be enabled.

           tx on|off
                  Specifies whether TX pause should be enabled.

You need to check the driver to see if it actually implements this
ethtool call, but that is how it should be configured.

	Andrew

Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API

[Daniel Santos]

On 6/25/19 2:02 PM, Andrew Lunn wrote:
>> But will there still be a mechanism to ignore link partner's advertising
>> and force these parameters?
> >From man 1 ethtool:
>
>        -a --show-pause
>               Queries the specified Ethernet device for pause parameter information.
>
>        -A --pause
>               Changes the pause parameters of the specified Ethernet device.
>
>            autoneg on|off
>                   Specifies whether pause autonegotiation should be enabled.
>
>            rx on|off
>                   Specifies whether RX pause should be enabled.
>
>            tx on|off
>                   Specifies whether TX pause should be enabled.
>
> You need to check the driver to see if it actually implements this
> ethtool call, but that is how it should be configured.
>
> 	Andrew
>
Thank you Andrew,

So in this context, my question is the difference between "enabling" and
"forcing".  Here's that register for the mt7620 (which has an mt7530 on
its die): https://imgur.com/a/pTk0668  I believe this is also what René
is seeking clarity on?

Thanks,
Daniel

Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API

[Andrew Lunn]

On Tue, Jun 25, 2019 at 02:27:55PM -0500, Daniel Santos wrote:
> On 6/25/19 2:02 PM, Andrew Lunn wrote:
> >> But will there still be a mechanism to ignore link partner's advertising
> >> and force these parameters?
> > >From man 1 ethtool:
> >
> >        -a --show-pause
> >               Queries the specified Ethernet device for pause parameter information.
> >
> >        -A --pause
> >               Changes the pause parameters of the specified Ethernet device.
> >
> >            autoneg on|off
> >                   Specifies whether pause autonegotiation should be enabled.
> >
> >            rx on|off
> >                   Specifies whether RX pause should be enabled.
> >
> >            tx on|off
> >                   Specifies whether TX pause should be enabled.
> >
> > You need to check the driver to see if it actually implements this
> > ethtool call, but that is how it should be configured.
> >
> > 	Andrew
> >
> Thank you Andrew,
> 
> So in this context, my question is the difference between "enabling" and
> "forcing".  Here's that register for the mt7620 (which has an mt7530 on
> its die): https://imgur.com/a/pTk0668  I believe this is also what René
> is seeking clarity on?

Lets start with normal operation. If the MAC supports pause or asym
pause, it calls phy_support_sym_pause() or phy_support_asym_pause().
phylib will then configure the PHY to advertise pause as appropriate.
Once auto-neg has completed, the results of the negotiation are set in
phydev. So phdev->pause and phydev->asym_pause. The MAC callback is
then used to tell the MAC about the autoneg results. The MAC should be
programmed using the values in phdev->pause and phydev->asym_pause.

For ethtool, the MAC driver needs to implement .get_pauseparam and
.set_pauseparam. The set_pauseparam needs to validate the settings,
using phy_validate_pause(). If valid, phy_set_asym_pause() is used to
tell the PHY about the new configuration. This will trigger a new
auto-neg if auto-neg is enabled, and the results will be passed back
in the usual way. If auto-neg is disabled, or pause auto-neg is
disabled, the MAC should configure pause directly based on the
settings passed.

Looking at the data sheet page, you want FORCE_MODE_Pn set. You never
want the MAC directly talking to the PHY. Bad things will happen.
Then use FORCE_RX_FC_Pn and FORCE_TX_Pn to reflect phydev->pause and
phydev->asym_pause.

The same idea applies when using phylink.

    Andrew

Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API

[René van Dorst]

Quoting Andrew Lunn <andrew@lunn.ch>:

Hi Andrew,

> On Tue, Jun 25, 2019 at 02:27:55PM -0500, Daniel Santos wrote:
>> On 6/25/19 2:02 PM, Andrew Lunn wrote:
>> >> But will there still be a mechanism to ignore link partner's advertising
>> >> and force these parameters?
>> > >From man 1 ethtool:
>> >
>> >        -a --show-pause
>> >               Queries the specified Ethernet device for pause  
>> parameter information.
>> >
>> >        -A --pause
>> >               Changes the pause parameters of the specified  
>> Ethernet device.
>> >
>> >            autoneg on|off
>> >                   Specifies whether pause autonegotiation should  
>> be enabled.
>> >
>> >            rx on|off
>> >                   Specifies whether RX pause should be enabled.
>> >
>> >            tx on|off
>> >                   Specifies whether TX pause should be enabled.
>> >
>> > You need to check the driver to see if it actually implements this
>> > ethtool call, but that is how it should be configured.
>> >
>> > 	Andrew
>> >
>> Thank you Andrew,
>>
>> So in this context, my question is the difference between "enabling" and
>> "forcing".  Here's that register for the mt7620 (which has an mt7530 on
>> its die): https://imgur.com/a/pTk0668  I believe this is also what René
>> is seeking clarity on?
>
> Lets start with normal operation. If the MAC supports pause or asym
> pause, it calls phy_support_sym_pause() or phy_support_asym_pause().
> phylib will then configure the PHY to advertise pause as appropriate.
> Once auto-neg has completed, the results of the negotiation are set in
> phydev. So phdev->pause and phydev->asym_pause. The MAC callback is
> then used to tell the MAC about the autoneg results. The MAC should be
> programmed using the values in phdev->pause and phydev->asym_pause.
>
> For ethtool, the MAC driver needs to implement .get_pauseparam and
> .set_pauseparam. The set_pauseparam needs to validate the settings,
> using phy_validate_pause(). If valid, phy_set_asym_pause() is used to
> tell the PHY about the new configuration. This will trigger a new
> auto-neg if auto-neg is enabled, and the results will be passed back
> in the usual way. If auto-neg is disabled, or pause auto-neg is
> disabled, the MAC should configure pause directly based on the
> settings passed.
>
> Looking at the data sheet page, you want FORCE_MODE_Pn set. You never
> want the MAC directly talking to the PHY. Bad things will happen.
> Then use FORCE_RX_FC_Pn and FORCE_TX_Pn to reflect phydev->pause and
> phydev->asym_pause.
>
> The same idea applies when using phylink.

Thanks for this information.


I updated mt7530_phylink_mac_config(), I think I done it right this time
with the pause bits.

Now mt7530_phylink_mac_config() looks like this:

static void mt7530_phylink_mac_config(struct dsa_switch *ds, int port,
				      unsigned int mode,
				      const struct phylink_link_state *state)
{
	struct mt7530_priv *priv = ds->priv;
	u32 mcr_cur, mcr_new = 0;

	switch (port) {
	case 0: /* Internal phy */
	case 1:
	case 2:
	case 3:
	case 4:
		if (state->interface != PHY_INTERFACE_MODE_GMII)
			return;
		break;
	case 5: /* 2nd cpu port with phy of port 0 or 4 / external phy */
		if (!phy_interface_mode_is_rgmii(state->interface) &&
		    state->interface != PHY_INTERFACE_MODE_MII)
			return;
		if (priv->p5_intf_sel != P5_INTF_SEL_GMAC5) {
			priv->p5_intf_sel = P5_INTF_SEL_GMAC5;
			mt7530_setup_port5(ds, state->interface);
		}
		/* We are connected to external phy */
		if (dsa_is_user_port(ds, 5))
			mcr_new |= PMCR_EXT_PHY;
		break;
	case 6: /* 1st cpu port */
		if (state->interface != PHY_INTERFACE_MODE_RGMII &&
		    state->interface != PHY_INTERFACE_MODE_TRGMII)
			return;

		if (priv->p6_interface == state->interface)
			break;
		/* Setup TX circuit incluing relevant PAD and driving */
		mt7530_pad_clk_setup(ds, state->interface);

		if (priv->id == ID_MT7530) {
			/* Setup RX circuit, relevant PAD and driving on the
			 * host which must be placed after the setup on the
			 * device side is all finished.
			 */
			mt7623_pad_clk_setup(ds);
		}
		priv->p6_interface = state->interface;
		break;
	default:
		dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
		return;
	}

	mcr_cur = mt7530_read(priv, MT7530_PMCR_P(port));
	mcr_new |= mcr_cur;
	mcr_new &= ~(PMCR_FORCE_SPEED_1000 | PMCR_FORCE_SPEED_100 |
		     PMCR_FORCE_FDX | PMCR_TX_FC_EN | PMCR_RX_FC_EN);
	mcr_new |= PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | PMCR_BACKOFF_EN |
		   PMCR_BACKPR_EN | PMCR_FORCE_MODE | PMCR_FORCE_LNK;

	switch (state->speed) {
	case SPEED_1000:
		mcr_new |= PMCR_FORCE_SPEED_1000;
		break;
	case SPEED_100:
		mcr_new |= PMCR_FORCE_SPEED_100;
		break;
	}
	if (state->duplex == DUPLEX_FULL) {
		mcr_new |= PMCR_FORCE_FDX;
		if (state->pause & MLO_PAUSE_TX)
			mcr_new |= PMCR_TX_FC_EN;
		if (state->pause & MLO_PAUSE_RX)
			mcr_new |= PMCR_RX_FC_EN;
	}

	if (mcr_new != mcr_cur)
		mt7530_write(priv, MT7530_PMCR_P(port), mcr_new);
}

Greats,

René

Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API

[Russell King - ARM Linux admin]

On Tue, Jun 25, 2019 at 10:41:48PM +0200, Andrew Lunn wrote:
> On Tue, Jun 25, 2019 at 02:27:55PM -0500, Daniel Santos wrote:
> > On 6/25/19 2:02 PM, Andrew Lunn wrote:
> > >> But will there still be a mechanism to ignore link partner's advertising
> > >> and force these parameters?
> > > >From man 1 ethtool:
> > >
> > >        -a --show-pause
> > >               Queries the specified Ethernet device for pause parameter information.
> > >
> > >        -A --pause
> > >               Changes the pause parameters of the specified Ethernet device.
> > >
> > >            autoneg on|off
> > >                   Specifies whether pause autonegotiation should be enabled.
> > >
> > >            rx on|off
> > >                   Specifies whether RX pause should be enabled.
> > >
> > >            tx on|off
> > >                   Specifies whether TX pause should be enabled.
> > >
> > > You need to check the driver to see if it actually implements this
> > > ethtool call, but that is how it should be configured.
> > >
> > > 	Andrew
> > >
> > Thank you Andrew,
> > 
> > So in this context, my question is the difference between "enabling" and
> > "forcing".  Here's that register for the mt7620 (which has an mt7530 on
> > its die): https://imgur.com/a/pTk0668  I believe this is also what René
> > is seeking clarity on?
> 
> Lets start with normal operation. If the MAC supports pause or asym
> pause, it calls phy_support_sym_pause() or phy_support_asym_pause().
> phylib will then configure the PHY to advertise pause as appropriate.
> Once auto-neg has completed, the results of the negotiation are set in
> phydev. So phdev->pause and phydev->asym_pause. The MAC callback is
> then used to tell the MAC about the autoneg results. The MAC should be
> programmed using the values in phdev->pause and phydev->asym_pause.
> 
> For ethtool, the MAC driver needs to implement .get_pauseparam and
> .set_pauseparam. The set_pauseparam needs to validate the settings,
> using phy_validate_pause(). If valid, phy_set_asym_pause() is used to
> tell the PHY about the new configuration. This will trigger a new
> auto-neg if auto-neg is enabled, and the results will be passed back
> in the usual way. If auto-neg is disabled, or pause auto-neg is
> disabled, the MAC should configure pause directly based on the
> settings passed.
> 
> Looking at the data sheet page, you want FORCE_MODE_Pn set. You never
> want the MAC directly talking to the PHY. Bad things will happen.
> Then use FORCE_RX_FC_Pn and FORCE_TX_Pn to reflect phydev->pause and
> phydev->asym_pause.
> 
> The same idea applies when using phylink.

Except when using phylink, use pause & MLO_PAUSE_RX to determine whether
FORCE_RX_FC_Pn should be set, and use pause & MLO_PAUSE_TX to determine
whether FORCE_TX_Pn should be set.

phylink will take care of the results of negotiation with the link
partner and tell you what should be set if pause autoneg is enabled.
If the user has decided to force it via ethtool, and the MAC driver
passes the calls on to phylink, phylink will instead set MLO_PAUSE_RX
and MLO_PAUSE_TX according to the users settings.

So, with phylink, it's quite literally "if MLO_PAUSE_RX is set in
mac_config, enable receiption of pause frames.  if MLO_PAUSE_TX is set,
enable transmission of pause frames."

The above applies for phylink's PHY, FIXED, and SGMII in-band modes.
For 802.3z in-band modes, pause is negotiated between the two link
parters (which could be the PCS built into the MACs at either end)
and in some cases its possible to set the MAC to automatically adjust
to the results of the built-in PCS negotiation.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API

[Daniel Santos]

On 6/25/19 3:41 PM, Andrew Lunn wrote:
> On Tue, Jun 25, 2019 at 02:27:55PM -0500, Daniel Santos wrote:
>> On 6/25/19 2:02 PM, Andrew Lunn wrote:
>>>> But will there still be a mechanism to ignore link partner's advertising
>>>> and force these parameters?
>>> >From man 1 ethtool:
>>>
>>>        -a --show-pause
>>>               Queries the specified Ethernet device for pause parameter information.
>>>
>>>        -A --pause
>>>               Changes the pause parameters of the specified Ethernet device.
>>>
>>>            autoneg on|off
>>>                   Specifies whether pause autonegotiation should be enabled.
>>>
>>>            rx on|off
>>>                   Specifies whether RX pause should be enabled.
>>>
>>>            tx on|off
>>>                   Specifies whether TX pause should be enabled.
>>>
>>> You need to check the driver to see if it actually implements this
>>> ethtool call, but that is how it should be configured.
>>>
>>> 	Andrew
>>>
>> Thank you Andrew,
>>
>> So in this context, my question is the difference between "enabling" and
>> "forcing".  Here's that register for the mt7620 (which has an mt7530 on
>> its die): https://imgur.com/a/pTk0668  I believe this is also what René
>> is seeking clarity on?
> Lets start with normal operation. If the MAC supports pause or asym
> pause, it calls phy_support_sym_pause() or phy_support_asym_pause().
> phylib will then configure the PHY to advertise pause as appropriate.
> Once auto-neg has completed, the results of the negotiation are set in
> phydev. So phdev->pause and phydev->asym_pause. The MAC callback is
> then used to tell the MAC about the autoneg results. The MAC should be
> programmed using the values in phdev->pause and phydev->asym_pause.
>
> For ethtool, the MAC driver needs to implement .get_pauseparam and
> .set_pauseparam. The set_pauseparam needs to validate the settings,
> using phy_validate_pause(). If valid, phy_set_asym_pause() is used to
> tell the PHY about the new configuration. This will trigger a new
> auto-neg if auto-neg is enabled, and the results will be passed back
> in the usual way. If auto-neg is disabled, or pause auto-neg is
> disabled, the MAC should configure pause directly based on the
> settings passed.
>
> Looking at the data sheet page, you want FORCE_MODE_Pn set. You never
> want the MAC directly talking to the PHY. Bad things will happen.

So what exactly do you mean by the MAC directly talking to the PHY?  Do
you mean setting speed, duplex, etc. via the MAC registers instead of
via MDIO to the MII registers of the PHY?

> Then use FORCE_RX_FC_Pn and FORCE_TX_Pn to reflect phydev->pause and
> phydev->asym_pause.
>
> The same idea applies when using phylink.
>
>     Andrew

You're help is greatly appreciated here.  Admittedly, I'm also trying to
get this working in the now deprecated swconfig for a 3.18 kernel that's
in production.  In my code, I had just set the appropriate bits in both
the MAC and mii registers -- did I just shoot myself in the foot or only
toe or two? :)  I should probably start a separate thread for this. 
(And probably attempt to wrestle an mt7530 programmer's guide out of
MediaTek!)

Thanks,
Daniel

PS: I found a rather humorous quote from the mt7621 datasheet regarding
the MAC registers (at 0x3000 for port 0, 0x3100 for port 1, etc.):

    2.4 Link Status

    You can find MAC control register put at 0x3500 for MAC 5, and
    0x3600 for MAC 6. You can change
    MAC ability at this register. We would suggest don’t use the
    register 0x3000 to 0x3400. It may not
    work.

I'm not sure if this only applies to something in between the mt7621 and
it's internal mt7530 or not.

Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API

[Andrew Lunn]

> > Looking at the data sheet page, you want FORCE_MODE_Pn set. You never
> > want the MAC directly talking to the PHY. Bad things will happen.
> 
> So what exactly do you mean by the MAC directly talking to the PHY?  Do
> you mean setting speed, duplex, etc. via the MAC registers instead of
> via MDIO to the MII registers of the PHY?

The data sheet implies the MAC hardware performs reads on the PHY to
get the status, and then uses that to configure the MAC. This is often
called PHY polling. The MAC hardware however has no idea what the PHY
driver is doing. The MAC does not take the PHY mutex. So the PHY
driver might be doing something at the same time the MAC hardware
polls the PHY, and it gets odd results. Some PHYs have multiple pages,
and for example reading the temperature sensor means swapping
pages. If the MAC hardware was to poll while the sensor is being read,
it would not get the link status, it would read some random
temperature register.

So you want the PHY driver to read the results of auto-neg and it then
tell the MAC the results, so the MAC can be configured correctly.

> > Then use FORCE_RX_FC_Pn and FORCE_TX_Pn to reflect phydev->pause and
> > phydev->asym_pause.
> >
> > The same idea applies when using phylink.
> >
> >     Andrew
> 
> You're help is greatly appreciated here.  Admittedly, I'm also trying to
> get this working in the now deprecated swconfig for a 3.18 kernel that's
> in production.

I'm not very familiar with swconfig. Is there software driving the
PHY? If not, it is then safe for the MAC hardware to poll the PHY.

     Andrew

Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API

[Daniel Santos]

Hello Andrew,

On 6/27/19 2:28 PM, Andrew Lunn wrote:
>>> Looking at the data sheet page, you want FORCE_MODE_Pn set. You never
>>> want the MAC directly talking to the PHY. Bad things will happen.
>> So what exactly do you mean by the MAC directly talking to the PHY?  Do
>> you mean setting speed, duplex, etc. via the MAC registers instead of
>> via MDIO to the MII registers of the PHY?
> The data sheet implies the MAC hardware performs reads on the PHY to
> get the status, and then uses that to configure the MAC. This is often
> called PHY polling. The MAC hardware however has no idea what the PHY
> driver is doing. The MAC does not take the PHY mutex. So the PHY
> driver might be doing something at the same time the MAC hardware
> polls the PHY, and it gets odd results. Some PHYs have multiple pages,
> and for example reading the temperature sensor means swapping
> pages. If the MAC hardware was to poll while the sensor is being read,
> it would not get the link status, it would read some random
> temperature register.
>
> So you want the PHY driver to read the results of auto-neg and it then
> tell the MAC the results, so the MAC can be configured correctly.

Thank you, this is very helpful!  I finally understand why these
settings are in two different places. :)  Unfortunately this driver (in
OpenWRT) does a lot of "magic" during init to registers that I don't
have documentation for, but I see where auto-polling can be disabled now.

>>> Then use FORCE_RX_FC_Pn and FORCE_TX_Pn to reflect phydev->pause and
>>> phydev->asym_pause.
>>>
>>> The same idea applies when using phylink.
>>>
>>>     Andrew
>> You're help is greatly appreciated here.  Admittedly, I'm also trying to
>> get this working in the now deprecated swconfig for a 3.18 kernel that's
>> in production.
> I'm not very familiar with swconfig. Is there software driving the
> PHY? If not, it is then safe for the MAC hardware to poll the PHY.
>
>      Andrew

swconfig is an netlink-based interface the OpenWRT team developed for
configuring switches before DSA was converted into a vendor-neutral
interface.  Now that DSA does what swconfig was designed for it has been
deprecated, although (to my knowledge) we don't yet have DSA for all
devices that OpenWRT supports.

Daniel

Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API

[Russell King - ARM Linux admin]

On Tue, Jun 25, 2019 at 09:02:46PM +0200, Andrew Lunn wrote:
> > But will there still be a mechanism to ignore link partner's advertising
> > and force these parameters?
> 
> From man 1 ethtool:
> 
>        -a --show-pause
>               Queries the specified Ethernet device for pause parameter information.
> 
>        -A --pause
>               Changes the pause parameters of the specified Ethernet device.
> 
>            autoneg on|off
>                   Specifies whether pause autonegotiation should be enabled.
> 
>            rx on|off
>                   Specifies whether RX pause should be enabled.
> 
>            tx on|off
>                   Specifies whether TX pause should be enabled.
> 
> You need to check the driver to see if it actually implements this
> ethtool call, but that is how it should be configured.

Note that phylink provides this call, and provided mac_config() is
correctly implemented, will result in the pause mode parameters in
the MAC being correctly set.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API

[Vladimir Oltean]

Hi Russell,

On 6/24/19 6:39 PM, Russell King - ARM Linux admin wrote:
> Hi,
> 
> On Mon, Jun 24, 2019 at 04:52:47PM +0200, René van Dorst wrote:
>> Convert mt7530 to PHYLINK API
>>
>> Signed-off-by: René van Dorst <opensource@vdorst.com>
>> ---
>>   drivers/net/dsa/mt7530.c | 237 +++++++++++++++++++++++++++++----------
>>   drivers/net/dsa/mt7530.h |   9 ++
>>   2 files changed, 187 insertions(+), 59 deletions(-)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index 3181e95586d6..9c5e4dd00826 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -13,7 +13,7 @@
>>   #include <linux/of_mdio.h>
>>   #include <linux/of_net.h>
>>   #include <linux/of_platform.h>
>> -#include <linux/phy.h>
>> +#include <linux/phylink.h>
>>   #include <linux/regmap.h>
>>   #include <linux/regulator/consumer.h>
>>   #include <linux/reset.h>
>> @@ -633,63 +633,6 @@ mt7530_get_sset_count(struct dsa_switch *ds, int port, int sset)
>>   	return ARRAY_SIZE(mt7530_mib);
>>   }
>>   
>> -static void mt7530_adjust_link(struct dsa_switch *ds, int port,
>> -			       struct phy_device *phydev)
>> -{
>> -	struct mt7530_priv *priv = ds->priv;
>> -
>> -	if (phy_is_pseudo_fixed_link(phydev)) {
>> -		dev_dbg(priv->dev, "phy-mode for master device = %x\n",
>> -			phydev->interface);
>> -
>> -		/* Setup TX circuit incluing relevant PAD and driving */
>> -		mt7530_pad_clk_setup(ds, phydev->interface);
>> -
>> -		if (priv->id == ID_MT7530) {
>> -			/* Setup RX circuit, relevant PAD and driving on the
>> -			 * host which must be placed after the setup on the
>> -			 * device side is all finished.
>> -			 */
>> -			mt7623_pad_clk_setup(ds);
>> -		}
>> -	} else {
>> -		u16 lcl_adv = 0, rmt_adv = 0;
>> -		u8 flowctrl;
>> -		u32 mcr = PMCR_USERP_LINK | PMCR_FORCE_MODE;
>> -
>> -		switch (phydev->speed) {
>> -		case SPEED_1000:
>> -			mcr |= PMCR_FORCE_SPEED_1000;
>> -			break;
>> -		case SPEED_100:
>> -			mcr |= PMCR_FORCE_SPEED_100;
>> -			break;
>> -		}
>> -
>> -		if (phydev->link)
>> -			mcr |= PMCR_FORCE_LNK;
>> -
>> -		if (phydev->duplex) {
>> -			mcr |= PMCR_FORCE_FDX;
>> -
>> -			if (phydev->pause)
>> -				rmt_adv = LPA_PAUSE_CAP;
>> -			if (phydev->asym_pause)
>> -				rmt_adv |= LPA_PAUSE_ASYM;
>> -
>> -			lcl_adv = linkmode_adv_to_lcl_adv_t(
>> -				phydev->advertising);
>> -			flowctrl = mii_resolve_flowctrl_fdx(lcl_adv, rmt_adv);
>> -
>> -			if (flowctrl & FLOW_CTRL_TX)
>> -				mcr |= PMCR_TX_FC_EN;
>> -			if (flowctrl & FLOW_CTRL_RX)
>> -				mcr |= PMCR_RX_FC_EN;
>> -		}
>> -		mt7530_write(priv, MT7530_PMCR_P(port), mcr);
>> -	}
>> -}
>> -
>>   static int
>>   mt7530_cpu_port_enable(struct mt7530_priv *priv,
>>   		       int port)
>> @@ -1323,6 +1266,178 @@ mt7530_setup(struct dsa_switch *ds)
>>   	return 0;
>>   }
>>   
>> +static void mt7530_phylink_mac_config(struct dsa_switch *ds, int port,
>> +				      unsigned int mode,
>> +				      const struct phylink_link_state *state)
>> +{
>> +	struct mt7530_priv *priv = ds->priv;
>> +	u32 mcr = PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | PMCR_BACKOFF_EN |
>> +		  PMCR_BACKPR_EN | PMCR_TX_EN | PMCR_RX_EN;
>> +
>> +	switch (port) {
>> +	case 0: /* Internal phy */
>> +	case 1:
>> +	case 2:
>> +	case 3:
>> +	case 4:
>> +		if (state->interface != PHY_INTERFACE_MODE_GMII)
>> +			goto unsupported;
>> +		break;
>> +	/* case 5: Port 5 is not supported! */
>> +	case 6: /* 1st cpu port */
>> +		if (state->interface != PHY_INTERFACE_MODE_RGMII &&
>> +		    state->interface != PHY_INTERFACE_MODE_TRGMII)
>> +			goto unsupported;
>> +
>> +		/* Setup TX circuit incluing relevant PAD and driving */
>> +		mt7530_pad_clk_setup(ds, state->interface);
>> +
>> +		if (priv->id == ID_MT7530) {
>> +			/* Setup RX circuit, relevant PAD and driving on the
>> +			 * host which must be placed after the setup on the
>> +			 * device side is all finished.
>> +			 */
>> +			mt7623_pad_clk_setup(ds);
>> +		}
>> +		break;
>> +	default:
>> +		dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
>> +		return;
>> +	}
>> +
>> +	if (!state->an_enabled || mode == MLO_AN_FIXED) {
>> +		mcr |= PMCR_FORCE_MODE;
>> +
>> +		if (state->speed == SPEED_1000)
>> +			mcr |= PMCR_FORCE_SPEED_1000;
>> +		if (state->speed == SPEED_100)
>> +			mcr |= PMCR_FORCE_SPEED_100;
>> +		if (state->duplex == DUPLEX_FULL)
>> +			mcr |= PMCR_FORCE_FDX;
>> +		if (state->link || mode == MLO_AN_FIXED)
>> +			mcr |= PMCR_FORCE_LNK;
> 
> This should be removed - state->link is not for use in mac_config.
> Even in fixed mode, the link can be brought up/down by means of a
> gpio, and this should be dealt with via the mac_link_* functions.
> 

What do you mean exactly that state->link is not for use, is that true 
in general?
In drivers/net/dsa/sja1105/sja1105_main.c, if I remove the "if 
(!state->link)" guard, I see PHYLINK calls with a SPEED_UNKNOWN argument 
for ports that are BR_STATE_DISABLED. Is that normal?

>> +		if (state->pause || phylink_test(state->advertising, Pause))
>> +			mcr |= PMCR_TX_FC_EN | PMCR_RX_FC_EN;
>> +		if (state->pause & MLO_PAUSE_TX)
>> +			mcr |= PMCR_TX_FC_EN;
>> +		if (state->pause & MLO_PAUSE_RX)
>> +			mcr |= PMCR_RX_FC_EN;
> 
> This is clearly wrong - if any bit in state->pause is set, then we
> end up with both PMCR_TX_FC_EN | PMCR_RX_FC_EN set.  If we have Pause
> Pause set in the advertising mask, then both are set.  This doesn't
> seem right - are these bits setting the advertisement, or are they
> telling the MAC to use flow control?
> 
>> +	}
>> +
>> +	mt7530_write(priv, MT7530_PMCR_P(port), mcr);
>> +
>> +	return;
>> +
>> +unsupported:
>> +	dev_err(ds->dev, "%s: P%d: Unsupported phy_interface mode: %d (%s)\n",
>> +		__func__, port, state->interface, phy_modes(state->interface));
>> +}
>> +
>> +static void mt7530_phylink_mac_link_down(struct dsa_switch *ds, int port,
>> +					 unsigned int mode,
>> +					 phy_interface_t interface)
>> +{
>> +	/* Do nothing */
>> +}
>> +
>> +static void mt7530_phylink_mac_link_up(struct dsa_switch *ds, int port,
>> +				       unsigned int mode,
>> +				       phy_interface_t interface,
>> +				       struct phy_device *phydev)
>> +{
>> +	/* Do nothing */
>> +}
> 
> These two are where you should be forcing the link up or down if
> required (basically, inband modes should let the link come up/down
> irrespective of these functions, otherwise it should be forced.)
> 
>> +
>> +static void mt7530_phylink_validate(struct dsa_switch *ds, int port,
>> +				    unsigned long *supported,
>> +				    struct phylink_link_state *state)
>> +{
>> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
>> +
>> +	switch (port) {
>> +	case 0: /* Internal phy */
>> +	case 1:
>> +	case 2:
>> +	case 3:
>> +	case 4:
>> +		if (state->interface != PHY_INTERFACE_MODE_NA &&
>> +		    state->interface != PHY_INTERFACE_MODE_GMII)
>> +			goto unsupported;
>> +		break;
>> +	/* case 5: Port 5 not supported! */
>> +	case 6: /* 1st cpu port */
>> +		if (state->interface != PHY_INTERFACE_MODE_RGMII &&
>> +		    state->interface != PHY_INTERFACE_MODE_TRGMII)
> 
> PHY_INTERFACE_MODE_NA ?
> 
>> +			goto unsupported;
>> +		break;
>> +	default:
>> +		linkmode_zero(supported);
>> +		dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
>> +		return;
>> +	}
>> +
>> +	phylink_set(mask, Autoneg);
>> +	phylink_set(mask, Pause);
>> +	phylink_set(mask, Asym_Pause);
>> +	phylink_set(mask, MII);
>> +
>> +	phylink_set(mask, 10baseT_Half);
>> +	phylink_set(mask, 10baseT_Full);
>> +	phylink_set(mask, 100baseT_Half);
>> +	phylink_set(mask, 100baseT_Full);
>> +	phylink_set(mask, 1000baseT_Full);
>> +	phylink_set(mask, 1000baseT_Half);
> 
> You seem to be missing phylink_set_port_modes() here.
> 
>> +
>> +	linkmode_and(supported, supported, mask);
>> +	linkmode_and(state->advertising, state->advertising, mask);
>> +	return;
>> +
>> +unsupported:
>> +	linkmode_zero(supported);
>> +	dev_err(ds->dev, "%s: unsupported interface mode: [0x%x] %s\n",
>> +		__func__, state->interface, phy_modes(state->interface));
> 
> Not a good idea to print this at error level; sometimes we just probe
> for support.
> 
> Eg, think about a SFP cage, and a SFP is plugged in that uses a PHY
> interface mode that the MAC can't support - we detect that by the
> validation failing, and printing a more meaningful message in phylink
> itself.
> 
>> +}
>> +
>> +static int
>> +mt7530_phylink_mac_link_state(struct dsa_switch *ds, int port,
>> +			      struct phylink_link_state *state)
>> +{
>> +	struct mt7530_priv *priv = ds->priv;
>> +	u32 pmsr;
>> +
>> +	if (port < 0 || port >= MT7530_NUM_PORTS)
>> +		return -EINVAL;
>> +
>> +	pmsr = mt7530_read(priv, MT7530_PMSR_P(port));
>> +
>> +	state->link = (pmsr & PMSR_LINK);
>> +	state->an_complete = state->link;
>> +	state->duplex = (pmsr & PMSR_DPX) >> 1;
>> +
>> +	switch (pmsr & (PMSR_SPEED_1000 | PMSR_SPEED_100)) {
>> +	case 0:
>> +		state->speed = SPEED_10;
>> +		break;
>> +	case PMSR_SPEED_100:
>> +		state->speed = SPEED_100;
>> +		break;
>> +	case PMSR_SPEED_1000:
>> +		state->speed = SPEED_1000;
>> +		break;
>> +	default:
>> +		state->speed = SPEED_UNKNOWN;
>> +		break;
>> +	}
>> +
>> +	state->pause = 0;
>> +	if (pmsr & PMSR_RX_FC)
>> +		state->pause |= MLO_PAUSE_RX;
>> +	if (pmsr & PMSR_TX_FC)
>> +		state->pause |= MLO_PAUSE_TX;
>> +
>> +	return 1;
>> +}
>> +
>>   static const struct dsa_switch_ops mt7530_switch_ops = {
>>   	.get_tag_protocol	= mtk_get_tag_protocol,
>>   	.setup			= mt7530_setup,
>> @@ -1331,7 +1446,6 @@ static const struct dsa_switch_ops mt7530_switch_ops = {
>>   	.phy_write		= mt7530_phy_write,
>>   	.get_ethtool_stats	= mt7530_get_ethtool_stats,
>>   	.get_sset_count		= mt7530_get_sset_count,
>> -	.adjust_link		= mt7530_adjust_link,
>>   	.port_enable		= mt7530_port_enable,
>>   	.port_disable		= mt7530_port_disable,
>>   	.port_stp_state_set	= mt7530_stp_state_set,
>> @@ -1344,6 +1458,11 @@ static const struct dsa_switch_ops mt7530_switch_ops = {
>>   	.port_vlan_prepare	= mt7530_port_vlan_prepare,
>>   	.port_vlan_add		= mt7530_port_vlan_add,
>>   	.port_vlan_del		= mt7530_port_vlan_del,
>> +	.phylink_validate	= mt7530_phylink_validate,
>> +	.phylink_mac_link_state = mt7530_phylink_mac_link_state,
>> +	.phylink_mac_config	= mt7530_phylink_mac_config,
>> +	.phylink_mac_link_down	= mt7530_phylink_mac_link_down,
>> +	.phylink_mac_link_up	= mt7530_phylink_mac_link_up,
>>   };
>>   
>>   static const struct of_device_id mt7530_of_match[] = {
>> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
>> index bfac90f48102..41d9a132ac70 100644
>> --- a/drivers/net/dsa/mt7530.h
>> +++ b/drivers/net/dsa/mt7530.h
>> @@ -198,6 +198,7 @@ enum mt7530_vlan_port_attr {
>>   #define  PMCR_FORCE_SPEED_100		BIT(2)
>>   #define  PMCR_FORCE_FDX			BIT(1)
>>   #define  PMCR_FORCE_LNK			BIT(0)
>> +#define  PMCR_FORCE_LNK_DOWN		PMCR_FORCE_MODE
>>   #define  PMCR_COMMON_LINK		(PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | \
>>   					 PMCR_BACKOFF_EN | PMCR_BACKPR_EN | \
>>   					 PMCR_TX_EN | PMCR_RX_EN | \
>> @@ -218,6 +219,14 @@ enum mt7530_vlan_port_attr {
>>   					 PMCR_TX_FC_EN | PMCR_RX_FC_EN)
>>   
>>   #define MT7530_PMSR_P(x)		(0x3008 + (x) * 0x100)
>> +#define  PMSR_EEE1G			BIT(7)
>> +#define  PMSR_EEE100M			BIT(6)
>> +#define  PMSR_RX_FC			BIT(5)
>> +#define  PMSR_TX_FC			BIT(4)
>> +#define  PMSR_SPEED_1000		BIT(3)
>> +#define  PMSR_SPEED_100			BIT(2)
>> +#define  PMSR_DPX			BIT(1)
>> +#define  PMSR_LINK			BIT(0)
>>   
>>   /* Register for MIB */
>>   #define MT7530_PORT_MIB_COUNTER(x)	(0x4000 + (x) * 0x100)
>> -- 
>> 2.20.1
>>
>>
> 

Regards,
-Vladimir

Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API

[Russell King - ARM Linux admin]

On Tue, Jun 25, 2019 at 11:24:01PM +0300, Vladimir Oltean wrote:
> Hi Russell,
> 
> On 6/24/19 6:39 PM, Russell King - ARM Linux admin wrote:
> > This should be removed - state->link is not for use in mac_config.
> > Even in fixed mode, the link can be brought up/down by means of a
> > gpio, and this should be dealt with via the mac_link_* functions.
> > 
> 
> What do you mean exactly that state->link is not for use, is that true in
> general?

Yes.  mac_config() should not touch it; it is not always in a defined
state.  For example, if you set modes via ethtool (the
ethtool_ksettings_set API) then state->link will probably contain
zero irrespective of the true link state.

It exists in this structure because it was convenient to just use one
structure to store all the link information in various parts of the
code, and when requesting the negotiated in-band MAC configuration.

I've come to the conclusion that that decision was a mistake, based
on patches such as the above mistakenly thinking that everything in
the state structure is fair game.  I've since updated the docs to
explicitly spell it out, but I'm also looking at the feasibility of
changing the mac_config() interface entirely - splitting it into two
(mac_config_fixed() and mac_config_inband()) and passing only the
appropriate parameters to each.

However, having looked at that, I think such a change will make some
MAC drivers quite a bit more complicated - having all the config
steps in one method appears to make the configuration of MAC drivers
easier (eg, mvneta, mvpp2.)

> In drivers/net/dsa/sja1105/sja1105_main.c, if I remove the "if
> (!state->link)" guard, I see PHYLINK calls with a SPEED_UNKNOWN argument for
> ports that are BR_STATE_DISABLED. Is that normal?

This looks like another driver which has been converted to phylink
without my review; I certainly wasn't aware of it.  It gets a few
things wrong, such as:

1) not checking state->interface in the validate callback - so it
   is basically saying that it can support any PHY interface mode
   that the kernel happens to support.

2) if phylink is configured to use in-band, then state->speed is
   undefined; this driver will fail.  (If folk don't want to support
   that, we ought to have a way to tell phylink to reject anything
   that attempts to set it to in-band mode!)

3) it doesn't implement phylink_mac_link_state DSA ops, so it doesn't
   support SGMII or 802.3z phy interface modes (see 1).

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API

[Vladimir Oltean]

On Wed, 26 Jun 2019 at 00:53, Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Tue, Jun 25, 2019 at 11:24:01PM +0300, Vladimir Oltean wrote:
> > Hi Russell,
> >
> > On 6/24/19 6:39 PM, Russell King - ARM Linux admin wrote:
> > > This should be removed - state->link is not for use in mac_config.
> > > Even in fixed mode, the link can be brought up/down by means of a
> > > gpio, and this should be dealt with via the mac_link_* functions.
> > >
> >
> > What do you mean exactly that state->link is not for use, is that true in
> > general?
>
> Yes.  mac_config() should not touch it; it is not always in a defined
> state.  For example, if you set modes via ethtool (the
> ethtool_ksettings_set API) then state->link will probably contain
> zero irrespective of the true link state.
>

Experimentally, state->link is zero at the same time as state->speed
is -1, so just ignoring !state->link made sense. This is not in-band
AN. What is your suggestion? Should I proceed to try and configure the
MAC for SPEED_UNKNOWN?

> It exists in this structure because it was convenient to just use one
> structure to store all the link information in various parts of the
> code, and when requesting the negotiated in-band MAC configuration.
>
> I've come to the conclusion that that decision was a mistake, based
> on patches such as the above mistakenly thinking that everything in
> the state structure is fair game.  I've since updated the docs to
> explicitly spell it out, but I'm also looking at the feasibility of
> changing the mac_config() interface entirely - splitting it into two
> (mac_config_fixed() and mac_config_inband()) and passing only the
> appropriate parameters to each.
>
> However, having looked at that, I think such a change will make some
> MAC drivers quite a bit more complicated - having all the config
> steps in one method appears to make the configuration of MAC drivers
> easier (eg, mvneta, mvpp2.)
>
> > In drivers/net/dsa/sja1105/sja1105_main.c, if I remove the "if
> > (!state->link)" guard, I see PHYLINK calls with a SPEED_UNKNOWN argument for
> > ports that are BR_STATE_DISABLED. Is that normal?
>
> This looks like another driver which has been converted to phylink
> without my review; I certainly wasn't aware of it.  It gets a few
> things wrong, such as:
>
> 1) not checking state->interface in the validate callback - so it
>    is basically saying that it can support any PHY interface mode
>    that the kernel happens to support.
>

Partially true. It does check the DT bindings for supported MII modes
in sja1105_init_mii_settings (for fundamental reasons... the switch
expects an 'all-in-one' configuration buffer with the operating modes
of all MACs - don't ask me to delay the uploading of this static
config until all ports collected their interface_mode from phylink via
the mac_config callback - it's a deadlock).
It is a gigabit switch with MII/RMII/RGMII MACs - I have never seen
any PHY wired for these modes that can change system interface type.

> 2) if phylink is configured to use in-band, then state->speed is
>    undefined; this driver will fail.  (If folk don't want to support
>    that, we ought to have a way to tell phylink to reject anything
>    that attempts to set it to in-band mode!)
>

Ok.

> 3) it doesn't implement phylink_mac_link_state DSA ops, so it doesn't
>    support SGMII or 802.3z phy interface modes (see 1).
>

No, it doesn't.
Some odd switch in this device family supports SGMII on 1 of its
ports, however I haven't put my hands on it.
When I do I'll add checks for strange scenarios, like connecting it to
an Aquantia PHY that can switch between SGMII and USXGMII (although
why would anyone pair a 10G capable PHY to a 1G capable MAC...)

> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up

Thanks,
-Vladimir

Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API

[Russell King - ARM Linux admin]

On Wed, Jun 26, 2019 at 01:14:59AM +0300, Vladimir Oltean wrote:
> On Wed, 26 Jun 2019 at 00:53, Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> >
> > On Tue, Jun 25, 2019 at 11:24:01PM +0300, Vladimir Oltean wrote:
> > > Hi Russell,
> > >
> > > On 6/24/19 6:39 PM, Russell King - ARM Linux admin wrote:
> > > > This should be removed - state->link is not for use in mac_config.
> > > > Even in fixed mode, the link can be brought up/down by means of a
> > > > gpio, and this should be dealt with via the mac_link_* functions.
> > > >
> > >
> > > What do you mean exactly that state->link is not for use, is that true in
> > > general?
> >
> > Yes.  mac_config() should not touch it; it is not always in a defined
> > state.  For example, if you set modes via ethtool (the
> > ethtool_ksettings_set API) then state->link will probably contain
> > zero irrespective of the true link state.
> >
> 
> Experimentally, state->link is zero at the same time as state->speed
> is -1, so just ignoring !state->link made sense. This is not in-band
> AN. What is your suggestion? Should I proceed to try and configure the
> MAC for SPEED_UNKNOWN?

What would you have done with a PHY when the link is down, what speed
would you have configured in the phylib adjust_link callback?  phylib
also sets SPEED_UNKNOWN/DUPLEX_UNKNOWN when the link is down.

What we do in Marvell drivers is set to the lowest speed (10M) in such
cases, which is fine as the MAC supports 10M.

It wouldn't be appropriate for phylink to force something on MAC
drivers, it's easier if the MAC just defaults SPEED_UNKNOWN to something
itself.

> 
> > It exists in this structure because it was convenient to just use one
> > structure to store all the link information in various parts of the
> > code, and when requesting the negotiated in-band MAC configuration.
> >
> > I've come to the conclusion that that decision was a mistake, based
> > on patches such as the above mistakenly thinking that everything in
> > the state structure is fair game.  I've since updated the docs to
> > explicitly spell it out, but I'm also looking at the feasibility of
> > changing the mac_config() interface entirely - splitting it into two
> > (mac_config_fixed() and mac_config_inband()) and passing only the
> > appropriate parameters to each.
> >
> > However, having looked at that, I think such a change will make some
> > MAC drivers quite a bit more complicated - having all the config
> > steps in one method appears to make the configuration of MAC drivers
> > easier (eg, mvneta, mvpp2.)
> >
> > > In drivers/net/dsa/sja1105/sja1105_main.c, if I remove the "if
> > > (!state->link)" guard, I see PHYLINK calls with a SPEED_UNKNOWN argument for
> > > ports that are BR_STATE_DISABLED. Is that normal?
> >
> > This looks like another driver which has been converted to phylink
> > without my review; I certainly wasn't aware of it.  It gets a few
> > things wrong, such as:
> >
> > 1) not checking state->interface in the validate callback - so it
> >    is basically saying that it can support any PHY interface mode
> >    that the kernel happens to support.
> >
> 
> Partially true. It does check the DT bindings for supported MII modes
> in sja1105_init_mii_settings (for fundamental reasons... the switch
> expects an 'all-in-one' configuration buffer with the operating modes
> of all MACs - don't ask me to delay the uploading of this static
> config until all ports collected their interface_mode from phylink via
> the mac_config callback - it's a deadlock).

Ok, so you need to reject interface modes that are not compatible
with the currently configured mode in the validate() callback, but
please keep PHY_INTERFACE_MODE_NA reporting back the capabilities.
(this is now documented.)

> It is a gigabit switch with MII/RMII/RGMII MACs - I have never seen
> any PHY wired for these modes that can change system interface type.

It is unlikely that MII/RMII/RGMII will switch modes, but in terms of
correct implementation, sticking to the way the function is expected
to behave means that I don't get surprises when changing phylink layer
in the future.

> 
> > 2) if phylink is configured to use in-band, then state->speed is
> >    undefined; this driver will fail.  (If folk don't want to support
> >    that, we ought to have a way to tell phylink to reject anything
> >    that attempts to set it to in-band mode!)
> >
> 
> Ok.
> 
> > 3) it doesn't implement phylink_mac_link_state DSA ops, so it doesn't
> >    support SGMII or 802.3z phy interface modes (see 1).
> >
> 
> No, it doesn't.
> Some odd switch in this device family supports SGMII on 1 of its
> ports, however I haven't put my hands on it.
> When I do I'll add checks for strange scenarios, like connecting it to
> an Aquantia PHY that can switch between SGMII and USXGMII (although
> why would anyone pair a 10G capable PHY to a 1G capable MAC...)

It's unlikely that it would switch between SGMII and USXGMII
dynamically, as USXGMII supports speeds from 10G down to 10M.

Where interface mode switching tends to be used is with modes such
as 10GBASE-R, which doesn't support anything except 10G.  In order
for the PHY to operate at slower speeds, it has a few options:

1) perform rate adaption.
2) dynamically switch interface type to an interface type that
   supports the desired speed.
3) just not support slower speeds.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API

[Vladimir Oltean]

On Wed, 26 Jun 2019 at 01:58, Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Wed, Jun 26, 2019 at 01:14:59AM +0300, Vladimir Oltean wrote:
> > On Wed, 26 Jun 2019 at 00:53, Russell King - ARM Linux admin
> > <linux@armlinux.org.uk> wrote:
> > >
> > > On Tue, Jun 25, 2019 at 11:24:01PM +0300, Vladimir Oltean wrote:
> > > > Hi Russell,
> > > >
> > > > On 6/24/19 6:39 PM, Russell King - ARM Linux admin wrote:
> > > > > This should be removed - state->link is not for use in mac_config.
> > > > > Even in fixed mode, the link can be brought up/down by means of a
> > > > > gpio, and this should be dealt with via the mac_link_* functions.
> > > > >
> > > >
> > > > What do you mean exactly that state->link is not for use, is that true in
> > > > general?
> > >
> > > Yes.  mac_config() should not touch it; it is not always in a defined
> > > state.  For example, if you set modes via ethtool (the
> > > ethtool_ksettings_set API) then state->link will probably contain
> > > zero irrespective of the true link state.
> > >
> >
> > Experimentally, state->link is zero at the same time as state->speed
> > is -1, so just ignoring !state->link made sense. This is not in-band
> > AN. What is your suggestion? Should I proceed to try and configure the
> > MAC for SPEED_UNKNOWN?
>
> What would you have done with a PHY when the link is down, what speed
> would you have configured in the phylib adjust_link callback?  phylib
> also sets SPEED_UNKNOWN/DUPLEX_UNKNOWN when the link is down.
>

With phylib, I'd make the driver ignore the speed and do nothing.
With phylink, I'd make the core not call mac_config.
But what happened is I saw phylink call mac_config anyway, said
'weird' and proceeded to ignore it as I would have for phylib.
I'm just not understanding your position - it seems like you're
implying there's a bug in phylink and the function call with
MLO_AN_FIXED, state->link=0 and state->speed=-1 should not have taken
place, which is what I wanted to confirm.

> What we do in Marvell drivers is set to the lowest speed (10M) in such
> cases, which is fine as the MAC supports 10M.
>
> It wouldn't be appropriate for phylink to force something on MAC
> drivers, it's easier if the MAC just defaults SPEED_UNKNOWN to something
> itself.
>
> >
> > > It exists in this structure because it was convenient to just use one
> > > structure to store all the link information in various parts of the
> > > code, and when requesting the negotiated in-band MAC configuration.
> > >
> > > I've come to the conclusion that that decision was a mistake, based
> > > on patches such as the above mistakenly thinking that everything in
> > > the state structure is fair game.  I've since updated the docs to
> > > explicitly spell it out, but I'm also looking at the feasibility of
> > > changing the mac_config() interface entirely - splitting it into two
> > > (mac_config_fixed() and mac_config_inband()) and passing only the
> > > appropriate parameters to each.
> > >
> > > However, having looked at that, I think such a change will make some
> > > MAC drivers quite a bit more complicated - having all the config
> > > steps in one method appears to make the configuration of MAC drivers
> > > easier (eg, mvneta, mvpp2.)
> > >
> > > > In drivers/net/dsa/sja1105/sja1105_main.c, if I remove the "if
> > > > (!state->link)" guard, I see PHYLINK calls with a SPEED_UNKNOWN argument for
> > > > ports that are BR_STATE_DISABLED. Is that normal?
> > >
> > > This looks like another driver which has been converted to phylink
> > > without my review; I certainly wasn't aware of it.  It gets a few
> > > things wrong, such as:
> > >
> > > 1) not checking state->interface in the validate callback - so it
> > >    is basically saying that it can support any PHY interface mode
> > >    that the kernel happens to support.
> > >
> >
> > Partially true. It does check the DT bindings for supported MII modes
> > in sja1105_init_mii_settings (for fundamental reasons... the switch
> > expects an 'all-in-one' configuration buffer with the operating modes
> > of all MACs - don't ask me to delay the uploading of this static
> > config until all ports collected their interface_mode from phylink via
> > the mac_config callback - it's a deadlock).
>
> Ok, so you need to reject interface modes that are not compatible
> with the currently configured mode in the validate() callback, but
> please keep PHY_INTERFACE_MODE_NA reporting back the capabilities.
> (this is now documented.)
>

Ok.

> > It is a gigabit switch with MII/RMII/RGMII MACs - I have never seen
> > any PHY wired for these modes that can change system interface type.
>
> It is unlikely that MII/RMII/RGMII will switch modes, but in terms of
> correct implementation, sticking to the way the function is expected
> to behave means that I don't get surprises when changing phylink layer
> in the future.
>
> >
> > > 2) if phylink is configured to use in-band, then state->speed is
> > >    undefined; this driver will fail.  (If folk don't want to support
> > >    that, we ought to have a way to tell phylink to reject anything
> > >    that attempts to set it to in-band mode!)
> > >
> >
> > Ok.
> >
> > > 3) it doesn't implement phylink_mac_link_state DSA ops, so it doesn't
> > >    support SGMII or 802.3z phy interface modes (see 1).
> > >
> >
> > No, it doesn't.
> > Some odd switch in this device family supports SGMII on 1 of its
> > ports, however I haven't put my hands on it.
> > When I do I'll add checks for strange scenarios, like connecting it to
> > an Aquantia PHY that can switch between SGMII and USXGMII (although
> > why would anyone pair a 10G capable PHY to a 1G capable MAC...)
>
> It's unlikely that it would switch between SGMII and USXGMII
> dynamically, as USXGMII supports speeds from 10G down to 10M.
>
> Where interface mode switching tends to be used is with modes such
> as 10GBASE-R, which doesn't support anything except 10G.  In order
> for the PHY to operate at slower speeds, it has a few options:
>
> 1) perform rate adaption.
> 2) dynamically switch interface type to an interface type that
>    supports the desired speed.
> 3) just not support slower speeds.
>

So am I reading this correctly - it kind of makes sense for gigabit
MAC drivers to not check for the MII interface changing protocol?

> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up

Thanks,
-Vladimir

Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API

[Vladimir Oltean]

On Wed, 26 Jun 2019 at 02:10, Vladimir Oltean <olteanv@gmail.com> wrote:
>
> On Wed, 26 Jun 2019 at 01:58, Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> >
> > On Wed, Jun 26, 2019 at 01:14:59AM +0300, Vladimir Oltean wrote:
> > > On Wed, 26 Jun 2019 at 00:53, Russell King - ARM Linux admin
> > > <linux@armlinux.org.uk> wrote:
> > > >
> > > > On Tue, Jun 25, 2019 at 11:24:01PM +0300, Vladimir Oltean wrote:
> > > > > Hi Russell,
> > > > >
> > > > > On 6/24/19 6:39 PM, Russell King - ARM Linux admin wrote:
> > > > > > This should be removed - state->link is not for use in mac_config.
> > > > > > Even in fixed mode, the link can be brought up/down by means of a
> > > > > > gpio, and this should be dealt with via the mac_link_* functions.
> > > > > >
> > > > >
> > > > > What do you mean exactly that state->link is not for use, is that true in
> > > > > general?
> > > >
> > > > Yes.  mac_config() should not touch it; it is not always in a defined
> > > > state.  For example, if you set modes via ethtool (the
> > > > ethtool_ksettings_set API) then state->link will probably contain
> > > > zero irrespective of the true link state.
> > > >
> > >
> > > Experimentally, state->link is zero at the same time as state->speed
> > > is -1, so just ignoring !state->link made sense. This is not in-band
> > > AN. What is your suggestion? Should I proceed to try and configure the
> > > MAC for SPEED_UNKNOWN?
> >
> > What would you have done with a PHY when the link is down, what speed
> > would you have configured in the phylib adjust_link callback?  phylib
> > also sets SPEED_UNKNOWN/DUPLEX_UNKNOWN when the link is down.
> >
>
> With phylib, I'd make the driver ignore the speed and do nothing.
> With phylink, I'd make the core not call mac_config.
> But what happened is I saw phylink call mac_config anyway, said
> 'weird' and proceeded to ignore it as I would have for phylib.
> I'm just not understanding your position - it seems like you're
> implying there's a bug in phylink and the function call with
> MLO_AN_FIXED, state->link=0 and state->speed=-1 should not have taken

I meant MLO_AN_PHY, sorry.

> place, which is what I wanted to confirm.
>
> > What we do in Marvell drivers is set to the lowest speed (10M) in such
> > cases, which is fine as the MAC supports 10M.
> >
> > It wouldn't be appropriate for phylink to force something on MAC
> > drivers, it's easier if the MAC just defaults SPEED_UNKNOWN to something
> > itself.
> >
> > >
> > > > It exists in this structure because it was convenient to just use one
> > > > structure to store all the link information in various parts of the
> > > > code, and when requesting the negotiated in-band MAC configuration.
> > > >
> > > > I've come to the conclusion that that decision was a mistake, based
> > > > on patches such as the above mistakenly thinking that everything in
> > > > the state structure is fair game.  I've since updated the docs to
> > > > explicitly spell it out, but I'm also looking at the feasibility of
> > > > changing the mac_config() interface entirely - splitting it into two
> > > > (mac_config_fixed() and mac_config_inband()) and passing only the
> > > > appropriate parameters to each.
> > > >
> > > > However, having looked at that, I think such a change will make some
> > > > MAC drivers quite a bit more complicated - having all the config
> > > > steps in one method appears to make the configuration of MAC drivers
> > > > easier (eg, mvneta, mvpp2.)
> > > >
> > > > > In drivers/net/dsa/sja1105/sja1105_main.c, if I remove the "if
> > > > > (!state->link)" guard, I see PHYLINK calls with a SPEED_UNKNOWN argument for
> > > > > ports that are BR_STATE_DISABLED. Is that normal?
> > > >
> > > > This looks like another driver which has been converted to phylink
> > > > without my review; I certainly wasn't aware of it.  It gets a few
> > > > things wrong, such as:
> > > >
> > > > 1) not checking state->interface in the validate callback - so it
> > > >    is basically saying that it can support any PHY interface mode
> > > >    that the kernel happens to support.
> > > >
> > >
> > > Partially true. It does check the DT bindings for supported MII modes
> > > in sja1105_init_mii_settings (for fundamental reasons... the switch
> > > expects an 'all-in-one' configuration buffer with the operating modes
> > > of all MACs - don't ask me to delay the uploading of this static
> > > config until all ports collected their interface_mode from phylink via
> > > the mac_config callback - it's a deadlock).
> >
> > Ok, so you need to reject interface modes that are not compatible
> > with the currently configured mode in the validate() callback, but
> > please keep PHY_INTERFACE_MODE_NA reporting back the capabilities.
> > (this is now documented.)
> >
>
> Ok.
>
> > > It is a gigabit switch with MII/RMII/RGMII MACs - I have never seen
> > > any PHY wired for these modes that can change system interface type.
> >
> > It is unlikely that MII/RMII/RGMII will switch modes, but in terms of
> > correct implementation, sticking to the way the function is expected
> > to behave means that I don't get surprises when changing phylink layer
> > in the future.
> >
> > >
> > > > 2) if phylink is configured to use in-band, then state->speed is
> > > >    undefined; this driver will fail.  (If folk don't want to support
> > > >    that, we ought to have a way to tell phylink to reject anything
> > > >    that attempts to set it to in-band mode!)
> > > >
> > >
> > > Ok.
> > >
> > > > 3) it doesn't implement phylink_mac_link_state DSA ops, so it doesn't
> > > >    support SGMII or 802.3z phy interface modes (see 1).
> > > >
> > >
> > > No, it doesn't.
> > > Some odd switch in this device family supports SGMII on 1 of its
> > > ports, however I haven't put my hands on it.
> > > When I do I'll add checks for strange scenarios, like connecting it to
> > > an Aquantia PHY that can switch between SGMII and USXGMII (although
> > > why would anyone pair a 10G capable PHY to a 1G capable MAC...)
> >
> > It's unlikely that it would switch between SGMII and USXGMII
> > dynamically, as USXGMII supports speeds from 10G down to 10M.
> >
> > Where interface mode switching tends to be used is with modes such
> > as 10GBASE-R, which doesn't support anything except 10G.  In order
> > for the PHY to operate at slower speeds, it has a few options:
> >
> > 1) perform rate adaption.
> > 2) dynamically switch interface type to an interface type that
> >    supports the desired speed.
> > 3) just not support slower speeds.
> >
>
> So am I reading this correctly - it kind of makes sense for gigabit
> MAC drivers to not check for the MII interface changing protocol?
>
> > --
> > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> > FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> > According to speedtest.net: 11.9Mbps down 500kbps up
>
> Thanks,
> -Vladimir

Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API

[Andrew Lunn]

On Wed, Jun 26, 2019 at 02:13:25AM +0300, Vladimir Oltean wrote:
> On Wed, 26 Jun 2019 at 02:10, Vladimir Oltean <olteanv@gmail.com> wrote:
> >
> > On Wed, 26 Jun 2019 at 01:58, Russell King - ARM Linux admin
> > <linux@armlinux.org.uk> wrote:
> > >
> > > On Wed, Jun 26, 2019 at 01:14:59AM +0300, Vladimir Oltean wrote:
> > > > On Wed, 26 Jun 2019 at 00:53, Russell King - ARM Linux admin
> > > > <linux@armlinux.org.uk> wrote:
> > > > >
> > > > > On Tue, Jun 25, 2019 at 11:24:01PM +0300, Vladimir Oltean wrote:
> > > > > > Hi Russell,
> > > > > >
> > > > > > On 6/24/19 6:39 PM, Russell King - ARM Linux admin wrote:
> > > > > > > This should be removed - state->link is not for use in mac_config.
> > > > > > > Even in fixed mode, the link can be brought up/down by means of a
> > > > > > > gpio, and this should be dealt with via the mac_link_* functions.
> > > > > > >
> > > > > >
> > > > > > What do you mean exactly that state->link is not for use, is that true in
> > > > > > general?
> > > > >
> > > > > Yes.  mac_config() should not touch it; it is not always in a defined
> > > > > state.  For example, if you set modes via ethtool (the
> > > > > ethtool_ksettings_set API) then state->link will probably contain
> > > > > zero irrespective of the true link state.
> > > > >
> > > >
> > > > Experimentally, state->link is zero at the same time as state->speed
> > > > is -1, so just ignoring !state->link made sense. This is not in-band
> > > > AN. What is your suggestion? Should I proceed to try and configure the
> > > > MAC for SPEED_UNKNOWN?
> > >
> > > What would you have done with a PHY when the link is down, what speed
> > > would you have configured in the phylib adjust_link callback?  phylib
> > > also sets SPEED_UNKNOWN/DUPLEX_UNKNOWN when the link is down.
> > >
> >
> > With phylib, I'd make the driver ignore the speed and do nothing.
> > With phylink, I'd make the core not call mac_config.
> > But what happened is I saw phylink call mac_config anyway, said
> > 'weird' and proceeded to ignore it as I would have for phylib.
> > I'm just not understanding your position - it seems like you're
> > implying there's a bug in phylink and the function call with
> > MLO_AN_FIXED, state->link=0 and state->speed=-1 should not have taken
> 
> I meant MLO_AN_PHY, sorry.

The MAC could go into a low power mode.

    Andrew

Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API

[Russell King - ARM Linux admin]

On Wed, Jun 26, 2019 at 02:10:27AM +0300, Vladimir Oltean wrote:
> On Wed, 26 Jun 2019 at 01:58, Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> >
> > On Wed, Jun 26, 2019 at 01:14:59AM +0300, Vladimir Oltean wrote:
> > > On Wed, 26 Jun 2019 at 00:53, Russell King - ARM Linux admin
> > > <linux@armlinux.org.uk> wrote:
> > > >
> > > > On Tue, Jun 25, 2019 at 11:24:01PM +0300, Vladimir Oltean wrote:
> > > > > Hi Russell,
> > > > >
> > > > > On 6/24/19 6:39 PM, Russell King - ARM Linux admin wrote:
> > > > > > This should be removed - state->link is not for use in mac_config.
> > > > > > Even in fixed mode, the link can be brought up/down by means of a
> > > > > > gpio, and this should be dealt with via the mac_link_* functions.
> > > > > >
> > > > >
> > > > > What do you mean exactly that state->link is not for use, is that true in
> > > > > general?
> > > >
> > > > Yes.  mac_config() should not touch it; it is not always in a defined
> > > > state.  For example, if you set modes via ethtool (the
> > > > ethtool_ksettings_set API) then state->link will probably contain
> > > > zero irrespective of the true link state.
> > > >
> > >
> > > Experimentally, state->link is zero at the same time as state->speed
> > > is -1, so just ignoring !state->link made sense. This is not in-band
> > > AN. What is your suggestion? Should I proceed to try and configure the
> > > MAC for SPEED_UNKNOWN?
> >
> > What would you have done with a PHY when the link is down, what speed
> > would you have configured in the phylib adjust_link callback?  phylib
> > also sets SPEED_UNKNOWN/DUPLEX_UNKNOWN when the link is down.
> >
> 
> With phylib, I'd make the driver ignore the speed and do nothing.
> With phylink, I'd make the core not call mac_config.
> But what happened is I saw phylink call mac_config anyway, said
> 'weird' and proceeded to ignore it as I would have for phylib.
> I'm just not understanding your position - it seems like you're
> implying there's a bug in phylink and the function call with
> MLO_AN_FIXED, state->link=0 and state->speed=-1 should not have taken
> place, which is what I wanted to confirm.

It is not a bug.  It is a request to configure the MAC, and what it's
saying is "we don't know what speed and/or duplex".

Take for instance when the network adapter is brought up initially.
The link is most likely down, but we should configure the initial MAC
operating parameters (such as the PHY interface).  Phylink makes a
mac_config() call with the speed and duplex set to UNKNOWN.

Using your theory, we shouldn't be making that call.  In which case,
MAC drivers aren't going to initially configure their interface
settings.

_That_ would be a bug.

> > It's unlikely that it would switch between SGMII and USXGMII
> > dynamically, as USXGMII supports speeds from 10G down to 10M.
> >
> > Where interface mode switching tends to be used is with modes such
> > as 10GBASE-R, which doesn't support anything except 10G.  In order
> > for the PHY to operate at slower speeds, it has a few options:
> >
> > 1) perform rate adaption.
> > 2) dynamically switch interface type to an interface type that
> >    supports the desired speed.
> > 3) just not support slower speeds.
> >
> 
> So am I reading this correctly - it kind of makes sense for gigabit
> MAC drivers to not check for the MII interface changing protocol?

Again, that's incorrect in the general case.  Gigabit includes SGMII
and 802.3z PHY protocols which need to be switched between for SFPs.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API

[Vladimir Oltean]

On Wed, 26 Jun 2019 at 10:42, Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Wed, Jun 26, 2019 at 02:10:27AM +0300, Vladimir Oltean wrote:
> > On Wed, 26 Jun 2019 at 01:58, Russell King - ARM Linux admin
> > <linux@armlinux.org.uk> wrote:
> > >
> > > On Wed, Jun 26, 2019 at 01:14:59AM +0300, Vladimir Oltean wrote:
> > > > On Wed, 26 Jun 2019 at 00:53, Russell King - ARM Linux admin
> > > > <linux@armlinux.org.uk> wrote:
> > > > >
> > > > > On Tue, Jun 25, 2019 at 11:24:01PM +0300, Vladimir Oltean wrote:
> > > > > > Hi Russell,
> > > > > >
> > > > > > On 6/24/19 6:39 PM, Russell King - ARM Linux admin wrote:
> > > > > > > This should be removed - state->link is not for use in mac_config.
> > > > > > > Even in fixed mode, the link can be brought up/down by means of a
> > > > > > > gpio, and this should be dealt with via the mac_link_* functions.
> > > > > > >
> > > > > >
> > > > > > What do you mean exactly that state->link is not for use, is that true in
> > > > > > general?
> > > > >
> > > > > Yes.  mac_config() should not touch it; it is not always in a defined
> > > > > state.  For example, if you set modes via ethtool (the
> > > > > ethtool_ksettings_set API) then state->link will probably contain
> > > > > zero irrespective of the true link state.
> > > > >
> > > >
> > > > Experimentally, state->link is zero at the same time as state->speed
> > > > is -1, so just ignoring !state->link made sense. This is not in-band
> > > > AN. What is your suggestion? Should I proceed to try and configure the
> > > > MAC for SPEED_UNKNOWN?
> > >
> > > What would you have done with a PHY when the link is down, what speed
> > > would you have configured in the phylib adjust_link callback?  phylib
> > > also sets SPEED_UNKNOWN/DUPLEX_UNKNOWN when the link is down.
> > >
> >
> > With phylib, I'd make the driver ignore the speed and do nothing.
> > With phylink, I'd make the core not call mac_config.
> > But what happened is I saw phylink call mac_config anyway, said
> > 'weird' and proceeded to ignore it as I would have for phylib.
> > I'm just not understanding your position - it seems like you're
> > implying there's a bug in phylink and the function call with
> > MLO_AN_FIXED, state->link=0 and state->speed=-1 should not have taken
> > place, which is what I wanted to confirm.
>
> It is not a bug.  It is a request to configure the MAC, and what it's
> saying is "we don't know what speed and/or duplex".
>
> Take for instance when the network adapter is brought up initially.
> The link is most likely down, but we should configure the initial MAC
> operating parameters (such as the PHY interface).  Phylink makes a
> mac_config() call with the speed and duplex set to UNKNOWN.
>
> Using your theory, we shouldn't be making that call.  In which case,
> MAC drivers aren't going to initially configure their interface
> settings.
>
> _That_ would be a bug.
>

So you're saying that:
- state->link should not be checked, because it is not guaranteed to be valid
- state->speed, state->duplex, state->pause *should* be checked,
because it is not guaranteed to be valid
Is state->interface always valid?
I don't think I follow the pattern here. Or shouldn't I check speed,
duplex and pause either, and try to pass the MAC UNKNOWN values,
inevitably failing at some point? Do Marvell MACs have an UNKNOWN
setting?


> > It's unlikely that it would switch between SGMII and USXGMII
> > > dynamically, as USXGMII supports speeds from 10G down to 10M.
> > >
> > > Where interface mode switching tends to be used is with modes such
> > > as 10GBASE-R, which doesn't support anything except 10G.  In order
> > > for the PHY to operate at slower speeds, it has a few options:
> > >
> > > 1) perform rate adaption.
> > > 2) dynamically switch interface type to an interface type that
> > >    supports the desired speed.
> > > 3) just not support slower speeds.
> > >
> >
> > So am I reading this correctly - it kind of makes sense for gigabit
> > MAC drivers to not check for the MII interface changing protocol?
>
> Again, that's incorrect in the general case.  Gigabit includes SGMII
> and 802.3z PHY protocols which need to be switched between for SFPs.
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up

Thanks,
-Vladimir

Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API

[Russell King - ARM Linux admin]

On Wed, Jun 26, 2019 at 11:46:15AM +0300, Vladimir Oltean wrote:
> On Wed, 26 Jun 2019 at 10:42, Russell King - ARM Linux admin
> <linux@armlinux.org.uk> wrote:
> >
> > On Wed, Jun 26, 2019 at 02:10:27AM +0300, Vladimir Oltean wrote:
> > > On Wed, 26 Jun 2019 at 01:58, Russell King - ARM Linux admin
> > > <linux@armlinux.org.uk> wrote:
> > > >
> > > > On Wed, Jun 26, 2019 at 01:14:59AM +0300, Vladimir Oltean wrote:
> > > > > On Wed, 26 Jun 2019 at 00:53, Russell King - ARM Linux admin
> > > > > <linux@armlinux.org.uk> wrote:
> > > > > >
> > > > > > On Tue, Jun 25, 2019 at 11:24:01PM +0300, Vladimir Oltean wrote:
> > > > > > > Hi Russell,
> > > > > > >
> > > > > > > On 6/24/19 6:39 PM, Russell King - ARM Linux admin wrote:
> > > > > > > > This should be removed - state->link is not for use in mac_config.
> > > > > > > > Even in fixed mode, the link can be brought up/down by means of a
> > > > > > > > gpio, and this should be dealt with via the mac_link_* functions.
> > > > > > > >
> > > > > > >
> > > > > > > What do you mean exactly that state->link is not for use, is that true in
> > > > > > > general?
> > > > > >
> > > > > > Yes.  mac_config() should not touch it; it is not always in a defined
> > > > > > state.  For example, if you set modes via ethtool (the
> > > > > > ethtool_ksettings_set API) then state->link will probably contain
> > > > > > zero irrespective of the true link state.
> > > > > >
> > > > >
> > > > > Experimentally, state->link is zero at the same time as state->speed
> > > > > is -1, so just ignoring !state->link made sense. This is not in-band
> > > > > AN. What is your suggestion? Should I proceed to try and configure the
> > > > > MAC for SPEED_UNKNOWN?
> > > >
> > > > What would you have done with a PHY when the link is down, what speed
> > > > would you have configured in the phylib adjust_link callback?  phylib
> > > > also sets SPEED_UNKNOWN/DUPLEX_UNKNOWN when the link is down.
> > > >
> > >
> > > With phylib, I'd make the driver ignore the speed and do nothing.
> > > With phylink, I'd make the core not call mac_config.
> > > But what happened is I saw phylink call mac_config anyway, said
> > > 'weird' and proceeded to ignore it as I would have for phylib.
> > > I'm just not understanding your position - it seems like you're
> > > implying there's a bug in phylink and the function call with
> > > MLO_AN_FIXED, state->link=0 and state->speed=-1 should not have taken
> > > place, which is what I wanted to confirm.
> >
> > It is not a bug.  It is a request to configure the MAC, and what it's
> > saying is "we don't know what speed and/or duplex".
> >
> > Take for instance when the network adapter is brought up initially.
> > The link is most likely down, but we should configure the initial MAC
> > operating parameters (such as the PHY interface).  Phylink makes a
> > mac_config() call with the speed and duplex set to UNKNOWN.
> >
> > Using your theory, we shouldn't be making that call.  In which case,
> > MAC drivers aren't going to initially configure their interface
> > settings.
> >
> > _That_ would be a bug.
> >
> 
> So you're saying that:
> - state->link should not be checked, because it is not guaranteed to be valid

state->link is undefined.

> - state->speed, state->duplex, state->pause *should* be checked,

These will always be valid for FIXED and PHY modes, but _may_ be
UNKNOWN, meaning phylink does not have any information about what
the speed should be.

speed and duplex are not defined for inband modes, since the purpose
of inband modes is to communicate this information through... inband
information, which the MAC driver already has access to.  pause is
a different matter because it is present in some inband modes but
not others.

Which fields may be examined are now documented in the phylink
documentation in mainline kernels.

> Is state->interface always valid?

Yes.

> I don't think I follow the pattern here. Or shouldn't I check speed,
> duplex and pause either, and try to pass the MAC UNKNOWN values,
> inevitably failing at some point? Do Marvell MACs have an UNKNOWN
> setting?

Why do you think that just because state->speed is SPEED_UNKNOWN you
have to dream up some weird "unknown" value for the MAC?  Default it
to something sensible, just like you would do if phylib reports
SPEED_UNKNOWN during link-down.  I really don't get what the problem
is here.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API

[Daniel Santos]

On 6/24/19 9:52 AM, René van Dorst wrote:
> Convert mt7530 to PHYLINK API
>
> Signed-off-by: René van Dorst <opensource@vdorst.com>
> ---
>  drivers/net/dsa/mt7530.c | 237 +++++++++++++++++++++++++++++----------
>  drivers/net/dsa/mt7530.h |   9 ++
>  2 files changed, 187 insertions(+), 59 deletions(-)
>
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 3181e95586d6..9c5e4dd00826 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -13,7 +13,7 @@
>  #include <linux/of_mdio.h>
>  #include <linux/of_net.h>
>  #include <linux/of_platform.h>
> -#include <linux/phy.h>
> +#include <linux/phylink.h>
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/reset.h>
> @@ -633,63 +633,6 @@ mt7530_get_sset_count(struct dsa_switch *ds, int port, int sset)
>  	return ARRAY_SIZE(mt7530_mib);
>  }
>  
> -static void mt7530_adjust_link(struct dsa_switch *ds, int port,
> -			       struct phy_device *phydev)
> -{
> -	struct mt7530_priv *priv = ds->priv;
> -
> -	if (phy_is_pseudo_fixed_link(phydev)) {
> -		dev_dbg(priv->dev, "phy-mode for master device = %x\n",
> -			phydev->interface);
> -
> -		/* Setup TX circuit incluing relevant PAD and driving */
> -		mt7530_pad_clk_setup(ds, phydev->interface);
> -
> -		if (priv->id == ID_MT7530) {
> -			/* Setup RX circuit, relevant PAD and driving on the
> -			 * host which must be placed after the setup on the
> -			 * device side is all finished.
> -			 */
> -			mt7623_pad_clk_setup(ds);
> -		}
> -	} else {
> -		u16 lcl_adv = 0, rmt_adv = 0;
> -		u8 flowctrl;
> -		u32 mcr = PMCR_USERP_LINK | PMCR_FORCE_MODE;
> -
> -		switch (phydev->speed) {
> -		case SPEED_1000:
> -			mcr |= PMCR_FORCE_SPEED_1000;
> -			break;
> -		case SPEED_100:
> -			mcr |= PMCR_FORCE_SPEED_100;
> -			break;
> -		}
> -
> -		if (phydev->link)
> -			mcr |= PMCR_FORCE_LNK;
> -
> -		if (phydev->duplex) {
> -			mcr |= PMCR_FORCE_FDX;
> -
> -			if (phydev->pause)
> -				rmt_adv = LPA_PAUSE_CAP;
> -			if (phydev->asym_pause)
> -				rmt_adv |= LPA_PAUSE_ASYM;
> -
> -			lcl_adv = linkmode_adv_to_lcl_adv_t(
> -				phydev->advertising);
> -			flowctrl = mii_resolve_flowctrl_fdx(lcl_adv, rmt_adv);
> -
> -			if (flowctrl & FLOW_CTRL_TX)
> -				mcr |= PMCR_TX_FC_EN;
> -			if (flowctrl & FLOW_CTRL_RX)
> -				mcr |= PMCR_RX_FC_EN;
> -		}
> -		mt7530_write(priv, MT7530_PMCR_P(port), mcr);
> -	}
> -}
> -
>  static int
>  mt7530_cpu_port_enable(struct mt7530_priv *priv,
>  		       int port)
> @@ -1323,6 +1266,178 @@ mt7530_setup(struct dsa_switch *ds)
>  	return 0;
>  }
>  
> +static void mt7530_phylink_mac_config(struct dsa_switch *ds, int port,
> +				      unsigned int mode,
> +				      const struct phylink_link_state *state)
> +{
> +	struct mt7530_priv *priv = ds->priv;
> +	u32 mcr = PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | PMCR_BACKOFF_EN |
> +		  PMCR_BACKPR_EN | PMCR_TX_EN | PMCR_RX_EN;
> +
> +	switch (port) {
> +	case 0: /* Internal phy */
> +	case 1:
> +	case 2:
> +	case 3:
> +	case 4:
> +		if (state->interface != PHY_INTERFACE_MODE_GMII)
> +			goto unsupported;
> +		break;
> +	/* case 5: Port 5 is not supported! */
> +	case 6: /* 1st cpu port */
> +		if (state->interface != PHY_INTERFACE_MODE_RGMII &&
> +		    state->interface != PHY_INTERFACE_MODE_TRGMII)
> +			goto unsupported;
> +
> +		/* Setup TX circuit incluing relevant PAD and driving */
> +		mt7530_pad_clk_setup(ds, state->interface);
> +
> +		if (priv->id == ID_MT7530) {
> +			/* Setup RX circuit, relevant PAD and driving on the
> +			 * host which must be placed after the setup on the
> +			 * device side is all finished.
> +			 */
> +			mt7623_pad_clk_setup(ds);
> +		}
> +		break;
> +	default:
> +		dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
> +		return;
> +	}
> +
> +	if (!state->an_enabled || mode == MLO_AN_FIXED) {
> +		mcr |= PMCR_FORCE_MODE;
> +
> +		if (state->speed == SPEED_1000)
> +			mcr |= PMCR_FORCE_SPEED_1000;
> +		if (state->speed == SPEED_100)
> +			mcr |= PMCR_FORCE_SPEED_100;

I would suggest using the defines

#define PMCR_FORCE_SPEED	0x0000000c /* or PMCR_FORCE_SPEED_MASK */
#define PMCR_FORCE_SPEED_10	0x00000000
#define PMCR_FORCE_SPEED_100	0x00000004
#define PMCR_FORCE_SPEED_1000	0x00000008

and a switch statement such as

	switch (state->speed) {
	case SPEED_10:
		mcr |= PMCR_FORCE_SPEED_10;
		break;
	case SPEED_100:
		mcr |= PMCR_FORCE_SPEED_100;
		break;
	case SPEED_1000:
		mcr |= PMCR_FORCE_SPEED_1000;
		break;
	}

This will compile the same (i.e, the mcr |= 0 will optimize away, etc.),
while alleviating the need to intimately know the hardware in order to
easily understand what the code is doing at a glance.  It's also better
form as we're treating the two bits as a composite value, rather than
two separate bits.


> +		if (state->duplex == DUPLEX_FULL)
> +			mcr |= PMCR_FORCE_FDX;
> +		if (state->link || mode == MLO_AN_FIXED)
> +			mcr |= PMCR_FORCE_LNK;
> +		if (state->pause || phylink_test(state->advertising, Pause))
> +			mcr |= PMCR_TX_FC_EN | PMCR_RX_FC_EN;
> +		if (state->pause & MLO_PAUSE_TX)
> +			mcr |= PMCR_TX_FC_EN;
> +		if (state->pause & MLO_PAUSE_RX)
> +			mcr |= PMCR_RX_FC_EN;
> +	}
> +
> +	mt7530_write(priv, MT7530_PMCR_P(port), mcr);
> +
> +	return;
> +
> +unsupported:
> +	dev_err(ds->dev, "%s: P%d: Unsupported phy_interface mode: %d (%s)\n",
> +		__func__, port, state->interface, phy_modes(state->interface));
> +}
> +
> +static void mt7530_phylink_mac_link_down(struct dsa_switch *ds, int port,
> +					 unsigned int mode,
> +					 phy_interface_t interface)
> +{
> +	/* Do nothing */
> +}
> +
> +static void mt7530_phylink_mac_link_up(struct dsa_switch *ds, int port,
> +				       unsigned int mode,
> +				       phy_interface_t interface,
> +				       struct phy_device *phydev)
> +{
> +	/* Do nothing */
> +}
> +
> +static void mt7530_phylink_validate(struct dsa_switch *ds, int port,
> +				    unsigned long *supported,
> +				    struct phylink_link_state *state)
> +{
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> +
> +	switch (port) {
> +	case 0: /* Internal phy */
> +	case 1:
> +	case 2:
> +	case 3:
> +	case 4:
> +		if (state->interface != PHY_INTERFACE_MODE_NA &&
> +		    state->interface != PHY_INTERFACE_MODE_GMII)
> +			goto unsupported;
> +		break;
> +	/* case 5: Port 5 not supported! */
> +	case 6: /* 1st cpu port */
> +		if (state->interface != PHY_INTERFACE_MODE_RGMII &&
> +		    state->interface != PHY_INTERFACE_MODE_TRGMII)
> +			goto unsupported;
> +		break;
> +	default:
> +		linkmode_zero(supported);
> +		dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
> +		return;
> +	}
> +
> +	phylink_set(mask, Autoneg);
> +	phylink_set(mask, Pause);
> +	phylink_set(mask, Asym_Pause);
> +	phylink_set(mask, MII);
> +
> +	phylink_set(mask, 10baseT_Half);
> +	phylink_set(mask, 10baseT_Full);
> +	phylink_set(mask, 100baseT_Half);
> +	phylink_set(mask, 100baseT_Full);
> +	phylink_set(mask, 1000baseT_Full);
> +	phylink_set(mask, 1000baseT_Half);
> +
> +	linkmode_and(supported, supported, mask);
> +	linkmode_and(state->advertising, state->advertising, mask);
> +	return;
> +
> +unsupported:
> +	linkmode_zero(supported);
> +	dev_err(ds->dev, "%s: unsupported interface mode: [0x%x] %s\n",
> +		__func__, state->interface, phy_modes(state->interface));
> +}
> +
> +static int
> +mt7530_phylink_mac_link_state(struct dsa_switch *ds, int port,
> +			      struct phylink_link_state *state)
> +{
> +	struct mt7530_priv *priv = ds->priv;
> +	u32 pmsr;
> +
> +	if (port < 0 || port >= MT7530_NUM_PORTS)
> +		return -EINVAL;
> +
> +	pmsr = mt7530_read(priv, MT7530_PMSR_P(port));
> +
> +	state->link = (pmsr & PMSR_LINK);
> +	state->an_complete = state->link;
> +	state->duplex = (pmsr & PMSR_DPX) >> 1;
> +
> +	switch (pmsr & (PMSR_SPEED_1000 | PMSR_SPEED_100)) {
> +	case 0:
> +		state->speed = SPEED_10;
> +		break;
> +	case PMSR_SPEED_100:
> +		state->speed = SPEED_100;
> +		break;
> +	case PMSR_SPEED_1000:
> +		state->speed = SPEED_1000;
> +		break;
> +	default:
> +		state->speed = SPEED_UNKNOWN;
> +		break;
> +	}
> +

Same as above: add PMSR_SPEED_10, and and with PMSR_SPEED (or
PMSR_SPEED_MASK if you prefer).

> +	state->pause = 0;
> +	if (pmsr & PMSR_RX_FC)
> +		state->pause |= MLO_PAUSE_RX;
> +	if (pmsr & PMSR_TX_FC)
> +		state->pause |= MLO_PAUSE_TX;
> +
> +	return 1;
> +}
> +
>  static const struct dsa_switch_ops mt7530_switch_ops = {
>  	.get_tag_protocol	= mtk_get_tag_protocol,
>  	.setup			= mt7530_setup,
> @@ -1331,7 +1446,6 @@ static const struct dsa_switch_ops mt7530_switch_ops = {
>  	.phy_write		= mt7530_phy_write,
>  	.get_ethtool_stats	= mt7530_get_ethtool_stats,
>  	.get_sset_count		= mt7530_get_sset_count,
> -	.adjust_link		= mt7530_adjust_link,
>  	.port_enable		= mt7530_port_enable,
>  	.port_disable		= mt7530_port_disable,
>  	.port_stp_state_set	= mt7530_stp_state_set,
> @@ -1344,6 +1458,11 @@ static const struct dsa_switch_ops mt7530_switch_ops = {
>  	.port_vlan_prepare	= mt7530_port_vlan_prepare,
>  	.port_vlan_add		= mt7530_port_vlan_add,
>  	.port_vlan_del		= mt7530_port_vlan_del,
> +	.phylink_validate	= mt7530_phylink_validate,
> +	.phylink_mac_link_state = mt7530_phylink_mac_link_state,
> +	.phylink_mac_config	= mt7530_phylink_mac_config,
> +	.phylink_mac_link_down	= mt7530_phylink_mac_link_down,
> +	.phylink_mac_link_up	= mt7530_phylink_mac_link_up,
>  };
>  
>  static const struct of_device_id mt7530_of_match[] = {
> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
> index bfac90f48102..41d9a132ac70 100644
> --- a/drivers/net/dsa/mt7530.h
> +++ b/drivers/net/dsa/mt7530.h
> @@ -198,6 +198,7 @@ enum mt7530_vlan_port_attr {
>  #define  PMCR_FORCE_SPEED_100		BIT(2)
>  #define  PMCR_FORCE_FDX			BIT(1)
>  #define  PMCR_FORCE_LNK			BIT(0)
> +#define  PMCR_FORCE_LNK_DOWN		PMCR_FORCE_MODE
>  #define  PMCR_COMMON_LINK		(PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | \
>  					 PMCR_BACKOFF_EN | PMCR_BACKPR_EN | \
>  					 PMCR_TX_EN | PMCR_RX_EN | \
> @@ -218,6 +219,14 @@ enum mt7530_vlan_port_attr {
>  					 PMCR_TX_FC_EN | PMCR_RX_FC_EN)
>  
>  #define MT7530_PMSR_P(x)		(0x3008 + (x) * 0x100)
> +#define  PMSR_EEE1G			BIT(7)
> +#define  PMSR_EEE100M			BIT(6)
> +#define  PMSR_RX_FC			BIT(5)
> +#define  PMSR_TX_FC			BIT(4)
> +#define  PMSR_SPEED_1000		BIT(3)
> +#define  PMSR_SPEED_100			BIT(2)
> +#define  PMSR_DPX			BIT(1)
> +#define  PMSR_LINK			BIT(0)
>  
>  /* Register for MIB */
>  #define MT7530_PORT_MIB_COUNTER(x)	(0x4000 + (x) * 0x100)

Cheers,
Daniel

Re: [PATCH RFC net-next 1/5] net: dsa: mt7530: Convert to PHYLINK API

[René van Dorst]

Quoting Daniel Santos <daniel.santos@pobox.com>:

Hi Daniel,

> On 6/24/19 9:52 AM, René van Dorst wrote:
>> Convert mt7530 to PHYLINK API
>>
>> Signed-off-by: René van Dorst <opensource@vdorst.com>
>> ---
>>  drivers/net/dsa/mt7530.c | 237 +++++++++++++++++++++++++++++----------
>>  drivers/net/dsa/mt7530.h |   9 ++
>>  2 files changed, 187 insertions(+), 59 deletions(-)
>>
>> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
>> index 3181e95586d6..9c5e4dd00826 100644
>> --- a/drivers/net/dsa/mt7530.c
>> +++ b/drivers/net/dsa/mt7530.c
>> @@ -13,7 +13,7 @@
>>  #include <linux/of_mdio.h>
>>  #include <linux/of_net.h>
>>  #include <linux/of_platform.h>
>> -#include <linux/phy.h>
>> +#include <linux/phylink.h>
>>  #include <linux/regmap.h>
>>  #include <linux/regulator/consumer.h>
>>  #include <linux/reset.h>
>> @@ -633,63 +633,6 @@ mt7530_get_sset_count(struct dsa_switch *ds,  
>> int port, int sset)
>>  	return ARRAY_SIZE(mt7530_mib);
>>  }
>>
>> -static void mt7530_adjust_link(struct dsa_switch *ds, int port,
>> -			       struct phy_device *phydev)
>> -{
>> -	struct mt7530_priv *priv = ds->priv;
>> -
>> -	if (phy_is_pseudo_fixed_link(phydev)) {
>> -		dev_dbg(priv->dev, "phy-mode for master device = %x\n",
>> -			phydev->interface);
>> -
>> -		/* Setup TX circuit incluing relevant PAD and driving */
>> -		mt7530_pad_clk_setup(ds, phydev->interface);
>> -
>> -		if (priv->id == ID_MT7530) {
>> -			/* Setup RX circuit, relevant PAD and driving on the
>> -			 * host which must be placed after the setup on the
>> -			 * device side is all finished.
>> -			 */
>> -			mt7623_pad_clk_setup(ds);
>> -		}
>> -	} else {
>> -		u16 lcl_adv = 0, rmt_adv = 0;
>> -		u8 flowctrl;
>> -		u32 mcr = PMCR_USERP_LINK | PMCR_FORCE_MODE;
>> -
>> -		switch (phydev->speed) {
>> -		case SPEED_1000:
>> -			mcr |= PMCR_FORCE_SPEED_1000;
>> -			break;
>> -		case SPEED_100:
>> -			mcr |= PMCR_FORCE_SPEED_100;
>> -			break;
>> -		}
>> -
>> -		if (phydev->link)
>> -			mcr |= PMCR_FORCE_LNK;
>> -
>> -		if (phydev->duplex) {
>> -			mcr |= PMCR_FORCE_FDX;
>> -
>> -			if (phydev->pause)
>> -				rmt_adv = LPA_PAUSE_CAP;
>> -			if (phydev->asym_pause)
>> -				rmt_adv |= LPA_PAUSE_ASYM;
>> -
>> -			lcl_adv = linkmode_adv_to_lcl_adv_t(
>> -				phydev->advertising);
>> -			flowctrl = mii_resolve_flowctrl_fdx(lcl_adv, rmt_adv);
>> -
>> -			if (flowctrl & FLOW_CTRL_TX)
>> -				mcr |= PMCR_TX_FC_EN;
>> -			if (flowctrl & FLOW_CTRL_RX)
>> -				mcr |= PMCR_RX_FC_EN;
>> -		}
>> -		mt7530_write(priv, MT7530_PMCR_P(port), mcr);
>> -	}
>> -}
>> -
>>  static int
>>  mt7530_cpu_port_enable(struct mt7530_priv *priv,
>>  		       int port)
>> @@ -1323,6 +1266,178 @@ mt7530_setup(struct dsa_switch *ds)
>>  	return 0;
>>  }
>>
>> +static void mt7530_phylink_mac_config(struct dsa_switch *ds, int port,
>> +				      unsigned int mode,
>> +				      const struct phylink_link_state *state)
>> +{
>> +	struct mt7530_priv *priv = ds->priv;
>> +	u32 mcr = PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | PMCR_BACKOFF_EN |
>> +		  PMCR_BACKPR_EN | PMCR_TX_EN | PMCR_RX_EN;
>> +
>> +	switch (port) {
>> +	case 0: /* Internal phy */
>> +	case 1:
>> +	case 2:
>> +	case 3:
>> +	case 4:
>> +		if (state->interface != PHY_INTERFACE_MODE_GMII)
>> +			goto unsupported;
>> +		break;
>> +	/* case 5: Port 5 is not supported! */
>> +	case 6: /* 1st cpu port */
>> +		if (state->interface != PHY_INTERFACE_MODE_RGMII &&
>> +		    state->interface != PHY_INTERFACE_MODE_TRGMII)
>> +			goto unsupported;
>> +
>> +		/* Setup TX circuit incluing relevant PAD and driving */
>> +		mt7530_pad_clk_setup(ds, state->interface);
>> +
>> +		if (priv->id == ID_MT7530) {
>> +			/* Setup RX circuit, relevant PAD and driving on the
>> +			 * host which must be placed after the setup on the
>> +			 * device side is all finished.
>> +			 */
>> +			mt7623_pad_clk_setup(ds);
>> +		}
>> +		break;
>> +	default:
>> +		dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
>> +		return;
>> +	}
>> +
>> +	if (!state->an_enabled || mode == MLO_AN_FIXED) {
>> +		mcr |= PMCR_FORCE_MODE;
>> +
>> +		if (state->speed == SPEED_1000)
>> +			mcr |= PMCR_FORCE_SPEED_1000;
>> +		if (state->speed == SPEED_100)
>> +			mcr |= PMCR_FORCE_SPEED_100;
>
> I would suggest using the defines
>
> #define PMCR_FORCE_SPEED	0x0000000c /* or PMCR_FORCE_SPEED_MASK */
> #define PMCR_FORCE_SPEED_10	0x00000000
> #define PMCR_FORCE_SPEED_100	0x00000004
> #define PMCR_FORCE_SPEED_1000	0x00000008
>
> and a switch statement such as
>
> 	switch (state->speed) {
> 	case SPEED_10:
> 		mcr |= PMCR_FORCE_SPEED_10;
> 		break;
> 	case SPEED_100:
> 		mcr |= PMCR_FORCE_SPEED_100;
> 		break;
> 	case SPEED_1000:
> 		mcr |= PMCR_FORCE_SPEED_1000;
> 		break;
> 	}
>
> This will compile the same (i.e, the mcr |= 0 will optimize away, etc.),
> while alleviating the need to intimately know the hardware in order to
> easily understand what the code is doing at a glance.  It's also better
> form as we're treating the two bits as a composite value, rather than
> two separate bits.

I will change it based on your surgestion.

>
>
>> +		if (state->duplex == DUPLEX_FULL)
>> +			mcr |= PMCR_FORCE_FDX;
>> +		if (state->link || mode == MLO_AN_FIXED)
>> +			mcr |= PMCR_FORCE_LNK;
>> +		if (state->pause || phylink_test(state->advertising, Pause))
>> +			mcr |= PMCR_TX_FC_EN | PMCR_RX_FC_EN;
>> +		if (state->pause & MLO_PAUSE_TX)
>> +			mcr |= PMCR_TX_FC_EN;
>> +		if (state->pause & MLO_PAUSE_RX)
>> +			mcr |= PMCR_RX_FC_EN;
>> +	}
>> +
>> +	mt7530_write(priv, MT7530_PMCR_P(port), mcr);
>> +
>> +	return;
>> +
>> +unsupported:
>> +	dev_err(ds->dev, "%s: P%d: Unsupported phy_interface mode: %d (%s)\n",
>> +		__func__, port, state->interface, phy_modes(state->interface));
>> +}
>> +
>> +static void mt7530_phylink_mac_link_down(struct dsa_switch *ds, int port,
>> +					 unsigned int mode,
>> +					 phy_interface_t interface)
>> +{
>> +	/* Do nothing */
>> +}
>> +
>> +static void mt7530_phylink_mac_link_up(struct dsa_switch *ds, int port,
>> +				       unsigned int mode,
>> +				       phy_interface_t interface,
>> +				       struct phy_device *phydev)
>> +{
>> +	/* Do nothing */
>> +}
>> +
>> +static void mt7530_phylink_validate(struct dsa_switch *ds, int port,
>> +				    unsigned long *supported,
>> +				    struct phylink_link_state *state)
>> +{
>> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
>> +
>> +	switch (port) {
>> +	case 0: /* Internal phy */
>> +	case 1:
>> +	case 2:
>> +	case 3:
>> +	case 4:
>> +		if (state->interface != PHY_INTERFACE_MODE_NA &&
>> +		    state->interface != PHY_INTERFACE_MODE_GMII)
>> +			goto unsupported;
>> +		break;
>> +	/* case 5: Port 5 not supported! */
>> +	case 6: /* 1st cpu port */
>> +		if (state->interface != PHY_INTERFACE_MODE_RGMII &&
>> +		    state->interface != PHY_INTERFACE_MODE_TRGMII)
>> +			goto unsupported;
>> +		break;
>> +	default:
>> +		linkmode_zero(supported);
>> +		dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
>> +		return;
>> +	}
>> +
>> +	phylink_set(mask, Autoneg);
>> +	phylink_set(mask, Pause);
>> +	phylink_set(mask, Asym_Pause);
>> +	phylink_set(mask, MII);
>> +
>> +	phylink_set(mask, 10baseT_Half);
>> +	phylink_set(mask, 10baseT_Full);
>> +	phylink_set(mask, 100baseT_Half);
>> +	phylink_set(mask, 100baseT_Full);
>> +	phylink_set(mask, 1000baseT_Full);
>> +	phylink_set(mask, 1000baseT_Half);
>> +
>> +	linkmode_and(supported, supported, mask);
>> +	linkmode_and(state->advertising, state->advertising, mask);
>> +	return;
>> +
>> +unsupported:
>> +	linkmode_zero(supported);
>> +	dev_err(ds->dev, "%s: unsupported interface mode: [0x%x] %s\n",
>> +		__func__, state->interface, phy_modes(state->interface));
>> +}
>> +
>> +static int
>> +mt7530_phylink_mac_link_state(struct dsa_switch *ds, int port,
>> +			      struct phylink_link_state *state)
>> +{
>> +	struct mt7530_priv *priv = ds->priv;
>> +	u32 pmsr;
>> +
>> +	if (port < 0 || port >= MT7530_NUM_PORTS)
>> +		return -EINVAL;
>> +
>> +	pmsr = mt7530_read(priv, MT7530_PMSR_P(port));
>> +
>> +	state->link = (pmsr & PMSR_LINK);
>> +	state->an_complete = state->link;
>> +	state->duplex = (pmsr & PMSR_DPX) >> 1;
>> +
>> +	switch (pmsr & (PMSR_SPEED_1000 | PMSR_SPEED_100)) {
>> +	case 0:
>> +		state->speed = SPEED_10;
>> +		break;
>> +	case PMSR_SPEED_100:
>> +		state->speed = SPEED_100;
>> +		break;
>> +	case PMSR_SPEED_1000:
>> +		state->speed = SPEED_1000;
>> +		break;
>> +	default:
>> +		state->speed = SPEED_UNKNOWN;
>> +		break;
>> +	}
>> +
>
> Same as above: add PMSR_SPEED_10, and and with PMSR_SPEED (or
> PMSR_SPEED_MASK if you prefer).

Same as above.

>
>> +	state->pause = 0;
>> +	if (pmsr & PMSR_RX_FC)
>> +		state->pause |= MLO_PAUSE_RX;
>> +	if (pmsr & PMSR_TX_FC)
>> +		state->pause |= MLO_PAUSE_TX;
>> +
>> +	return 1;
>> +}
>> +
>>  static const struct dsa_switch_ops mt7530_switch_ops = {
>>  	.get_tag_protocol	= mtk_get_tag_protocol,
>>  	.setup			= mt7530_setup,
>> @@ -1331,7 +1446,6 @@ static const struct dsa_switch_ops  
>> mt7530_switch_ops = {
>>  	.phy_write		= mt7530_phy_write,
>>  	.get_ethtool_stats	= mt7530_get_ethtool_stats,
>>  	.get_sset_count		= mt7530_get_sset_count,
>> -	.adjust_link		= mt7530_adjust_link,
>>  	.port_enable		= mt7530_port_enable,
>>  	.port_disable		= mt7530_port_disable,
>>  	.port_stp_state_set	= mt7530_stp_state_set,
>> @@ -1344,6 +1458,11 @@ static const struct dsa_switch_ops  
>> mt7530_switch_ops = {
>>  	.port_vlan_prepare	= mt7530_port_vlan_prepare,
>>  	.port_vlan_add		= mt7530_port_vlan_add,
>>  	.port_vlan_del		= mt7530_port_vlan_del,
>> +	.phylink_validate	= mt7530_phylink_validate,
>> +	.phylink_mac_link_state = mt7530_phylink_mac_link_state,
>> +	.phylink_mac_config	= mt7530_phylink_mac_config,
>> +	.phylink_mac_link_down	= mt7530_phylink_mac_link_down,
>> +	.phylink_mac_link_up	= mt7530_phylink_mac_link_up,
>>  };
>>
>>  static const struct of_device_id mt7530_of_match[] = {
>> diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
>> index bfac90f48102..41d9a132ac70 100644
>> --- a/drivers/net/dsa/mt7530.h
>> +++ b/drivers/net/dsa/mt7530.h
>> @@ -198,6 +198,7 @@ enum mt7530_vlan_port_attr {
>>  #define  PMCR_FORCE_SPEED_100		BIT(2)
>>  #define  PMCR_FORCE_FDX			BIT(1)
>>  #define  PMCR_FORCE_LNK			BIT(0)
>> +#define  PMCR_FORCE_LNK_DOWN		PMCR_FORCE_MODE
>>  #define  PMCR_COMMON_LINK		(PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | \
>>  					 PMCR_BACKOFF_EN | PMCR_BACKPR_EN | \
>>  					 PMCR_TX_EN | PMCR_RX_EN | \
>> @@ -218,6 +219,14 @@ enum mt7530_vlan_port_attr {
>>  					 PMCR_TX_FC_EN | PMCR_RX_FC_EN)
>>
>>  #define MT7530_PMSR_P(x)		(0x3008 + (x) * 0x100)
>> +#define  PMSR_EEE1G			BIT(7)
>> +#define  PMSR_EEE100M			BIT(6)
>> +#define  PMSR_RX_FC			BIT(5)
>> +#define  PMSR_TX_FC			BIT(4)
>> +#define  PMSR_SPEED_1000		BIT(3)
>> +#define  PMSR_SPEED_100			BIT(2)
>> +#define  PMSR_DPX			BIT(1)
>> +#define  PMSR_LINK			BIT(0)
>>
>>  /* Register for MIB */
>>  #define MT7530_PORT_MIB_COUNTER(x)	(0x4000 + (x) * 0x100)
>
> Cheers,
> Daniel

Greats,

René

[PATCH RFC net-next 2/5] dt-bindings: net: dsa: mt7530: Add support for port 5

[René van Dorst]

MT7530 port 5 has many modes/configurations.
Update the documentation how to use port 5.

Signed-off-by: René van Dorst <opensource@vdorst.com>
CC: devicetree@vger.kernel.org
---
 .../devicetree/bindings/net/dsa/mt7530.txt    | 215 ++++++++++++++++++
 1 file changed, 215 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/dsa/mt7530.txt b/Documentation/devicetree/bindings/net/dsa/mt7530.txt
index 47aa205ee0bd..f3486780f2c2 100644
--- a/Documentation/devicetree/bindings/net/dsa/mt7530.txt
+++ b/Documentation/devicetree/bindings/net/dsa/mt7530.txt
@@ -35,6 +35,39 @@ Required properties for the child nodes within ports container:
 - phy-mode: String, must be either "trgmii" or "rgmii" for port labeled
 	 "cpu".
 
+Port 5 of the switch is muxed between:
+1. GMAC5: GMAC5 can interface with another external MAC or PHY.
+2. PHY of port 0 or port 4: PHY interfaces with an external MAC like 2nd GMAC
+   of the SOC. Used in many setups where port 0/4 becomes the WAN port.
+
+Port 5 modes/configurations:
+1. Port 5 is disabled and isolated: An external phy can interface to the 2nd
+   GMAC of the SOC.
+   In the case of a build-in MT7530 switch, port 5 shares the RGMII bus with 2nd
+   GMAC and an optional external phy. Mind the GPIO/pinctl settings of the SOC!
+2. Port 5 is muxed to PHY of port 0/4: Port 0/4 interfaces with 2nd GMAC.
+   It is a simple MAC to PHY interface, port 5 needs to be setup for xMII mode
+   and RGMII delay.
+3. Port 5 is muxed to GMAC5 and can interface to an external phy.
+   Port 5 becomes an extra switch port.
+   Only works on platform where external phy TX<->RX lines are swapped.
+   Like in the Ubiquiti ER-X-SFP.
+4. Port 5 is muxed to GMAC5 and interfaces with the 2nd GAMC as 2nd CPU port.
+   Currently a 2nd CPU port is not supported by DSA code.
+
+Depending on how the external PHY is wired:
+1. normal: The PHY can only connect to 2nd GMAC but not to the switch
+2. swapped: RGMII TX, RX are swapped; external phy interface with the switch as
+   a ethernet port. But can't interface to the 2nd GMAC.
+
+Based on the DT the port 5 mode is configured.
+
+Driver tries to lookup the phy-handle of the 2nd GMAC of the master device.
+When phy-handle matches PHY of port 0 or 4 then port 5 set-up as mode 2.
+phy-mode must be set, see also example 2 below!
+ * mt7621: phy-mode = "rgmii-txid";
+ * mt7623: phy-mode = "rgmii";
+
 See Documentation/devicetree/bindings/net/dsa/dsa.txt for a list of additional
 required, optional properties and how the integrated switch subnodes must
 be specified.
@@ -94,3 +127,185 @@ Example:
 			};
 		};
 	};
+
+Example 2: MT7621: Port 4 is WAN port: 2nd GMAC -> Port 5 -> PHY port 4.
+
+&eth {
+	status = "okay";
+
+	gmac0: mac@0 {
+		compatible = "mediatek,eth-mac";
+		reg = <0>;
+		phy-mode = "rgmii";
+
+		fixed-link {
+			speed = <1000>;
+			full-duplex;
+			pause;
+		};
+	};
+
+	gmac1: mac@1 {
+		compatible = "mediatek,eth-mac";
+		reg = <1>;
+		phy-mode = "rgmii-txid";
+		phy-handle = <&phy4>;
+	};
+
+	mdio: mdio-bus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		/* Internal phy */
+		phy4: ethernet-phy@4 {
+			reg = <4>;
+		};
+
+		mt7530: switch@1f {
+			compatible = "mediatek,mt7621";
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <0x1f>;
+			pinctrl-names = "default";
+			mediatek,mcm;
+
+			resets = <&rstctrl 2>;
+			reset-names = "mcm";
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				port@0 {
+					reg = <0>;
+					label = "lan0";
+				};
+
+				port@1 {
+					reg = <1>;
+					label = "lan1";
+				};
+
+				port@2 {
+					reg = <2>;
+					label = "lan2";
+				};
+
+				port@3 {
+					reg = <3>;
+					label = "lan3";
+				};
+
+/* Commented out. Port 4 is handled by 2nd GMAC.
+				port@4 {
+					reg = <4>;
+					label = "lan4";
+				};
+*/
+
+				cpu_port0: port@6 {
+					reg = <6>;
+					label = "cpu";
+					ethernet = <&gmac0>;
+					phy-mode = "rgmii";
+
+					fixed-link {
+						speed = <1000>;
+						full-duplex;
+						pause;
+					};
+				};
+			};
+		};
+	};
+};
+
+Example 3: MT7621: Port 5 is connected to external PHY: Port 5 -> external PHY.
+
+&eth {
+	status = "okay";
+
+	gmac0: mac@0 {
+		compatible = "mediatek,eth-mac";
+		reg = <0>;
+		phy-mode = "rgmii";
+
+		fixed-link {
+			speed = <1000>;
+			full-duplex;
+			pause;
+		};
+	};
+
+	mdio: mdio-bus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		/* External phy */
+		ephy5: ethernet-phy@7 {
+			reg = <7>;
+		};
+
+		mt7530: switch@1f {
+			compatible = "mediatek,mt7621";
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <0x1f>;
+			pinctrl-names = "default";
+			mediatek,mcm;
+
+			resets = <&rstctrl 2>;
+			reset-names = "mcm";
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				port@0 {
+					reg = <0>;
+					label = "lan0";
+				};
+
+				port@1 {
+					reg = <1>;
+					label = "lan1";
+				};
+
+				port@2 {
+					reg = <2>;
+					label = "lan2";
+				};
+
+				port@3 {
+					reg = <3>;
+					label = "lan3";
+				};
+
+				port@4 {
+					reg = <4>;
+					label = "lan4";
+				};
+
+				port@5 {
+					reg = <5>;
+					label = "lan5";
+					phy-mode = "rgmii";
+					phy-handle = <&ephy5>;
+				};
+
+				cpu_port0: port@6 {
+					reg = <6>;
+					label = "cpu";
+					ethernet = <&gmac0>;
+					phy-mode = "rgmii";
+
+					fixed-link {
+						speed = <1000>;
+						full-duplex;
+						pause;
+					};
+				};
+			};
+		};
+	};
+};
-- 
2.20.1

[PATCH RFC net-next 3/5] net: dsa: mt7530: Add support for port 5

[René van Dorst]

Adding support for port 5.

Port 5 ca muxed/interface to:
- internal 5th GMAC of the switch; can be used as 2nd CPU port or as
  extra port with an external phy for a 6th ethernet port.
- internal PHY of port 0 or 4; Used in most applications so that port 0
  or 4 is the WAN port and interfaces with the 2nd GMAC of the SOC.

Signed-off-by: René van Dorst <opensource@vdorst.com>
---
 drivers/net/dsa/mt7530.c | 135 +++++++++++++++++++++++++++++++++++++--
 drivers/net/dsa/mt7530.h |  28 ++++++++
 2 files changed, 159 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 9c5e4dd00826..838a921ca83e 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -633,6 +633,74 @@ mt7530_get_sset_count(struct dsa_switch *ds, int port, int sset)
 	return ARRAY_SIZE(mt7530_mib);
 }
 
+static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
+{
+	struct mt7530_priv *priv = ds->priv;
+	u8 tx_delay = 0;
+	int val;
+
+	mutex_lock(&priv->reg_mutex);
+
+	val = mt7530_read(priv, MT7530_MHWTRAP);
+
+	val |= MHWTRAP_MANUAL | MHWTRAP_P5_MAC_SEL | MHWTRAP_P5_DIS;
+	val &= ~MHWTRAP_P5_RGMII_MODE & ~MHWTRAP_PHY0_SEL;
+
+	switch (priv->p5_mode) {
+	case P5_MODE_GPHY_P0:
+		/* MT7530_P5_MODE_GPHY_P0: 2nd GMAC -> P5 -> P0 */
+		val |= MHWTRAP_PHY0_SEL;
+		/* fall through */
+	case P5_MODE_GPHY_P4:
+		/* MT7530_P5_MODE_GPHY_P4: 2nd GMAC -> P5 -> P4 */
+		val &= ~MHWTRAP_P5_MAC_SEL & ~MHWTRAP_P5_DIS;
+
+		/* Setup the MAC by default for the cpu port */
+		mt7530_write(priv, MT7530_PMCR_P(5), 0x56300);
+		break;
+	case P5_MODE_GMAC:
+		/* MT7530_P5_MODE_GMAC: P5 -> External phy or 2nd GMAC */
+		val &= ~MHWTRAP_P5_DIS;
+		break;
+	case P5_MODE_DISABLED:
+		interface = PHY_INTERFACE_MODE_NA;
+		break;
+	default:
+		dev_err(ds->dev, "Unsupported p5_mode %d\n", priv->p5_mode);
+		goto unlock_exit;
+	}
+
+	/* Setup RGMII settings */
+	if (phy_interface_mode_is_rgmii(interface)) {
+		val |= MHWTRAP_P5_RGMII_MODE;
+
+		/* P5 RGMII RX Clock Control: delay setting for 1000M */
+		mt7530_write(priv, MT7530_P5RGMIIRXCR, CSR_RGMII_EDGE_ALIGN);
+
+		/* Don't set delay in DSA mode */
+		if (!dsa_is_dsa_port(priv->ds, 5) &&
+		    (interface == PHY_INTERFACE_MODE_RGMII_TXID ||
+		    interface == PHY_INTERFACE_MODE_RGMII_ID))
+			tx_delay = 4; /* n * 0.5 ns */
+
+		/* P5 RGMII TX Clock Control: delay x */
+		mt7530_write(priv, MT7530_P5RGMIITXCR,
+			     CSR_RGMII_TXC_CFG(0x10 + tx_delay));
+
+		/* reduce P5 RGMII Tx driving, 8mA */
+		mt7530_write(priv, MT7530_IO_DRV_CR,
+			     P5_IO_CLK_DRV(1) | P5_IO_DATA_DRV(1));
+	}
+
+	mt7530_write(priv, MT7530_MHWTRAP, val);
+
+	dev_info(ds->dev, "Setup P5, HWTRAP=0x%x, port-mode=%s, phy-mode=%s\n",
+		 val, p5_modes(priv->p5_mode), phy_modes(interface));
+
+unlock_exit:
+	mutex_unlock(&priv->reg_mutex);
+}
+
 static int
 mt7530_cpu_port_enable(struct mt7530_priv *priv,
 		       int port)
@@ -1173,6 +1241,10 @@ mt7530_setup(struct dsa_switch *ds)
 	u32 id, val;
 	struct device_node *dn;
 	struct mt7530_dummy_poll p;
+	phy_interface_t interface;
+	struct device_node *mac_np;
+	struct device_node *phy_node;
+	const __be32 *_id;
 
 	/* The parent node of master netdev which holds the common system
 	 * controller also is the container for two GMACs nodes representing
@@ -1258,6 +1330,40 @@ mt7530_setup(struct dsa_switch *ds)
 			mt7530_port_disable(ds, i);
 	}
 
+	/* Setup port 5 */
+	priv->p5_mode = P5_MODE_DISABLED;
+	interface = PHY_INTERFACE_MODE_NA;
+
+	if (!dsa_is_unused_port(ds, 5)) {
+		priv->p5_mode = P5_MODE_GMAC;
+		interface = of_get_phy_mode(ds->ports[5].dn);
+	} else {
+		/* Scan the ethernet nodes. Look for GMAC1, Lookup used phy */
+		for_each_child_of_node(dn, mac_np) {
+			if (!of_device_is_compatible(mac_np,
+						     "mediatek,eth-mac"))
+				continue;
+			_id = of_get_property(mac_np, "reg", NULL);
+			if (be32_to_cpup(_id)  != 1)
+				continue;
+
+			interface = of_get_phy_mode(mac_np);
+			phy_node = of_parse_phandle(mac_np, "phy-handle", 0);
+
+			if (phy_node->parent == priv->dev->of_node->parent) {
+				_id = of_get_property(phy_node, "reg", NULL);
+				id = be32_to_cpup(_id);
+				if (id == 0)
+					priv->p5_mode = P5_MODE_GPHY_P0;
+				if (id == 4)
+					priv->p5_mode = P5_MODE_GPHY_P4;
+			}
+			break;
+		}
+	}
+
+	mt7530_setup_port5(ds, interface);
+
 	/* Flush the FDB table */
 	ret = mt7530_fdb_cmd(priv, MT7530_FDB_FLUSH, NULL);
 	if (ret < 0)
@@ -1283,7 +1389,20 @@ static void mt7530_phylink_mac_config(struct dsa_switch *ds, int port,
 		if (state->interface != PHY_INTERFACE_MODE_GMII)
 			goto unsupported;
 		break;
-	/* case 5: Port 5 is not supported! */
+	case 5: /* 2nd cpu port with phy of port 0 or 4 / external phy */
+		if (!phy_interface_mode_is_rgmii(state->interface) &&
+		    state->interface != PHY_INTERFACE_MODE_MII)
+			goto unsupported;
+		if (priv->p5_mode != P5_MODE_GMAC) {
+			priv->p5_mode = P5_MODE_GMAC;
+			mt7530_port_disable(ds, port);
+			mt7530_setup_port5(ds, state->interface);
+			mt7530_port_enable(ds, port, NULL);
+		}
+		/* We are connected to external phy */
+		if (dsa_is_user_port(ds, 5))
+			mcr |= PMCR_EXT_PHY;
+		break;
 	case 6: /* 1st cpu port */
 		if (state->interface != PHY_INTERFACE_MODE_RGMII &&
 		    state->interface != PHY_INTERFACE_MODE_TRGMII)
@@ -1364,7 +1483,12 @@ static void mt7530_phylink_validate(struct dsa_switch *ds, int port,
 		    state->interface != PHY_INTERFACE_MODE_GMII)
 			goto unsupported;
 		break;
-	/* case 5: Port 5 not supported! */
+	case 5: /* 2nd cpu port with phy of port 0 or 4 / external phy */
+		if (!phy_interface_mode_is_rgmii(state->interface) &&
+		    state->interface != PHY_INTERFACE_MODE_MII &&
+		    state->interface != PHY_INTERFACE_MODE_NA)
+			goto unsupported;
+		break;
 	case 6: /* 1st cpu port */
 		if (state->interface != PHY_INTERFACE_MODE_RGMII &&
 		    state->interface != PHY_INTERFACE_MODE_TRGMII)
@@ -1385,8 +1509,11 @@ static void mt7530_phylink_validate(struct dsa_switch *ds, int port,
 	phylink_set(mask, 10baseT_Full);
 	phylink_set(mask, 100baseT_Half);
 	phylink_set(mask, 100baseT_Full);
-	phylink_set(mask, 1000baseT_Full);
-	phylink_set(mask, 1000baseT_Half);
+
+	if (state->interface != PHY_INTERFACE_MODE_MII) {
+		phylink_set(mask, 1000baseT_Full);
+		phylink_set(mask, 1000baseT_Half);
+	}
 
 	linkmode_and(supported, supported, mask);
 	linkmode_and(state->advertising, state->advertising, mask);
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index 41d9a132ac70..f2a84ef48548 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -186,6 +186,7 @@ enum mt7530_vlan_port_attr {
 /* Register for port MAC control register */
 #define MT7530_PMCR_P(x)		(0x3000 + ((x) * 0x100))
 #define  PMCR_IFG_XMIT(x)		(((x) & 0x3) << 18)
+#define  PMCR_EXT_PHY			BIT(17)
 #define  PMCR_MAC_MODE			BIT(16)
 #define  PMCR_FORCE_MODE		BIT(15)
 #define  PMCR_TX_EN			BIT(14)
@@ -260,6 +261,7 @@ enum mt7530_vlan_port_attr {
 
 /* Register for hw trap modification */
 #define MT7530_MHWTRAP			0x7804
+#define  MHWTRAP_PHY0_SEL		BIT(20)
 #define  MHWTRAP_MANUAL			BIT(16)
 #define  MHWTRAP_P5_MAC_SEL		BIT(13)
 #define  MHWTRAP_P6_DIS			BIT(8)
@@ -417,6 +419,30 @@ struct mt7530_port {
 	u16 pvid;
 };
 
+/* Port 5 Mode definitions */
+enum p5_mode {
+	P5_MODE_DISABLED = 0,
+	P5_MODE_GPHY_P0,
+	P5_MODE_GPHY_P4,
+	P5_MODE_GMAC,
+};
+
+static const char *p5_modes(unsigned int p5_mode)
+{
+	switch (p5_mode) {
+	case P5_MODE_DISABLED:
+		return "DISABLED";
+	case P5_MODE_GPHY_P0:
+		return "PHY P0";
+	case P5_MODE_GPHY_P4:
+		return "PHY P4";
+	case P5_MODE_GMAC:
+		return "GMAC";
+	default:
+		return "unknown";
+	}
+}
+
 /* struct mt7530_priv -	This is the main data structure for holding the state
  *			of the driver
  * @dev:		The device pointer
@@ -432,6 +458,7 @@ struct mt7530_port {
  * @ports:		Holding the state among ports
  * @reg_mutex:		The lock for protecting among process accessing
  *			registers
+ * @p5_mode:		PORT 5 mode status
  */
 struct mt7530_priv {
 	struct device		*dev;
@@ -444,6 +471,7 @@ struct mt7530_priv {
 	struct gpio_desc	*reset;
 	unsigned int		id;
 	bool			mcm;
+	unsigned int		p5_mode;
 
 	struct mt7530_port	ports[MT7530_NUM_PORTS];
 	/* protect among processes for registers access*/
-- 
2.20.1

[PATCH RFC net-next 4/5] dt-bindings: net: dsa: mt7530: Add mediatek,ephy-handle to isolate ext. phy

[René van Dorst]

On some platforum the external phy can only interface to the port 5 of the
switch because the RGMII TX and RX lines are swapped. But it still can be
useful to use the internal phy of the switch to act as a WAN port which
connectes to the 2nd GMAC. This gives WAN port dedicated bandwidth to
the SOC. This increases the LAN and WAN routing.

By adding the optional property mediatek,ephy-handle, the external phy
is put in isolation mode when internal phy is connected to 2nd GMAC via
phy-handle property.

Signed-off-by: René van Dorst <opensource@vdorst.com>
---
 .../devicetree/bindings/net/dsa/mt7530.txt    | 116 +++++++++++++++++-
 1 file changed, 115 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/dsa/mt7530.txt b/Documentation/devicetree/bindings/net/dsa/mt7530.txt
index f3486780f2c2..1e79fba5a774 100644
--- a/Documentation/devicetree/bindings/net/dsa/mt7530.txt
+++ b/Documentation/devicetree/bindings/net/dsa/mt7530.txt
@@ -60,10 +60,20 @@ Depending on how the external PHY is wired:
 2. swapped: RGMII TX, RX are swapped; external phy interface with the switch as
    a ethernet port. But can't interface to the 2nd GMAC.
 
+Optional property:
+
+- mediatek,ephy-handle: Phandle of the external phy. In case you want to use
+			P0/4 as WAN port and have an external phy attached.
+			With this property the external phy is put in isolation
+			and powerdown mode in mode 2.
+
 Based on the DT the port 5 mode is configured.
 
 Driver tries to lookup the phy-handle of the 2nd GMAC of the master device.
-When phy-handle matches PHY of port 0 or 4 then port 5 set-up as mode 2.
+When phy-handle matches PHY of port 0 or 4 then port 5 set-up as mode 2 and when
+propertly "mediatek,ephy-handle" is valid it puts the externel phy in isolation
+mode.
+
 phy-mode must be set, see also example 2 below!
  * mt7621: phy-mode = "rgmii-txid";
  * mt7623: phy-mode = "rgmii";
@@ -309,3 +319,107 @@ Example 3: MT7621: Port 5 is connected to external PHY: Port 5 -> external PHY.
 		};
 	};
 };
+
+Example 4: MT7621: Port 4 is WAN port: 2nd GMAC -> P5 -> PHY P4
+		   with an external phy.
+
+&eth {
+	status = "okay";
+
+	gmac0: mac@0 {
+		compatible = "mediatek,eth-mac";
+		reg = <0>;
+		phy-mode = "rgmii";
+
+		fixed-link {
+			speed = <1000>;
+			full-duplex;
+			pause;
+		};
+	};
+
+	gmac1: mac@1 {
+		compatible = "mediatek,eth-mac";
+		reg = <1>;
+		phy-mode = "rgmii-txid";
+		phy-handle = <&phy4>;
+	};
+
+	mdio: mdio-bus {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		/* Internal phy 4 */
+		phy4: ethernet-phy@4 {
+			reg = <4>;
+		};
+
+		/* external phy addr 0x07 */
+		ephy5: ethernet-phy@7 {
+			reg = <7>;
+		};
+
+		mt7530: switch@1f {
+			compatible = "mediatek,mt7621";
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <0x1f>;
+			pinctrl-names = "default";
+			mediatek,mcm;
+
+			/* Put this external phy in power-down and isolation
+			 * when port 5 is used in PHY P0/P4 or DSA mode. Because
+			 * external phy and port 5 share same bus to 2nd GMAC.
+			 */
+			mediatek,ephy-handle = <&ephy5>;
+
+			resets = <&rstctrl 2>;
+			reset-names = "mcm";
+
+			ports {
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				port@0 {
+					reg = <0>;
+					label = "lan0";
+				};
+
+				port@1 {
+					reg = <1>;
+					label = "lan1";
+				};
+
+				port@2 {
+					reg = <2>;
+					label = "lan2";
+				};
+
+				port@3 {
+					reg = <3>;
+					label = "lan3";
+				};
+
+/* Commented out. Port 4 is handled by 2nd GMAC.
+				port@4 {
+					reg = <4>;
+					label = "lan4";
+				};
+*/
+
+				cpu_port0: port@6 {
+					reg = <6>;
+					label = "cpu";
+					ethernet = <&gmac0>;
+					phy-mode = "rgmii";
+
+					fixed-link {
+						speed = <1000>;
+						full-duplex;
+						pause;
+					};
+				};
+			};
+		};
+	};
+};
-- 
2.20.1

Re: [PATCH RFC net-next 4/5] dt-bindings: net: dsa: mt7530: Add mediatek,ephy-handle to isolate ext. phy

[Florian Fainelli]

On 6/24/19 7:52 AM, René van Dorst wrote:
> On some platforum the external phy can only interface to the port 5 of the
> switch because the RGMII TX and RX lines are swapped. But it still can be
> useful to use the internal phy of the switch to act as a WAN port which
> connectes to the 2nd GMAC. This gives WAN port dedicated bandwidth to
> the SOC. This increases the LAN and WAN routing.
> 
> By adding the optional property mediatek,ephy-handle, the external phy
> is put in isolation mode when internal phy is connected to 2nd GMAC via
> phy-handle property.

Most platforms we have seen so far implement this logic with a mdio-mux,
can you see if that is possible here? stmmac has plenty of examples like
those.
-- 
Florian

Re: [PATCH RFC net-next 4/5] dt-bindings: net: dsa: mt7530: Add mediatek,ephy-handle to isolate ext. phy

[René van Dorst]

Quoting Florian Fainelli <f.fainelli@gmail.com>:

Hi Florian

> On 6/24/19 7:52 AM, René van Dorst wrote:
>> On some platforum the external phy can only interface to the port 5 of the
>> switch because the RGMII TX and RX lines are swapped. But it still can be
>> useful to use the internal phy of the switch to act as a WAN port which
>> connectes to the 2nd GMAC. This gives WAN port dedicated bandwidth to
>> the SOC. This increases the LAN and WAN routing.
>>
>> By adding the optional property mediatek,ephy-handle, the external phy
>> is put in isolation mode when internal phy is connected to 2nd GMAC via
>> phy-handle property.
>
> Most platforms we have seen so far implement this logic with a mdio-mux,
> can you see if that is possible here? stmmac has plenty of examples like
> those.

May I don't understand it correctly, but all the devices are on the same MDIO
bus.
I tried to make a ASCII diagram to make it a bit more clear.

  +-------------------+                     
+----------------------------------------------+
  | SOC MT7621/3      |                    | MT7530 SWITCH              
                    |
  |   +-------------+ |(T)RGMII BUS        |  +-------------+    
INTERNAL                  |
  |   |  1st GMAC   <------------------------->  PORT 6     |   MII  
BUS  +----------+     |
  |   +-------------+ |                    |  +-------------+      
+------>  GMAC5   |     |
  |                   |                    |                      |     
   +----------+     |
  |   +-------------+ | RGMII BUS          |  +-------------+     |     
                    |
  |   |  2nd GMAC   <------------------+------>  PORT 5     +-----+     
   +----------+     |
  |   +-------------+ |                |   |  +-------------+      
+------>  PHY P0  +<--+ |
  |                   |                |   |                      |     
   +----------+   | |
  |   +-------------+ |  MDIO BUS      |   |                      |     
                  | |
  |   |  MDIO       +--------+         |   |                      |     
   +----------+   | |
  |   +-------------+ |      |         |   |                       
+------>  PHY P4  +<--+ |
  |                   |      |         |   |                            
   +----------+   | |
  +-------------------+      |         |    
+--------------------------------------------|-+
                             |         |       +-------------+          
                  |
                             |         |       |  EXTERNAL   |          
                  |
                             |         +------->  PHY AT8033 | SGMII  
BUS +----------+   |
                             |                 |              
+-----------> SFP CAGE |   |
                             +---------------->+             |          
   +----------+   |
                             |                 |             |          
                  |
                             |                 +-------------+          
                  |
                              
+----------------------------------------------------------+

I don't see why I need a MDIO mux. Devices; 2nd GMAC, external phy and port 5
share the same RGMII bus. Depending on the port 5 mux settings, port 5  
is acting
as a GMAC of PHY of port 0/4. As long as the unused devices doesn't drive the
RGMII bus we are good.

2nd GMAC RGMII control is currently set with a pinctrl "RGMII2".
Port 5 and external phy are done in mt7530_port5_setup() depending on the
device-tree.

Greats,

René

> --
> Florian

Re: [PATCH RFC net-next 4/5] dt-bindings: net: dsa: mt7530: Add mediatek,ephy-handle to isolate ext. phy

[Florian Fainelli]

On 6/25/19 2:30 AM, René van Dorst wrote:
> Quoting Florian Fainelli <f.fainelli@gmail.com>:
> 
> Hi Florian
> 
>> On 6/24/19 7:52 AM, René van Dorst wrote:
>>> On some platforum the external phy can only interface to the port 5
>>> of the
>>> switch because the RGMII TX and RX lines are swapped. But it still
>>> can be
>>> useful to use the internal phy of the switch to act as a WAN port which
>>> connectes to the 2nd GMAC. This gives WAN port dedicated bandwidth to
>>> the SOC. This increases the LAN and WAN routing.
>>>
>>> By adding the optional property mediatek,ephy-handle, the external phy
>>> is put in isolation mode when internal phy is connected to 2nd GMAC via
>>> phy-handle property.
>>
>> Most platforms we have seen so far implement this logic with a mdio-mux,
>> can you see if that is possible here? stmmac has plenty of examples like
>> those.
> 
> May I don't understand it correctly, but all the devices are on the same
> MDIO
> bus.
> I tried to make a ASCII diagram to make it a bit more clear.

Based on your diagram and your explanation, then I do not really see a
need for that property you can scan all of the switch port's properties
and determined which configuration is applied and perform the PHY
isolation as necessary. It looks like you are using this property as a
way to simplify your configuration logic, that is not quite what Device
Tree is meant for.
-- 
Florian

[PATCH RFC net-next 5/5] net: dsa: mt7530: Add mediatek,ephy-handle to isolate external phy

[René van Dorst]

On some platforms the external phy can only interface with the port 5 of
the switch because the xMII TX and RX lines are swapped. But it still can
be useful to use the internal phy of the switch to act as a WAN port which
connectes to the 2nd GMAC. This gives the SOC a double the bandwidth
between LAN and WAN. Because LAN and WAN don't share the same interface
anymore.

By adding an optional property mediatek,ephy-handle, the external phy
is put in isolation mode when internal phy is linked with 2nd GMAC via
phy-handle property.

Signed-off-by: René van Dorst <opensource@vdorst.com>
---
 drivers/net/dsa/mt7530.c | 28 ++++++++++++++++++++++++++++
 drivers/net/dsa/mt7530.h |  2 ++
 2 files changed, 30 insertions(+)

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 838a921ca83e..25b0f35df75b 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -633,6 +633,26 @@ mt7530_get_sset_count(struct dsa_switch *ds, int port, int sset)
 	return ARRAY_SIZE(mt7530_mib);
 }
 
+static int mt7530_isolate_ephy(struct dsa_switch *ds,
+			       struct device_node *ephy_node)
+{
+	struct phy_device *phydev = of_phy_find_device(ephy_node);
+	int ret;
+
+	if (!phydev)
+		return 0;
+
+	ret = phy_modify(phydev, MII_BMCR, 0, (BMCR_ISOLATE | BMCR_PDOWN));
+	if (ret)
+		dev_err(ds->dev, "Failed to put phy %s in isolation mode!\n",
+			ephy_node->full_name);
+	else
+		dev_info(ds->dev, "Phy %s in isolation mode!\n",
+			 ephy_node->full_name);
+
+	return ret;
+}
+
 static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
 {
 	struct mt7530_priv *priv = ds->priv;
@@ -655,6 +675,10 @@ static void mt7530_setup_port5(struct dsa_switch *ds, phy_interface_t interface)
 		/* MT7530_P5_MODE_GPHY_P4: 2nd GMAC -> P5 -> P4 */
 		val &= ~MHWTRAP_P5_MAC_SEL & ~MHWTRAP_P5_DIS;
 
+		/* Isolate the external phy */
+		if (priv->ephy_node)
+			if (mt7530_isolate_ephy(ds, priv->ephy_node) < 0)
+				goto unlock_exit;
 		/* Setup the MAC by default for the cpu port */
 		mt7530_write(priv, MT7530_PMCR_P(5), 0x56300);
 		break;
@@ -1330,6 +1354,10 @@ mt7530_setup(struct dsa_switch *ds)
 			mt7530_port_disable(ds, i);
 	}
 
+	/* Get external phy phandle */
+	priv->ephy_node = of_parse_phandle(priv->dev->of_node,
+					   "mediatek,ephy-handle", 0);
+
 	/* Setup port 5 */
 	priv->p5_mode = P5_MODE_DISABLED;
 	interface = PHY_INTERFACE_MODE_NA;
diff --git a/drivers/net/dsa/mt7530.h b/drivers/net/dsa/mt7530.h
index f2a84ef48548..eb079e81a8e8 100644
--- a/drivers/net/dsa/mt7530.h
+++ b/drivers/net/dsa/mt7530.h
@@ -459,6 +459,7 @@ static const char *p5_modes(unsigned int p5_mode)
  * @reg_mutex:		The lock for protecting among process accessing
  *			registers
  * @p5_mode:		PORT 5 mode status
+ * @ephy_node:		External phy of_node.
  */
 struct mt7530_priv {
 	struct device		*dev;
@@ -472,6 +473,7 @@ struct mt7530_priv {
 	unsigned int		id;
 	bool			mcm;
 	unsigned int		p5_mode;
+	struct device_node	*ephy_node;
 
 	struct mt7530_port	ports[MT7530_NUM_PORTS];
 	/* protect among processes for registers access*/
-- 
2.20.1

Re: [PATCH RFC net-next 5/5] net: dsa: mt7530: Add mediatek,ephy-handle to isolate external phy

[Andrew Lunn]

> +static int mt7530_isolate_ephy(struct dsa_switch *ds,
> +			       struct device_node *ephy_node)
> +{
> +	struct phy_device *phydev = of_phy_find_device(ephy_node);
> +	int ret;
> +
> +	if (!phydev)
> +		return 0;
> +
> +	ret = phy_modify(phydev, MII_BMCR, 0, (BMCR_ISOLATE | BMCR_PDOWN));

genphy_suspend() does what you want.

> +	if (ret)
> +		dev_err(ds->dev, "Failed to put phy %s in isolation mode!\n",
> +			ephy_node->full_name);
> +	else
> +		dev_info(ds->dev, "Phy %s in isolation mode!\n",
> +			 ephy_node->full_name);

No need to clog up the system with yet more kernel messages.

   Andrew

Re: [PATCH RFC net-next 5/5] net: dsa: mt7530: Add mediatek,ephy-handle to isolate external phy

[Daniel Santos]

On 6/24/19 4:52 PM, Andrew Lunn wrote:
>> +static int mt7530_isolate_ephy(struct dsa_switch *ds,
>> +			       struct device_node *ephy_node)
>> +{
>> +	struct phy_device *phydev = of_phy_find_device(ephy_node);
>> +	int ret;
>> +
>> +	if (!phydev)
>> +		return 0;
>> +
>> +	ret = phy_modify(phydev, MII_BMCR, 0, (BMCR_ISOLATE | BMCR_PDOWN));
> genphy_suspend() does what you want.
>
>> +	if (ret)
>> +		dev_err(ds->dev, "Failed to put phy %s in isolation mode!\n",
>> +			ephy_node->full_name);
>> +	else
>> +		dev_info(ds->dev, "Phy %s in isolation mode!\n",
>> +			 ephy_node->full_name);
> No need to clog up the system with yet more kernel messages.
>
>    Andrew
>
Yes, keep in mind that many mt7530-based devices have a 56k serial
console that gets ring buffer spew.  This created a real problem on the
mt7620 wifi drivers when they spewed every time a packet needed to be
dropped.  So at the very least, for any message that can be spammed,
rate limit it please.

Daniel

Re: [PATCH RFC net-next 5/5] net: dsa: mt7530: Add mediatek,ephy-handle to isolate external phy

[René van Dorst]

Quoting Andrew Lunn <andrew@lunn.ch>:

Hi Andrew,

>> +static int mt7530_isolate_ephy(struct dsa_switch *ds,
>> +			       struct device_node *ephy_node)
>> +{
>> +	struct phy_device *phydev = of_phy_find_device(ephy_node);
>> +	int ret;
>> +
>> +	if (!phydev)
>> +		return 0;
>> +
>> +	ret = phy_modify(phydev, MII_BMCR, 0, (BMCR_ISOLATE | BMCR_PDOWN));
>
> genphy_suspend() does what you want.

In case my device has AT8033 PHY which act as a RGMII-to-SGMII  
converter for the
SFP cage.

Qoute of the AR8031/33 datasheet:

The AR8033 device supports the low power mode with software power-down.
To enter the standard IEEE power-down mode, set the bit[11] POWER_DOWN of
Control register - copper page or Control register — fiber page to 1.
In this mode, AR8033 ignores all MAC interface signals except the MDC/MDIO and
does not respond to any activity on the media side. AR8033 cannot wake  
up on its
own and is only waken up by setting the POWER_DOWN bit to 0.


Does "standard IEEE power-down mode" describ this behavior that in power-down
mode the RGMII are also put in tri-state?

Reading the datasheet does not give me any clues.
Putting RGMII signals in tri-state is important in this case.

>
>> +	if (ret)
>> +		dev_err(ds->dev, "Failed to put phy %s in isolation mode!\n",
>> +			ephy_node->full_name);
>> +	else
>> +		dev_info(ds->dev, "Phy %s in isolation mode!\n",
>> +			 ephy_node->full_name);
>
> No need to clog up the system with yet more kernel messages.

OK, I remove it.

>
>    Andrew

Greats,

René