| Message ID | 1357178098-4057-3-git-send-email-barry@grussling.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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.
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
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.
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; }
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(-)