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

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

[Antoine Tenart]

Hello,

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

Thanks,
Antoine

Since v2:
  - Removed the third patch, setting the PHY in low power by default, as
    the change was controversial.
  - Rebased on the latest net-next.

Since v1:
  - Fixed a mix up in the patches where two implementations of the
    suspend/resume callbacks were kept in the driver.
  - Rebased on the latest net-next.

Antoine Tenart (2):
  net: phy: marvell10g: implement suspend/resume callbacks
  net: phy: marvell10g: add the suspend/resume callbacks for the 88x2210

 drivers/net/phy/marvell10g.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

-- 
2.20.1

[PATCH net-next v3 1/2] 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>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/phy/marvell10g.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 100b401b1f4a..b56cd35182d5 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -226,11 +226,25 @@ static int mv3310_probe(struct phy_device *phydev)
 
 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);
 }
 
-- 
2.20.1

[PATCH net-next v3 2/2] 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>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 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 b56cd35182d5..08dd2ea08236 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -488,6 +488,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	= genphy_no_soft_reset,
 		.config_init	= mv3310_config_init,
 		.config_aneg	= mv3310_config_aneg,
-- 
2.20.1

Re: [PATCH net-next v3 1/2] net: phy: marvell10g: implement suspend/resume callbacks

[Heiner Kallweit]

On 26.03.2019 15:53, Antoine Tenart wrote:
> 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.
> 
At least for the 3310 the datasheet mentions that via VEND2.f001.11
the port can be powered down completely. As this would simplify the code,
did you test this?

> Signed-off-by: Antoine Tenart <antoine.tenart@bootlin.com>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/net/phy/marvell10g.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> index 100b401b1f4a..b56cd35182d5 100644
> --- a/drivers/net/phy/marvell10g.c
> +++ b/drivers/net/phy/marvell10g.c
> @@ -226,11 +226,25 @@ static int mv3310_probe(struct phy_device *phydev)
>  
>  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);
>  }
>  
> 

Re: [PATCH net-next v3 0/2] net: phy: marvell10g: implement suspend/resume callbacks

[David Miller]

From: Antoine Tenart <antoine.tenart@bootlin.com>
Date: Tue, 26 Mar 2019 15:53:00 +0100

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

Heiner gave you feedback, you have not responded.

Therefore I am removing your patches from my queue.

Re: [PATCH net-next v3 1/2] net: phy: marvell10g: implement suspend/resume callbacks

[Heiner Kallweit]

On 26.03.2019 15:53, Antoine Tenart wrote:
> 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>
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/net/phy/marvell10g.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> index 100b401b1f4a..b56cd35182d5 100644
> --- a/drivers/net/phy/marvell10g.c
> +++ b/drivers/net/phy/marvell10g.c
> @@ -226,11 +226,25 @@ static int mv3310_probe(struct phy_device *phydev)
>  
>  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);
> +
When you re-submit this series, in addition to my previous comment:
Instead of phy_modify_mmd() it would be clearer to use
phy_set/clear_bits_mmd(). And checking the return value wouldn't hurt.

>  	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);
>  }
>  
> 

Re: [PATCH net-next v3 1/2] net: phy: marvell10g: implement suspend/resume callbacks

[Antoine Tenart]

Hi Heiner,

Sorry for the late answer.

On Tue, Mar 26, 2019 at 07:43:27PM +0100, Heiner Kallweit wrote:
> On 26.03.2019 15:53, Antoine Tenart wrote:
> > 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.
> > 
> At least for the 3310 the datasheet mentions that via VEND2.f001.11
> the port can be powered down completely. As this would simplify the code,
> did you test this?

I just tested this, and it does simplify the code a lot. Thanks for
pointing out this bit.

I'll update and make a v2.

Antoine

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

Re: [PATCH net-next v3 1/2] net: phy: marvell10g: implement suspend/resume callbacks

[Antoine Tenart]

Hi Heiner,

On Fri, Mar 29, 2019 at 10:32:02PM +0100, Heiner Kallweit wrote:
> On 26.03.2019 15:53, Antoine Tenart wrote:
> > 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>
> > Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> > ---
> >  drivers/net/phy/marvell10g.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
> > index 100b401b1f4a..b56cd35182d5 100644
> > --- a/drivers/net/phy/marvell10g.c
> > +++ b/drivers/net/phy/marvell10g.c
> > @@ -226,11 +226,25 @@ static int mv3310_probe(struct phy_device *phydev)
> >  
> >  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);
> > +
> When you re-submit this series, in addition to my previous comment:
> Instead of phy_modify_mmd() it would be clearer to use
> phy_set/clear_bits_mmd(). And checking the return value wouldn't hurt.

OK, will do.

Thanks!
Antoine

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