| Message ID | 20220906130451.1483448-1-ioana.ciornei@nxp.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ca2dccdeeb49a7e408112d681bf447984c845292 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v2,net] net: phy: aquantia: wait for the suspend/resume operations to finish | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net |
| netdev/fixes_present | success | Fixes tag present in non-next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Single patches do not need cover letters |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 9 of 9 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | Fixes tag looks correct |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | warning | WARNING: line length of 83 exceeds 80 columns |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
> +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
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
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!
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)
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(-)