Patchwork [2/4] DSA: Convert msleep calls to usleep_range calls

login
register
mail settings
Submitter Barry Grussling
Date Jan. 3, 2013, 1:54 a.m.
Message ID <1357178098-4057-3-git-send-email-barry@grussling.com>
Download mbox | patch
Permalink /patch/209169/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Barry Grussling - Jan. 3, 2013, 1:54 a.m.
Convert DSA msleep calls to usleep_range calls as reported by
checkpatch.pl.

Values of sleep duration were verified on Marvell hardware
platform and appear to work.  Values chosen are not special
and no strong "vetting" has gone into them other than verifying
correct operation on available hardware.


Signed-off-by: Barry Grussling <barry@grussling.com>
---
 drivers/net/dsa/mv88e6060.c       |    7 ++++---
 drivers/net/dsa/mv88e6123_61_65.c |    7 ++++---
 drivers/net/dsa/mv88e6131.c       |    7 ++++---
 drivers/net/dsa/mv88e6xxx.c       |    5 +++--
 4 files changed, 15 insertions(+), 11 deletions(-)
Ben Hutchings - Jan. 3, 2013, 8:14 p.m.
On Wed, 2013-01-02 at 17:54 -0800, Barry Grussling wrote:
> Convert DSA msleep calls to usleep_range calls as reported by
> checkpatch.pl.
> 
> Values of sleep duration were verified on Marvell hardware
> platform and appear to work.  Values chosen are not special
> and no strong "vetting" has gone into them other than verifying
> correct operation on available hardware.
[...]

I seriously doubt that it is worth the trouble to save wake-ups during
the occasional hardware reset.  And using usleep_range() 1000 times is
weird.  If the sleep duration can vary then the right thing to do is
probably to calculate a deadline first (jiffies + HZ) and then sleep
repeatedly until the deadline is in the past.  This also accounts for
the fact that HZ may be < 1000...

Ben.
Barry Grussling - Jan. 3, 2013, 10:33 p.m.
On Thu, Jan 3, 2013 at 12:14 PM, Ben Hutchings
<bhutchings@solarflare.com> wrote:
> I seriously doubt that it is worth the trouble to save wake-ups during
> the occasional hardware reset.  And using usleep_range() 1000 times is
> weird.  If the sleep duration can vary then the right thing to do is
> probably to calculate a deadline first (jiffies + HZ) and then sleep
> repeatedly until the deadline is in the past.  This also accounts for
> the fact that HZ may be < 1000...

I don't think the idea here is variable sleep time.  Its more just
giving hardware a little
bit of time to catch up with firmware.  I don't really care about
variable sleep times,
but checkpatch.pl says msleep of less than 20 ms is bad and may result in sleep
times of up to 20 ms for requests of shorter durations.

Should I use udelay instead?  What is the recommended method for sleeping 1 ms?

Thanks for your help.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings - Jan. 3, 2013, 10:56 p.m.
On Thu, 2013-01-03 at 14:33 -0800, Barry Grussling wrote:
> On Thu, Jan 3, 2013 at 12:14 PM, Ben Hutchings
> <bhutchings@solarflare.com> wrote:
> > I seriously doubt that it is worth the trouble to save wake-ups during
> > the occasional hardware reset.  And using usleep_range() 1000 times is
> > weird.  If the sleep duration can vary then the right thing to do is
> > probably to calculate a deadline first (jiffies + HZ) and then sleep
> > repeatedly until the deadline is in the past.  This also accounts for
> > the fact that HZ may be < 1000...
> 
> I don't think the idea here is variable sleep time.  Its more just
> giving hardware a little
> bit of time to catch up with firmware.  I don't really care about
> variable sleep times,
> but checkpatch.pl says msleep of less than 20 ms is bad and may result in sleep
> times of up to 20 ms for requests of shorter durations.

Still true for usleep_range() on hardware that doesn't have an hrtimer.

> Should I use udelay instead?  What is the recommended method for sleeping 1 ms?

This driver doesn't want to wait 1 ms, it wants to wait for up to 1
second and poll periodically.  Think about the whole loop, not just that
one function call.

Ben.

Patch

diff --git a/drivers/net/dsa/mv88e6060.c b/drivers/net/dsa/mv88e6060.c
index 16ec763..fa6bc7d 100644
--- a/drivers/net/dsa/mv88e6060.c
+++ b/drivers/net/dsa/mv88e6060.c
@@ -8,6 +8,7 @@ 
  * (at your option) any later version.
  */
 
+#include <linux/delay.h>
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/netdevice.h>
@@ -76,20 +77,20 @@  static int mv88e6060_switch_reset(struct dsa_switch *ds)
 
 	/* Wait for transmit queues to drain.
 	 */
-	msleep(2);
+	usleep_range(2000, 4000);
 
 	/* Reset the switch.
 	 */
 	REG_WRITE(REG_GLOBAL, 0x0a, 0xa130);
 
-	/* Wait up to one second for reset to complete.
+	/* Wait up to two seconds for reset to complete.
 	 */
 	for (i = 0; i < 1000; i++) {
 		ret = REG_READ(REG_GLOBAL, 0x00);
 		if ((ret & 0x8000) == 0x0000)
 			break;
 
-		msleep(1);
+		usleep_range(1000, 2000);
 	}
 	if (i == 1000)
 		return -ETIMEDOUT;
diff --git a/drivers/net/dsa/mv88e6123_61_65.c b/drivers/net/dsa/mv88e6123_61_65.c
index f964bfb..0ead9b4 100644
--- a/drivers/net/dsa/mv88e6123_61_65.c
+++ b/drivers/net/dsa/mv88e6123_61_65.c
@@ -8,6 +8,7 @@ 
  * (at your option) any later version.
  */
 
+#include <linux/delay.h>
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/netdevice.h>
@@ -60,20 +61,20 @@  static int mv88e6123_61_65_switch_reset(struct dsa_switch *ds)
 
 	/* Wait for transmit queues to drain.
 	 */
-	msleep(2);
+	usleep_range(2000, 4000);
 
 	/* Reset the switch.
 	 */
 	REG_WRITE(REG_GLOBAL, 0x04, 0xc400);
 
-	/* Wait up to one second for reset to complete.
+	/* Wait up to two seconds for reset to complete.
 	 */
 	for (i = 0; i < 1000; i++) {
 		ret = REG_READ(REG_GLOBAL, 0x00);
 		if ((ret & 0xc800) == 0xc800)
 			break;
 
-		msleep(1);
+		usleep_range(1000, 2000);
 	}
 	if (i == 1000)
 		return -ETIMEDOUT;
diff --git a/drivers/net/dsa/mv88e6131.c b/drivers/net/dsa/mv88e6131.c
index 7a7bcc2..7872507 100644
--- a/drivers/net/dsa/mv88e6131.c
+++ b/drivers/net/dsa/mv88e6131.c
@@ -8,6 +8,7 @@ 
  * (at your option) any later version.
  */
 
+#include <linux/delay.h>
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/netdevice.h>
@@ -53,20 +54,20 @@  static int mv88e6131_switch_reset(struct dsa_switch *ds)
 
 	/* Wait for transmit queues to drain.
 	 */
-	msleep(2);
+	usleep_range(2000, 4000);
 
 	/* Reset the switch.
 	 */
 	REG_WRITE(REG_GLOBAL, 0x04, 0xc400);
 
-	/* Wait up to one second for reset to complete.
+	/* Wait up to two seconds for reset to complete.
 	 */
 	for (i = 0; i < 1000; i++) {
 		ret = REG_READ(REG_GLOBAL, 0x00);
 		if ((ret & 0xc800) == 0xc800)
 			break;
 
-		msleep(1);
+		usleep_range(1000, 2000);
 	}
 	if (i == 1000)
 		return -ETIMEDOUT;
diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index eaa341a..a2f9e9b6 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -8,6 +8,7 @@ 
  * (at your option) any later version.
  */
 
+#include <linux/delay.h>
 #include <linux/list.h>
 #include <linux/module.h>
 #include <linux/netdevice.h>
@@ -215,7 +216,7 @@  static int mv88e6xxx_ppu_disable(struct dsa_switch *ds)
 
 	for (i = 0; i < 1000; i++) {
 	        ret = REG_READ(REG_GLOBAL, 0x00);
-	        msleep(1);
+		usleep_range(1000, 2000);
 	        if ((ret & 0xc000) != 0xc000)
 	                return 0;
 	}
@@ -233,7 +234,7 @@  static int mv88e6xxx_ppu_enable(struct dsa_switch *ds)
 
 	for (i = 0; i < 1000; i++) {
 	        ret = REG_READ(REG_GLOBAL, 0x00);
-	        msleep(1);
+		usleep_range(1000, 2000);
 	        if ((ret & 0xc000) == 0xc000)
 	                return 0;
 	}