[PATCH net-next 0/3] marvell10g tunable and power saving support

[PATCH net-next 0/3] marvell10g tunable and power saving support

[Russell King - ARM Linux admin]

Hi,

This patch series adds support for:
- mdix configuration (auto, mdi, mdix)
- energy detect power down (edpd)
- placing in edpd mode at probe

for both the 88x3310 and 88x2110 PHYs.

Antione, could you test this for the 88x2110 PHY please?

 drivers/net/phy/marvell10g.c | 181 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 173 insertions(+), 8 deletions(-)

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

[PATCH net-next 1/3] net: phy: marvell10g: add mdix control

[Russell King]

Add support for controlling the MDI-X state of the PHY.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/marvell10g.c | 40 ++++++++++++++++++++++++++++++++++--
 1 file changed, 38 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 9a4e12a2af07..20b24b1971a3 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -39,6 +39,12 @@ enum {
 	MV_PCS_BASE_R		= 0x1000,
 	MV_PCS_1000BASEX	= 0x2000,
 
+	MV_PCS_CSCR1		= 0x8000,
+	MV_PCS_CSCR1_MDIX_MASK	= 0x0060,
+	MV_PCS_CSCR1_MDIX_MDI	= 0x0000,
+	MV_PCS_CSCR1_MDIX_MDIX	= 0x0020,
+	MV_PCS_CSCR1_MDIX_AUTO	= 0x0060,
+
 	MV_PCS_CSSR1		= 0x8008,
 	MV_PCS_CSSR1_SPD1_MASK	= 0xc000,
 	MV_PCS_CSSR1_SPD1_SPD2	= 0xc000,
@@ -316,6 +322,8 @@ static int mv3310_config_init(struct phy_device *phydev)
 	    phydev->interface != PHY_INTERFACE_MODE_10GBASER)
 		return -ENODEV;
 
+	phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
+
 	return 0;
 }
 
@@ -345,14 +353,42 @@ static int mv3310_get_features(struct phy_device *phydev)
 	return 0;
 }
 
+static int mv3310_config_mdix(struct phy_device *phydev)
+{
+	u16 val;
+	int err;
+
+	switch (phydev->mdix_ctrl) {
+	case ETH_TP_MDI_AUTO:
+		val = MV_PCS_CSCR1_MDIX_AUTO;
+		break;
+	case ETH_TP_MDI_X:
+		val = MV_PCS_CSCR1_MDIX_MDIX;
+		break;
+	case ETH_TP_MDI:
+		val = MV_PCS_CSCR1_MDIX_MDI;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	err = phy_modify_mmd_changed(phydev, MDIO_MMD_PCS, MV_PCS_CSCR1,
+				     MV_PCS_CSCR1_MDIX_MASK, val);
+	if (err < 0)
+		return err;
+
+	return mv3310_maybe_reset(phydev, MV_PCS_BASE_T, err > 0);
+}
+
 static int mv3310_config_aneg(struct phy_device *phydev)
 {
 	bool changed = false;
 	u16 reg;
 	int ret;
 
-	/* We don't support manual MDI control */
-	phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
+	ret = mv3310_config_mdix(phydev);
+	if (ret < 0)
+		return ret;
 
 	if (phydev->autoneg == AUTONEG_DISABLE)
 		return genphy_c45_pma_setup_forced(phydev);
-- 
2.20.1

[PATCH net-next 2/3] net: phy: marvell10g: add energy detect power down tunable

[Russell King]

Add support for the energy detect power down tunable, which saves
around 600mW when the link is down. The 88x3310 supports off, rx-only
and NLP every second. Enable EDPD by default for 88x3310.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/marvell10g.c | 111 ++++++++++++++++++++++++++++++++++-
 1 file changed, 109 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 20b24b1971a3..e1116d091d84 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -23,6 +23,7 @@
  * link takes priority and the other port is completely locked out.
  */
 #include <linux/ctype.h>
+#include <linux/delay.h>
 #include <linux/hwmon.h>
 #include <linux/marvell_phy.h>
 #include <linux/phy.h>
@@ -40,6 +41,10 @@ enum {
 	MV_PCS_1000BASEX	= 0x2000,
 
 	MV_PCS_CSCR1		= 0x8000,
+	MV_PCS_CSCR1_ED_MASK	= 0x0300,
+	MV_PCS_CSCR1_ED_OFF	= 0x0000,
+	MV_PCS_CSCR1_ED_RX	= 0x0200,
+	MV_PCS_CSCR1_ED_NLP	= 0x0300,
 	MV_PCS_CSCR1_MDIX_MASK	= 0x0060,
 	MV_PCS_CSCR1_MDIX_MDI	= 0x0000,
 	MV_PCS_CSCR1_MDIX_MDIX	= 0x0020,
@@ -222,6 +227,82 @@ static int mv3310_hwmon_probe(struct phy_device *phydev)
 }
 #endif
 
+static int mv3310_maybe_reset(struct phy_device *phydev, u32 unit, bool reset)
+{
+	int retries, val, err;
+
+	if (!reset)
+		return 0;
+
+	err = phy_modify_mmd(phydev, MDIO_MMD_PCS, unit + MDIO_CTRL1,
+			     MDIO_CTRL1_RESET, MDIO_CTRL1_RESET);
+	if (err < 0)
+		return err;
+
+	retries = 20;
+	do {
+		msleep(5);
+		val = phy_read_mmd(phydev, MDIO_MMD_PCS, unit + MDIO_CTRL1);
+		if (val < 0)
+			return val;
+	} while (val & MDIO_CTRL1_RESET && --retries);
+
+	return val & MDIO_CTRL1_RESET ? -ETIMEDOUT : 0;
+}
+
+static int mv3310_get_edpd(struct phy_device *phydev, u16 *edpd)
+{
+	int val;
+
+	val = phy_read_mmd(phydev, MDIO_MMD_PCS, MV_PCS_CSCR1);
+	if (val < 0)
+		return val;
+
+	switch (val & MV_PCS_CSCR1_ED_MASK) {
+	case MV_PCS_CSCR1_ED_NLP:
+		*edpd = 1000;
+		break;
+	case MV_PCS_CSCR1_ED_RX:
+		*edpd = ETHTOOL_PHY_EDPD_NO_TX;
+		break;
+	default:
+		*edpd = ETHTOOL_PHY_EDPD_DISABLE;
+		break;
+	}
+	return 0;
+}
+
+static int mv3310_set_edpd(struct phy_device *phydev, u16 edpd)
+{
+	u16 val;
+	int err;
+
+	switch (edpd) {
+	case 1000:
+	case ETHTOOL_PHY_EDPD_DFLT_TX_MSECS:
+		val = MV_PCS_CSCR1_ED_NLP;
+		break;
+
+	case ETHTOOL_PHY_EDPD_NO_TX:
+		val = MV_PCS_CSCR1_ED_RX;
+		break;
+
+	case ETHTOOL_PHY_EDPD_DISABLE:
+		val = MV_PCS_CSCR1_ED_OFF;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	err = phy_modify_mmd_changed(phydev, MDIO_MMD_PCS, MV_PCS_CSCR1,
+				     MV_PCS_CSCR1_ED_MASK, val);
+	if (err < 0)
+		return err;
+
+	return mv3310_maybe_reset(phydev, MV_PCS_BASE_T, err > 0);
+}
+
 static int mv3310_sfp_insert(void *upstream, const struct sfp_eeprom_id *id)
 {
 	struct phy_device *phydev = upstream;
@@ -324,7 +405,8 @@ static int mv3310_config_init(struct phy_device *phydev)
 
 	phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
 
-	return 0;
+	/* Enable EDPD mode - saving 600mW */
+	return mv3310_set_edpd(phydev, ETHTOOL_PHY_EDPD_DFLT_TX_MSECS);
 }
 
 static int mv3310_get_features(struct phy_device *phydev)
@@ -573,6 +655,28 @@ static int mv3310_read_status(struct phy_device *phydev)
 	return 0;
 }
 
+static int mv3310_get_tunable(struct phy_device *phydev,
+			      struct ethtool_tunable *tuna, void *data)
+{
+	switch (tuna->id) {
+	case ETHTOOL_PHY_EDPD:
+		return mv3310_get_edpd(phydev, data);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int mv3310_set_tunable(struct phy_device *phydev,
+			      struct ethtool_tunable *tuna, const void *data)
+{
+	switch (tuna->id) {
+	case ETHTOOL_PHY_EDPD:
+		return mv3310_set_edpd(phydev, *(u16 *)data);
+	default:
+		return -EINVAL;
+	}
+}
+
 static struct phy_driver mv3310_drivers[] = {
 	{
 		.phy_id		= MARVELL_PHY_ID_88X3310,
@@ -580,13 +684,14 @@ static struct phy_driver mv3310_drivers[] = {
 		.name		= "mv88x3310",
 		.get_features	= mv3310_get_features,
 		.soft_reset	= genphy_no_soft_reset,
-		.config_init	= mv3310_config_init,
 		.probe		= mv3310_probe,
 		.suspend	= mv3310_suspend,
 		.resume		= mv3310_resume,
 		.config_aneg	= mv3310_config_aneg,
 		.aneg_done	= mv3310_aneg_done,
 		.read_status	= mv3310_read_status,
+		.get_tunable	= mv3310_get_tunable,
+		.set_tunable	= mv3310_set_tunable,
 	},
 	{
 		.phy_id		= MARVELL_PHY_ID_88E2110,
@@ -600,6 +705,8 @@ static struct phy_driver mv3310_drivers[] = {
 		.config_aneg	= mv3310_config_aneg,
 		.aneg_done	= mv3310_aneg_done,
 		.read_status	= mv3310_read_status,
+		.get_tunable	= mv3310_get_tunable,
+		.set_tunable	= mv3310_set_tunable,
 	},
 };
 
-- 
2.20.1

[PATCH net-next 3/3] net: phy: marvell10g: place in powersave mode at probe

[Russell King]

Place the 88x3310 into powersaving mode when probing, which saves 600mW
per PHY. For both PHYs on the Macchiatobin double-shot, this saves
about 10% of the board idle power.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/marvell10g.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index e1116d091d84..ec699fb6f2ea 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -227,6 +227,18 @@ static int mv3310_hwmon_probe(struct phy_device *phydev)
 }
 #endif
 
+static int mv3310_power_down(struct phy_device *phydev)
+{
+	return phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL,
+				MV_V2_PORT_CTRL_PWRDOWN);
+}
+
+static int mv3310_power_up(struct phy_device *phydev)
+{
+	return phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL,
+				  MV_V2_PORT_CTRL_PWRDOWN);
+}
+
 static int mv3310_maybe_reset(struct phy_device *phydev, u32 unit, bool reset)
 {
 	int retries, val, err;
@@ -351,6 +363,11 @@ static int mv3310_probe(struct phy_device *phydev)
 
 	dev_set_drvdata(&phydev->mdio.dev, priv);
 
+	/* Powering down the port when not in use saves about 600mW */
+	ret = mv3310_power_down(phydev);
+	if (ret)
+		return ret;
+
 	ret = mv3310_hwmon_probe(phydev);
 	if (ret)
 		return ret;
@@ -360,16 +377,14 @@ static int mv3310_probe(struct phy_device *phydev)
 
 static int mv3310_suspend(struct phy_device *phydev)
 {
-	return phy_set_bits_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL,
-				MV_V2_PORT_CTRL_PWRDOWN);
+	return mv3310_power_down(phydev);
 }
 
 static int mv3310_resume(struct phy_device *phydev)
 {
 	int ret;
 
-	ret = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND2, MV_V2_PORT_CTRL,
-				 MV_V2_PORT_CTRL_PWRDOWN);
+	ret = mv3310_power_up(phydev);
 	if (ret)
 		return ret;
 
@@ -395,6 +410,8 @@ static bool mv3310_has_pma_ngbaset_quirk(struct phy_device *phydev)
 
 static int mv3310_config_init(struct phy_device *phydev)
 {
+	int err;
+
 	/* Check that the PHY interface type is compatible */
 	if (phydev->interface != PHY_INTERFACE_MODE_SGMII &&
 	    phydev->interface != PHY_INTERFACE_MODE_2500BASEX &&
@@ -405,6 +422,11 @@ static int mv3310_config_init(struct phy_device *phydev)
 
 	phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
 
+	/* Power up so reset works */
+	err = mv3310_power_up(phydev);
+	if (err)
+		return err;
+
 	/* Enable EDPD mode - saving 600mW */
 	return mv3310_set_edpd(phydev, ETHTOOL_PHY_EDPD_DFLT_TX_MSECS);
 }
-- 
2.20.1

Re: [PATCH net-next 2/3] net: phy: marvell10g: add energy detect power down tunable

[Antoine Tenart]

Hi Russell,

On Tue, Mar 03, 2020 at 02:44:02PM +0000, Russell King wrote:
>  drivers/net/phy/marvell10g.c | 111 ++++++++++++++++++++++++++++++++++-
>  
> +static int mv3310_maybe_reset(struct phy_device *phydev, u32 unit, bool reset)
> +{
> +	int retries, val, err;
> +
> +	if (!reset)
> +		return 0;

You could also call mv3310_maybe_reset after testing the 'reset'
condition, that would make it easier to read the code.

>  static struct phy_driver mv3310_drivers[] = {
>  	{
>  		.phy_id		= MARVELL_PHY_ID_88X3310,
> @@ -580,13 +684,14 @@ static struct phy_driver mv3310_drivers[] = {
>  		.name		= "mv88x3310",
>  		.get_features	= mv3310_get_features,
>  		.soft_reset	= genphy_no_soft_reset,
> -		.config_init	= mv3310_config_init,

Having a quick look at the code, it seems this is a leftover and you
don't actually want to remove config_init for the 3310.

Thanks,
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next 1/3] net: phy: marvell10g: add mdix control

[Antoine Tenart]

Hello Russell,

On Tue, Mar 03, 2020 at 02:43:57PM +0000, Russell King wrote:
>  
> +static int mv3310_config_mdix(struct phy_device *phydev)
> +{
> +	u16 val;
> +	int err;
> +
> +	switch (phydev->mdix_ctrl) {
> +	case ETH_TP_MDI_AUTO:
> +		val = MV_PCS_CSCR1_MDIX_AUTO;
> +		break;
> +	case ETH_TP_MDI_X:
> +		val = MV_PCS_CSCR1_MDIX_MDIX;
> +		break;
> +	case ETH_TP_MDI:
> +		val = MV_PCS_CSCR1_MDIX_MDI;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	err = phy_modify_mmd_changed(phydev, MDIO_MMD_PCS, MV_PCS_CSCR1,
> +				     MV_PCS_CSCR1_MDIX_MASK, val);
> +	if (err < 0)
> +		return err;
> +
> +	return mv3310_maybe_reset(phydev, MV_PCS_BASE_T, err > 0);

This helper is only introduced in patch 2.

Thanks,
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next 2/3] net: phy: marvell10g: add energy detect power down tunable

[Russell King - ARM Linux admin]

On Tue, Mar 03, 2020 at 04:07:41PM +0100, Antoine Tenart wrote:
> Hi Russell,
> 
> On Tue, Mar 03, 2020 at 02:44:02PM +0000, Russell King wrote:
> >  drivers/net/phy/marvell10g.c | 111 ++++++++++++++++++++++++++++++++++-
> >  
> > +static int mv3310_maybe_reset(struct phy_device *phydev, u32 unit, bool reset)
> > +{
> > +	int retries, val, err;
> > +
> > +	if (!reset)
> > +		return 0;
> 
> You could also call mv3310_maybe_reset after testing the 'reset'
> condition, that would make it easier to read the code.

I'm not too convinced:

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index ef1ed9415d9f..3daf73e61dff 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -279,13 +279,10 @@ static int mv3310_power_up(struct phy_device *phydev)
 				  MV_V2_PORT_CTRL_PWRDOWN);
 }
 
-static int mv3310_maybe_reset(struct phy_device *phydev, u32 unit, bool reset)
+static int mv3310_reset(struct phy_device *phydev, u32 unit)
 {
 	int retries, val, err;
 
-	if (!reset)
-		return 0;
-
 	err = phy_modify_mmd(phydev, MDIO_MMD_PCS, unit + MDIO_CTRL1,
 			     MDIO_CTRL1_RESET, MDIO_CTRL1_RESET);
 	if (err < 0)
@@ -684,10 +681,10 @@ static int mv3310_config_mdix(struct phy_device *phydev)
 
 	err = phy_modify_mmd_changed(phydev, MDIO_MMD_PCS, MV_PCS_CSCR1,
 				     MV_PCS_CSCR1_MDIX_MASK, val);
-	if (err < 0)
+	if (err <= 0)
 		return err;
 
-	return mv3310_maybe_reset(phydev, MV_PCS_BASE_T, err > 0);
+	return mv3310_reset(phydev, MV_PCS_BASE_T);
 }
 
 static int mv3310_config_aneg(struct phy_device *phydev)

The change from:

	if (err < 0)

to:

	if (err <= 0)

could easily be mistaken as a bug, and someone may decide to try to
"fix" that back to being the former instead.  The way I have the code
makes the intention explicit.

> 
> >  static struct phy_driver mv3310_drivers[] = {
> >  	{
> >  		.phy_id		= MARVELL_PHY_ID_88X3310,
> > @@ -580,13 +684,14 @@ static struct phy_driver mv3310_drivers[] = {
> >  		.name		= "mv88x3310",
> >  		.get_features	= mv3310_get_features,
> >  		.soft_reset	= genphy_no_soft_reset,
> > -		.config_init	= mv3310_config_init,
> 
> Having a quick look at the code, it seems this is a leftover and you
> don't actually want to remove config_init for the 3310.

Hmm, I wonder how that crept in... it shouldn't be there!

-- 
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 net-next 2/3] net: phy: marvell10g: add energy detect power down tunable

[Antoine Tenart]

On Tue, Mar 03, 2020 at 03:12:32PM +0000, Russell King - ARM Linux admin wrote:
> On Tue, Mar 03, 2020 at 04:07:41PM +0100, Antoine Tenart wrote:
> > On Tue, Mar 03, 2020 at 02:44:02PM +0000, Russell King wrote:
> > >  drivers/net/phy/marvell10g.c | 111 ++++++++++++++++++++++++++++++++++-
> > >  
> > > +static int mv3310_maybe_reset(struct phy_device *phydev, u32 unit, bool reset)
> > > +{
> > > +	int retries, val, err;
> > > +
> > > +	if (!reset)
> > > +		return 0;
> > 
> > You could also call mv3310_maybe_reset after testing the 'reset'
> > condition, that would make it easier to read the code.
> 
> I'm not too convinced:
> 
> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> index ef1ed9415d9f..3daf73e61dff 100644
> --- a/drivers/net/phy/marvell10g.c
> +++ b/drivers/net/phy/marvell10g.c
> @@ -279,13 +279,10 @@ static int mv3310_power_up(struct phy_device *phydev)
>  				  MV_V2_PORT_CTRL_PWRDOWN);
>  }
>  
> -static int mv3310_maybe_reset(struct phy_device *phydev, u32 unit, bool reset)
> +static int mv3310_reset(struct phy_device *phydev, u32 unit)
>  {
>  	int retries, val, err;
>  
> -	if (!reset)
> -		return 0;
> -
>  	err = phy_modify_mmd(phydev, MDIO_MMD_PCS, unit + MDIO_CTRL1,
>  			     MDIO_CTRL1_RESET, MDIO_CTRL1_RESET);
>  	if (err < 0)
> @@ -684,10 +681,10 @@ static int mv3310_config_mdix(struct phy_device *phydev)
>  
>  	err = phy_modify_mmd_changed(phydev, MDIO_MMD_PCS, MV_PCS_CSCR1,
>  				     MV_PCS_CSCR1_MDIX_MASK, val);
> -	if (err < 0)
> +	if (err <= 0)
>  		return err;
>  
> -	return mv3310_maybe_reset(phydev, MV_PCS_BASE_T, err > 0);
> +	return mv3310_reset(phydev, MV_PCS_BASE_T);
>  }
>  
>  static int mv3310_config_aneg(struct phy_device *phydev)
> 
> The change from:
> 
> 	if (err < 0)
> 
> to:
> 
> 	if (err <= 0)
> 
> could easily be mistaken as a bug, and someone may decide to try to
> "fix" that back to being the former instead.  The way I have the code
> makes the intention explicit.

Using a single line to test both the error and the 'return 0'
conditions, yes, I agree. Another solution would be to do something of
the like:

	phy_modify_mmd_changed()
	if (err < 0)
		return err;

	if (err)
		mv3310_reset();

	return 0;

I find it more readable, but this kind of thing is also a matter of
personal taste.

Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH net-next 1/3] net: phy: marvell10g: add mdix control

[Russell King - ARM Linux admin]

On Tue, Mar 03, 2020 at 04:09:58PM +0100, Antoine Tenart wrote:
> Hello Russell,
> 
> On Tue, Mar 03, 2020 at 02:43:57PM +0000, Russell King wrote:
> >  
> > +static int mv3310_config_mdix(struct phy_device *phydev)
> > +{
> > +	u16 val;
> > +	int err;
> > +
> > +	switch (phydev->mdix_ctrl) {
> > +	case ETH_TP_MDI_AUTO:
> > +		val = MV_PCS_CSCR1_MDIX_AUTO;
> > +		break;
> > +	case ETH_TP_MDI_X:
> > +		val = MV_PCS_CSCR1_MDIX_MDIX;
> > +		break;
> > +	case ETH_TP_MDI:
> > +		val = MV_PCS_CSCR1_MDIX_MDI;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	err = phy_modify_mmd_changed(phydev, MDIO_MMD_PCS, MV_PCS_CSCR1,
> > +				     MV_PCS_CSCR1_MDIX_MASK, val);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	return mv3310_maybe_reset(phydev, MV_PCS_BASE_T, err > 0);
> 
> This helper is only introduced in patch 2.

Hmm, must have happened when I reordered the patches, and I hadn't
noticed.  Thanks, v2 coming soon.

-- 
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 net-next 2/3] net: phy: marvell10g: add energy detect power down tunable

[Russell King - ARM Linux admin]

On Tue, Mar 03, 2020 at 04:19:58PM +0100, Antoine Tenart wrote:
> On Tue, Mar 03, 2020 at 03:12:32PM +0000, Russell King - ARM Linux admin wrote:
> > On Tue, Mar 03, 2020 at 04:07:41PM +0100, Antoine Tenart wrote:
> > > On Tue, Mar 03, 2020 at 02:44:02PM +0000, Russell King wrote:
> > > >  drivers/net/phy/marvell10g.c | 111 ++++++++++++++++++++++++++++++++++-
> > > >  
> > > > +static int mv3310_maybe_reset(struct phy_device *phydev, u32 unit, bool reset)
> > > > +{
> > > > +	int retries, val, err;
> > > > +
> > > > +	if (!reset)
> > > > +		return 0;
> > > 
> > > You could also call mv3310_maybe_reset after testing the 'reset'
> > > condition, that would make it easier to read the code.
> > 
> > I'm not too convinced:
> > 
> > diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> > index ef1ed9415d9f..3daf73e61dff 100644
> > --- a/drivers/net/phy/marvell10g.c
> > +++ b/drivers/net/phy/marvell10g.c
> > @@ -279,13 +279,10 @@ static int mv3310_power_up(struct phy_device *phydev)
> >  				  MV_V2_PORT_CTRL_PWRDOWN);
> >  }
> >  
> > -static int mv3310_maybe_reset(struct phy_device *phydev, u32 unit, bool reset)
> > +static int mv3310_reset(struct phy_device *phydev, u32 unit)
> >  {
> >  	int retries, val, err;
> >  
> > -	if (!reset)
> > -		return 0;
> > -
> >  	err = phy_modify_mmd(phydev, MDIO_MMD_PCS, unit + MDIO_CTRL1,
> >  			     MDIO_CTRL1_RESET, MDIO_CTRL1_RESET);
> >  	if (err < 0)
> > @@ -684,10 +681,10 @@ static int mv3310_config_mdix(struct phy_device *phydev)
> >  
> >  	err = phy_modify_mmd_changed(phydev, MDIO_MMD_PCS, MV_PCS_CSCR1,
> >  				     MV_PCS_CSCR1_MDIX_MASK, val);
> > -	if (err < 0)
> > +	if (err <= 0)
> >  		return err;
> >  
> > -	return mv3310_maybe_reset(phydev, MV_PCS_BASE_T, err > 0);
> > +	return mv3310_reset(phydev, MV_PCS_BASE_T);
> >  }
> >  
> >  static int mv3310_config_aneg(struct phy_device *phydev)
> > 
> > The change from:
> > 
> > 	if (err < 0)
> > 
> > to:
> > 
> > 	if (err <= 0)
> > 
> > could easily be mistaken as a bug, and someone may decide to try to
> > "fix" that back to being the former instead.  The way I have the code
> > makes the intention explicit.
> 
> Using a single line to test both the error and the 'return 0'
> conditions, yes, I agree. Another solution would be to do something of
> the like:
> 
> 	phy_modify_mmd_changed()
> 	if (err < 0)
> 		return err;
> 
> 	if (err)
> 		mv3310_reset();
> 
> 	return 0;
> 
> I find it more readable, but this kind of thing is also a matter of
> personal taste.

Well, it either becomes:

        err = phy_modify_mmd_changed(phydev, MDIO_MMD_PCS, MV_PCS_CSCR1,
                                     MV_PCS_CSCR1_MDIX_MASK, val);
        if (err < 0)
                return err;

        if (err > 0)
                return mv3310_reset(phydev, MV_PCS_BASE_T);

        return 0;

or:

        err = phy_modify_mmd_changed(phydev, MDIO_MMD_PCS, MV_PCS_CSCR1,
                                     MV_PCS_CSCR1_MDIX_MASK, val);
        if (err > 0)
                err = mv3310_reset(phydev, MV_PCS_BASE_T);

        return err;

In the former case, we have two success-exit paths - one via a successful
mv3310_reset() and one by dropping through to the final return statement.

The latter case looks a bit better, at least to me.

-- 
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 net-next 2/3] net: phy: marvell10g: add energy detect power down tunable

[Antoine Tenart]

On Tue, Mar 03, 2020 at 03:30:13PM +0000, Russell King - ARM Linux admin wrote:
> On Tue, Mar 03, 2020 at 04:19:58PM +0100, Antoine Tenart wrote:
> > On Tue, Mar 03, 2020 at 03:12:32PM +0000, Russell King - ARM Linux admin wrote:
> > > On Tue, Mar 03, 2020 at 04:07:41PM +0100, Antoine Tenart wrote:
> > > > On Tue, Mar 03, 2020 at 02:44:02PM +0000, Russell King wrote:
> > > > >  drivers/net/phy/marvell10g.c | 111 ++++++++++++++++++++++++++++++++++-
> > > > >  
> > > > > +static int mv3310_maybe_reset(struct phy_device *phydev, u32 unit, bool reset)
> > > > > +{
> > > > > +	int retries, val, err;
> > > > > +
> > > > > +	if (!reset)
> > > > > +		return 0;
> > > > 
> > > > You could also call mv3310_maybe_reset after testing the 'reset'
> > > > condition, that would make it easier to read the code.
> > > 
> > > I'm not too convinced:
> > > 
> > > diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> > > index ef1ed9415d9f..3daf73e61dff 100644
> > > --- a/drivers/net/phy/marvell10g.c
> > > +++ b/drivers/net/phy/marvell10g.c
> > > @@ -279,13 +279,10 @@ static int mv3310_power_up(struct phy_device *phydev)
> > >  				  MV_V2_PORT_CTRL_PWRDOWN);
> > >  }
> > >  
> > > -static int mv3310_maybe_reset(struct phy_device *phydev, u32 unit, bool reset)
> > > +static int mv3310_reset(struct phy_device *phydev, u32 unit)
> > >  {
> > >  	int retries, val, err;
> > >  
> > > -	if (!reset)
> > > -		return 0;
> > > -
> > >  	err = phy_modify_mmd(phydev, MDIO_MMD_PCS, unit + MDIO_CTRL1,
> > >  			     MDIO_CTRL1_RESET, MDIO_CTRL1_RESET);
> > >  	if (err < 0)
> > > @@ -684,10 +681,10 @@ static int mv3310_config_mdix(struct phy_device *phydev)
> > >  
> > >  	err = phy_modify_mmd_changed(phydev, MDIO_MMD_PCS, MV_PCS_CSCR1,
> > >  				     MV_PCS_CSCR1_MDIX_MASK, val);
> > > -	if (err < 0)
> > > +	if (err <= 0)
> > >  		return err;
> > >  
> > > -	return mv3310_maybe_reset(phydev, MV_PCS_BASE_T, err > 0);
> > > +	return mv3310_reset(phydev, MV_PCS_BASE_T);
> > >  }
> > >  
> > >  static int mv3310_config_aneg(struct phy_device *phydev)
> > > 
> > > The change from:
> > > 
> > > 	if (err < 0)
> > > 
> > > to:
> > > 
> > > 	if (err <= 0)
> > > 
> > > could easily be mistaken as a bug, and someone may decide to try to
> > > "fix" that back to being the former instead.  The way I have the code
> > > makes the intention explicit.
> > 
> > Using a single line to test both the error and the 'return 0'
> > conditions, yes, I agree. Another solution would be to do something of
> > the like:
> > 
> > 	phy_modify_mmd_changed()
> > 	if (err < 0)
> > 		return err;
> > 
> > 	if (err)
> > 		mv3310_reset();
> > 
> > 	return 0;
> > 
> > I find it more readable, but this kind of thing is also a matter of
> > personal taste.
> 
> Well, it either becomes:
> 
>         err = phy_modify_mmd_changed(phydev, MDIO_MMD_PCS, MV_PCS_CSCR1,
>                                      MV_PCS_CSCR1_MDIX_MASK, val);
>         if (err < 0)
>                 return err;
> 
>         if (err > 0)
>                 return mv3310_reset(phydev, MV_PCS_BASE_T);
> 
>         return 0;
> 
> or:
> 
>         err = phy_modify_mmd_changed(phydev, MDIO_MMD_PCS, MV_PCS_CSCR1,
>                                      MV_PCS_CSCR1_MDIX_MASK, val);
>         if (err > 0)
>                 err = mv3310_reset(phydev, MV_PCS_BASE_T);
> 
>         return err;
> 
> In the former case, we have two success-exit paths - one via a successful
> mv3310_reset() and one by dropping through to the final return statement.
> 
> The latter case looks a bit better, at least to me.

I do agree, the latter looks good.

Thanks,
Antoine

-- 
Antoine Ténart, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com