2020-03-22 17:50:35

by Dejin Zheng

[permalink] [raw]
Subject: [PATCH net-next v5 00/10] introduce read_poll_timeout

This patch sets is introduce read_poll_timeout macro, it is an extension
of readx_poll_timeout macro. the accessor function op just supports only
one parameter in the readx_poll_timeout macro, but this macro can
supports multiple variable parameters for it. so functions like
phy_read(struct phy_device *phydev, u32 regnum) and
phy_read_mmd(struct phy_device *phydev, int devad, u32 regnum) can
use this poll timeout framework.

the first patch introduce read_poll_timeout macro, and the second patch
redefined readx_poll_timeout macro by read_poll_timeout(), and the other
patches are examples using read_poll_timeout macro.

v4 -> v5:
- add some msleep() before call phy_read_mmd_poll_timeout() to
keep the code more similar in patch 6 and patch 9.
- add a patch of drop by v4, it can add msleep before call
phy_read_poll_timeout() to keep the code more similar.
v3 -> v4:
- add 3 examples of using new functions.
- deal with precedence issues for parameter cond.
- drop a patch about phy_poll_reset() function.
v2 -> v3:
- modify the parameter order of newly added functions.
phy_read_mmd_poll_timeout(val, cond, sleep_us, timeout_us, \
phydev, devaddr, regnum)
||
\/
phy_read_mmd_poll_timeout(phydev, devaddr regnum, val, cond, \
sleep_us, timeout_us)

phy_read_poll_timeout(val, cond, sleep_us, timeout_us, \
phydev, regnum)
||
\/
phy_read_poll_timeout(phydev, regnum, val, cond, sleep_us, \
timeout_us)
v1 -> v2:
- passed a phydev, device address and a reg to replace args...
parameter in phy_read_mmd_poll_timeout() by Andrew Lunn 's
suggestion in patch 3. Andrew Lunn <[email protected]>, Thanks
very much for your help!
- also in patch 3, handle phy_read_mmd return an error(the return
value < 0) in phy_read_mmd_poll_timeout(). Thanks Andrew
again.
- in patch 6, pass a phydev and a reg to replace args...
parameter in phy_read_poll_timeout(), and also handle the
phy_read() function's return error.

Dejin Zheng (10):
iopoll: introduce read_poll_timeout macro
iopoll: redefined readx_poll_timeout macro to simplify the code
net: phy: introduce phy_read_mmd_poll_timeout macro
net: phy: bcm84881: use phy_read_mmd_poll_timeout() to simplify the
code
net: phy: aquantia: use phy_read_mmd_poll_timeout() to simplify the
code
net: phy: marvell10g: use phy_read_mmd_poll_timeout() to simplify the
code
net: phy: introduce phy_read_poll_timeout macro
net: phy: use phy_read_poll_timeout() to simplify the code
net: phy: smsc: use phy_read_poll_timeout() to simplify the code
net: phy: tja11xx: use phy_read_poll_timeout() to simplify the code

drivers/net/phy/aquantia_main.c | 13 ++++--------
drivers/net/phy/bcm84881.c | 27 ++++---------------------
drivers/net/phy/marvell10g.c | 16 ++++++---------
drivers/net/phy/nxp-tja11xx.c | 16 +++------------
drivers/net/phy/phy_device.c | 17 +++++-----------
drivers/net/phy/smsc.c | 17 ++++++----------
include/linux/iopoll.h | 36 ++++++++++++++++++++++++++-------
include/linux/phy.h | 28 +++++++++++++++++++++++++
8 files changed, 85 insertions(+), 85 deletions(-)

--
2.25.0


2020-03-22 17:50:44

by Dejin Zheng

[permalink] [raw]
Subject: [PATCH net-next v5 01/10] iopoll: introduce read_poll_timeout macro

this macro is an extension of readx_poll_timeout macro. the accessor
function op just supports only one parameter in the readx_poll_timeout
macro, but this macro can supports multiple variable parameters for
it. so functions like phy_read(struct phy_device *phydev, u32 regnum)
and phy_read_mmd(struct phy_device *phydev, int devad, u32 regnum) can
also use this poll timeout framework.

Signed-off-by: Dejin Zheng <[email protected]>
---
v4 -> v5:
- no changed
v3 -> v4:
- no changed
v2 -> v3:
- no changed
v1 -> v2:
- no changed

include/linux/iopoll.h | 40 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 40 insertions(+)

diff --git a/include/linux/iopoll.h b/include/linux/iopoll.h
index 35e15dfd4155..7d44a2e20267 100644
--- a/include/linux/iopoll.h
+++ b/include/linux/iopoll.h
@@ -13,6 +13,46 @@
#include <linux/errno.h>
#include <linux/io.h>

+/**
+ * read_poll_timeout - Periodically poll an address until a condition is
+ * met or a timeout occurs
+ * @op: accessor function (takes @args as its arguments)
+ * @val: Variable to read the value into
+ * @cond: Break condition (usually involving @val)
+ * @sleep_us: Maximum time to sleep between reads in us (0
+ * tight-loops). Should be less than ~20ms since usleep_range
+ * is used (see Documentation/timers/timers-howto.rst).
+ * @timeout_us: Timeout in us, 0 means never timeout
+ * @args: arguments for @op poll
+ *
+ * Returns 0 on success and -ETIMEDOUT upon a timeout. In either
+ * case, the last read value at @args is stored in @val. Must not
+ * be called from atomic context if sleep_us or timeout_us are used.
+ *
+ * When available, you'll probably want to use one of the specialized
+ * macros defined below rather than this macro directly.
+ */
+#define read_poll_timeout(op, val, cond, sleep_us, timeout_us, args...) \
+({ \
+ u64 __timeout_us = (timeout_us); \
+ unsigned long __sleep_us = (sleep_us); \
+ ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \
+ might_sleep_if((__sleep_us) != 0); \
+ for (;;) { \
+ (val) = op(args); \
+ if (cond) \
+ break; \
+ if (__timeout_us && \
+ ktime_compare(ktime_get(), __timeout) > 0) { \
+ (val) = op(args); \
+ break; \
+ } \
+ if (__sleep_us) \
+ usleep_range((__sleep_us >> 2) + 1, __sleep_us); \
+ } \
+ (cond) ? 0 : -ETIMEDOUT; \
+})
+
/**
* readx_poll_timeout - Periodically poll an address until a condition is met or a timeout occurs
* @op: accessor function (takes @addr as its only argument)
--
2.25.0

2020-03-22 17:50:49

by Dejin Zheng

[permalink] [raw]
Subject: [PATCH net-next v5 02/10] iopoll: redefined readx_poll_timeout macro to simplify the code

redefined readx_poll_timeout macro by read_poll_timeout to
simplify the code.

Signed-off-by: Dejin Zheng <[email protected]>
---
v4 -> v5:
- no changed
v3 -> v4:
- no changed
v2 -> v3:
- no changed
v1 -> v2:
- no changed

include/linux/iopoll.h | 20 +-------------------
1 file changed, 1 insertion(+), 19 deletions(-)

diff --git a/include/linux/iopoll.h b/include/linux/iopoll.h
index 7d44a2e20267..29c016cd6249 100644
--- a/include/linux/iopoll.h
+++ b/include/linux/iopoll.h
@@ -72,25 +72,7 @@
* macros defined below rather than this macro directly.
*/
#define readx_poll_timeout(op, addr, val, cond, sleep_us, timeout_us) \
-({ \
- u64 __timeout_us = (timeout_us); \
- unsigned long __sleep_us = (sleep_us); \
- ktime_t __timeout = ktime_add_us(ktime_get(), __timeout_us); \
- might_sleep_if((__sleep_us) != 0); \
- for (;;) { \
- (val) = op(addr); \
- if (cond) \
- break; \
- if (__timeout_us && \
- ktime_compare(ktime_get(), __timeout) > 0) { \
- (val) = op(addr); \
- break; \
- } \
- if (__sleep_us) \
- usleep_range((__sleep_us >> 2) + 1, __sleep_us); \
- } \
- (cond) ? 0 : -ETIMEDOUT; \
-})
+ read_poll_timeout(op, val, cond, sleep_us, timeout_us, addr)

/**
* readx_poll_timeout_atomic - Periodically poll an address until a condition is met or a timeout occurs
--
2.25.0

2020-03-22 17:51:19

by Dejin Zheng

[permalink] [raw]
Subject: [PATCH net-next v5 07/10] net: phy: introduce phy_read_poll_timeout macro

it is sometimes necessary to poll a phy register by phy_read()
function until its value satisfies some condition. introduce
phy_read_poll_timeout() macros that do this.

Suggested-by: Andrew Lunn <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Signed-off-by: Dejin Zheng <[email protected]>
---
v4 -> v5:
- no changed.
v3 -> v4:
- deal with precedence issues for parameter cond.
v2 -> v3:
- modify the parameter order of newly added functions.
phy_read_poll_timeout(val, cond, sleep_us, timeout_us, \
phydev, regnum)
||
\/
phy_read_poll_timeout(phydev, regnum, val, cond, sleep_us, \
timeout_us)
v1 -> v2:
- pass a phydev and a regnum to replace args... parameter in
the phy_read_poll_timeout(), and also handle the
phy_read() function's return error.

include/linux/phy.h | 13 +++++++++++++
1 file changed, 13 insertions(+)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 42a5ec9288d5..f2e0aea13a2f 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -714,6 +714,19 @@ static inline int phy_read(struct phy_device *phydev, u32 regnum)
return mdiobus_read(phydev->mdio.bus, phydev->mdio.addr, regnum);
}

+#define phy_read_poll_timeout(phydev, regnum, val, cond, sleep_us, timeout_us) \
+({ \
+ int ret = 0; \
+ ret = read_poll_timeout(phy_read, val, (cond) || val < 0, sleep_us, \
+ timeout_us, phydev, regnum); \
+ if (val < 0) \
+ ret = val; \
+ if (ret) \
+ phydev_err(phydev, "%s failed: %d\n", __func__, ret); \
+ ret; \
+})
+
+
/**
* __phy_read - convenience function for reading a given PHY register
* @phydev: the phy_device struct
--
2.25.0

2020-03-22 17:51:54

by Dejin Zheng

[permalink] [raw]
Subject: [PATCH net-next v5 03/10] net: phy: introduce phy_read_mmd_poll_timeout macro

it is sometimes necessary to poll a phy register by phy_read_mmd()
function until its value satisfies some condition. introduce
phy_read_mmd_poll_timeout() macros that do this.

Suggested-by: Andrew Lunn <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Signed-off-by: Dejin Zheng <[email protected]>
---
v4 -> v5:
- no changed
v3 -> v4:
- deal with precedence issues for parameter cond.
v2 -> v3:
- modify the parameter order of newly added functions.
phy_read_mmd_poll_timeout(val, cond, sleep_us, timeout_us, \
phydev, devaddr, regnum)
||
\/
phy_read_mmd_poll_timeout(phydev, devaddr regnum, val, cond, \
sleep_us, timeout_us)
v1 -> v2:
- passed a phydev, device address and a reg to replace args...
parameter in phy_read_mmd_poll_timeout() by Andrew Lunn 's
suggestion. Andrew Lunn <[email protected]>, Thanks very much for
your help!
- handle phy_read_mmd return an error(the return value < 0) in
phy_read_mmd_poll_timeout(). Thanks Andrew again.

include/linux/phy.h | 15 +++++++++++++++
1 file changed, 15 insertions(+)

diff --git a/include/linux/phy.h b/include/linux/phy.h
index 36d9dea04016..42a5ec9288d5 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -24,6 +24,7 @@
#include <linux/mod_devicetable.h>
#include <linux/u64_stats_sync.h>
#include <linux/irqreturn.h>
+#include <linux/iopoll.h>

#include <linux/atomic.h>

@@ -784,6 +785,20 @@ static inline int __phy_modify_changed(struct phy_device *phydev, u32 regnum,
*/
int phy_read_mmd(struct phy_device *phydev, int devad, u32 regnum);

+#define phy_read_mmd_poll_timeout(phydev, devaddr, regnum, val, cond, \
+ sleep_us, timeout_us) \
+({ \
+ int ret = 0; \
+ ret = read_poll_timeout(phy_read_mmd, val, (cond) || val < 0, \
+ sleep_us, timeout_us, phydev, devaddr, \
+ regnum); \
+ if (val < 0) \
+ ret = val; \
+ if (ret) \
+ phydev_err(phydev, "%s failed: %d\n", __func__, ret); \
+ ret; \
+})
+
/**
* __phy_read_mmd - Convenience function for reading a register
* from an MMD on a given PHY.
--
2.25.0

2020-03-22 17:51:55

by Dejin Zheng

[permalink] [raw]
Subject: [PATCH net-next v5 09/10] net: phy: smsc: use phy_read_poll_timeout() to simplify the code

use phy_read_poll_timeout() to replace the poll codes for
simplify lan87xx_read_status() function.

it should be add msleep(10) before call phy_read_poll_timeout()
to keep the code more similar, but it will report that warning, so
modify it to msleep(20).

./scripts/checkpatch.pl
v5-0009-net-phy-smsc-use-phy_read_poll_timeout-to-simplif.patch
WARNING: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.rst
#42: FILE: drivers/net/phy/smsc.c:126:
+ msleep(10);

Suggested-by: Andrew Lunn <[email protected]>
Signed-off-by: Dejin Zheng <[email protected]>
---
v4 -> v5:
- add msleep before phy_read_poll_timeout() to keep the
code more similar
v3 -> v4:
- add this patch by Andrew's suggestion. Thanks Andrew!

drivers/net/phy/smsc.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/net/phy/smsc.c b/drivers/net/phy/smsc.c
index b73298250793..f888523086ed 100644
--- a/drivers/net/phy/smsc.c
+++ b/drivers/net/phy/smsc.c
@@ -112,8 +112,6 @@ static int lan87xx_read_status(struct phy_device *phydev)
int err = genphy_read_status(phydev);

if (!phydev->link && priv->energy_enable) {
- int i;
-
/* Disable EDPD to wake up PHY */
int rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
if (rc < 0)
@@ -125,15 +123,12 @@ static int lan87xx_read_status(struct phy_device *phydev)
return rc;

/* Wait max 640 ms to detect energy */
- for (i = 0; i < 64; i++) {
- /* Sleep to allow link test pulses to be sent */
- msleep(10);
- rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
- if (rc < 0)
- return rc;
- if (rc & MII_LAN83C185_ENERGYON)
- break;
- }
+ msleep(20);
+ phy_read_poll_timeout(phydev, MII_LAN83C185_CTRL_STATUS, rc,
+ rc & MII_LAN83C185_ENERGYON, 10000,
+ 620000);
+ if (rc < 0)
+ return rc;

/* Re-enable EDPD */
rc = phy_read(phydev, MII_LAN83C185_CTRL_STATUS);
--
2.25.0

2020-03-22 17:52:04

by Dejin Zheng

[permalink] [raw]
Subject: [PATCH net-next v5 06/10] net: phy: marvell10g: use phy_read_mmd_poll_timeout() to simplify the code

use phy_read_mmd_poll_timeout() to replace the poll codes for
simplify mv3310_reset() function.

it should be add msleep(5) before call phy_read_mmd_poll_timeout()
to keep the code more similar, but it will report that warning, so
modify it to msleep(20).

./scripts/checkpatch.pl
v5-0006-net-phy-marvell10g-use-phy_read_mmd_poll_timeout-.patch
WARNING: msleep < 20ms can sleep for up to 20ms; see Documentation/timers/timers-howto.rst
#41: FILE: drivers/net/phy/marvell10g.c:251:
+ msleep(5);

Suggested-by: Andrew Lunn <[email protected]>
Signed-off-by: Dejin Zheng <[email protected]>
---
v4 -> v5:
- add msleep() to before call phy_read_mmd_poll_timeout()
to keep the code more similar.
v3 -> v4:
- add this patch by Andrew's suggestion. Thanks Andrew!

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

diff --git a/drivers/net/phy/marvell10g.c b/drivers/net/phy/marvell10g.c
index 7e05b92504f0..c0fb8391c75b 100644
--- a/drivers/net/phy/marvell10g.c
+++ b/drivers/net/phy/marvell10g.c
@@ -241,22 +241,18 @@ static int mv3310_power_up(struct phy_device *phydev)

static int mv3310_reset(struct phy_device *phydev, u32 unit)
{
- int retries, val, err;
+ int val, err;

err = phy_modify_mmd(phydev, MDIO_MMD_PCS, unit + MDIO_CTRL1,
MDIO_CTRL1_RESET, MDIO_CTRL1_RESET);
if (err < 0)
return err;

- retries = 20;
- do {
- msleep(5);
- val = phy_read_mmd(phydev, MDIO_MMD_PCS, unit + MDIO_CTRL1);
- if (val < 0)
- return val;
- } while (val & MDIO_CTRL1_RESET && --retries);
-
- return val & MDIO_CTRL1_RESET ? -ETIMEDOUT : 0;
+ msleep(20);
+ return phy_read_mmd_poll_timeout(phydev, MDIO_MMD_PCS,
+ unit + MDIO_CTRL1, val,
+ !(val & MDIO_CTRL1_RESET),
+ 5000, 80000);
}

static int mv3310_get_edpd(struct phy_device *phydev, u16 *edpd)
--
2.25.0

2020-03-22 17:52:11

by Dejin Zheng

[permalink] [raw]
Subject: [PATCH net-next v5 10/10] net: phy: tja11xx: use phy_read_poll_timeout() to simplify the code

use phy_read_poll_timeout() to replace the poll codes for
simplify tja11xx_check() function.

Suggested-by: Andrew Lunn <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Signed-off-by: Dejin Zheng <[email protected]>
---
v4 -> v5:
- no changed.
v3 -> v4:
- add this patch by Andrew's suggestion. Thanks Andrew!

drivers/net/phy/nxp-tja11xx.c | 16 +++-------------
1 file changed, 3 insertions(+), 13 deletions(-)

diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
index b705d0bd798b..32ef32a4af3c 100644
--- a/drivers/net/phy/nxp-tja11xx.c
+++ b/drivers/net/phy/nxp-tja11xx.c
@@ -72,20 +72,10 @@ static struct tja11xx_phy_stats tja11xx_hw_stats[] = {

static int tja11xx_check(struct phy_device *phydev, u8 reg, u16 mask, u16 set)
{
- int i, ret;
-
- for (i = 0; i < 200; i++) {
- ret = phy_read(phydev, reg);
- if (ret < 0)
- return ret;
-
- if ((ret & mask) == set)
- return 0;
-
- usleep_range(100, 150);
- }
+ int val;

- return -ETIMEDOUT;
+ return phy_read_poll_timeout(phydev, reg, val, (val & mask) == set,
+ 150, 30000);
}

static int phy_modify_check(struct phy_device *phydev, u8 reg,
--
2.25.0

2020-03-22 17:52:14

by Dejin Zheng

[permalink] [raw]
Subject: [PATCH net-next v5 08/10] net: phy: use phy_read_poll_timeout() to simplify the code

use phy_read_poll_timeout() to replace the poll codes for
simplify the code in phy_poll_reset() function.

Signed-off-by: Dejin Zheng <[email protected]>
---
v4 -> v5:
- it can add msleep before call phy_read_poll_timeout()
to keep the code more similar. so add it.
v3 -> v4:
- drop it.
v2 -> v3:
- adapt to it after modifying the parameter order of the
newly added function
v1 -> v2:
- remove the handle of phy_read()'s return error.


drivers/net/phy/phy_device.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index a585faf8b844..24297ee7f626 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1059,23 +1059,16 @@ EXPORT_SYMBOL(phy_disconnect);
static int phy_poll_reset(struct phy_device *phydev)
{
/* Poll until the reset bit clears (50ms per retry == 0.6 sec) */
- unsigned int retries = 12;
- int ret;
-
- do {
- msleep(50);
- ret = phy_read(phydev, MII_BMCR);
- if (ret < 0)
- return ret;
- } while (ret & BMCR_RESET && --retries);
- if (ret & BMCR_RESET)
- return -ETIMEDOUT;
+ int ret, val;

+ msleep(50);
+ ret = phy_read_poll_timeout(phydev, MII_BMCR, val, !(val & BMCR_RESET),
+ 50000, 550000);
/* Some chips (smsc911x) may still need up to another 1ms after the
* BMCR_RESET bit is cleared before they are usable.
*/
msleep(1);
- return 0;
+ return ret;
}

int phy_init_hw(struct phy_device *phydev)
--
2.25.0

2020-03-22 17:52:19

by Dejin Zheng

[permalink] [raw]
Subject: [PATCH net-next v5 05/10] net: phy: aquantia: use phy_read_mmd_poll_timeout() to simplify the code

use phy_read_mmd_poll_timeout() to replace the poll codes for
simplify aqr107_wait_reset_complete() function.

Reviewed-by: Andrew Lunn <[email protected]>
Signed-off-by: Dejin Zheng <[email protected]>
---
v4 -> v5:
- no changed
v3 -> v4:
- no changed
v2 -> v3:
- adapt to it after modifying the parameter order of the
newly added function
v1 -> v2:
- remove the handle of phy_read_mmd's return error.

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

diff --git a/drivers/net/phy/aquantia_main.c b/drivers/net/phy/aquantia_main.c
index 31927b2c7d5a..db3e20938729 100644
--- a/drivers/net/phy/aquantia_main.c
+++ b/drivers/net/phy/aquantia_main.c
@@ -451,16 +451,11 @@ static int aqr107_set_tunable(struct phy_device *phydev,
*/
static int aqr107_wait_reset_complete(struct phy_device *phydev)
{
- int val, retries = 100;
-
- do {
- val = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_GLOBAL_FW_ID);
- if (val < 0)
- return val;
- msleep(20);
- } while (!val && --retries);
+ int val;

- return val ? 0 : -ETIMEDOUT;
+ return phy_read_mmd_poll_timeout(phydev, MDIO_MMD_VEND1,
+ VEND1_GLOBAL_FW_ID, val,
+ val != 0, 20000, 2000000);
}

static void aqr107_chip_info(struct phy_device *phydev)
--
2.25.0

2020-03-22 17:52:37

by Dejin Zheng

[permalink] [raw]
Subject: [PATCH net-next v5 04/10] net: phy: bcm84881: use phy_read_mmd_poll_timeout() to simplify the code

use phy_read_mmd_poll_timeout() to replace the poll codes for
simplify bcm84881_wait_init() function.

Reviewed-by: Andrew Lunn <[email protected]>
Signed-off-by: Dejin Zheng <[email protected]>
---
v4 -> v5:
- no changed
v3 -> v4:
- no changed
v2 -> v3:
- adapt to it after modifying the parameter order of the
newly added function
v1 -> v2:
- remove the handle of phy_read_mmd's return error.

drivers/net/phy/bcm84881.c | 27 ++++-----------------------
1 file changed, 4 insertions(+), 23 deletions(-)

diff --git a/drivers/net/phy/bcm84881.c b/drivers/net/phy/bcm84881.c
index 14d55a77eb28..b70be775909c 100644
--- a/drivers/net/phy/bcm84881.c
+++ b/drivers/net/phy/bcm84881.c
@@ -22,30 +22,11 @@ enum {

static int bcm84881_wait_init(struct phy_device *phydev)
{
- unsigned int tries = 20;
- int ret, val;
-
- do {
- val = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1);
- if (val < 0) {
- ret = val;
- break;
- }
- if (!(val & MDIO_CTRL1_RESET)) {
- ret = 0;
- break;
- }
- if (!--tries) {
- ret = -ETIMEDOUT;
- break;
- }
- msleep(100);
- } while (1);
+ int val;

- if (ret)
- phydev_err(phydev, "%s failed: %d\n", __func__, ret);
-
- return ret;
+ return phy_read_mmd_poll_timeout(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1,
+ val, !(val & MDIO_CTRL1_RESET),
+ 100000, 2000000);
}

static int bcm84881_config_init(struct phy_device *phydev)
--
2.25.0

2020-03-22 19:07:35

by Andrew Lunn

[permalink] [raw]
Subject: Re: [PATCH net-next v5 08/10] net: phy: use phy_read_poll_timeout() to simplify the code

On Mon, Mar 23, 2020 at 01:49:41AM +0800, Dejin Zheng wrote:
> use phy_read_poll_timeout() to replace the poll codes for
> simplify the code in phy_poll_reset() function.
>
> Signed-off-by: Dejin Zheng <[email protected]>

Reviewed-by: Andrew Lunn <[email protected]>

Andrew

2020-03-22 19:20:24

by Russell King (Oracle)

[permalink] [raw]
Subject: Re: [PATCH net-next v5 08/10] net: phy: use phy_read_poll_timeout() to simplify the code

On Mon, Mar 23, 2020 at 01:49:41AM +0800, Dejin Zheng wrote:
> use phy_read_poll_timeout() to replace the poll codes for
> simplify the code in phy_poll_reset() function.
>
> Signed-off-by: Dejin Zheng <[email protected]>
> ---
> v4 -> v5:
> - it can add msleep before call phy_read_poll_timeout()
> to keep the code more similar. so add it.
> v3 -> v4:
> - drop it.
> v2 -> v3:
> - adapt to it after modifying the parameter order of the
> newly added function
> v1 -> v2:
> - remove the handle of phy_read()'s return error.
>
>
> drivers/net/phy/phy_device.c | 17 +++++------------
> 1 file changed, 5 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index a585faf8b844..24297ee7f626 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1059,23 +1059,16 @@ EXPORT_SYMBOL(phy_disconnect);
> static int phy_poll_reset(struct phy_device *phydev)
> {
> /* Poll until the reset bit clears (50ms per retry == 0.6 sec) */
> - unsigned int retries = 12;
> - int ret;
> -
> - do {
> - msleep(50);
> - ret = phy_read(phydev, MII_BMCR);
> - if (ret < 0)
> - return ret;
> - } while (ret & BMCR_RESET && --retries);
> - if (ret & BMCR_RESET)
> - return -ETIMEDOUT;
> + int ret, val;
>
> + msleep(50);
> + ret = phy_read_poll_timeout(phydev, MII_BMCR, val, !(val & BMCR_RESET),
> + 50000, 550000);
> /* Some chips (smsc911x) may still need up to another 1ms after the
> * BMCR_RESET bit is cleared before they are usable.
> */
> msleep(1);
> - return 0;
> + return ret;

This isn't actually equivaent behaviour. If the read timed out, the old
code didn't wait 1ms. Your new code does. However, since we've waited
about 600ms already, it probably doesn't matter.

These sorts of things should be documented in the commit log, IMHO, so
it's obvious that it's been considered.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up