| Message ID | 514A4CBA.1040203@ti.com |
|---|---|
| State | Rejected |
| Delegated to: | Wolfgang Denk |
| Headers | show |
Dear Jon Hunter, > On 01/24/2013 05:05 AM, Jim Lin wrote: > > Autoboot timeout defined by CONFIG_BOOTDELAY will not be accurate if > > CONFIG_USB_KEYBOARD and CONFIG_SYS_USB_EVENT_POLL are defined in > > configuration file and when tstc() function for checking key pressed > > takes longer time than 10 ms (e.g., 50 ms) to finish. > > > > Signed-off-by: Jim Lin <jilin@nvidia.com> > > --- > > > > Changes in v2: > > - use do-while and get_timer to count timeout. > > > > Changes in v3: > > - revert original udelay(10000); for safety. > > > > common/main.c | 10 +++++----- > > 1 files changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/common/main.c b/common/main.c > > index b145f85..dcd2a42 100644 > > --- a/common/main.c > > +++ b/common/main.c > > @@ -225,6 +225,7 @@ static inline > > > > int abortboot(int bootdelay) > > { > > > > int abort = 0; > > > > + unsigned long ts; > > > > #ifdef CONFIG_MENUPROMPT > > > > printf(CONFIG_MENUPROMPT); > > > > @@ -248,11 +249,10 @@ int abortboot(int bootdelay) > > > > #endif > > > > while ((bootdelay > 0) && (!abort)) { > > > > - int i; > > - > > > > --bootdelay; > > > > - /* delay 100 * 10ms */ > > - for (i=0; !abort && i<100; ++i) { > > + /* delay 1000 ms */ > > + ts = get_timer(0); > > + do { > > > > if (tstc()) { /* we got a key press */ > > > > abort = 1; /* don't auto boot */ > > bootdelay = 0; /* no more delay */ > > > > @@ -264,7 +264,7 @@ int abortboot(int bootdelay) > > > > break; > > > > } > > udelay(10000); > > > > - } > > + } while (!abort && get_timer(ts) < 1000); > > > > printf("\b\b\b%2d ", bootdelay); > > > > } > > This change is causing problems with auto-delay on one of my boards by > making it inaccurate :-( > > The question is what should get_timer() be returning? If it is meant to > be milliseconds then I guess I need to fix get_timer() for my board. > However, if it is just meant to be timer ticks at the SYS_HZ rate then I > don't see how the above change guarantees the do-while loop waits 1000 > ms per iteration without normalising to SYS_HZ. What board is it ? Best regards, Marek Vasut
On 03/20/2013 05:56 PM, Jon Hunter wrote: > > On 01/24/2013 05:05 AM, Jim Lin wrote: >> Autoboot timeout defined by CONFIG_BOOTDELAY will not be accurate if >> CONFIG_USB_KEYBOARD and CONFIG_SYS_USB_EVENT_POLL are defined in >> configuration file and when tstc() function for checking key pressed >> takes longer time than 10 ms (e.g., 50 ms) to finish. ... > This change is causing problems with auto-delay on one of my boards by > making it inaccurate :-( Interesting. I just noticed the same problem, and posted the following patch to fix it: http://lists.denx.de/pipermail/u-boot/2013-March/149625.html This was on the Raspberry Pi.
Dear Stephen Warren, In message <514AA642.3090107@wwwdotorg.org> you wrote: > > Interesting. I just noticed the same problem, and posted the following > patch to fix it: > > http://lists.denx.de/pipermail/u-boot/2013-March/149625.html > > This was on the Raspberry Pi. You mean the Raspberry Pi uses SYS_HZ != 1000 ??? This needs fixing!! Best regards, Wolfgang Denk
Hi Marek, On 03/20/2013 07:19 PM, Marek Vasut wrote: > Dear Jon Hunter, > >> On 01/24/2013 05:05 AM, Jim Lin wrote: >>> Autoboot timeout defined by CONFIG_BOOTDELAY will not be accurate if >>> CONFIG_USB_KEYBOARD and CONFIG_SYS_USB_EVENT_POLL are defined in >>> configuration file and when tstc() function for checking key pressed >>> takes longer time than 10 ms (e.g., 50 ms) to finish. >>> >>> Signed-off-by: Jim Lin <jilin@nvidia.com> >>> --- >>> >>> Changes in v2: >>> - use do-while and get_timer to count timeout. >>> >>> Changes in v3: >>> - revert original udelay(10000); for safety. >>> >>> common/main.c | 10 +++++----- >>> 1 files changed, 5 insertions(+), 5 deletions(-) >>> >>> diff --git a/common/main.c b/common/main.c >>> index b145f85..dcd2a42 100644 >>> --- a/common/main.c >>> +++ b/common/main.c >>> @@ -225,6 +225,7 @@ static inline >>> >>> int abortboot(int bootdelay) >>> { >>> >>> int abort = 0; >>> >>> + unsigned long ts; >>> >>> #ifdef CONFIG_MENUPROMPT >>> >>> printf(CONFIG_MENUPROMPT); >>> >>> @@ -248,11 +249,10 @@ int abortboot(int bootdelay) >>> >>> #endif >>> >>> while ((bootdelay > 0) && (!abort)) { >>> >>> - int i; >>> - >>> >>> --bootdelay; >>> >>> - /* delay 100 * 10ms */ >>> - for (i=0; !abort && i<100; ++i) { >>> + /* delay 1000 ms */ >>> + ts = get_timer(0); >>> + do { >>> >>> if (tstc()) { /* we got a key press */ >>> >>> abort = 1; /* don't auto boot */ >>> bootdelay = 0; /* no more delay */ >>> >>> @@ -264,7 +264,7 @@ int abortboot(int bootdelay) >>> >>> break; >>> >>> } >>> udelay(10000); >>> >>> - } >>> + } while (!abort && get_timer(ts) < 1000); >>> >>> printf("\b\b\b%2d ", bootdelay); >>> >>> } >> >> This change is causing problems with auto-delay on one of my boards by >> making it inaccurate :-( >> >> The question is what should get_timer() be returning? If it is meant to >> be milliseconds then I guess I need to fix get_timer() for my board. >> However, if it is just meant to be timer ticks at the SYS_HZ rate then I >> don't see how the above change guarantees the do-while loop waits 1000 >> ms per iteration without normalising to SYS_HZ. > > What board is it ? It is an OMAP2420-H4 board (ARM11). The timer is using a very odd SYS_HZ value of ~46875 (I believe this is because this is the most they can divide down the 12MHz clock source by). The timer could be switched to use a 32kHz clock source instead of the 12MHz clock source and get something closer to 1ms. However, I would need to test to see if this causes any other problems. Cheers Jon
On 03/21/2013 01:57 AM, Wolfgang Denk wrote: > Dear Stephen Warren, > > In message <514AA642.3090107@wwwdotorg.org> you wrote: >> >> Interesting. I just noticed the same problem, and posted the following >> patch to fix it: >> >> http://lists.denx.de/pipermail/u-boot/2013-March/149625.html >> >> This was on the Raspberry Pi. > > You mean the Raspberry Pi uses SYS_HZ != 1000 ??? > > This needs fixing!! I understand that you may wish to have SYS_HZ == 1000, but shouldn't the code still normalise the get_timer() value to SYS_HZ? If I grep through the u-boot source I see a lot of instances where get_timer() is normalised to SYS_HZ. grep -r "get_timer" * | grep "SYS_HZ" Cheers Jon
Dear Jon Hunter, In message <514B2482.5000002@ti.com> you wrote: > > I understand that you may wish to have SYS_HZ == 1000, but shouldn't the > code still normalise the get_timer() value to SYS_HZ? > > If I grep through the u-boot source I see a lot of instances where > get_timer() is normalised to SYS_HZ. > > grep -r "get_timer" * | grep "SYS_HZ" get_timer() is supposed to work in milliseconds. Best regards, Wolfgang Denk
Dear Jon Hunter, In message <514B22FB.1000503@ti.com> you wrote: > > >> The question is what should get_timer() be returning? If it is meant to > >> be milliseconds then I guess I need to fix get_timer() for my board. > >> However, if it is just meant to be timer ticks at the SYS_HZ rate then I It is ticks at SYS_HZ rate with SYS_HZ==1000, i. e. milliseconds. Best regards, Wolfgang Denk
diff --git a/common/main.c b/common/main.c index a15f020..32c4f8a 100644 --- a/common/main.c +++ b/common/main.c @@ -264,7 +264,7 @@ int abortboot(int bootdelay) break; } udelay(10000); - } while (!abort && get_timer(ts) < 1000); + } while (!abort && !(get_timer(ts) / CONFIG_SYS_HZ)); printf("\b\b\b%2d ", bootdelay);