r990 udelay fails for me ...

[Urja Rannikko]

Hi...
first:
[root@phenom trunk]# ./flashrom -V
flashrom v0.9.1-r990 on Linux 2.6.32-ARCH (x86_64), built with libpci 3.1.7,
GCC 4.4.3 20100316 (prerelease)
No coreboot table found.
DMI string system-manufacturer: "System Manufacturer"
DMI string system-product-name: "System Name"
DMI string system-version: "System Version"
DMI string baseboard-manufacturer: "ASUSTeK Computer INC."
DMI string baseboard-product-name: "K8S-MX"
DMI string baseboard-version: "Rev 1.xx"
DMI string chassis-type: "Desktop"
This chipset supports the following protocols: Non-SPI.
WARNING: No chipset found. Flash detection will most likely fail.
Calibrating delay loop... 424M loops per second, delay more than
1011355431720o short, recalculating...

... and it halts there... (actually it doesnt, look at the end of mail) This
is the first cosmetic fix:

Index: udelay.c
===================================================================
--- udelay.c    (revision 990)
+++ udelay.c    (working copy)
@@ -93,7 +93,7 @@
          */
         for (i = 0; i < 4; i++) {
             if (measure_delay(100) < 90) {
-                msg_pdbg("delay more than 10% too short, "
+                msg_pdbg("delay more than 10%% too short, "
                      "recalculating... ");
                 goto recalibrate;
             }

That doesnt help with the 100% CPU + "halt" (i dont know how long it would
take...).

I know that flashrom doesnt support K8S-MX (the internal driver anyways).
I'm just back at work with serprog on this machine,
so it seems funny to test the delay abilities of the host system even with
-p serprog ...
-- 21:15...
uh so it isnt halted, just taking a long time (continuation to that log)
" 370M loops per second, delay more than 1011355432146o short,
recalculating...373M loops per second, delay more than 1011355432350o short,
recalculating...  "
(I left it on while lookin at that % thing)...
--- 21:20...
<cont> "366M loops per second, delay more than 1011355432556o short,
recalculating... 373M loops per second, delay more than 1011355432350o
short, recalculating... "
-- 21:23
<cont> " 367M loops per second, delay loop is unreliable, trying to continue
10 myus = 7 us, 100 myus = 69 us, 1000 myus = 687 us, 10000 myus = 11233 us,
OK."
Then it scans for flash chips on this thing and finds nothing, which is
normal. Note that i'm running on x86_64 (if it works on 32-bit machines, i
think that might have something to do with this, not sure.).

Report complete,
urjaman out

[Urja Rannikko]

This seems to "fix" this for me - the delay loop is still unreliable, but
atleast the recalibration attempts only take 5s instead of minutes. This is
a combined diff for both my fix and the % fix.

Index: udelay.c
===================================================================
--- udelay.c    (revision 990)
+++ udelay.c    (working copy)
@@ -66,6 +66,7 @@
     printf("Calibrating delay loop... ");

recalibrate:
+    count = 1000;
     while (1) {
         timeusec = measure_delay(count);
         if (timeusec > 1000000 / 4)
@@ -93,7 +94,7 @@
          */
         for (i = 0; i < 4; i++) {
             if (measure_delay(100) < 90) {
-                msg_pdbg("delay more than 10% too short, "
+                msg_pdbg("delay more than 10%% too short, "
                      "recalculating... ");
                 goto recalibrate;

Signed-off-by: Urja Rannikko

[Carl-Daniel Hailfinger]

On 03.04.2010 01:28, Urja Rannikko wrote:
> This seems to "fix" this for me - the delay loop is still unreliable, but
> atleast the recalibration attempts only take 5s instead of minutes. This is
> a combined diff for both my fix and the % fix.
> Index: udelay.c
>
> --- udelay.c	(revision 990)
> +++ udelay.c	(working copy)
> @@ -66,6 +66,7 @@
>    printf("Calibrating delay loop... ");
>
> recalibrate:
> count = 1000;
> while (1) {
> timeusec = measure_delay(count);
> if (timeusec > 1000000 / 4)

Does the timing get more reliable if you try the following stuff:

- change the comparison above to

if (timeusec > 1000)

- Make sure there are no CPU frequency changes or sleep states
> @@ -93,7 +94,7 @@
>    	 */
>    	for (i = 0; i < 4; i++) {
>    		if (measure_delay(100) < 90) {
> 		msg_pdbg("delay more than 10% too short, "
> 		msg_pdbg("delay more than 10%% too short, "
> 		 "recalculating... ");
> 	goto recalibrate;
> }
>
> Signed-off-by: Urja Rannikko

In general, I like your patch. I just hope to get more input if reducing
the abort condition in the loop from 250 ms to 1 ms helps as well.

Regards,
Carl-Daniel

[Carl-Daniel Hailfinger]

On 03.04.2010 23:09, Carl-Daniel Hailfinger wrote:
> On 03.04.2010 01:28, Urja Rannikko wrote:
> > This seems to "fix" this for me - the delay loop is still unreliable, but
> > atleast the recalibration attempts only take 5s instead of minutes. This is
> > a combined diff for both my fix and the % fix.
> > Index: udelay.c
> >
> > --- udelay.c	(revision 990)
> > +++ udelay.c	(working copy)
> > @@ -66,6 +66,7 @@
> >    printf("Calibrating delay loop... ");
> >
> > recalibrate:
> > count = 1000;
> > while (1) {
> > timeusec = measure_delay(count);
> > if (timeusec > 1000000 / 4)
>
> Does the timing get more reliable if you try the following stuff:
> change the comparison above to
>
> if (timeusec > 1000)
> Make sure there are no CPU frequency changes or sleep states
> > @@ -93,7 +94,7 @@
> >    	 */
> >    	for (i = 0; i < 4; i++) {
> >    		if (measure_delay(100) < 90) {
> > 		msg_pdbg("delay more than 10% too short, "
> > 		msg_pdbg("delay more than 10%% too short, "
> > 		 "recalculating... ");
> > 	goto recalibrate;
> > }
> >
> > Signed-off-by: Urja Rannikko
>
> In general, I like your patch. I just hope to get more input if reducing
> the abort condition in the loop from 250 ms to 1 ms helps as well.

By the way, issuing a sleep/yield command before starting the timer
calibration could help to make sure we start calibrating at the start of
an OS scheduler timeslice, and with 1 ms calibration measurements there
is a fair chance that the complete calibration happens during one
timeslice. That should rule out all scheduler interaction.

Side note: flashrom was never designed to run on machines where any
other process uses significant amounts of CPU since that totally messes
up the delay loop.

Regards,
Carl-Daniel

[Carl-Daniel Hailfinger]

On 03.04.2010 23:09, Carl-Daniel Hailfinger wrote:
> On 03.04.2010 01:28, Urja Rannikko wrote:
> > This seems to "fix" this for me - the delay loop is still unreliable, but
> > atleast the recalibration attempts only take 5s instead of minutes. This is
> > a combined diff for both my fix and the % fix.
> > Index: udelay.c
> >
> > --- udelay.c	(revision 990)
> > +++ udelay.c	(working copy)
> > @@ -66,6 +66,7 @@
> >    printf("Calibrating delay loop... ");
> >
> > recalibrate:
> > count = 1000;
> > while (1) {
> > timeusec = measure_delay(count);
> > if (timeusec > 1000000 / 4)
>
> Does the timing get more reliable if you try the following stuff:
> change the comparison above to
>
> if (timeusec > 1000)

I expect this little change to improve precision dramatically.
> Make sure there are no CPU frequency changes or sleep states
> > Signed-off-by: Urja Rannikko
>
> In general, I like your patch. I just hope to get more input if reducing
> the abort condition in the loop from 250 ms to 1 ms helps as well.

We need your patch, I had just hoped that improved precision would be
possible with the additional change I suggested. Anyway, this is
Acked-by: Carl-Daniel Hailfinger

Can you commit?

Regards,
Carl-Daniel

[Carl-Daniel Hailfinger]

On 07.04.2010 14:19, Carl-Daniel Hailfinger wrote:
> On 03.04.2010 23:09, Carl-Daniel Hailfinger wrote:
> > On 03.04.2010 01:28, Urja Rannikko wrote:
> > > This seems to "fix" this for me - the delay loop is still unreliable, but
> > > atleast the recalibration attempts only take 5s instead of minutes. This is
> > > a combined diff for both my fix and the % fix.
> > >
> > > Signed-off-by: Urja Rannikko
>
> Acked-by: Carl-Daniel Hailfinger

Thanks, committed in r992.

Regards,
Carl-Daniel