| Message ID | 1309261269-4363-10-git-send-email-graeme.russ@gmail.com |
|---|---|
| State | RFC |
| Headers | show |
Hi Graeme, On Tue, Jun 28, 2011 at 4:41 AM, Graeme Russ <graeme.russ@gmail.com> wrote: > > Signed-off-by: Graeme Russ <graeme.russ@gmail.com> > --- > drivers/block/mg_disk.c | 9 ++++----- > 1 files changed, 4 insertions(+), 5 deletions(-) > > diff --git a/drivers/block/mg_disk.c b/drivers/block/mg_disk.c > index 2198017..c8cc195 100644 > --- a/drivers/block/mg_disk.c > +++ b/drivers/block/mg_disk.c > @@ -88,17 +88,16 @@ static void mg_dump_status (const char *msg, unsigned int stat, unsigned err) > static unsigned int mg_wait (u32 expect, u32 msec) > { > u8 status; > - u32 from, cur, err; > + u32 ts, err; > > err = MG_ERR_NONE; > #ifdef CONFIG_NIOS2 > reset_timer(); > #endif > - from = get_timer(0); > + ts = time_now_ms(); > > status = readb(mg_base() + MG_REG_STATUS); > do { > - cur = get_timer(from); ... > - } while (cur < msec); > + } while (time_since_ms(ts) < msec); > Well I know i have asked this before, but I feel I should ask again because I didn't like the answer much. Imagine we change this code to: ts = time_now_ms() + msec do { ... } while (time_since_ms(ts) < 0); That should be legal, right? But I don't think this can work since the 'since' functions return an unsigned. [aside: this provides for another idiom that I think we talked about: ts = time_future_ms(msec) do { ... } while (!time_passed(ts)) which I am not at all suggesting should be in the API :-) end aside] Regards. Simon > - if (cur >= msec) > + if (time_since_ms(ts) >= msec) > err = MG_ERR_TIMEOUT; > > return err; > -- > 1.7.5.2.317.g391b14 > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot >
Dear All, > Well I know i have asked this before, but I feel I should ask again > because I didn't like the answer much. > > Imagine we change this code to: > > ts = time_now_ms() + msec > do { > ... > } while (time_since_ms(ts)< 0); > > That should be legal, right? But I don't think this can work since the > 'since' functions return an unsigned. > > [aside: this provides for another idiom that I think we talked about: > > ts = time_future_ms(msec) > do { > ... > } while (!time_passed(ts)) > > which I am not at all suggesting should be in the API :-) > end aside] I still vouch for this concept, which is simple, clean, and easy to understand. Reinhard
Hi Reinhard, On Wed, Jun 29, 2011 at 3:06 PM, Reinhard Meyer <u-boot@emk-elektronik.de> wrote: > Dear All, >> >> Well I know i have asked this before, but I feel I should ask again >> because I didn't like the answer much. >> >> Imagine we change this code to: >> >> ts = time_now_ms() + msec >> do { >> ... >> } while (time_since_ms(ts)< 0); >> >> That should be legal, right? But I don't think this can work since the >> 'since' functions return an unsigned. >> >> [aside: this provides for another idiom that I think we talked about: >> >> ts = time_future_ms(msec) >> do { >> ... >> } while (!time_passed(ts)) >> >> which I am not at all suggesting should be in the API :-) >> end aside] > > I still vouch for this concept, which is simple, clean, and easy to > understand. It really is a matter of personal taste ;) I find u32 start = time_now_ms(); do { ...blah... } while(time_since_ms(start) < timeout); much easier to understand (Do whatever while time elapsed since I started is less than the timeout) u32 end = time_future_ms(timeout); do { ...blah... } while(time_now_ms() < end); to me is a bit more clunky. Yes, it is probably computationally more efficient, but it does not naturally support: u32 start = time_now_ms(); u32 duration; ...blah... duration = time_since_ms(start); /* or duration = time_max_since_ms(start); */ Which we want for profiling. Also there are a few instances where there are multiple cascaded timeouts u32 start = time_now_ms(); do { ...blah... } while(time_since_ms(start) < timeout_1); do { ...blah... } while(time_since_ms(start) < timeout_2); Which means setting up all your timeouts in advance Regards, Graeme
Hi Graeme, On Tue, Jun 28, 2011 at 10:19 PM, Graeme Russ <graeme.russ@gmail.com> wrote: > Hi Reinhard, > > On Wed, Jun 29, 2011 at 3:06 PM, Reinhard Meyer > <u-boot@emk-elektronik.de> wrote: >> Dear All, >>> >>> Well I know i have asked this before, but I feel I should ask again >>> because I didn't like the answer much. >>> >>> Imagine we change this code to: >>> >>> ts = time_now_ms() + msec >>> do { >>> ... >>> } while (time_since_ms(ts)< 0); >>> >>> That should be legal, right? But I don't think this can work since the >>> 'since' functions return an unsigned. >>> >>> [aside: this provides for another idiom that I think we talked about: >>> >>> ts = time_future_ms(msec) >>> do { >>> ... >>> } while (!time_passed(ts)) >>> >>> which I am not at all suggesting should be in the API :-) >>> end aside] >> >> I still vouch for this concept, which is simple, clean, and easy to >> understand. > > It really is a matter of personal taste ;) I find > > u32 start = time_now_ms(); > > do { > ...blah... > } while(time_since_ms(start) < timeout); > > much easier to understand (Do whatever while time elapsed since I started > is less than the timeout) > > u32 end = time_future_ms(timeout); > > do { > ...blah... > } while(time_now_ms() < end); ... Actually: } while (time_passed_ms(end)) but anyway I agree it is a matter of taste and I'm quite happy with the approach here at the moment. But what about my question about signed ints for deltas? Regards, Simon
Hi Simon, >> >> u32 end = time_future_ms(timeout); >> >> do { >> ...blah... >> } while(time_now_ms() < end); > ... > > Actually: > > } while (time_passed_ms(end)) Sorry, but I think you've lost me here... > > but anyway I agree it is a matter of taste and I'm quite happy with > the approach here at the moment. > > But what about my question about signed ints for deltas? We use signed int's to allow seamless roll-overs of the timer counter. One thing the API does not require is that a given low-level timer counts from zero - It can start with any value and therefore may roll-over at any time. By using unsigned provided there is at most one rollover between timing events (which for a 32-bit millisecond counter is a very long time) the logic remain trivial (time = end - start) - We don't have to try and detect the rollover. Also, we assume the u-Boot will never be installed in a time machine, and will therefore never need to calculate negative time. Please let us know if you plan to boot a TARDIS using U-Boot ;) Regards, Graeme
diff --git a/drivers/block/mg_disk.c b/drivers/block/mg_disk.c index 2198017..c8cc195 100644 --- a/drivers/block/mg_disk.c +++ b/drivers/block/mg_disk.c @@ -88,17 +88,16 @@ static void mg_dump_status (const char *msg, unsigned int stat, unsigned err) static unsigned int mg_wait (u32 expect, u32 msec) { u8 status; - u32 from, cur, err; + u32 ts, err; err = MG_ERR_NONE; #ifdef CONFIG_NIOS2 reset_timer(); #endif - from = get_timer(0); + ts = time_now_ms(); status = readb(mg_base() + MG_REG_STATUS); do { - cur = get_timer(from); if (status & MG_REG_STATUS_BIT_BUSY) { if (expect == MG_REG_STATUS_BIT_BUSY) break; @@ -119,9 +118,9 @@ static unsigned int mg_wait (u32 expect, u32 msec) break; } status = readb(mg_base() + MG_REG_STATUS); - } while (cur < msec); + } while (time_since_ms(ts) < msec); - if (cur >= msec) + if (time_since_ms(ts) >= msec) err = MG_ERR_TIMEOUT; return err;
Signed-off-by: Graeme Russ <graeme.russ@gmail.com> --- drivers/block/mg_disk.c | 9 ++++----- 1 files changed, 4 insertions(+), 5 deletions(-)