| Message ID | 20190326145302.9563-2-antoine.tenart@bootlin.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: phy: marvell10g: implement suspend/resume callbacks | expand |
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); > } > >
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); > } > >
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
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
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); }