[PATCH net-next 0/5] net: phy: broadcom: Wirespeed/downshift support

[PATCH net-next 0/5] net: phy: broadcom: Wirespeed/downshift support

[Florian Fainelli]

Hi all,

This patch series adds support for the Broadcom Wirespeed, aka downsfhit feature
utilizing the recently added ethtool PHY tunables.

Tested with two Gigabit link partners with a 4-wire cable having only 2 pairs
connected.

Last patch in the series is a fix that was required for testing, which should
make it to -stable, which I can submit separate against net if you prefer David.

Thanks!

Florian Fainelli (5):
  net: phy: broadcom: Move bcm54xx_auxctl_{read,write} to common library
  net: phy: broadcom: Add support code for downshift/Wirespeed
  net: phy: broadcom: Allow enabling or disabling of EEE
  net: phy: bcm7xxx: Add support for downshift/Wirespeed
  net: dsa: bcm_sf2: Ensure we re-negotiate EEE during after link change

 drivers/net/dsa/bcm_sf2.c     |   4 ++
 drivers/net/phy/bcm-cygnus.c  |   2 +-
 drivers/net/phy/bcm-phy-lib.c | 117 ++++++++++++++++++++++++++++++++++++++++--
 drivers/net/phy/bcm-phy-lib.h |  10 +++-
 drivers/net/phy/bcm7xxx.c     |  51 +++++++++++++++++-
 drivers/net/phy/broadcom.c    |  15 ------
 include/linux/brcmphy.h       |  10 ++++
 7 files changed, 187 insertions(+), 22 deletions(-)

-- 
2.9.3

[PATCH net-next 1/5] net: phy: broadcom: Move bcm54xx_auxctl_{read,write} to common library

[Florian Fainelli]

We are going to need these functions to implement support for Broadcom
Wirespeed, aka downshift.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/bcm-phy-lib.c | 17 +++++++++++++++++
 drivers/net/phy/bcm-phy-lib.h |  3 +++
 drivers/net/phy/broadcom.c    | 15 ---------------
 3 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
index df0416db0b88..18e11b3a0f41 100644
--- a/drivers/net/phy/bcm-phy-lib.c
+++ b/drivers/net/phy/bcm-phy-lib.c
@@ -50,6 +50,23 @@ int bcm_phy_read_exp(struct phy_device *phydev, u16 reg)
 }
 EXPORT_SYMBOL_GPL(bcm_phy_read_exp);
 
+int bcm54xx_auxctl_read(struct phy_device *phydev, u16 regnum)
+{
+	/* The register must be written to both the Shadow Register Select and
+	 * the Shadow Read Register Selector
+	 */
+	phy_write(phydev, MII_BCM54XX_AUX_CTL, regnum |
+		  regnum << MII_BCM54XX_AUXCTL_SHDWSEL_READ_SHIFT);
+	return phy_read(phydev, MII_BCM54XX_AUX_CTL);
+}
+EXPORT_SYMBOL_GPL(bcm54xx_auxctl_read);
+
+int bcm54xx_auxctl_write(struct phy_device *phydev, u16 regnum, u16 val)
+{
+	return phy_write(phydev, MII_BCM54XX_AUX_CTL, regnum | val);
+}
+EXPORT_SYMBOL(bcm54xx_auxctl_write);
+
 int bcm_phy_write_misc(struct phy_device *phydev,
 		       u16 reg, u16 chl, u16 val)
 {
diff --git a/drivers/net/phy/bcm-phy-lib.h b/drivers/net/phy/bcm-phy-lib.h
index b2091c88b44d..31cb4fdf5d5a 100644
--- a/drivers/net/phy/bcm-phy-lib.h
+++ b/drivers/net/phy/bcm-phy-lib.h
@@ -19,6 +19,9 @@
 int bcm_phy_write_exp(struct phy_device *phydev, u16 reg, u16 val);
 int bcm_phy_read_exp(struct phy_device *phydev, u16 reg);
 
+int bcm54xx_auxctl_write(struct phy_device *phydev, u16 regnum, u16 val);
+int bcm54xx_auxctl_read(struct phy_device *phydev, u16 regnum);
+
 int bcm_phy_write_misc(struct phy_device *phydev,
 		       u16 reg, u16 chl, u16 value);
 int bcm_phy_read_misc(struct phy_device *phydev,
diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index b1e32e9be1b3..409b365f12b1 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -30,21 +30,6 @@ MODULE_DESCRIPTION("Broadcom PHY driver");
 MODULE_AUTHOR("Maciej W. Rozycki");
 MODULE_LICENSE("GPL");
 
-static int bcm54xx_auxctl_read(struct phy_device *phydev, u16 regnum)
-{
-	/* The register must be written to both the Shadow Register Select and
-	 * the Shadow Read Register Selector
-	 */
-	phy_write(phydev, MII_BCM54XX_AUX_CTL, regnum |
-		  regnum << MII_BCM54XX_AUXCTL_SHDWSEL_READ_SHIFT);
-	return phy_read(phydev, MII_BCM54XX_AUX_CTL);
-}
-
-static int bcm54xx_auxctl_write(struct phy_device *phydev, u16 regnum, u16 val)
-{
-	return phy_write(phydev, MII_BCM54XX_AUX_CTL, regnum | val);
-}
-
 static int bcm54810_config(struct phy_device *phydev)
 {
 	int rc, val;
-- 
2.9.3

[PATCH net-next 2/5] net: phy: broadcom: Add support code for downshift/Wirespeed

[Florian Fainelli]

Broadcom's Wirespeed feature allows us to configure how auto-negotiation
should behave with fewer working pairs of wires on a cable. Add support
code for retrieving and setting such downshift counters using the
recently added ethtool downshift tunables.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/bcm-phy-lib.c | 86 +++++++++++++++++++++++++++++++++++++++++++
 drivers/net/phy/bcm-phy-lib.h |  5 +++
 include/linux/brcmphy.h       | 10 +++++
 3 files changed, 101 insertions(+)

diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
index 18e11b3a0f41..d742894816f6 100644
--- a/drivers/net/phy/bcm-phy-lib.c
+++ b/drivers/net/phy/bcm-phy-lib.c
@@ -225,6 +225,92 @@ int bcm_phy_enable_eee(struct phy_device *phydev)
 }
 EXPORT_SYMBOL_GPL(bcm_phy_enable_eee);
 
+int bcm_phy_downshift_get(struct phy_device *phydev, u8 *count)
+{
+	int val;
+
+	val = bcm54xx_auxctl_read(phydev, MII_BCM54XX_AUXCTL_SHDWSEL_MISC);
+	if (val < 0)
+		return val;
+
+	/* Check if wirespeed is enabled or not */
+	if (!(val & MII_BCM54XX_AUXCTL_SHDWSEL_MISC_WIRESPEED_EN)) {
+		*count = DOWNSHIFT_DEV_DISABLE;
+		return 0;
+	}
+
+	val = bcm_phy_read_shadow(phydev, BCM54XX_SHD_SCR2);
+	if (val < 0)
+		return val;
+
+	/* Downgrade after one link attempt */
+	if (val & BCM54XX_SHD_SCR2_WSPD_RTRY_DIS) {
+		*count = 1;
+	} else {
+		/* Downgrade after configured retry count */
+		val >>= BCM54XX_SHD_SCR2_WSPD_RTRY_LMT_SHIFT;
+		val &= BCM54XX_SHD_SCR2_WSPD_RTRY_LMT_MASK;
+		*count = val + BCM54XX_SHD_SCR2_WSPD_RTRY_LMT_OFFSET;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(bcm_phy_downshift_get);
+
+int bcm_phy_downshift_set(struct phy_device *phydev, u8 count)
+{
+	int val = 0, ret = 0;
+
+	/* Range check the number given */
+	if (count - BCM54XX_SHD_SCR2_WSPD_RTRY_LMT_OFFSET >
+	    BCM54XX_SHD_SCR2_WSPD_RTRY_LMT_MASK &&
+	    count != DOWNSHIFT_DEV_DEFAULT_COUNT) {
+		return -ERANGE;
+	}
+
+	val = bcm54xx_auxctl_read(phydev, MII_BCM54XX_AUXCTL_SHDWSEL_MISC);
+	if (val < 0)
+		return val;
+
+	/* Se the write enable bit */
+	val |= MII_BCM54XX_AUXCTL_MISC_WREN;
+
+	if (count == DOWNSHIFT_DEV_DISABLE) {
+		val &= ~MII_BCM54XX_AUXCTL_SHDWSEL_MISC_WIRESPEED_EN;
+		return bcm54xx_auxctl_write(phydev,
+					    MII_BCM54XX_AUXCTL_SHDWSEL_MISC,
+					    val);
+	} else {
+		val |= MII_BCM54XX_AUXCTL_SHDWSEL_MISC_WIRESPEED_EN;
+		ret = bcm54xx_auxctl_write(phydev,
+					   MII_BCM54XX_AUXCTL_SHDWSEL_MISC,
+					   val);
+		if (ret < 0)
+			return ret;
+	}
+
+	val = bcm_phy_read_shadow(phydev, BCM54XX_SHD_SCR2);
+	val &= ~(BCM54XX_SHD_SCR2_WSPD_RTRY_LMT_MASK <<
+		 BCM54XX_SHD_SCR2_WSPD_RTRY_LMT_SHIFT |
+		 BCM54XX_SHD_SCR2_WSPD_RTRY_DIS);
+
+	switch (count) {
+	case 1:
+		val |= BCM54XX_SHD_SCR2_WSPD_RTRY_DIS;
+		break;
+	case DOWNSHIFT_DEV_DEFAULT_COUNT:
+		val |= 1 << BCM54XX_SHD_SCR2_WSPD_RTRY_LMT_SHIFT;
+		break;
+	default:
+		val |= (count - BCM54XX_SHD_SCR2_WSPD_RTRY_LMT_OFFSET) <<
+			BCM54XX_SHD_SCR2_WSPD_RTRY_LMT_SHIFT;
+		break;
+	}
+
+	return bcm_phy_write_shadow(phydev, BCM54XX_SHD_SCR2, val);
+}
+EXPORT_SYMBOL_GPL(bcm_phy_downshift_set);
+
 MODULE_DESCRIPTION("Broadcom PHY Library");
 MODULE_LICENSE("GPL v2");
 MODULE_AUTHOR("Broadcom Corporation");
diff --git a/drivers/net/phy/bcm-phy-lib.h b/drivers/net/phy/bcm-phy-lib.h
index 31cb4fdf5d5a..3f492e629094 100644
--- a/drivers/net/phy/bcm-phy-lib.h
+++ b/drivers/net/phy/bcm-phy-lib.h
@@ -37,4 +37,9 @@ int bcm_phy_config_intr(struct phy_device *phydev);
 int bcm_phy_enable_apd(struct phy_device *phydev, bool dll_pwr_down);
 
 int bcm_phy_enable_eee(struct phy_device *phydev);
+
+int bcm_phy_downshift_get(struct phy_device *phydev, u8 *count);
+
+int bcm_phy_downshift_set(struct phy_device *phydev, u8 count);
+
 #endif /* _LINUX_BCM_PHY_LIB_H */
diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
index 848dc508ef57..f9f8aaf9c943 100644
--- a/include/linux/brcmphy.h
+++ b/include/linux/brcmphy.h
@@ -114,6 +114,7 @@
 #define MII_BCM54XX_AUXCTL_SHDWSEL_MISC	0x0007
 #define MII_BCM54XX_AUXCTL_SHDWSEL_READ_SHIFT	12
 #define MII_BCM54XX_AUXCTL_SHDWSEL_MISC_RGMII_SKEW_EN	(1 << 8)
+#define MII_BCM54XX_AUXCTL_SHDWSEL_MISC_WIRESPEED_EN	(1 << 4)
 
 #define MII_BCM54XX_AUXCTL_SHDWSEL_MASK	0x0007
 
@@ -130,6 +131,7 @@
 #define BCM_LED_SRC_INTR	0x6
 #define BCM_LED_SRC_QUALITY	0x7
 #define BCM_LED_SRC_RCVLED	0x8
+#define BCM_LED_SRC_WIRESPEED	0x9
 #define BCM_LED_SRC_MULTICOLOR1	0xa
 #define BCM_LED_SRC_OPENSHORT	0xb
 #define BCM_LED_SRC_OFF		0xe	/* Tied high */
@@ -141,6 +143,14 @@
  * Shadow values go into bits [14:10] of register 0x1c to select a shadow
  * register to access.
  */
+
+/* 00100: Reserved control register 2 */
+#define BCM54XX_SHD_SCR2		0x04
+#define  BCM54XX_SHD_SCR2_WSPD_RTRY_DIS	0x100
+#define  BCM54XX_SHD_SCR2_WSPD_RTRY_LMT_SHIFT	2
+#define  BCM54XX_SHD_SCR2_WSPD_RTRY_LMT_OFFSET	2
+#define  BCM54XX_SHD_SCR2_WSPD_RTRY_LMT_MASK	0x7
+
 /* 00101: Spare Control Register 3 */
 #define BCM54XX_SHD_SCR3		0x05
 #define  BCM54XX_SHD_SCR3_DEF_CLK125	0x0001
-- 
2.9.3

[PATCH net-next 3/5] net: phy: broadcom: Allow enabling or disabling of EEE

[Florian Fainelli]

In preparation for adding support for Wirespeed/downshift, we need to
change bcm_phy_eee_enable() to allow enabling or disabling EEE, so make
the function take an extra enable/disable boolean parameter and rename
it to illustrate it sets EEE, not necessarily just enables it.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/bcm-cygnus.c  |  2 +-
 drivers/net/phy/bcm-phy-lib.c | 14 ++++++++++----
 drivers/net/phy/bcm-phy-lib.h |  2 +-
 drivers/net/phy/bcm7xxx.c     |  2 +-
 4 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/bcm-cygnus.c b/drivers/net/phy/bcm-cygnus.c
index 49bbc6826883..196400cddf68 100644
--- a/drivers/net/phy/bcm-cygnus.c
+++ b/drivers/net/phy/bcm-cygnus.c
@@ -104,7 +104,7 @@ static int bcm_cygnus_config_init(struct phy_device *phydev)
 		return rc;
 
 	/* Advertise EEE */
-	rc = bcm_phy_enable_eee(phydev);
+	rc = bcm_phy_set_eee(phydev, true);
 	if (rc)
 		return rc;
 
diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
index d742894816f6..3156ce6d5861 100644
--- a/drivers/net/phy/bcm-phy-lib.c
+++ b/drivers/net/phy/bcm-phy-lib.c
@@ -195,7 +195,7 @@ int bcm_phy_enable_apd(struct phy_device *phydev, bool dll_pwr_down)
 }
 EXPORT_SYMBOL_GPL(bcm_phy_enable_apd);
 
-int bcm_phy_enable_eee(struct phy_device *phydev)
+int bcm_phy_set_eee(struct phy_device *phydev, bool enable)
 {
 	int val;
 
@@ -205,7 +205,10 @@ int bcm_phy_enable_eee(struct phy_device *phydev)
 	if (val < 0)
 		return val;
 
-	val |= LPI_FEATURE_EN | LPI_FEATURE_EN_DIG1000X;
+	if (enable)
+		val |= LPI_FEATURE_EN | LPI_FEATURE_EN_DIG1000X;
+	else
+		val &= ~(LPI_FEATURE_EN | LPI_FEATURE_EN_DIG1000X);
 
 	phy_write_mmd_indirect(phydev, BRCM_CL45VEN_EEE_CONTROL,
 			       MDIO_MMD_AN, (u32)val);
@@ -216,14 +219,17 @@ int bcm_phy_enable_eee(struct phy_device *phydev)
 	if (val < 0)
 		return val;
 
-	val |= (MDIO_AN_EEE_ADV_100TX | MDIO_AN_EEE_ADV_1000T);
+	if (enable)
+		val |= (MDIO_AN_EEE_ADV_100TX | MDIO_AN_EEE_ADV_1000T);
+	else
+		val &= ~(MDIO_AN_EEE_ADV_100TX | MDIO_AN_EEE_ADV_1000T);
 
 	phy_write_mmd_indirect(phydev, BCM_CL45VEN_EEE_ADV,
 			       MDIO_MMD_AN, (u32)val);
 
 	return 0;
 }
-EXPORT_SYMBOL_GPL(bcm_phy_enable_eee);
+EXPORT_SYMBOL_GPL(bcm_phy_set_eee);
 
 int bcm_phy_downshift_get(struct phy_device *phydev, u8 *count)
 {
diff --git a/drivers/net/phy/bcm-phy-lib.h b/drivers/net/phy/bcm-phy-lib.h
index 3f492e629094..a117f657c6d7 100644
--- a/drivers/net/phy/bcm-phy-lib.h
+++ b/drivers/net/phy/bcm-phy-lib.h
@@ -36,7 +36,7 @@ int bcm_phy_config_intr(struct phy_device *phydev);
 
 int bcm_phy_enable_apd(struct phy_device *phydev, bool dll_pwr_down);
 
-int bcm_phy_enable_eee(struct phy_device *phydev);
+int bcm_phy_set_eee(struct phy_device *phydev, bool enable);
 
 int bcm_phy_downshift_get(struct phy_device *phydev, u8 *count);
 
diff --git a/drivers/net/phy/bcm7xxx.c b/drivers/net/phy/bcm7xxx.c
index 9636da0b6efc..b7789e879670 100644
--- a/drivers/net/phy/bcm7xxx.c
+++ b/drivers/net/phy/bcm7xxx.c
@@ -199,7 +199,7 @@ static int bcm7xxx_28nm_config_init(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
-	ret = bcm_phy_enable_eee(phydev);
+	ret = bcm_phy_set_eee(phydev, true);
 	if (ret)
 		return ret;
 
-- 
2.9.3

[PATCH net-next 4/5] net: phy: bcm7xxx: Add support for downshift/Wirespeed

[Florian Fainelli]

Add support for configuring the downshift/Wirespeed enable/disable
toggles and specify a link retry value ranging from 1 to 9. Since the
integrated BCM7xxx have issues when wirespeed is enabled and EEE is also
enabled, we do disable EEE if wirespeed is enabled.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/bcm7xxx.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/bcm7xxx.c b/drivers/net/phy/bcm7xxx.c
index b7789e879670..5b3be4c67be8 100644
--- a/drivers/net/phy/bcm7xxx.c
+++ b/drivers/net/phy/bcm7xxx.c
@@ -167,6 +167,7 @@ static int bcm7xxx_28nm_config_init(struct phy_device *phydev)
 {
 	u8 rev = PHY_BRCM_7XXX_REV(phydev->dev_flags);
 	u8 patch = PHY_BRCM_7XXX_PATCH(phydev->dev_flags);
+	u8 count;
 	int ret = 0;
 
 	pr_info_once("%s: %s PHY revision: 0x%02x, patch: %d\n",
@@ -199,7 +200,12 @@ static int bcm7xxx_28nm_config_init(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
-	ret = bcm_phy_set_eee(phydev, true);
+	ret = bcm_phy_downshift_get(phydev, &count);
+	if (ret)
+		return ret;
+
+	/* Only enable EEE if Wirespeed/downshift is disabled */
+	ret = bcm_phy_set_eee(phydev, count == DOWNSHIFT_DEV_DISABLE);
 	if (ret)
 		return ret;
 
@@ -303,6 +309,47 @@ static int bcm7xxx_suspend(struct phy_device *phydev)
 	return 0;
 }
 
+static int bcm7xxx_28nm_get_tunable(struct phy_device *phydev,
+				    struct ethtool_tunable *tuna,
+				    void *data)
+{
+	switch (tuna->id) {
+	case ETHTOOL_PHY_DOWNSHIFT:
+		return bcm_phy_downshift_get(phydev, (u8 *)data);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static int bcm7xxx_28nm_set_tunable(struct phy_device *phydev,
+				    struct ethtool_tunable *tuna,
+				    const void *data)
+{
+	u8 count = *(u8 *)data;
+	int ret;
+
+	switch (tuna->id) {
+	case ETHTOOL_PHY_DOWNSHIFT:
+		ret = bcm_phy_downshift_set(phydev, count);
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	if (ret)
+		return ret;
+
+	/* Disable EEE advertisment since this prevents the PHY
+	 * from successfully linking up, trigger auto-negotiation restart
+	 * to let the MAC decide what to do.
+	 */
+	ret = bcm_phy_set_eee(phydev, count == DOWNSHIFT_DEV_DISABLE);
+	if (ret)
+		return ret;
+
+	return genphy_restart_aneg(phydev);
+}
+
 #define BCM7XXX_28NM_GPHY(_oui, _name)					\
 {									\
 	.phy_id		= (_oui),					\
@@ -315,6 +362,8 @@ static int bcm7xxx_suspend(struct phy_device *phydev)
 	.config_aneg	= genphy_config_aneg,				\
 	.read_status	= genphy_read_status,				\
 	.resume		= bcm7xxx_28nm_resume,				\
+	.get_tunable	= bcm7xxx_28nm_get_tunable,			\
+	.set_tunable	= bcm7xxx_28nm_set_tunable,			\
 }
 
 #define BCM7XXX_40NM_EPHY(_oui, _name)					\
-- 
2.9.3

[PATCH net-next 5/5] net: dsa: bcm_sf2: Ensure we re-negotiate EEE during after link change

[Florian Fainelli]

In case the link change and EEE is enabled or disabled, always try to
re-negotiate this with the link partner.

Fixes: 450b05c15f9c ("net: dsa: bcm_sf2: add support for controlling EEE")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/bcm_sf2.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index e3ee27ce13dd..9ec33b51a0ed 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -588,6 +588,7 @@ static void bcm_sf2_sw_adjust_link(struct dsa_switch *ds, int port,
 				   struct phy_device *phydev)
 {
 	struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
+	struct ethtool_eee *p = &priv->port_sts[port].eee;
 	u32 id_mode_dis = 0, port_mode;
 	const char *str = NULL;
 	u32 reg;
@@ -662,6 +663,9 @@ static void bcm_sf2_sw_adjust_link(struct dsa_switch *ds, int port,
 		reg |= DUPLX_MODE;
 
 	core_writel(priv, reg, CORE_STS_OVERRIDE_GMIIP_PORT(port));
+
+	if (!phydev->is_pseudo_fixed_link)
+		p->eee_enabled = bcm_sf2_eee_init(ds, port, phydev);
 }
 
 static void bcm_sf2_sw_fixed_link_update(struct dsa_switch *ds, int port,
-- 
2.9.3

Re: [PATCH net-next 4/5] net: phy: bcm7xxx: Add support for downshift/Wirespeed

[Andrew Lunn]

> +static int bcm7xxx_28nm_set_tunable(struct phy_device *phydev,
> +				    struct ethtool_tunable *tuna,
> +				    const void *data)
> +{
> +	u8 count = *(u8 *)data;
> +	int ret;
> +
> +	switch (tuna->id) {
> +	case ETHTOOL_PHY_DOWNSHIFT:
> +		ret = bcm_phy_downshift_set(phydev, count);
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (ret)
> +		return ret;
> +
> +	/* Disable EEE advertisment since this prevents the PHY
> +	 * from successfully linking up, trigger auto-negotiation restart
> +	 * to let the MAC decide what to do.
> +	 */
> +	ret = bcm_phy_set_eee(phydev, count == DOWNSHIFT_DEV_DISABLE);
> +	if (ret)
> +		return ret;
> +
> +	return genphy_restart_aneg(phydev);
> +}

Hi Florian

Is the locking O.K. here? The core code does not take the phy lock.
But i think your shadow register accesses at least need to be
protected by the lock?

Maybe we should think about this locking a bit. It is normal for the
lock to be held when using ops in the phy driver structure. The
exception is suspend/resume. Maybe we should also take the lock before
calling the phydev->drv->get_tunable() and phydev->drv->set_tunable()?

	  Andrew

Re: [PATCH net-next 4/5] net: phy: bcm7xxx: Add support for downshift/Wirespeed

[Florian Fainelli]

On 11/22/2016 12:02 PM, Andrew Lunn wrote:
>> +static int bcm7xxx_28nm_set_tunable(struct phy_device *phydev,
>> +				    struct ethtool_tunable *tuna,
>> +				    const void *data)
>> +{
>> +	u8 count = *(u8 *)data;
>> +	int ret;
>> +
>> +	switch (tuna->id) {
>> +	case ETHTOOL_PHY_DOWNSHIFT:
>> +		ret = bcm_phy_downshift_set(phydev, count);
>> +		break;
>> +	default:
>> +		return -EOPNOTSUPP;
>> +	}
>> +
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Disable EEE advertisment since this prevents the PHY
>> +	 * from successfully linking up, trigger auto-negotiation restart
>> +	 * to let the MAC decide what to do.
>> +	 */
>> +	ret = bcm_phy_set_eee(phydev, count == DOWNSHIFT_DEV_DISABLE);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return genphy_restart_aneg(phydev);
>> +}
> 
> Hi Florian
> 
> Is the locking O.K. here? The core code does not take the phy lock.
> But i think your shadow register accesses at least need to be
> protected by the lock?

There should be some kind of protection, but I was expecting it to be
done at the caller level, so that when {get,set}_tunable run, they are
serialized with respect to each other, clearly, by looking at the code,
this is not the case.

> 
> Maybe we should think about this locking a bit. It is normal for the
> lock to be held when using ops in the phy driver structure. The
> exception is suspend/resume. Maybe we should also take the lock before
> calling the phydev->drv->get_tunable() and phydev->drv->set_tunable()?

Yes, that certainly seems like a good approach to me, let me cook a
patch doing that.
-- 
Florian

Re: [PATCH net-next 4/5] net: phy: bcm7xxx: Add support for downshift/Wirespeed

[Andrew Lunn]

> > Maybe we should think about this locking a bit. It is normal for the
> > lock to be held when using ops in the phy driver structure. The
> > exception is suspend/resume. Maybe we should also take the lock before
> > calling the phydev->drv->get_tunable() and phydev->drv->set_tunable()?
> 
> Yes, that certainly seems like a good approach to me, let me cook a
> patch doing that.

Hi Florian

There are a couple of mutex locks/unlocks you will need to remove from
mscc.c when you centralize this mutex.

       Andrew

Re: [PATCH net-next 4/5] net: phy: bcm7xxx: Add support for downshift/Wirespeed

[Florian Fainelli]

On 11/22/2016 12:57 PM, Andrew Lunn wrote:
>>> Maybe we should think about this locking a bit. It is normal for the
>>> lock to be held when using ops in the phy driver structure. The
>>> exception is suspend/resume. Maybe we should also take the lock before
>>> calling the phydev->drv->get_tunable() and phydev->drv->set_tunable()?
>>
>> Yes, that certainly seems like a good approach to me, let me cook a
>> patch doing that.
> 
> Hi Florian
> 
> There are a couple of mutex locks/unlocks you will need to remove from
> mscc.c when you centralize this mutex.

Good point, thanks, let me review the mscc PHY driver and propose a more
proper fix.
-- 
Florian

Re: [PATCH net-next 4/5] net: phy: bcm7xxx: Add support for downshift/Wirespeed

[Allan W. Nielsen]

Hi,

On 22/11/16 12:07, Florian Fainelli wrote:
> On 11/22/2016 12:02 PM, Andrew Lunn wrote:
> >> +static int bcm7xxx_28nm_set_tunable(struct phy_device *phydev,
> >> +                                struct ethtool_tunable *tuna,
> >> +                                const void *data)
> >> +{
> >> +    u8 count = *(u8 *)data;
> >> +    int ret;
> >> +
> >> +    switch (tuna->id) {
> >> +    case ETHTOOL_PHY_DOWNSHIFT:
> >> +            ret = bcm_phy_downshift_set(phydev, count);
> >> +            break;
> >> +    default:
> >> +            return -EOPNOTSUPP;
> >> +    }
> >> +
> >> +    if (ret)
> >> +            return ret;
> >> +
> >> +    /* Disable EEE advertisment since this prevents the PHY
> >> +     * from successfully linking up, trigger auto-negotiation restart
> >> +     * to let the MAC decide what to do.
> >> +     */
> >> +    ret = bcm_phy_set_eee(phydev, count == DOWNSHIFT_DEV_DISABLE);
> >> +    if (ret)
> >> +            return ret;
> >> +
> >> +    return genphy_restart_aneg(phydev);
> >> +}
> >
> > Hi Florian
> >
> > Is the locking O.K. here? The core code does not take the phy lock.
> > But i think your shadow register accesses at least need to be
> > protected by the lock?
> 
> There should be some kind of protection, but I was expecting it to be
> done at the caller level, so that when {get,set}_tunable run, they are
> serialized with respect to each other, clearly, by looking at the code,
> this is not the case.
> 
> >
> > Maybe we should think about this locking a bit. It is normal for the
> > lock to be held when using ops in the phy driver structure. The
> > exception is suspend/resume. Maybe we should also take the lock before
> > calling the phydev->drv->get_tunable() and phydev->drv->set_tunable()?
> 
> Yes, that certainly seems like a good approach to me, let me cook a
> patch doing that.

Just for my understanding (such that I will not make the same mistake again)...

Why is it that phy functions such as get_wol needs to take the phy_lock and
others like get_tunable does not.

I do understand the arguments on why the lock should be held by the caller of
get_tunable, but I do not understand why the same argument does not apply for
get_wol.

/Allan

Re: [PATCH net-next 4/5] net: phy: bcm7xxx: Add support for downshift/Wirespeed

[Andrew Lunn]

> > > Maybe we should think about this locking a bit. It is normal for the
> > > lock to be held when using ops in the phy driver structure. The
> > > exception is suspend/resume. Maybe we should also take the lock before
> > > calling the phydev->drv->get_tunable() and phydev->drv->set_tunable()?
> > 
> > Yes, that certainly seems like a good approach to me, let me cook a
> > patch doing that.
> 
> Just for my understanding (such that I will not make the same mistake again)...
> 
> Why is it that phy functions such as get_wol needs to take the phy_lock and
> others like get_tunable does not.
> 
> I do understand the arguments on why the lock should be held by the caller of
> get_tunable, but I do not understand why the same argument does not apply for
> get_wol.

Hi Allan

phy_ethtool_get_wol and friends probably should take the
phy_lock. This inconsistency is probably leading to locking
bugs. e.g. at803x_set_wol() does a read-modify-write, and does not
take the lock.

There is no comment in the patch adding phy_ethtool_set_wol() to say
why the lock is not taken, and a quick look at the code does not
suggest a reason why it could not be taken/released by
phy_ethtool_set_wol().

I think it would be a good idea to change this.

phy_suspend()/phy_resume() might have good reasons to avoid the lock,
i've no idea how it is supposed to work. Is there a danger something
else is holding the lock and has already been suspended? I guess not,
otherwise there is little hope suspend would work at all.

	  Andrew

Re: [PATCH net-next 4/5] net: phy: bcm7xxx: Add support for downshift/Wirespeed

[Florian Fainelli]

On 11/23/2016 06:46 AM, Andrew Lunn wrote:
>>>> Maybe we should think about this locking a bit. It is normal for the
>>>> lock to be held when using ops in the phy driver structure. The
>>>> exception is suspend/resume. Maybe we should also take the lock before
>>>> calling the phydev->drv->get_tunable() and phydev->drv->set_tunable()?
>>>
>>> Yes, that certainly seems like a good approach to me, let me cook a
>>> patch doing that.
>>
>> Just for my understanding (such that I will not make the same mistake again)...
>>
>> Why is it that phy functions such as get_wol needs to take the phy_lock and
>> others like get_tunable does not.
>>
>> I do understand the arguments on why the lock should be held by the caller of
>> get_tunable, but I do not understand why the same argument does not apply for
>> get_wol.
> 
> Hi Allan
> 
> phy_ethtool_get_wol and friends probably should take the
> phy_lock. This inconsistency is probably leading to locking
> bugs. e.g. at803x_set_wol() does a read-modify-write, and does not
> take the lock.
> 
> There is no comment in the patch adding phy_ethtool_set_wol() to say
> why the lock is not taken, and a quick look at the code does not
> suggest a reason why it could not be taken/released by
> phy_ethtool_set_wol().

Yes, this should happen. I don't see how we cannot have two user-space
processes not racing with each other here for instance, see
mv643xx_eth_get_wol and cpsw_get_wol.

> 
> I think it would be a good idea to change this.
> 
> phy_suspend()/phy_resume() might have good reasons to avoid the lock,
> i've no idea how it is supposed to work. Is there a danger something
> else is holding the lock and has already been suspended? I guess not,
> otherwise there is little hope suspend would work at all.

phy_suspend() and phy_resume() usually get called after phy_disconnect()
or phy_stop() have been invoked, and even then this is during the
Ethernet driver's suspend resume/resume path, so there is no room for
concurrency to occur (user space is quiesced, and the PHY state machine
is stopped/halted), but still, if we were to change the calling context
it would be a good idea to acquire phydev->lock.
-- 
Florian

Re: [PATCH net-next 0/5] net: phy: broadcom: Wirespeed/downshift support

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Tue, 22 Nov 2016 11:40:53 -0800

> This patch series adds support for the Broadcom Wirespeed, aka downsfhit feature
> utilizing the recently added ethtool PHY tunables.
> 
> Tested with two Gigabit link partners with a 4-wire cable having only 2 pairs
> connected.
> 
> Last patch in the series is a fix that was required for testing, which should
> make it to -stable, which I can submit separate against net if you prefer David.

Series applied to net-next, patch #5 applied also to 'net' and queued up for
-stable.

Thanks.