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

[PATCH net-next v2 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

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 (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 v2 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 | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 79106e70010f..92462344c5ad 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

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

[Andrew Lunn]

On Fri, Mar 01, 2019 at 12:00:45PM +0100, 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>

    Andrew

[PATCH net-next v2 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 92462344c5ad..4f6f96080182 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	= gen10g_no_soft_reset,
 		.config_init	= mv3310_config_init,
 		.config_aneg	= mv3310_config_aneg,
-- 
2.20.1

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

[Andrew Lunn]

On Fri, Mar 01, 2019 at 12:00:46PM +0100, Antoine Tenart wrote:
> 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>

    Andrew

[PATCH net-next v2 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 | 47 ++++++++++++++++++------------------
 1 file changed, 24 insertions(+), 23 deletions(-)

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 4f6f96080182..765edd34a7dd 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -201,29 +201,6 @@ static int mv3310_hwmon_probe(struct phy_device *phydev)
 }
 #endif
 
-static int mv3310_probe(struct phy_device *phydev)
-{
-	struct mv3310_priv *priv;
-	u32 mmd_mask = MDIO_DEVS_PMAPMD | MDIO_DEVS_AN;
-	int ret;
-
-	if (!phydev->is_c45 ||
-	    (phydev->c45_ids.devices_in_package & mmd_mask) != mmd_mask)
-		return -ENODEV;
-
-	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
-	if (!priv)
-		return -ENOMEM;
-
-	dev_set_drvdata(&phydev->mdio.dev, priv);
-
-	ret = mv3310_hwmon_probe(phydev);
-	if (ret)
-		return ret;
-
-	return 0;
-}
-
 static int mv3310_suspend(struct phy_device *phydev)
 {
 	phy_modify_mmd(phydev, MDIO_MMD_PCS, MV_PCS_BASE_T + MDIO_CTRL1,
@@ -248,6 +225,30 @@ static int mv3310_resume(struct phy_device *phydev)
 	return mv3310_hwmon_config(phydev, true);
 }
 
+static int mv3310_probe(struct phy_device *phydev)
+{
+	struct mv3310_priv *priv;
+	u32 mmd_mask = MDIO_DEVS_PMAPMD | MDIO_DEVS_AN;
+	int ret;
+
+	if (!phydev->is_c45 ||
+	    (phydev->c45_ids.devices_in_package & mmd_mask) != mmd_mask)
+		return -ENODEV;
+
+	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	dev_set_drvdata(&phydev->mdio.dev, priv);
+
+	ret = mv3310_hwmon_probe(phydev);
+	if (ret)
+		return ret;
+
+	/* 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
  * don't set bit 14 in PMA Extended Abilities (1.11), although they do
  * support 2.5GBASET and 5GBASET. For these models, we can still read their
-- 
2.20.1

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

[Andrew Lunn]

On Fri, Mar 01, 2019 at 12:00:47PM +0100, Antoine Tenart wrote:
> 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.

Hi Antoine

Florian did some work for c22 PHYs so that the existing link state
could be used at boot. So for example, the bootloader configured the
PHY up and it got link, there is no need to down/up the PHY when linux
takes control. The networking comes up faster that way.

Can this work for this PHY?

    Andrew

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

[Antoine Tenart]

Hi Andrew,

On Fri, Mar 01, 2019 at 03:19:53PM +0100, Andrew Lunn wrote:
> On Fri, Mar 01, 2019 at 12:00:47PM +0100, Antoine Tenart wrote:
> > 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.
> 
> Florian did some work for c22 PHYs so that the existing link state
> could be used at boot. So for example, the bootloader configured the
> PHY up and it got link, there is no need to down/up the PHY when linux
> takes control. The networking comes up faster that way.
> 
> Can this work for this PHY?

This use case (the bootloader configures the PHY, Linux boots and sets
an interface using this PHY up) would work, and is what's happening in
some situations right now (the 3310 reset is never asserted prior to
this series).

But consider this case (let's say we use a 10G link):

  ----------------               ----------------
  |    Board 1   |               |    Board 2   |
  | MAC — 3310 — | — SFP cable — | — 3310 — MAC |
  ----------------               ----------------

Board 1: The userspace do not set the interface up. The MAC is in reset
         (default state during the MAC driver probe), the PHY was
	 configured by the bootloader.
Board 2: The userspace set the interface up. The MAC is configured, the
         PHY is configured as well.

The two PHY's PCS will establish a link and report it as being up. In
this case, phylink's AN mode is MLO_AN_PHY and thus will report the
overall link as being the PHY's link status: up.

My understanding is that the issue arises because the PHYs were never
set in reset, or low power, and thus act as if the user wanted the port
to be up. As the default behaviour for networking ports is to be down at
boot, I thought to set the PHY as well in a default low power state.

Thanks!
Antoine

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

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

[Florian Fainelli]

On 3/1/2019 7:07 AM, Antoine Tenart wrote:
> Hi Andrew,
> 
> On Fri, Mar 01, 2019 at 03:19:53PM +0100, Andrew Lunn wrote:
>> On Fri, Mar 01, 2019 at 12:00:47PM +0100, Antoine Tenart wrote:
>>> 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.
>>
>> Florian did some work for c22 PHYs so that the existing link state
>> could be used at boot. So for example, the bootloader configured the
>> PHY up and it got link, there is no need to down/up the PHY when linux
>> takes control. The networking comes up faster that way.
>>
>> Can this work for this PHY?
> 
> This use case (the bootloader configures the PHY, Linux boots and sets
> an interface using this PHY up) would work, and is what's happening in
> some situations right now (the 3310 reset is never asserted prior to
> this series).
> 
> But consider this case (let's say we use a 10G link):
> 
>   ----------------               ----------------
>   |    Board 1   |               |    Board 2   |
>   | MAC — 3310 — | — SFP cable — | — 3310 — MAC |
>   ----------------               ----------------
> 
> Board 1: The userspace do not set the interface up. The MAC is in reset
>          (default state during the MAC driver probe), the PHY was
> 	 configured by the bootloader.
> Board 2: The userspace set the interface up. The MAC is configured, the
>          PHY is configured as well.
> 
> The two PHY's PCS will establish a link and report it as being up. In
> this case, phylink's AN mode is MLO_AN_PHY and thus will report the
> overall link as being the PHY's link status: up.
> 
> My understanding is that the issue arises because the PHYs were never
> set in reset, or low power, and thus act as if the user wanted the port
> to be up. As the default behaviour for networking ports is to be down at
> boot, I thought to set the PHY as well in a default low power state.

The policy you are creating here for the marvell10g driver is entirely
applicable to any PHY <=> PHY configuration where either of the two
software agents on Board 1 or Board 2 has not had a chance to bring-up
its bootloader/OS/applications to control the PHY.

A number of PHYs come up fully on (or in isolate or super isolate mode)
and will AN with their link partner if connected. For some people it's a
feature, for some it is a waste of power. I don't necessarily have an
issue with your patch per-se, but it does create an one off behavior
that other PHY drivers may not follow.
-- 
Florian

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

[Antoine Tenart]

Hi Florian,

On Fri, Mar 01, 2019 at 07:08:56PM -0800, Florian Fainelli wrote:
> On 3/1/2019 7:07 AM, Antoine Tenart wrote:
> > On Fri, Mar 01, 2019 at 03:19:53PM +0100, Andrew Lunn wrote:
> >> On Fri, Mar 01, 2019 at 12:00:47PM +0100, Antoine Tenart wrote:
> >>> 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.
> >>
> >> Florian did some work for c22 PHYs so that the existing link state
> >> could be used at boot. So for example, the bootloader configured the
> >> PHY up and it got link, there is no need to down/up the PHY when linux
> >> takes control. The networking comes up faster that way.
> >>
> >> Can this work for this PHY?
> > 
> > This use case (the bootloader configures the PHY, Linux boots and sets
> > an interface using this PHY up) would work, and is what's happening in
> > some situations right now (the 3310 reset is never asserted prior to
> > this series).
> > 
> > But consider this case (let's say we use a 10G link):
> > 
> >   ----------------               ----------------
> >   |    Board 1   |               |    Board 2   |
> >   | MAC — 3310 — | — SFP cable — | — 3310 — MAC |
> >   ----------------               ----------------
> > 
> > Board 1: The userspace do not set the interface up. The MAC is in reset
> >          (default state during the MAC driver probe), the PHY was
> > 	 configured by the bootloader.
> > Board 2: The userspace set the interface up. The MAC is configured, the
> >          PHY is configured as well.
> > 
> > The two PHY's PCS will establish a link and report it as being up. In
> > this case, phylink's AN mode is MLO_AN_PHY and thus will report the
> > overall link as being the PHY's link status: up.
> > 
> > My understanding is that the issue arises because the PHYs were never
> > set in reset, or low power, and thus act as if the user wanted the port
> > to be up. As the default behaviour for networking ports is to be down at
> > boot, I thought to set the PHY as well in a default low power state.
> 
> The policy you are creating here for the marvell10g driver is entirely
> applicable to any PHY <=> PHY configuration where either of the two
> software agents on Board 1 or Board 2 has not had a chance to bring-up
> its bootloader/OS/applications to control the PHY.

Right.

> A number of PHYs come up fully on (or in isolate or super isolate mode)
> and will AN with their link partner if connected. For some people it's a
> feature, for some it is a waste of power. I don't necessarily have an
> issue with your patch per-se, but it does create an one off behavior
> that other PHY drivers may not follow.

I agree having a per-driver behaviour is not something we want. As I
understand it, there is no behaviour enforced currently regarding this
matter. I agree both cases have their pros and cons:
- It's weird to have an interface reporting being UP when it's not
  really.
- Having the link come up faster can be a feature.

I have some questions then:
- Do you think calling suspend() in the core when probing a PHY driver
  would work for all PHYs?
- Would a new Kconfig option selecting the default behaviour at boot
  time be a solution?
- Or this is a WONTFIX kind of (small) issues? :)

Thanks!
Antoine

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

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

[Russell King - ARM Linux admin]

On Mon, Mar 04, 2019 at 11:47:00AM +0100, Antoine Tenart wrote:
> Hi Florian,
> 
> I agree having a per-driver behaviour is not something we want. As I
> understand it, there is no behaviour enforced currently regarding this
> matter. I agree both cases have their pros and cons:
> - It's weird to have an interface reporting being UP when it's not
>   really.

What about when an interface is listening for wake-on-lan packets?

Let's say board 1 is powered down and has WoL enabled.  Board 2 is
as per your configuration.  Board 2's interface reports that the
link is up.  Most of the packets that would be sent out the
interface end up disappearing into a black hole in the same way
as your original scenario.

How is this "weird" ?

There are many cases where exactly this happens - you are trying to
make one particular scenario behave "better" without considering
whether it's possible with all or even the majority of scenarios.

The only case where what you're suggesting makes sense is a point-to-
point link between two systems, which is not the norm.

More than that, when board 1 boots, initially the link will be up
from reset.  When the kernel eventually boots with your patch, the
link will then go down until board 1 configures the interface.  So,
board 2 sees that the interface comes up, and could assume that
board 1 is alive and well - but it isn't because (eg) it's in the
boot loader.

Basically, what I'm pointing out is that even in your minority
scenario, reasoning that board 2 should not see "link up" status
until board 1's interface is configured is not reasonable.

The link status is about the physical connectivity on the local
interface, it is not about the link between the interfaces on the
two machines.

-- 
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 v2 3/3] net: phy: marvell10g: set the PHY in low power by default

[Antoine Tenart]

Hi Russell,

On Mon, Mar 04, 2019 at 04:10:47PM +0000, Russell King - ARM Linux admin wrote:
> On Mon, Mar 04, 2019 at 11:47:00AM +0100, Antoine Tenart wrote:
> > 
> > I agree having a per-driver behaviour is not something we want. As I
> > understand it, there is no behaviour enforced currently regarding this
> > matter. I agree both cases have their pros and cons:
> > - It's weird to have an interface reporting being UP when it's not
> >   really.
> 
> What about when an interface is listening for wake-on-lan packets?
> 
> Let's say board 1 is powered down and has WoL enabled.  Board 2 is
> as per your configuration.  Board 2's interface reports that the
> link is up.  Most of the packets that would be sent out the
> interface end up disappearing into a black hole in the same way
> as your original scenario.

I see your point, in the WoL case the link would be reported as being up
as the PHY on board 1 would be functional (at least to process WoL
packets) so the link between the two PHYs would be established and
reporting a link being UP, but not entirely functional.

That's kind of a fair point, but not exactly the same. In such case some
packets might have an effect, whereas in the case I described earlier
not a single packet would make sense. But also I might not be aware of
other valid cases.

> The only case where what you're suggesting makes sense is a point-to-
> point link between two systems, which is not the norm.
> 
> More than that, when board 1 boots, initially the link will be up
> from reset.  When the kernel eventually boots with your patch, the
> link will then go down until board 1 configures the interface.  So,
> board 2 sees that the interface comes up, and could assume that
> board 1 is alive and well - but it isn't because (eg) it's in the
> boot loader.

This depends on the bootloader, but OK, that is what's happening in this
very case. I'm not fully convinced this relates to the scenario I
described. The link would be up and you're right could be not functional
because the way the bootloader works. And this is temporary.

> Basically, what I'm pointing out is that even in your minority
> scenario, reasoning that board 2 should not see "link up" status
> until board 1's interface is configured is not reasonable.
> 
> The link status is about the physical connectivity on the local
> interface, it is not about the link between the interfaces on the
> two machines.

Right. The thing I find odd here is there is no physical connectivity
between the MACs (one is in reset), only between one MAC and the LP's
PHY. Given your definition of a link being up "between the interfaces",
OK, that makes sense if we talk about the PHYs. It also means we do not
have the same behaviour at boot time between an interface using a PHY,
and one directly connected to serdes lanes.

So, I don't have a strong opinion about this, and it's not a big issue
either way. I think the discussion moved into what should be the default
state of a PHY a probe time, as it seems there are no policy being
enforced right now.

Thanks,
Antoine

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

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

[David Miller]

From: Antoine Tenart <antoine.tenart@bootlin.com>
Date: Fri,  1 Mar 2019 12:00:44 +0100

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

Looks like this needs some more discussion and Russell has some
concerns still.

So deferring to the next merge window, sorry.