[PATCH v2 net] net: phy: aquantia: wait for the suspend/resume operations to finish

[PATCH v2 net] net: phy: aquantia: wait for the suspend/resume operations to finish

[Ioana Ciornei]

The Aquantia datasheet notes that after issuing a Processor-Intensive
MDIO operation, like changing the low-power state of the device, the
driver should wait for the operation to finish before issuing a new MDIO
command.

The new aqr107_wait_processor_intensive_op() function is added which can
be used after these kind of MDIO operations. At the moment, we are only
adding it at the end of the suspend/resume calls.

The issue was identified on a board featuring the AQR113C PHY, on
which commands like 'ip link (..) up / down' issued without any delays
between them would render the link on the PHY to remain down.
The issue was easy to reproduce with a one-liner:
 $ ip link set dev ethX down; ip link set dev ethX up; \
 ip link set dev ethX down; ip link set dev ethX up;

Fixes: ac9e81c230eb ("net: phy: aquantia: add suspend / resume callbacks for AQR107 family")
Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
---
Changes in v2:
 - use phy_read_mmd_poll_timeout instead of readx_poll_timeout
 - increase a bit the sleep and timeout values for the poll

 drivers/net/phy/aquantia_main.c | 53 ++++++++++++++++++++++++++++++---
 1 file changed, 49 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c
index 8b7a46db30e0..7111e2e958e9 100644
--- a/drivers/net/phy/aquantia_main.c
+++ b/drivers/net/phy/aquantia_main.c
@@ -91,6 +91,9 @@
 #define VEND1_GLOBAL_FW_ID_MAJOR		GENMASK(15, 8)
 #define VEND1_GLOBAL_FW_ID_MINOR		GENMASK(7, 0)
 
+#define VEND1_GLOBAL_GEN_STAT2			0xc831
+#define VEND1_GLOBAL_GEN_STAT2_OP_IN_PROG	BIT(15)
+
 #define VEND1_GLOBAL_RSVD_STAT1			0xc885
 #define VEND1_GLOBAL_RSVD_STAT1_FW_BUILD_ID	GENMASK(7, 4)
 #define VEND1_GLOBAL_RSVD_STAT1_PROV_ID		GENMASK(3, 0)
@@ -125,6 +128,12 @@
 #define VEND1_GLOBAL_INT_VEND_MASK_GLOBAL2	BIT(1)
 #define VEND1_GLOBAL_INT_VEND_MASK_GLOBAL3	BIT(0)
 
+/* Sleep and timeout for checking if the Processor-Intensive
+ * MDIO operation is finished
+ */
+#define AQR107_OP_IN_PROG_SLEEP		1000
+#define AQR107_OP_IN_PROG_TIMEOUT	100000
+
 struct aqr107_hw_stat {
 	const char *name;
 	int reg;
@@ -597,16 +606,52 @@ static void aqr107_link_change_notify(struct phy_device *phydev)
 		phydev_info(phydev, "Aquantia 1000Base-T2 mode active\n");
 }
 
+static int aqr107_wait_processor_intensive_op(struct phy_device *phydev)
+{
+	int val, err;
+
+	/* The datasheet notes to wait at least 1ms after issuing a
+	 * processor intensive operation before checking.
+	 * We cannot use the 'sleep_before_read' parameter of read_poll_timeout
+	 * because that just determines the maximum time slept, not the minimum.
+	 */
+	usleep_range(1000, 5000);
+
+	err = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_VEND1,
+					VEND1_GLOBAL_GEN_STAT2, val,
+					!(val & VEND1_GLOBAL_GEN_STAT2_OP_IN_PROG),
+					AQR107_OP_IN_PROG_SLEEP,
+					AQR107_OP_IN_PROG_TIMEOUT, false);
+	if (err) {
+		phydev_err(phydev, "timeout: processor-intensive MDIO operation\n");
+		return err;
+	}
+
+	return 0;
+}
+
 static int aqr107_suspend(struct phy_device *phydev)
 {
-	return phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, MDIO_CTRL1,
-				MDIO_CTRL1_LPOWER);
+	int err;
+
+	err = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, MDIO_CTRL1,
+			       MDIO_CTRL1_LPOWER);
+	if (err)
+		return err;
+
+	return aqr107_wait_processor_intensive_op(phydev);
 }
 
 static int aqr107_resume(struct phy_device *phydev)
 {
-	return phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1, MDIO_CTRL1,
-				  MDIO_CTRL1_LPOWER);
+	int err;
+
+	err = phy_clear_bits_mmd(phydev, MDIO_MMD_VEND1, MDIO_CTRL1,
+				 MDIO_CTRL1_LPOWER);
+	if (err)
+		return err;
+
+	return aqr107_wait_processor_intensive_op(phydev);
 }
 
 static int aqr107_probe(struct phy_device *phydev)
-- 
2.33.1

Re: [PATCH v2 net] net: phy: aquantia: wait for the suspend/resume operations to finish

[Andrew Lunn]

> +static int aqr107_wait_processor_intensive_op(struct phy_device *phydev)
> +{
> +	int val, err;
> +
> +	/* The datasheet notes to wait at least 1ms after issuing a
> +	 * processor intensive operation before checking.
> +	 * We cannot use the 'sleep_before_read' parameter of read_poll_timeout
> +	 * because that just determines the maximum time slept, not the minimum.
> +	 */
> +	usleep_range(1000, 5000);
> +
> +	err = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_VEND1,
> +					VEND1_GLOBAL_GEN_STAT2, val,
> +					!(val & VEND1_GLOBAL_GEN_STAT2_OP_IN_PROG),
> +					AQR107_OP_IN_PROG_SLEEP,
> +					AQR107_OP_IN_PROG_TIMEOUT, false);
> +	if (err) {
> +		phydev_err(phydev, "timeout: processor-intensive MDIO operation\n");
> +		return err;
> +	}
> +
> +	return 0;

nitpick: You could simplify this to:

> +	if (err)
> +		phydev_err(phydev, "timeout: processor-intensive MDIO operation\n");
> +
> +	return err;
> +}

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v2 net] net: phy: aquantia: wait for the suspend/resume operations to finish

[Ioana Ciornei]

On Tue, Sep 06, 2022 at 03:11:06PM +0200, Andrew Lunn wrote:
> > +static int aqr107_wait_processor_intensive_op(struct phy_device *phydev)
> > +{
> > +	int val, err;
> > +
> > +	/* The datasheet notes to wait at least 1ms after issuing a
> > +	 * processor intensive operation before checking.
> > +	 * We cannot use the 'sleep_before_read' parameter of read_poll_timeout
> > +	 * because that just determines the maximum time slept, not the minimum.
> > +	 */
> > +	usleep_range(1000, 5000);
> > +
> > +	err = phy_read_mmd_poll_timeout(phydev, MDIO_MMD_VEND1,
> > +					VEND1_GLOBAL_GEN_STAT2, val,
> > +					!(val & VEND1_GLOBAL_GEN_STAT2_OP_IN_PROG),
> > +					AQR107_OP_IN_PROG_SLEEP,
> > +					AQR107_OP_IN_PROG_TIMEOUT, false);
> > +	if (err) {
> > +		phydev_err(phydev, "timeout: processor-intensive MDIO operation\n");
> > +		return err;
> > +	}
> > +
> > +	return 0;
> 
> nitpick: You could simplify this to:
> 
> > +	if (err)
> > +		phydev_err(phydev, "timeout: processor-intensive MDIO operation\n");
> > +
> > +	return err;
> > +}
> 

I would prefer to leave it as it is. I know that in this particular case
we could spare the braces but in general I am not so keen of this form
since if there is more to add to the function, it would force us to also
re-add the return statement.

> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> 
>     Andrew

Thanks,
Ioana

Re: [PATCH v2 net] net: phy: aquantia: wait for the suspend/resume operations to finish

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by Paolo Abeni <pabeni@redhat.com>:

On Tue,  6 Sep 2022 16:04:51 +0300 you wrote:
> The Aquantia datasheet notes that after issuing a Processor-Intensive
> MDIO operation, like changing the low-power state of the device, the
> driver should wait for the operation to finish before issuing a new MDIO
> command.
> 
> The new aqr107_wait_processor_intensive_op() function is added which can
> be used after these kind of MDIO operations. At the moment, we are only
> adding it at the end of the suspend/resume calls.
> 
> [...]

Here is the summary with links:
  - [v2,net] net: phy: aquantia: wait for the suspend/resume operations to finish
    https://git.kernel.org/netdev/net/c/ca2dccdeeb49

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html