[PATCH net-next 0/3] net: phy: marvell10g: implement suspend/resume callbacks

[PATCH net-next 0/3] net: phy: marvell10g: implement suspend/resume callbacks

[Antoine Tenart]

Hello,

This series implements the suspend/resume callbacks in the marvell10g
PHY driver:

- When the PHY isn't used, it is set in low power mode.
- At boot time we might now know the PHY status (as it's depending on
  the hardware configuration, or on what the previous stages
  configured), it is forced in low power.

Doing this prevents a PHY which was initialized in a previous stage from
negotiating with the link partner when a local port is down. If the PHY
isn't shutdown when a port is not used, the link partner's PHY may
report the link being up while it's not.

Thanks,
Antoine

Antoine Tenart (3):
  net: phy: marvell10g: implement suspend/resume callbacks
  net: phy: marvell10g: add the suspend/resume callbacks for the 88x2210
  net: phy: marvell10g: set the PHY in low power by default

 drivers/net/phy/marvell10g.c | 39 ++++++++++++++++++++++++++----------
 1 file changed, 28 insertions(+), 11 deletions(-)

-- 
2.20.1

[PATCH net-next 1/3] net: phy: marvell10g: implement suspend/resume callbacks

[Antoine Tenart]

This patch adds the suspend/resume callbacks for Marvell 10G PHYs. The
three PCS (base-t, base-r and 1000base-x) are set in low power (the PCS
are powered down) when the PHY isn't used.

Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---
 drivers/net/phy/marvell10g.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 79106e70010f..95aeeda0eb16 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -201,6 +201,30 @@ static int mv3310_hwmon_probe(struct phy_device *phydev)
 }
 #endif
 
+static int mv3310_suspend(struct phy_device *phydev)
+{
+	phy_modify_mmd(phydev, MDIO_MMD_PCS, MV_PCS_BASE_T + MDIO_CTRL1,
+		       MDIO_CTRL1_LPOWER, MDIO_CTRL1_LPOWER);
+	phy_modify_mmd(phydev, MDIO_MMD_PCS, MV_PCS_BASE_R + MDIO_CTRL1,
+		       MDIO_CTRL1_LPOWER, MDIO_CTRL1_LPOWER);
+	phy_modify_mmd(phydev, MDIO_MMD_PCS, MV_PCS_1000BASEX + MDIO_CTRL1,
+		       MDIO_CTRL1_LPOWER, MDIO_CTRL1_LPOWER);
+
+	return 0;
+}
+
+static int mv3310_resume(struct phy_device *phydev)
+{
+	phy_modify_mmd(phydev, MDIO_MMD_PCS, MV_PCS_BASE_T + MDIO_CTRL1,
+		       MDIO_CTRL1_LPOWER, 0);
+	phy_modify_mmd(phydev, MDIO_MMD_PCS, MV_PCS_BASE_R + MDIO_CTRL1,
+		       MDIO_CTRL1_LPOWER, 0);
+	phy_modify_mmd(phydev, MDIO_MMD_PCS, MV_PCS_1000BASEX + MDIO_CTRL1,
+		       MDIO_CTRL1_LPOWER, 0);
+
+	return mv3310_hwmon_config(phydev, true);
+}
+
 static int mv3310_probe(struct phy_device *phydev)
 {
 	struct mv3310_priv *priv;
-- 
2.20.1

[PATCH net-next 2/3] net: phy: marvell10g: add the suspend/resume callbacks for the 88x2210

[Antoine Tenart]

When the 88x2110 PHY support was added, the suspend and resume callbacks
were forgotten. This patch adds them to the 88x2110 PHY callback
definition.

Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---
 drivers/net/phy/marvell10g.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 95aeeda0eb16..e5d098bd33a6 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -498,6 +498,8 @@ static struct phy_driver mv3310_drivers[] = {
 		.name		= "mv88x2110",
 		.get_features	= genphy_c45_pma_read_abilities,
 		.probe		= mv3310_probe,
+		.suspend	= mv3310_suspend,
+		.resume		= mv3310_resume,
 		.soft_reset	= gen10g_no_soft_reset,
 		.config_init	= mv3310_config_init,
 		.config_aneg	= mv3310_config_aneg,
-- 
2.20.1

[PATCH net-next 3/3] net: phy: marvell10g: set the PHY in low power by default

[Antoine Tenart]

When the Marvell 10G PHYs are set out of reset, the LPOWER bit is set
depending on an hardware configuration choice. We also do not know what
is the PHY state at boot time. Hence, set the PHY in low power by
default when this driver probes.

Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
---
 drivers/net/phy/marvell10g.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index e5d098bd33a6..765edd34a7dd 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -245,17 +245,8 @@ static int mv3310_probe(struct phy_device *phydev)
 	if (ret)
 		return ret;
 
-	return 0;
-}
-
-static int mv3310_suspend(struct phy_device *phydev)
-{
-	return 0;
-}
-
-static int mv3310_resume(struct phy_device *phydev)
-{
-	return mv3310_hwmon_config(phydev, true);
+	/* Set the PHY in low power mode by default */
+	return mv3310_suspend(phydev);
 }
 
 /* Some PHYs in the Alaska family such as the 88X3310 and the 88E2010
-- 
2.20.1

Re: [PATCH net-next 3/3] net: phy: marvell10g: set the PHY in low power by default

[Andrew Lunn]

> Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
> ---
>  drivers/net/phy/marvell10g.c | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> index e5d098bd33a6..765edd34a7dd 100644
> --- a/drivers/net/phy/marvell10g.c
> +++ b/drivers/net/phy/marvell10g.c
> @@ -245,17 +245,8 @@ static int mv3310_probe(struct phy_device *phydev)
>  	if (ret)
>  		return ret;
>  
> -	return 0;
> -}
> -
> -static int mv3310_suspend(struct phy_device *phydev)
> -{
> -	return 0;
> -}
> -
> -static int mv3310_resume(struct phy_device *phydev)
> -{
> -	return mv3310_hwmon_config(phydev, true);

Hi Antoine

I'm confused.

Didn't patch 1 just add suspend and resume callback? And here you are
removing some other suspend and resume callbacks? Did we have two sets
for a short while?

    Andrew

Re: [PATCH net-next 3/3] net: phy: marvell10g: set the PHY in low power by default

[Antoine Tenart]

Hi Andrew,

On Thu, Feb 28, 2019 at 03:22:43PM +0100, Andrew Lunn wrote:
> > Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
> > ---
> >  drivers/net/phy/marvell10g.c | 13 ++-----------
> >  1 file changed, 2 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> > index e5d098bd33a6..765edd34a7dd 100644
> > --- a/drivers/net/phy/marvell10g.c
> > +++ b/drivers/net/phy/marvell10g.c
> > @@ -245,17 +245,8 @@ static int mv3310_probe(struct phy_device *phydev)
> >  	if (ret)
> >  		return ret;
> >  
> > -	return 0;
> > -}
> > -
> > -static int mv3310_suspend(struct phy_device *phydev)
> > -{
> > -	return 0;
> > -}
> > -
> > -static int mv3310_resume(struct phy_device *phydev)
> > -{
> > -	return mv3310_hwmon_config(phydev, true);
> 
> 
> I'm confused.
> 
> Didn't patch 1 just add suspend and resume callback? And here you are
> removing some other suspend and resume callbacks? Did we have two sets
> for a short while?

Indeed. I just check and what happened is I initially had the
suspend/resume callbacks implementation and this patch into the same
patch. And then I made a mistake when splitting them, leaving two
suspend/resume functions, and I did not see that when reviewing the
series...

Sorry about that, I'll fix it!

Thanks,
Antoine

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