[U-Boot] [PATCH] Fix bootdelay timeout calculation when SYS_HZ!=1000

[U-Boot] [PATCH] Fix bootdelay timeout calculation when SYS_HZ!=1000

[Stephen Warren]

Commit b2f3e0e "console: USB: KBD: Fix incorrect autoboot timeout"
re-wrote the bootdelay timeout loop. However, it hard-coded the value
that get_delay() was expected to increment in one second, rather than
calculating it based on CONFIG_SYS_HZ. On systems where SYS_HZ != 1000,
this caused bootdelay timeout to be incorrect. Fix this.

Cc: Jim Lin <jilin@nvidia.com>
Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
---
 common/main.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/main.c b/common/main.c
index a15f020..4f81ca9 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);
 	}
-- 
1.7.10.4

[U-Boot] [PATCH] Fix bootdelay timeout calculation when SYS_HZ!=1000

[Wolfgang Denk]

Dear Stephen Warren,

In message <1363842874-8286-1-git-send-email-swarren@wwwdotorg.org> you wrote:
> Commit b2f3e0e "console: USB: KBD: Fix incorrect autoboot timeout"
> re-wrote the bootdelay timeout loop. However, it hard-coded the value
> that get_delay() was expected to increment in one second, rather than
> calculating it based on CONFIG_SYS_HZ. On systems where SYS_HZ != 1000,
> this caused bootdelay timeout to be incorrect. Fix this.

Are there any such systems left?  I agree with your patch, but if you
know of systems that use incorrect values of CONFIG_SYS_HZ then please
point these out so they can be fixed!

A system with CONFIG_SYS_HZ != 1000 is _broken_.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
It is dangerous to be right on a subject  on  which  the  established
authorities are wrong.                                    -- Voltaire

[U-Boot] [PATCH] Fix bootdelay timeout calculation when SYS_HZ!=1000

[Tom Rini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/21/2013 02:55 AM, Wolfgang Denk wrote:
> Dear Stephen Warren,
> 
> In message <1363842874-8286-1-git-send-email-swarren@wwwdotorg.org>
> you wrote:
>> Commit b2f3e0e "console: USB: KBD: Fix incorrect autoboot
>> timeout" re-wrote the bootdelay timeout loop. However, it
>> hard-coded the value that get_delay() was expected to increment
>> in one second, rather than calculating it based on CONFIG_SYS_HZ.
>> On systems where SYS_HZ != 1000, this caused bootdelay timeout to
>> be incorrect. Fix this.
> 
> Are there any such systems left?  I agree with your patch, but if
> you know of systems that use incorrect values of CONFIG_SYS_HZ then
> please point these out so they can be fixed!
> 
> A system with CONFIG_SYS_HZ != 1000 is _broken_.

So, RPi is going higher, and Jon hit this on I suspect omap2420h4
which is also higher (after mathing that all out).  If we no longer
support CONFIG_SYS_HZ != 1000 we need to make that clear (and explain
why).

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJRSxA2AAoJENk4IS6UOR1WPzwP/3f+BiTw3s9cWGZKOb4gDza3
n0ke0rZdqLkwixK3Ugl5IqcquTLEh5QZcZb37cGgWQ0N0YvUoIxcBR73mWSHhQgy
VMlVU0vtlrGQsFmN0kbouO+qpzyoAkaDidt6T/WxDazgd+CoCuHguXki1peEB/HQ
ryQ6yNOog5dFUux85cL+AmWZYRkGXRWJfrIqKHZ9MPvz3TxXjhFBZ4lR+8JwMWgS
wPIFhNMTWnIHvXCfbcumFXV0Lgyn0DYnCbKIMLM/NucRKcHX8ZYnX1L8UOGvRzZn
jjqKFm95dNvPhZLLAaZI2TmVu5YMR9VZmmZpst0VC3eACkcQ+70G8CA+3ApmTC2C
3FcLnQQnWlG83/3qj23Qc6trLsk17spEIN0GaSy5ow261sbsXotLEsw0K7luaZPM
Y6KF5cZSyqQZ9TdnIbVxtcsI0Vwk5lLvtB91qleAJDrqn9o4ugEl0IWcRlwN9t9a
DQM0t/P+DotOlLdVQ6elVcimVxBvKLJEMIcdJVyKqsnOQIs9ct9fPm9j03ZRpS9S
YlvegCU/n0J25RVtZL6+hpOQa2ZkZOyFg3sZwSLx42jDilvIYemYcGaH21C64prJ
HJ7+rnxPvK1lC4DPVP0QoIrQEpgABnTZfDaLsts/z3sq2m4+S9uZHqiwyBJ4luoa
ua2LQXAovkSF1FA2kV1I
=VTCE
-----END PGP SIGNATURE-----

[U-Boot] [PATCH] Fix bootdelay timeout calculation when SYS_HZ!=1000

[Wolfgang Denk]

Dear Tom,

In message <514B1036.3090400@ti.com> you wrote:
>
> > A system with CONFIG_SYS_HZ != 1000 is _broken_.
> 
> So, RPi is going higher, and Jon hit this on I suspect omap2420h4
> which is also higher (after mathing that all out).  If we no longer
> support CONFIG_SYS_HZ != 1000 we need to make that clear (and explain
> why).

It has never been supported, so this is not a case of "no longer".
There are several longish threads (about the timer code, especially
on ARM) in the archives.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"The whole world is about three drinks behind."     - Humphrey Bogart

[U-Boot] [PATCH] Fix bootdelay timeout calculation when SYS_HZ!=1000

[Tom Rini]

On Thu, Mar 21, 2013 at 03:35:40PM +0100, Wolfgang Denk wrote:
> Dear Tom,
> 
> In message <514B1036.3090400@ti.com> you wrote:
> >
> > > A system with CONFIG_SYS_HZ != 1000 is _broken_.
> > 
> > So, RPi is going higher, and Jon hit this on I suspect omap2420h4
> > which is also higher (after mathing that all out).  If we no longer
> > support CONFIG_SYS_HZ != 1000 we need to make that clear (and explain
> > why).
> 
> It has never been supported, so this is not a case of "no longer".
> There are several longish threads (about the timer code, especially
> on ARM) in the archives.

OK, then we need to do something about these platforms today.  I'm
guessing RPi can just be tuned down to 1000 but for omap2420h4 it's an
interesting value and I don't know about the platform well enough to say
what we'd need to do to adapt it.  Jon?

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130321/17cd67b6/attachment.pgp>

[U-Boot] [PATCH] Fix bootdelay timeout calculation when SYS_HZ!=1000

[Jon Hunter]

On 03/21/2013 09:41 AM, Tom Rini wrote:
> On Thu, Mar 21, 2013 at 03:35:40PM +0100, Wolfgang Denk wrote:
>> Dear Tom,
>>
>> In message <514B1036.3090400@ti.com> you wrote:
>>>
>>>> A system with CONFIG_SYS_HZ != 1000 is _broken_.
>>>
>>> So, RPi is going higher, and Jon hit this on I suspect omap2420h4
>>> which is also higher (after mathing that all out).  If we no longer
>>> support CONFIG_SYS_HZ != 1000 we need to make that clear (and explain
>>> why).
>>
>> It has never been supported, so this is not a case of "no longer".
>> There are several longish threads (about the timer code, especially
>> on ARM) in the archives.
> 
> OK, then we need to do something about these platforms today.  I'm
> guessing RPi can just be tuned down to 1000 but for omap2420h4 it's an
> interesting value and I don't know about the platform well enough to say
> what we'd need to do to adapt it.  Jon?

For OMAP2420, we have a choice of using either a 12MHz or 32kHz clock to
drive the timer. With the 32kHz clock we can get close to 1000Hz tick
but we cannot get it dead on. That's why OMAP has been using 128Hz tick
in the kernel as opposed to 100Hz tick in the kernel for years (although
that was changed recently).

Cheers
Jon

[U-Boot] [PATCH] Fix bootdelay timeout calculation when SYS_HZ!=1000

[Tom Rini]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 03/21/2013 11:25 AM, Jon Hunter wrote:
> 
> On 03/21/2013 09:41 AM, Tom Rini wrote:
>> On Thu, Mar 21, 2013 at 03:35:40PM +0100, Wolfgang Denk wrote:
>>> Dear Tom,
>>> 
>>> In message <514B1036.3090400@ti.com> you wrote:
>>>> 
>>>>> A system with CONFIG_SYS_HZ != 1000 is _broken_.
>>>> 
>>>> So, RPi is going higher, and Jon hit this on I suspect
>>>> omap2420h4 which is also higher (after mathing that all out).
>>>> If we no longer support CONFIG_SYS_HZ != 1000 we need to make
>>>> that clear (and explain why).
>>> 
>>> It has never been supported, so this is not a case of "no
>>> longer". There are several longish threads (about the timer
>>> code, especially on ARM) in the archives.
>> 
>> OK, then we need to do something about these platforms today.
>> I'm guessing RPi can just be tuned down to 1000 but for
>> omap2420h4 it's an interesting value and I don't know about the
>> platform well enough to say what we'd need to do to adapt it.
>> Jon?
> 
> For OMAP2420, we have a choice of using either a 12MHz or 32kHz
> clock to drive the timer. With the 32kHz clock we can get close to
> 1000Hz tick but we cannot get it dead on. That's why OMAP has been
> using 128Hz tick in the kernel as opposed to 100Hz tick in the
> kernel for years (although that was changed recently).

The rest of the OMAP/related platforsm in U-Boot accept that as "good"
enough I guess since they've been setting to 1000 for a while / since
they went in (depending on the platform in particular).  So if you
have time to whack 2420 over and give it some quick testing I'd
appreciate it.  Thanks!

- -- 
Tom
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJRSycGAAoJENk4IS6UOR1WXIQQAIZOPSG04jMTavV8rBykXnjL
WQAvUmnGeAs7A2sMXfq+1HhXkLK/hMKHYR5IRLEx0asYuOXZ+sls9oPBxwpXHYQq
zMRcoEZRbWkz/Ell0Pe6gyN4LesCvTd+Zt+nV/Rs+oO0o3ITtqYOzTNOfaNwyftC
xXBUCXi9ecMnKhJb00IrAuELvU1kyb54v9tuJQNkw6ML0cWxTQiQFgzfMT8lRf/E
DwSaDxdQEi4+CzmXDE7VClx0P5R5GDd3NhtaQ8lz9w6mPoJ/GyFmYI4PIWAqsscO
ryRyrJlPx0KusmMZrtVMdBJqeXi4rU1vsrAqHsbntPWTt7WznyHOdELDtSoUbwHE
WotIW3+xqckNKNYjPYpWU1syRlWUqPK+wbHfB0bEVEtdPNQCBJ/jT34c/Ur4V7fn
aAJkhaimH/oWKtBiZD2hEUKZzsILrUpH0mk0S4xra5mHVDacypaeJ6xK1h0vkZTw
CoRHXH05JZlLJf1NXP3lRg/b8NUEH4Z/nGvQpmHUOySgPiGYs1CQx6f+3A8P2Rrd
W9TQ+fSWxtRzRUuHf18QdYFP5MCvWwgRuvquxsqlUAULzWYbisSb66TUY4vOyYZG
6htN9/wZEinsbmGlx6yTA6RBijSMP3kpSswwyN/Y1AHNP04MQO+Gflii9NvdtsRM
1jQ6as+M3YHCh7jAdINj
=yJ2h
-----END PGP SIGNATURE-----

[U-Boot] [PATCH] Fix bootdelay timeout calculation when SYS_HZ!=1000

[Stephen Warren]

On 03/21/2013 12:55 AM, Wolfgang Denk wrote:
> Dear Stephen Warren,
> 
> In message <1363842874-8286-1-git-send-email-swarren@wwwdotorg.org> you wrote:
>> Commit b2f3e0e "console: USB: KBD: Fix incorrect autoboot timeout"
>> re-wrote the bootdelay timeout loop. However, it hard-coded the value
>> that get_delay() was expected to increment in one second, rather than
>> calculating it based on CONFIG_SYS_HZ. On systems where SYS_HZ != 1000,
>> this caused bootdelay timeout to be incorrect. Fix this.
> 
> Are there any such systems left?  I agree with your patch, but if you
> know of systems that use incorrect values of CONFIG_SYS_HZ then please
> point these out so they can be fixed!
> 
> A system with CONFIG_SYS_HZ != 1000 is _broken_.

Then why does the config option still exist; surely it should be
removed, or defined somewhere other than the per-board config file?

[U-Boot] [PATCH] Fix bootdelay timeout calculation when SYS_HZ!=1000

[Jon Hunter]

On 03/21/2013 10:28 AM, Tom Rini wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On 03/21/2013 11:25 AM, Jon Hunter wrote:
>>
>> On 03/21/2013 09:41 AM, Tom Rini wrote:
>>> On Thu, Mar 21, 2013 at 03:35:40PM +0100, Wolfgang Denk wrote:
>>>> Dear Tom,
>>>>
>>>> In message <514B1036.3090400@ti.com> you wrote:
>>>>>
>>>>>> A system with CONFIG_SYS_HZ != 1000 is _broken_.
>>>>>
>>>>> So, RPi is going higher, and Jon hit this on I suspect
>>>>> omap2420h4 which is also higher (after mathing that all out).
>>>>> If we no longer support CONFIG_SYS_HZ != 1000 we need to make
>>>>> that clear (and explain why).
>>>>
>>>> It has never been supported, so this is not a case of "no
>>>> longer". There are several longish threads (about the timer
>>>> code, especially on ARM) in the archives.
>>>
>>> OK, then we need to do something about these platforms today.
>>> I'm guessing RPi can just be tuned down to 1000 but for
>>> omap2420h4 it's an interesting value and I don't know about the
>>> platform well enough to say what we'd need to do to adapt it.
>>> Jon?
>>
>> For OMAP2420, we have a choice of using either a 12MHz or 32kHz
>> clock to drive the timer. With the 32kHz clock we can get close to
>> 1000Hz tick but we cannot get it dead on. That's why OMAP has been
>> using 128Hz tick in the kernel as opposed to 100Hz tick in the
>> kernel for years (although that was changed recently).
> 
> The rest of the OMAP/related platforsm in U-Boot accept that as "good"
> enough I guess since they've been setting to 1000 for a while / since
> they went in (depending on the platform in particular).  So if you
> have time to whack 2420 over and give it some quick testing I'd
> appreciate it.  Thanks!

Looks like the OMAP timers are running faster than 1000Hz but the timer
tick rate is being normalised to 1000Hz by dividing the actual timer
rate by SYS_HZ. I see ...

	readl(&timer_base->tcrr) / (TIMER_CLOCK / CONFIG_SYS_HZ);

We could do the same for OMAP2420. In fact we could move
arch/arm/cpu/armv7/omap-common/timer.c to arch/arm/omap-common/timer.c
and use this for OMAP2420. The legacy OMAP2420 code is very similar to this.

Jon

[U-Boot] [PATCH] Fix bootdelay timeout calculation when SYS_HZ!=1000

[Tom Rini]

On Thu, Mar 21, 2013 at 01:07:18PM -0500, Jon Hunter wrote:
> 
> On 03/21/2013 10:28 AM, Tom Rini wrote:
> > -----BEGIN PGP SIGNED MESSAGE-----
> > Hash: SHA1
> > 
> > On 03/21/2013 11:25 AM, Jon Hunter wrote:
> >>
> >> On 03/21/2013 09:41 AM, Tom Rini wrote:
> >>> On Thu, Mar 21, 2013 at 03:35:40PM +0100, Wolfgang Denk wrote:
> >>>> Dear Tom,
> >>>>
> >>>> In message <514B1036.3090400@ti.com> you wrote:
> >>>>>
> >>>>>> A system with CONFIG_SYS_HZ != 1000 is _broken_.
> >>>>>
> >>>>> So, RPi is going higher, and Jon hit this on I suspect
> >>>>> omap2420h4 which is also higher (after mathing that all out).
> >>>>> If we no longer support CONFIG_SYS_HZ != 1000 we need to make
> >>>>> that clear (and explain why).
> >>>>
> >>>> It has never been supported, so this is not a case of "no
> >>>> longer". There are several longish threads (about the timer
> >>>> code, especially on ARM) in the archives.
> >>>
> >>> OK, then we need to do something about these platforms today.
> >>> I'm guessing RPi can just be tuned down to 1000 but for
> >>> omap2420h4 it's an interesting value and I don't know about the
> >>> platform well enough to say what we'd need to do to adapt it.
> >>> Jon?
> >>
> >> For OMAP2420, we have a choice of using either a 12MHz or 32kHz
> >> clock to drive the timer. With the 32kHz clock we can get close to
> >> 1000Hz tick but we cannot get it dead on. That's why OMAP has been
> >> using 128Hz tick in the kernel as opposed to 100Hz tick in the
> >> kernel for years (although that was changed recently).
> > 
> > The rest of the OMAP/related platforsm in U-Boot accept that as "good"
> > enough I guess since they've been setting to 1000 for a while / since
> > they went in (depending on the platform in particular).  So if you
> > have time to whack 2420 over and give it some quick testing I'd
> > appreciate it.  Thanks!
> 
> Looks like the OMAP timers are running faster than 1000Hz but the timer
> tick rate is being normalised to 1000Hz by dividing the actual timer
> rate by SYS_HZ. I see ...
> 
> 	readl(&timer_base->tcrr) / (TIMER_CLOCK / CONFIG_SYS_HZ);
> 
> We could do the same for OMAP2420. In fact we could move
> arch/arm/cpu/armv7/omap-common/timer.c to arch/arm/omap-common/timer.c
> and use this for OMAP2420. The legacy OMAP2420 code is very similar to this.

Sounds good to me, thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20130321/c817959c/attachment.pgp>

[U-Boot] [PATCH] Fix bootdelay timeout calculation when SYS_HZ!=1000

[Wolfgang Denk]

Dear Stephen Warren,

In message <514B2F8E.2090409@wwwdotorg.org> you wrote:
>
> Then why does the config option still exist; surely it should be
> removed, or defined somewhere other than the per-board config file?

Removing it requires work, including testing efforts...

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
A year spent in artificial intelligence is enough to make one believe
in God.