net: phy: phy drivers should not set SUPPORTED_Pause or SUPPORTED_Asym_Pause

RFC PATCH In review Timur Tabi 2 participants 10 messages Nov 1, 2016 – Nov 9, 2016

Subsystems (5)

Networking Network drivers Ethernet PHY Broadcom iproc Arm architecture +1 more subsystems

lore

Share & tools

lore

Timur Tabi

author

2016-11-01 17:19 UTC #0 lore raw

RFC PATCH 1/1 net: phy: phy drivers should not set SUPPORTED_Pause or SUPPORTED_Asym_Pause

Pause frames are a feature that is supported by the MAC. It is the MAC that generates the frames and that processes them. Thy PHY can only be configured to allow them to pass. Therefore, pause frame support in the PHY should only be enabled if the MAC wants it enabled. It should not be forced on all the time.

Signed-off-by Timur Tabi <[email protected]>

---
 drivers/net/phy/bcm-cygnus.c |  3 +--
 drivers/net/phy/bcm63xx.c    |  5 ++---
 drivers/net/phy/bcm7xxx.c    |  6 ++----
 drivers/net/phy/broadcom.c   | 39 +++++++++++++--------------------------
 drivers/net/phy/icplus.c     |  6 ++----
 drivers/net/phy/intel-xway.c | 24 ++++++++----------------
 drivers/net/phy/micrel.c     | 30 ++++++++++++------------------
 drivers/net/phy/microchip.c  |  3 +--
 drivers/net/phy/national.c   |  2 +-
 drivers/net/phy/smsc.c       | 18 ++++++------------
 drivers/net/phy/ste10Xp.c    |  4 ++--
 11 files changed, 50 insertions(+), 90 deletions(-)

drivers/net/phy/bcm-cygnus.c +1 -2

index 49bbc68..12c6d49 100644
@@ -134,8 +134,7 @@ static int bcm_cygnus_resume(struct phy_device *phydev)
 	.phy_id        = PHY_ID_BCM_CYGNUS,
 	.phy_id_mask   = 0xfffffff0,
 	.name          = "Broadcom Cygnus PHY",
-	.features      = PHY_GBIT_FEATURES |
-			SUPPORTED_Pause | SUPPORTED_Asym_Pause,
+	.features      = PHY_GBIT_FEATURES,
 	.config_init   = bcm_cygnus_config_init,
 	.config_aneg   = genphy_config_aneg,
 	.read_status   = genphy_read_status,

drivers/net/phy/bcm63xx.c +2 -3

index e741bf6..5e9922e 100644
@@ -48,8 +48,7 @@ static int bcm63xx_config_init(struct phy_device *phydev)
 	.phy_id		= 0x00406000,
 	.phy_id_mask	= 0xfffffc00,
 	.name		= "Broadcom BCM63XX (1)",
-	/* ASYM_PAUSE bit is marked RO in datasheet, so don't cheat */
-	.features	= (PHY_BASIC_FEATURES | SUPPORTED_Pause),
+	.features	= PHY_BASIC_FEATURES,
 	.flags		= PHY_HAS_INTERRUPT | PHY_IS_INTERNAL,
 	.config_init	= bcm63xx_config_init,
 	.config_aneg	= genphy_config_aneg,

@@ -61,7 +60,7 @@ static int bcm63xx_config_init(struct phy_device *phydev)
 	.phy_id		= 0x002bdc00,
 	.phy_id_mask	= 0xfffffc00,
 	.name		= "Broadcom BCM63XX (2)",
-	.features	= (PHY_BASIC_FEATURES | SUPPORTED_Pause),
+	.features	= PHY_BASIC_FEATURES,
 	.flags		= PHY_HAS_INTERRUPT | PHY_IS_INTERNAL,
 	.config_init	= bcm63xx_config_init,
 	.config_aneg	= genphy_config_aneg,

drivers/net/phy/bcm7xxx.c +2 -4

index 9636da0..c1629df 100644
@@ -308,8 +308,7 @@ static int bcm7xxx_suspend(struct phy_device *phydev)
 	.phy_id		= (_oui),					\
 	.phy_id_mask	= 0xfffffff0,					\
 	.name		= _name,					\
-	.features	= PHY_GBIT_FEATURES |				\
-			  SUPPORTED_Pause | SUPPORTED_Asym_Pause,	\
+	.features	= PHY_GBIT_FEATURES,				\
 	.flags		= PHY_IS_INTERNAL,				\
 	.config_init	= bcm7xxx_28nm_config_init,			\
 	.config_aneg	= genphy_config_aneg,				\

@@ -322,8 +321,7 @@ static int bcm7xxx_suspend(struct phy_device *phydev)
 	.phy_id         = (_oui),					\
 	.phy_id_mask    = 0xfffffff0,					\
 	.name           = _name,					\
-	.features       = PHY_BASIC_FEATURES |				\
-			  SUPPORTED_Pause | SUPPORTED_Asym_Pause,	\
+	.features       = PHY_BASIC_FEATURES,				\
 	.flags          = PHY_IS_INTERNAL,				\
 	.config_init    = bcm7xxx_config_init,				\
 	.config_aneg    = genphy_config_aneg,				\

drivers/net/phy/broadcom.c +13 ~~-26~~

index 583ef8a..1d5ee5f 100644
@@ -487,8 +487,7 @@ static int brcm_fet_config_intr(struct phy_device *phydev)
 	.phy_id		= PHY_ID_BCM5411,
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "Broadcom BCM5411",
-	.features	= PHY_GBIT_FEATURES |
-			  SUPPORTED_Pause | SUPPORTED_Asym_Pause,
+	.features	= PHY_GBIT_FEATURES,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.config_init	= bcm54xx_config_init,
 	.config_aneg	= genphy_config_aneg,

@@ -499,8 +498,7 @@ static int brcm_fet_config_intr(struct phy_device *phydev)
 	.phy_id		= PHY_ID_BCM5421,
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "Broadcom BCM5421",
-	.features	= PHY_GBIT_FEATURES |
-			  SUPPORTED_Pause | SUPPORTED_Asym_Pause,
+	.features	= PHY_GBIT_FEATURES,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.config_init	= bcm54xx_config_init,
 	.config_aneg	= genphy_config_aneg,

@@ -511,8 +509,7 @@ static int brcm_fet_config_intr(struct phy_device *phydev)
 	.phy_id		= PHY_ID_BCM5461,
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "Broadcom BCM5461",
-	.features	= PHY_GBIT_FEATURES |
-			  SUPPORTED_Pause | SUPPORTED_Asym_Pause,
+	.features	= PHY_GBIT_FEATURES,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.config_init	= bcm54xx_config_init,
 	.config_aneg	= genphy_config_aneg,

@@ -523,8 +520,7 @@ static int brcm_fet_config_intr(struct phy_device *phydev)
 	.phy_id		= PHY_ID_BCM54612E,
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "Broadcom BCM54612E",
-	.features	= PHY_GBIT_FEATURES |
-			  SUPPORTED_Pause | SUPPORTED_Asym_Pause,
+	.features	= PHY_GBIT_FEATURES,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.config_init	= bcm54xx_config_init,
 	.config_aneg	= bcm54612e_config_aneg,

@@ -535,8 +531,7 @@ static int brcm_fet_config_intr(struct phy_device *phydev)
 	.phy_id		= PHY_ID_BCM54616S,
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "Broadcom BCM54616S",
-	.features	= PHY_GBIT_FEATURES |
-			  SUPPORTED_Pause | SUPPORTED_Asym_Pause,
+	.features	= PHY_GBIT_FEATURES,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.config_init	= bcm54xx_config_init,
 	.config_aneg	= genphy_config_aneg,

@@ -547,8 +542,7 @@ static int brcm_fet_config_intr(struct phy_device *phydev)
 	.phy_id		= PHY_ID_BCM5464,
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "Broadcom BCM5464",
-	.features	= PHY_GBIT_FEATURES |
-			  SUPPORTED_Pause | SUPPORTED_Asym_Pause,
+	.features	= PHY_GBIT_FEATURES,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.config_init	= bcm54xx_config_init,
 	.config_aneg	= genphy_config_aneg,

@@ -559,8 +553,7 @@ static int brcm_fet_config_intr(struct phy_device *phydev)
 	.phy_id		= PHY_ID_BCM5481,
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "Broadcom BCM5481",
-	.features	= PHY_GBIT_FEATURES |
-			  SUPPORTED_Pause | SUPPORTED_Asym_Pause,
+	.features	= PHY_GBIT_FEATURES,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.config_init	= bcm54xx_config_init,
 	.config_aneg	= bcm5481_config_aneg,

@@ -571,8 +564,7 @@ static int brcm_fet_config_intr(struct phy_device *phydev)
 	.phy_id		= PHY_ID_BCM5482,
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "Broadcom BCM5482",
-	.features	= PHY_GBIT_FEATURES |
-			  SUPPORTED_Pause | SUPPORTED_Asym_Pause,
+	.features	= PHY_GBIT_FEATURES,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.config_init	= bcm5482_config_init,
 	.config_aneg	= genphy_config_aneg,

@@ -583,8 +575,7 @@ static int brcm_fet_config_intr(struct phy_device *phydev)
 	.phy_id		= PHY_ID_BCM50610,
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "Broadcom BCM50610",
-	.features	= PHY_GBIT_FEATURES |
-			  SUPPORTED_Pause | SUPPORTED_Asym_Pause,
+	.features	= PHY_GBIT_FEATURES,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.config_init	= bcm54xx_config_init,
 	.config_aneg	= genphy_config_aneg,

@@ -595,8 +586,7 @@ static int brcm_fet_config_intr(struct phy_device *phydev)
 	.phy_id		= PHY_ID_BCM50610M,
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "Broadcom BCM50610M",
-	.features	= PHY_GBIT_FEATURES |
-			  SUPPORTED_Pause | SUPPORTED_Asym_Pause,
+	.features	= PHY_GBIT_FEATURES,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.config_init	= bcm54xx_config_init,
 	.config_aneg	= genphy_config_aneg,

@@ -607,8 +597,7 @@ static int brcm_fet_config_intr(struct phy_device *phydev)
 	.phy_id		= PHY_ID_BCM57780,
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "Broadcom BCM57780",
-	.features	= PHY_GBIT_FEATURES |
-			  SUPPORTED_Pause | SUPPORTED_Asym_Pause,
+	.features	= PHY_GBIT_FEATURES,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.config_init	= bcm54xx_config_init,
 	.config_aneg	= genphy_config_aneg,

@@ -619,8 +608,7 @@ static int brcm_fet_config_intr(struct phy_device *phydev)
 	.phy_id		= PHY_ID_BCMAC131,
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "Broadcom BCMAC131",
-	.features	= PHY_BASIC_FEATURES |
-			  SUPPORTED_Pause | SUPPORTED_Asym_Pause,
+	.features	= PHY_BASIC_FEATURES,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.config_init	= brcm_fet_config_init,
 	.config_aneg	= genphy_config_aneg,

@@ -631,8 +619,7 @@ static int brcm_fet_config_intr(struct phy_device *phydev)
 	.phy_id		= PHY_ID_BCM5241,
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "Broadcom BCM5241",
-	.features	= PHY_BASIC_FEATURES |
-			  SUPPORTED_Pause | SUPPORTED_Asym_Pause,
+	.features	= PHY_BASIC_FEATURES,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.config_init	= brcm_fet_config_init,
 	.config_aneg	= genphy_config_aneg,

drivers/net/phy/icplus.c +2 -4

index e5f251b..22b51f0 100644
@@ -225,8 +225,7 @@ static int ip101a_g_ack_interrupt(struct phy_device *phydev)
 	.phy_id		= 0x02430d90,
 	.name		= "ICPlus IP1001",
 	.phy_id_mask	= 0x0ffffff0,
-	.features	= PHY_GBIT_FEATURES | SUPPORTED_Pause |
-			  SUPPORTED_Asym_Pause,
+	.features	= PHY_GBIT_FEATURES,
 	.config_init	= &ip1001_config_init,
 	.config_aneg	= &genphy_config_aneg,
 	.read_status	= &genphy_read_status,

@@ -236,8 +235,7 @@ static int ip101a_g_ack_interrupt(struct phy_device *phydev)
 	.phy_id		= 0x02430c54,
 	.name		= "ICPlus IP101A/G",
 	.phy_id_mask	= 0x0ffffff0,
-	.features	= PHY_BASIC_FEATURES | SUPPORTED_Pause |
-			  SUPPORTED_Asym_Pause,
+	.features	= PHY_BASIC_FEATURES,
 	.flags		= PHY_HAS_INTERRUPT,
 	.ack_interrupt	= ip101a_g_ack_interrupt,
 	.config_init	= &ip101a_g_config_init,

drivers/net/phy/intel-xway.c +8 ~~-16~~

index c300ab5..b1fd7bb 100644
@@ -239,8 +239,7 @@ static int xway_gphy_config_intr(struct phy_device *phydev)
 		.phy_id		= PHY_ID_PHY11G_1_3,
 		.phy_id_mask	= 0xffffffff,
 		.name		= "Intel XWAY PHY11G (PEF 7071/PEF 7072) v1.3",
-		.features	= (PHY_GBIT_FEATURES | SUPPORTED_Pause |
-				   SUPPORTED_Asym_Pause),
+		.features	= PHY_GBIT_FEATURES,
 		.flags		= PHY_HAS_INTERRUPT,
 		.config_init	= xway_gphy_config_init,
 		.config_aneg	= xway_gphy14_config_aneg,

@@ -254,8 +253,7 @@ static int xway_gphy_config_intr(struct phy_device *phydev)
 		.phy_id		= PHY_ID_PHY22F_1_3,
 		.phy_id_mask	= 0xffffffff,
 		.name		= "Intel XWAY PHY22F (PEF 7061) v1.3",
-		.features	= (PHY_BASIC_FEATURES | SUPPORTED_Pause |
-				   SUPPORTED_Asym_Pause),
+		.features	= PHY_BASIC_FEATURES,
 		.flags		= PHY_HAS_INTERRUPT,
 		.config_init	= xway_gphy_config_init,
 		.config_aneg	= xway_gphy14_config_aneg,

@@ -269,8 +267,7 @@ static int xway_gphy_config_intr(struct phy_device *phydev)
 		.phy_id		= PHY_ID_PHY11G_1_4,
 		.phy_id_mask	= 0xffffffff,
 		.name		= "Intel XWAY PHY11G (PEF 7071/PEF 7072) v1.4",
-		.features	= (PHY_GBIT_FEATURES | SUPPORTED_Pause |
-				   SUPPORTED_Asym_Pause),
+		.features	= PHY_GBIT_FEATURES,
 		.flags		= PHY_HAS_INTERRUPT,
 		.config_init	= xway_gphy_config_init,
 		.config_aneg	= xway_gphy14_config_aneg,

@@ -284,8 +281,7 @@ static int xway_gphy_config_intr(struct phy_device *phydev)
 		.phy_id		= PHY_ID_PHY22F_1_4,
 		.phy_id_mask	= 0xffffffff,
 		.name		= "Intel XWAY PHY22F (PEF 7061) v1.4",
-		.features	= (PHY_BASIC_FEATURES | SUPPORTED_Pause |
-				   SUPPORTED_Asym_Pause),
+		.features	= PHY_BASIC_FEATURES,
 		.flags		= PHY_HAS_INTERRUPT,
 		.config_init	= xway_gphy_config_init,
 		.config_aneg	= xway_gphy14_config_aneg,

@@ -299,8 +295,7 @@ static int xway_gphy_config_intr(struct phy_device *phydev)
 		.phy_id		= PHY_ID_PHY11G_1_5,
 		.phy_id_mask	= 0xffffffff,
 		.name		= "Intel XWAY PHY11G (PEF 7071/PEF 7072) v1.5 / v1.6",
-		.features	= (PHY_GBIT_FEATURES | SUPPORTED_Pause |
-				   SUPPORTED_Asym_Pause),
+		.features	= PHY_GBIT_FEATURES,
 		.flags		= PHY_HAS_INTERRUPT,
 		.config_init	= xway_gphy_config_init,
 		.config_aneg	= genphy_config_aneg,

@@ -314,8 +309,7 @@ static int xway_gphy_config_intr(struct phy_device *phydev)
 		.phy_id		= PHY_ID_PHY22F_1_5,
 		.phy_id_mask	= 0xffffffff,
 		.name		= "Intel XWAY PHY22F (PEF 7061) v1.5 / v1.6",
-		.features	= (PHY_BASIC_FEATURES | SUPPORTED_Pause |
-				   SUPPORTED_Asym_Pause),
+		.features	= PHY_BASIC_FEATURES,
 		.flags		= PHY_HAS_INTERRUPT,
 		.config_init	= xway_gphy_config_init,
 		.config_aneg	= genphy_config_aneg,

@@ -329,8 +323,7 @@ static int xway_gphy_config_intr(struct phy_device *phydev)
 		.phy_id		= PHY_ID_PHY11G_VR9,
 		.phy_id_mask	= 0xffffffff,
 		.name		= "Intel XWAY PHY11G (xRX integrated)",
-		.features	= (PHY_GBIT_FEATURES | SUPPORTED_Pause |
-				   SUPPORTED_Asym_Pause),
+		.features	= PHY_GBIT_FEATURES,
 		.flags		= PHY_HAS_INTERRUPT,
 		.config_init	= xway_gphy_config_init,
 		.config_aneg	= genphy_config_aneg,

@@ -344,8 +337,7 @@ static int xway_gphy_config_intr(struct phy_device *phydev)
 		.phy_id		= PHY_ID_PHY22F_VR9,
 		.phy_id_mask	= 0xffffffff,
 		.name		= "Intel XWAY PHY22F (xRX integrated)",
-		.features	= (PHY_BASIC_FEATURES | SUPPORTED_Pause |
-				   SUPPORTED_Asym_Pause),
+		.features	= PHY_BASIC_FEATURES,
 		.flags		= PHY_HAS_INTERRUPT,
 		.config_init	= xway_gphy_config_init,
 		.config_aneg	= genphy_config_aneg,

drivers/net/phy/micrel.c +12 ~~-18~~

index 081df68..76cc727 100644
@@ -790,7 +790,7 @@ static int kszphy_probe(struct phy_device *phydev)
 	.phy_id		= PHY_ID_KS8737,
 	.phy_id_mask	= MICREL_PHY_ID_MASK,
 	.name		= "Micrel KS8737",
-	.features	= (PHY_BASIC_FEATURES | SUPPORTED_Pause),
+	.features	= PHY_BASIC_FEATURES,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.driver_data	= &ks8737_type,
 	.config_init	= kszphy_config_init,

@@ -807,8 +807,7 @@ static int kszphy_probe(struct phy_device *phydev)
 	.phy_id		= PHY_ID_KSZ8021,
 	.phy_id_mask	= 0x00ffffff,
 	.name		= "Micrel KSZ8021 or KSZ8031",
-	.features	= (PHY_BASIC_FEATURES | SUPPORTED_Pause |
-			   SUPPORTED_Asym_Pause),
+	.features	= PHY_BASIC_FEATURES,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.driver_data	= &ksz8021_type,
 	.probe		= kszphy_probe,

@@ -826,8 +825,7 @@ static int kszphy_probe(struct phy_device *phydev)
 	.phy_id		= PHY_ID_KSZ8031,
 	.phy_id_mask	= 0x00ffffff,
 	.name		= "Micrel KSZ8031",
-	.features	= (PHY_BASIC_FEATURES | SUPPORTED_Pause |
-			   SUPPORTED_Asym_Pause),
+	.features	= PHY_BASIC_FEATURES,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.driver_data	= &ksz8021_type,
 	.probe		= kszphy_probe,

@@ -845,8 +843,7 @@ static int kszphy_probe(struct phy_device *phydev)
 	.phy_id		= PHY_ID_KSZ8041,
 	.phy_id_mask	= MICREL_PHY_ID_MASK,
 	.name		= "Micrel KSZ8041",
-	.features	= (PHY_BASIC_FEATURES | SUPPORTED_Pause
-				| SUPPORTED_Asym_Pause),
+	.features	= PHY_BASIC_FEATURES,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.driver_data	= &ksz8041_type,
 	.probe		= kszphy_probe,

@@ -864,8 +861,7 @@ static int kszphy_probe(struct phy_device *phydev)
 	.phy_id		= PHY_ID_KSZ8041RNLI,
 	.phy_id_mask	= MICREL_PHY_ID_MASK,
 	.name		= "Micrel KSZ8041RNLI",
-	.features	= PHY_BASIC_FEATURES |
-			  SUPPORTED_Pause | SUPPORTED_Asym_Pause,
+	.features	= PHY_BASIC_FEATURES,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.driver_data	= &ksz8041_type,
 	.probe		= kszphy_probe,

@@ -883,8 +879,7 @@ static int kszphy_probe(struct phy_device *phydev)
 	.phy_id		= PHY_ID_KSZ8051,
 	.phy_id_mask	= MICREL_PHY_ID_MASK,
 	.name		= "Micrel KSZ8051",
-	.features	= (PHY_BASIC_FEATURES | SUPPORTED_Pause
-				| SUPPORTED_Asym_Pause),
+	.features	= PHY_BASIC_FEATURES,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.driver_data	= &ksz8051_type,
 	.probe		= kszphy_probe,

@@ -902,7 +897,7 @@ static int kszphy_probe(struct phy_device *phydev)
 	.phy_id		= PHY_ID_KSZ8001,
 	.name		= "Micrel KSZ8001 or KS8721",
 	.phy_id_mask	= 0x00fffffc,
-	.features	= (PHY_BASIC_FEATURES | SUPPORTED_Pause),
+	.features	= PHY_BASIC_FEATURES,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.driver_data	= &ksz8041_type,
 	.probe		= kszphy_probe,

@@ -920,7 +915,7 @@ static int kszphy_probe(struct phy_device *phydev)
 	.phy_id		= PHY_ID_KSZ8081,
 	.name		= "Micrel KSZ8081 or KSZ8091",
 	.phy_id_mask	= MICREL_PHY_ID_MASK,
-	.features	= (PHY_BASIC_FEATURES | SUPPORTED_Pause),
+	.features	= PHY_BASIC_FEATURES,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.driver_data	= &ksz8081_type,
 	.probe		= kszphy_probe,

@@ -938,7 +933,7 @@ static int kszphy_probe(struct phy_device *phydev)
 	.phy_id		= PHY_ID_KSZ8061,
 	.name		= "Micrel KSZ8061",
 	.phy_id_mask	= MICREL_PHY_ID_MASK,
-	.features	= (PHY_BASIC_FEATURES | SUPPORTED_Pause),
+	.features	= PHY_BASIC_FEATURES,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.config_init	= kszphy_config_init,
 	.config_aneg	= genphy_config_aneg,

@@ -954,7 +949,7 @@ static int kszphy_probe(struct phy_device *phydev)
 	.phy_id		= PHY_ID_KSZ9021,
 	.phy_id_mask	= 0x000ffffe,
 	.name		= "Micrel KSZ9021 Gigabit PHY",
-	.features	= (PHY_GBIT_FEATURES | SUPPORTED_Pause),
+	.features	= PHY_GBIT_FEATURES,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.driver_data	= &ksz9021_type,
 	.config_init	= ksz9021_config_init,

@@ -973,7 +968,7 @@ static int kszphy_probe(struct phy_device *phydev)
 	.phy_id		= PHY_ID_KSZ9031,
 	.phy_id_mask	= MICREL_PHY_ID_MASK,
 	.name		= "Micrel KSZ9031 Gigabit PHY",
-	.features	= (PHY_GBIT_FEATURES | SUPPORTED_Pause),
+	.features	= PHY_GBIT_FEATURES,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.driver_data	= &ksz9021_type,
 	.config_init	= ksz9031_config_init,

@@ -990,7 +985,6 @@ static int kszphy_probe(struct phy_device *phydev)
 	.phy_id		= PHY_ID_KSZ8873MLL,
 	.phy_id_mask	= MICREL_PHY_ID_MASK,
 	.name		= "Micrel KSZ8873MLL Switch",
-	.features	= (SUPPORTED_Pause | SUPPORTED_Asym_Pause),
 	.flags		= PHY_HAS_MAGICANEG,
 	.config_init	= kszphy_config_init,
 	.config_aneg	= ksz8873mll_config_aneg,

@@ -1004,7 +998,7 @@ static int kszphy_probe(struct phy_device *phydev)
 	.phy_id		= PHY_ID_KSZ886X,
 	.phy_id_mask	= MICREL_PHY_ID_MASK,
 	.name		= "Micrel KSZ886X Switch",
-	.features	= (PHY_BASIC_FEATURES | SUPPORTED_Pause),
+	.features	= PHY_BASIC_FEATURES,
 	.flags		= PHY_HAS_MAGICANEG | PHY_HAS_INTERRUPT,
 	.config_init	= kszphy_config_init,
 	.config_aneg	= genphy_config_aneg,

drivers/net/phy/microchip.c +1 -2

index 7c00e50..4c83229 100644
@@ -112,8 +112,7 @@ static int lan88xx_set_wol(struct phy_device *phydev,
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "Microchip LAN88xx",
 
-	.features	= (PHY_GBIT_FEATURES |
-			   SUPPORTED_Pause | SUPPORTED_Asym_Pause),
+	.features	= PHY_GBIT_FEATURES,
 	.flags		= PHY_HAS_INTERRUPT | PHY_HAS_MAGICANEG,
 
 	.probe		= lan88xx_probe,

drivers/net/phy/national.c +1 -1

index 2a1b490..2addf1d 100644
@@ -133,7 +133,7 @@ static int ns_config_init(struct phy_device *phydev)
 	.phy_id = DP83865_PHY_ID,
 	.phy_id_mask = 0xfffffff0,
 	.name = "NatSemi DP83865",
-	.features = PHY_GBIT_FEATURES | SUPPORTED_Pause | SUPPORTED_Asym_Pause,
+	.features = PHY_GBIT_FEATURES,
 	.flags = PHY_HAS_INTERRUPT,
 	.config_init = ns_config_init,
 	.config_aneg = genphy_config_aneg,

drivers/net/phy/smsc.c +6 ~~-12~~

index b62c4aa..fb32eaf 100644
@@ -168,8 +168,7 @@ static int smsc_phy_probe(struct phy_device *phydev)
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "SMSC LAN83C185",
 
-	.features	= (PHY_BASIC_FEATURES | SUPPORTED_Pause
-				| SUPPORTED_Asym_Pause),
+	.features	= PHY_BASIC_FEATURES,
 	.flags		= PHY_HAS_INTERRUPT | PHY_HAS_MAGICANEG,
 
 	.probe		= smsc_phy_probe,

@@ -191,8 +190,7 @@ static int smsc_phy_probe(struct phy_device *phydev)
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "SMSC LAN8187",
 
-	.features	= (PHY_BASIC_FEATURES | SUPPORTED_Pause
-				| SUPPORTED_Asym_Pause),
+	.features	= PHY_BASIC_FEATURES,
 	.flags		= PHY_HAS_INTERRUPT | PHY_HAS_MAGICANEG,
 
 	.probe		= smsc_phy_probe,

@@ -214,8 +212,7 @@ static int smsc_phy_probe(struct phy_device *phydev)
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "SMSC LAN8700",
 
-	.features	= (PHY_BASIC_FEATURES | SUPPORTED_Pause
-				| SUPPORTED_Asym_Pause),
+	.features	= PHY_BASIC_FEATURES,
 	.flags		= PHY_HAS_INTERRUPT | PHY_HAS_MAGICANEG,
 
 	.probe		= smsc_phy_probe,

@@ -237,8 +234,7 @@ static int smsc_phy_probe(struct phy_device *phydev)
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "SMSC LAN911x Internal PHY",
 
-	.features	= (PHY_BASIC_FEATURES | SUPPORTED_Pause
-				| SUPPORTED_Asym_Pause),
+	.features	= PHY_BASIC_FEATURES,
 	.flags		= PHY_HAS_INTERRUPT | PHY_HAS_MAGICANEG,
 
 	.probe		= smsc_phy_probe,

@@ -259,8 +255,7 @@ static int smsc_phy_probe(struct phy_device *phydev)
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "SMSC LAN8710/LAN8720",
 
-	.features	= (PHY_BASIC_FEATURES | SUPPORTED_Pause
-				| SUPPORTED_Asym_Pause),
+	.features	= PHY_BASIC_FEATURES,
 	.flags		= PHY_HAS_INTERRUPT | PHY_HAS_MAGICANEG,
 
 	.probe		= smsc_phy_probe,

@@ -282,8 +277,7 @@ static int smsc_phy_probe(struct phy_device *phydev)
 	.phy_id_mask	= 0xfffffff0,
 	.name		= "SMSC LAN8740",
 
-	.features	= (PHY_BASIC_FEATURES | SUPPORTED_Pause
-				| SUPPORTED_Asym_Pause),
+	.features	= PHY_BASIC_FEATURES,
 	.flags		= PHY_HAS_INTERRUPT | PHY_HAS_MAGICANEG,
 
 	.probe		= smsc_phy_probe,

drivers/net/phy/ste10Xp.c +2 -2

index d00cfb6..1af1a4a 100644
@@ -86,7 +86,7 @@ static int ste10Xp_ack_interrupt(struct phy_device *phydev)
 	.phy_id = STE101P_PHY_ID,
 	.phy_id_mask = 0xfffffff0,
 	.name = "STe101p",
-	.features = PHY_BASIC_FEATURES | SUPPORTED_Pause,
+	.features = PHY_BASIC_FEATURES,
 	.flags = PHY_HAS_INTERRUPT,
 	.config_init = ste10Xp_config_init,
 	.config_aneg = genphy_config_aneg,

@@ -99,7 +99,7 @@ static int ste10Xp_ack_interrupt(struct phy_device *phydev)
 	.phy_id = STE100P_PHY_ID,
 	.phy_id_mask = 0xffffffff,
 	.name = "STe100p",
-	.features = PHY_BASIC_FEATURES | SUPPORTED_Pause,
+	.features = PHY_BASIC_FEATURES,
 	.flags = PHY_HAS_INTERRUPT,
 	.config_init = ste10Xp_config_init,
 	.config_aneg = genphy_config_aneg,

Signature · 3 lines

Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Florian Fainelli

2016-11-01 18:35 UTC #1 lore raw

Quoted 7 lines · Timur Tabi

Pause frames are a feature that is supported by the MAC. It is the MAC that generates the frames and that processes them. Thy PHY can only be configured to allow them to pass. Therefore, pause frame support in the PHY should only be enabled if the MAC wants it enabled. It should not be forced on all the time.
Signed-off-by Timur Tabi <[email protected]>

So in premise, this is good, and is exactly what I have in mind for the series that I am cooking, but if we apply this alone, without a change in drivers/net/phy/phy.c which adds SUPPORTED_Pause | SUPPORTED_AsymPause to phydev->features, we are basically breaking the Ethernet MAC drivers that don't explicitly override phydev->features and yet rely on that to get flow control to work.

Signature · 1 line

Florian

Timur Tabi

author

2016-11-01 18:45 UTC #2 lore raw

Quoted 6 lines · Florian Fainelli

So in premise, this is good, and is exactly what I have in mind for the series that I am cooking, but if we apply this alone, without a change in drivers/net/phy/phy.c which adds SUPPORTED_Pause | SUPPORTED_AsymPause to phydev->features, we are basically breaking the Ethernet MAC drivers that don't explicitly override phydev->features and yet rely on that to get flow control to work.

That's what I figured, but I wasn't sure how to handle that. However, isn't the ability to pass pause frames a feature that some PHYs do not have? That is, does every PHY support bits 10 and 11 in register 4?

Signature · 3 lines

Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Florian Fainelli

2016-11-01 19:04 UTC #3 lore raw

Quoted 8 lines · Timur Tabi

Quoted 6 lines · Florian Fainelli
So in premise, this is good, and is exactly what I have in mind for the series that I am cooking, but if we apply this alone, without a change in drivers/net/phy/phy.c which adds SUPPORTED_Pause | SUPPORTED_AsymPause to phydev->features, we are basically breaking the Ethernet MAC drivers that don't explicitly override phydev->features and yet rely on that to get flow control to work.

Quoted 4 lines

That's what I figured, but I wasn't sure how to handle that. However, isn't the ability to pass pause frames a feature that some PHYs do not have? That is, does every PHY support bits 10 and 11 in register 4?

The standard (IEEE Std 802.3-2008) describes this as the Auto-negotiation advertisement register and these bits are part of the Technology ability field register. These two bits A5 (PAUSE operation for full duplex links) and A6 (Asymetric PAUSE operation for full duplex links) have no cabling requirement, and earlier the paragraph mentions this is to be resolved by the "management" entity, so I don't think the PHY has any business in that other than reflecting what the MAC is capable of doing towards the link partner, but I could be reading the specification incorrectly.

Timur Tabi

author

2016-11-01 19:19 UTC #4 lore raw

Quoted 9 lines · Florian Fainelli

The standard (IEEE Std 802.3-2008) describes this as the Auto-negotiation advertisement register and these bits are part of the Technology ability field register. These two bits A5 (PAUSE operation for full duplex links) and A6 (Asymetric PAUSE operation for full duplex links) have no cabling requirement, and earlier the paragraph mentions this is to be resolved by the "management" entity, so I don't think the PHY has any business in that other than reflecting what the MAC is capable of doing towards the link partner, but I could be reading the specification incorrectly.

Fair enough. I'll post a V2 patch that enables those bits in phy.c.

On a side note, is there any reason for the MAC to ever disable support for accepting pause frames? If the PHY is configured to not advertise support for receiving pause frames, then does it matter if the MAC is configured to accept them? The MAC would never receive such a frame.

If so, then I can modify my other patch (net: qcom/emac: enable flow control if requested) to always set RXFC, and have it set TXFC only if the PHY will transmit pause frames.

Signature · 3 lines

Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Timur Tabi

author

2016-11-07 16:30 UTC #5 lore raw

In reply to Florian Fainelli

Quoted 6 lines · Florian Fainelli

So in premise, this is good, and is exactly what I have in mind for the series that I am cooking, but if we apply this alone, without a change in drivers/net/phy/phy.c which adds SUPPORTED_Pause | SUPPORTED_AsymPause to phydev->features, we are basically breaking the Ethernet MAC drivers that don't explicitly override phydev->features and yet rely on that to get flow control to work.

Can you tell me where I should set the SUPPORTED_Pause and SUPPORTED_AsymPause flags? I have a two candidates:

1. The settings[] array. Add these flags to each entry.

2. In phy_sanitize_settings(). Add

	phydev->supported |= SUPPORTED_Pause | SUPPORTED_AsymPause;

at the end of the function.

I'm still don't understand 100% how these flags really work, because I just can't shake the feeling that they should not be set for every phy.
If these flags are supposed to be turned on universally, then why are
they even an option?

Signature · 3 lines

Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Timur Tabi

author

2016-11-08 20:43 UTC #6 lore raw

Quoted 5 lines · Timur Tabi

I'm still don't understand 100% how these flags really work, because I just can't shake the feeling that they should not be set for every phy.
If these flags are supposed to be turned on universally, then why are
they even an option?

So I've been giving this more thought. Can you tell me if the following is correct:

1) PHY drivers and/or phylib sets the SUPPORTED_Pause | SUPPORTED_AsymPause bits in phydev->supported. This indicates that the PHY supports pause frames.

2) The MAC driver checks phydev->supported before it calls phy_start(). If (SUPPORTED_Pause | SUPPORTED_AsymPause) is set, then it sets those
bits in phydev->advertising if it wants to enable pause frame support.

3) When the link state changes, the MAC driver checks phydev->advertising, and if the bits are set, then it enables those features in the MAC.

Signature · 3 lines

Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Florian Fainelli

2016-11-09 17:06 UTC #7 lore raw

Quoted 7 lines · Timur Tabi

Quoted 5 lines · Timur Tabi
I'm still don't understand 100% how these flags really work, because I just can't shake the feeling that they should not be set for every phy.
If these flags are supposed to be turned on universally, then why are
they even an option?

Quoted 6 lines

So I've been giving this more thought. Can you tell me if the following is correct:
1) PHY drivers and/or phylib sets the SUPPORTED_Pause | SUPPORTED_AsymPause bits in phydev->supported. This indicates that the PHY supports pause frames.

Agreed.

Quoted 4 lines

2) The MAC driver checks phydev->supported before it calls phy_start(). If (SUPPORTED_Pause | SUPPORTED_AsymPause) is set, then it sets those
bits in phydev->advertising if it wants to enable pause frame support.

Also correct.

Quoted 4 lines

3) When the link state changes, the MAC driver checks phydev->advertising, and if the bits are set, then it enables those features in the MAC.

Almost, we need to see what the link partner advertised, by looking at phydev->pause / phydev->asym_pause and checking the local knobs whether flow control can be enabled, but other than that, this looks correct to me.

Signature · 1 line

Florian

Timur Tabi

author

2016-11-09 17:44 UTC #8 lore raw

Quoted 5 lines · Florian Fainelli

Quoted 3 lines
1) PHY drivers and/or phylib sets the SUPPORTED_Pause | SUPPORTED_AsymPause bits in phydev->supported. This indicates that the PHY supports pause frames.
Agreed.

So I'm still not sure *where* phylib should set the bits in phydev->supported. I previously thought that phy_sanitize_settings() would be a candidate, but that function is called only when autonegotiation is disabled.

So the only thing I can think of is to do this:

/* A mapping of all SUPPORTED settings to speed/duplex */

static const struct phy_setting settings[] = {
	{
		.speed = SPEED_10000,
		.duplex = DUPLEX_FULL,

- .setting = SUPPORTED_10000baseKR_Full,
+ .setting = SUPPORTED_10000baseKR_Full |
+ SUPPORTED_Pause | SUPPORTED_Asym_Pause,

... for all of the entries in settings[]

But this seems excessive, and I don't know if all of these SPEED_xxx standards actually support pause frames.

Quoted 7 lines

Quoted 3 lines
Quoted 3 lines
3) When the link state changes, the MAC driver checks phydev->advertising, and if the bits are set, then it enables those features in the MAC.
Almost, we need to see what the link partner advertised, by looking at phydev->pause / phydev->asym_pause and checking the local knobs whether flow control can be enabled, but other than that, this looks correct to me.

Yes, that's what I meant to write. Fortunately, my EMAC driver already does this.

Signature · 3 lines

Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Florian Fainelli

2016-11-09 17:55 UTC #9 lore raw

Quoted 24 lines · Timur Tabi

Quoted 5 lines · Florian Fainelli
Quoted 3 lines
1) PHY drivers and/or phylib sets the SUPPORTED_Pause | SUPPORTED_AsymPause bits in phydev->supported. This indicates that the PHY supports pause frames.
Agreed.
So I'm still not sure *where* phylib should set the bits in phydev->supported. I previously thought that phy_sanitize_settings() would be a candidate, but that function is called only when autonegotiation is disabled.
So the only thing I can think of is to do this:
/* A mapping of all SUPPORTED settings to speed/duplex */
static const struct phy_setting settings[] = {
    {
        .speed = SPEED_10000,
        .duplex = DUPLEX_FULL,
(quoted diff) +2 -1
-        .setting = SUPPORTED_10000baseKR_Full,
+        .setting = SUPPORTED_10000baseKR_Full |
+            SUPPORTED_Pause | SUPPORTED_Asym_Pause,

Quoted 4 lines

... for all of the entries in settings[]
But this seems excessive, and I don't know if all of these SPEED_xxx standards actually support pause frames.

You could set these bits in phy_probe() and later let the phy_sanitize_settings() make sure that these bits do not get advertised unless we have a full duplex link.

Quoted 5 lines

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
3) When the link state changes, the MAC driver checks phydev->advertising, and if the bits are set, then it enables those features in the MAC.

Quoted 7 lines

Quoted 4 lines
Almost, we need to see what the link partner advertised, by looking at phydev->pause / phydev->asym_pause and checking the local knobs whether flow control can be enabled, but other than that, this looks correct to me.
Yes, that's what I meant to write. Fortunately, my EMAC driver already does this.

OK, just making sure ;)

Signature · 1 line

Florian