Remove calls to [get, reset]_timer outside arch/

There is no need to use get_timer() and reset_timer() and there are build

Hi Graeme,

Quoted 1 line · Graeme Russ

There is no need to use get_timer() and reset_timer() and there are build

I must have missed something WRT reset_timer() -- my apologies if I'm covering old ground.

When the timestamp is incremented using an interrupt that occurs with a period greater than 1 ms, we can get early timeouts. reset_timer() solved the problem. What's the recommended approach for dealing with this without reset_timer() ?

Quoted 12 lines · Scott McNutt

Hi Graeme,

Quoted 1 line · Graeme Russ

There is no need to use get_timer() and reset_timer() and there are build

I must have missed something WRT reset_timer() -- my apologies if I'm covering old ground.

When the timestamp is incremented using an interrupt that occurs with a period greater than 1 ms, we can get early timeouts. reset_timer() solved the problem. What's the recommended approach for dealing with this without reset_timer() ?

There is an active thread on the timer API right now. Short answer - The API is broken - Calling reset_timer() is not the right solution because:
a) It breaks recursive or nested timing loops
b) For some arches, udelay() has a side-effect as well

All this needs fixing

Hi Bill,

Quoted 17 lines · J. William Campbell

Quoted 16 lines · Scott McNutt

Dear Graeme,

Quoted 13 lines · Graeme Russ

Quoted 12 lines · Scott McNutt

Hi Graeme,

Quoted 2 lines · Graeme Russ

There is no need to use get_timer() and reset_timer() and there are build

I must have missed something WRT reset_timer() -- my apologies if I'm covering old ground.

When the timestamp is incremented using an interrupt that occurs with a period greater than 1 ms, we can get early timeouts. reset_timer() solved the problem. What's the recommended approach for dealing with this without reset_timer() ?

Quoted 7 lines

Hi Scott,
Are you saying that the interrupt frequency is greater than
1000 times per second, or as I read it, the frequency is less than 1000 per second (period greater than 1 ms). If anything, that should make the timer run slow, not fast.
I wonder if it is a resolution issue. What are the typical delays in ms
you are using?

Some older nios2 implementations have _fixed_ 10 msec timers. Basically, the timestamp is incremented asynchronous to get_timer(0). So a 10 msec timeout can occur, for example, almost immediately if the timer isn't reset just prior to calling get_timer(0). There are more details in the comments for the following commits:

nios2: Reload timer count in reset_timer():
d8bc0a2889700ba063598de6d4e7d135360b537e

cfi_flash: reset timer in flash status check:
22d6c8faac4e9fa43232b0cf4da427ec14d72ad3

I'm totally in favor of cleaning this stuff up. It caused some headaches (and wasted time) about 13 months ago. My primary concern is to avoid breaking things that currently work for us nios2 weenies ... at least for any length of time.

Things are a bit tight for me until next week or so. I'll probably come up for air around June 1st ... and I'll be glad to help out.

Quoted 44 lines · Scott McNutt

Hi Bill,

Quoted 17 lines · J. William Campbell

Quoted 16 lines · Scott McNutt

Dear Graeme,

Quoted 13 lines · Graeme Russ

Quoted 12 lines · Scott McNutt

Hi Graeme,

Quoted 2 lines · Graeme Russ

There is no need to use get_timer() and reset_timer() and there are build

I must have missed something WRT reset_timer() -- my apologies if I'm covering old ground.

When the timestamp is incremented using an interrupt that occurs with a period greater than 1 ms, we can get early timeouts. reset_timer() solved the problem. What's the recommended approach for dealing with this without reset_timer() ?

Quoted 7 lines

Hi Scott,
Are you saying that the interrupt frequency is greater than
1000 times per second, or as I read it, the frequency is less than 1000 per second (period greater than 1 ms). If anything, that should make the timer run slow, not fast.
I wonder if it is a resolution issue. What are the typical delays in ms
you are using?

Some older nios2 implementations have _fixed_ 10 msec timers. Basically, the timestamp is incremented asynchronous to get_timer(0). So a 10 msec timeout can occur, for example, almost immediately if the timer isn't reset just prior to calling get_timer(0). There are more details in the comments for the following commits:

nios2: Reload timer count in reset_timer():

   d8bc0a2889700ba063598de6d4e7d135360b537e

cfi_flash: reset timer in flash status check:

   22d6c8faac4e9fa43232b0cf4da427ec14d72ad3

I'm totally in favor of cleaning this stuff up. It caused some headaches (and wasted time) about 13 months ago. My primary concern is to avoid breaking things that currently work for us nios2 weenies ... at least for any length of time.

Quoted 3 lines

Things are a bit tight for me until next week or so. I'll probably come up for air around June 1st ... and I'll be glad to help out.

Is there any reason why we cannot silently perform a reset_timer() any time set_timer() is called with a parameter of 0?

Regards,

Graeme

Quoted 45 lines · Graeme Russ

Quoted 44 lines · Scott McNutt

Hi Bill,

Quoted 17 lines · J. William Campbell

Quoted 16 lines · Scott McNutt

Dear Graeme,

Quoted 13 lines · Graeme Russ

Quoted 12 lines · Scott McNutt

Hi Graeme,

Quoted 2 lines · Graeme Russ

There is no need to use get_timer() and reset_timer() and there are build

I must have missed something WRT reset_timer() -- my apologies if I'm covering old ground.

When the timestamp is incremented using an interrupt that occurs with a period greater than 1 ms, we can get early timeouts. reset_timer() solved the problem. What's the recommended approach for dealing with this without reset_timer() ?

Quoted 7 lines

Hi Scott,
Are you saying that the interrupt frequency is greater than
1000 times per second, or as I read it, the frequency is less than 1000 per second (period greater than 1 ms). If anything, that should make the timer run slow, not fast.
I wonder if it is a resolution issue. What are the typical delays in ms
you are using?

Some older nios2 implementations have _fixed_ 10 msec timers. Basically, the timestamp is incremented asynchronous to get_timer(0). So a 10 msec timeout can occur, for example, almost immediately if the timer isn't reset just prior to calling get_timer(0). There are more details in the comments for the following commits:

nios2: Reload timer count in reset_timer():

   d8bc0a2889700ba063598de6d4e7d135360b537e

cfi_flash: reset timer in flash status check:

   22d6c8faac4e9fa43232b0cf4da427ec14d72ad3

I'm totally in favor of cleaning this stuff up. It caused some headaches (and wasted time) about 13 months ago. My primary concern is to avoid breaking things that currently work for us nios2 weenies ... at least for any length of time.

Quoted 5 lines

Quoted 3 lines

Things are a bit tight for me until next week or so. I'll probably come up for air around June 1st ... and I'll be glad to help out.

Is there any reason why we cannot silently perform a reset_timer() any time set_timer() is called with a parameter of 0?

Hi All,
I assume you mean get_timer(0)? In principle, you cannot do this

because it could be inside another get_timer(0) loop that has already some time elapsed before you hit the inner get_timer(0). I think what needs to

happen on the old NIOS with 10 ms resolution on the interrupt times is that all timer intervals must have 10 ms added and then rounded up to the nearest multiple of 10. Thus, if you wanted to wait for 1 millisecond, you must use an argument of 20 ms to be sure you wait at all! If you use an argument of 10, it won't help because you could get an interrupt right away and exit. If these routines are nios2 specific, you could add a local reset_timer, but I assume they are generic. . Note that if these routines are not nios2 specific, is there any harm in waiting "too long"?

Quoted 46 lines · J. William Campbell

Quoted 45 lines · Graeme Russ

Quoted 44 lines · Scott McNutt

Hi Bill,

Quoted 17 lines · J. William Campbell

Quoted 16 lines · Scott McNutt

Dear Graeme,

Quoted 13 lines · Graeme Russ

Quoted 12 lines · Scott McNutt

Hi Graeme,

Quoted 2 lines · Graeme Russ

There is no need to use get_timer() and reset_timer() and there are build

I must have missed something WRT reset_timer() -- my apologies if I'm covering old ground.

When the timestamp is incremented using an interrupt that occurs with a period greater than 1 ms, we can get early timeouts. reset_timer() solved the problem. What's the recommended approach for dealing with this without reset_timer() ?

Quoted 7 lines

Hi Scott,
Are you saying that the interrupt frequency is greater than
1000 times per second, or as I read it, the frequency is less than 1000 per second (period greater than 1 ms). If anything, that should make the timer run slow, not fast.
I wonder if it is a resolution issue. What are the typical delays in ms
you are using?

Some older nios2 implementations have _fixed_ 10 msec timers. Basically, the timestamp is incremented asynchronous to get_timer(0). So a 10 msec timeout can occur, for example, almost immediately if the timer isn't reset just prior to calling get_timer(0). There are more details in the comments for the following commits:

nios2: Reload timer count in reset_timer():

    d8bc0a2889700ba063598de6d4e7d135360b537e

cfi_flash: reset timer in flash status check:

    22d6c8faac4e9fa43232b0cf4da427ec14d72ad3

I'm totally in favor of cleaning this stuff up. It caused some headaches (and wasted time) about 13 months ago. My primary concern is to avoid breaking things that currently work for us nios2 weenies ... at least for any length of time.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Things are a bit tight for me until next week or so. I'll probably come up for air around June 1st ... and I'll be glad to help out.

Is there any reason why we cannot silently perform a reset_timer() any time set_timer() is called with a parameter of 0?

Hi All,
I assume you mean get_timer(0)? In principle, you cannot do this

Yes - it's early, no coffee yet ;)

Quoted 2 lines

because it could be inside another get_timer(0) loop that has already some time elapsed before you hit the inner get_timer(0). I think what needs to

Correct, but that is what is already happening for ALL arches in cfi due to the reset_timer() before get_timer(0) - I am suggesting sandboxing the problem to NIOS until we sort out the timer API properly

Quoted 8 lines

happen on the old NIOS with 10 ms resolution on the interrupt times is that all timer intervals must have 10 ms added and then rounded up to the nearest multiple of 10. Thus, if you wanted to wait for 1 millisecond, you must use an argument of 20 ms to be sure you wait at all! If you use an argument of 10, it won't help because you could get an interrupt right away and exit. If these routines are nios2 specific, you could add a local reset_timer, but I assume they are generic. . Note that if these routines are not nios2 specific, is there any harm in waiting "too long"?

Well, we have no control over the argument in cfi driver (unless you plan to put #ifdef NIOS all over the place)

Maybe we could round up the parameter inside get_timer() itself?

Regards,

Graeme

Well, we have no control over the argument in cfi driver (unless you plan to put #ifdef NIOS all over the place)

Maybe we could round up the parameter inside get_timer() itself?

On Tue, May 24, 2011 at 7:02 AM, Graeme Russ<[email protected]> wrote:

[snip]

Quoted 4 lines

Well, we have no control over the argument in cfi driver (unless you plan to put #ifdef NIOS all over the place)

Maybe we could round up the parameter inside get_timer() itself?

Wow, what was I on! - Oh, thats right, no coffee ;)

The parameter to get_timer() is not a timeout, it is a reference epoch

So I think adding reset_timer() to the NIOS get_timer() when the parameter is zero is the only option

Regards,

Graeme

Quoted 3 lines

Is there any reason why we cannot silently perform a reset_timer() any time set_timer() is called with a parameter of 0?

Hi All,
I assume you mean get_timer(0)? In principle, you cannot do this
because it could be inside another get_timer(0) loop that has already some time elapsed before you hit the inner get_timer(0). I think what needs to happen on the old NIOS with 10 ms resolution on the interrupt times is that all timer intervals must have 10 ms added and then rounded up to the nearest multiple of 10. Thus, if you wanted to wait for 1 millisecond, you must use an argument of 20 ms to be sure you wait at all! If you use an argument of 10, it won't help because you could get an interrupt right away and exit. If these routines are nios2 specific, you could add a local reset_timer, but I assume they are generic. . Note that if these routines are not nios2 specific, is there any harm in waiting "too long"?

Dear "J. William Campbell",

Quoted 16 lines · In message <[email protected]> you wrote:

Quoted 3 lines

Is there any reason why we cannot silently perform a reset_timer() any time set_timer() is called with a parameter of 0?

Hi All,
? ? ? I assume you mean get_timer(0)? ?In principle, you cannot do this because it could be inside another get_timer(0) loop that has already some time elapsed before you hit the inner get_timer(0). I think what needs to happen on the old NIOS with 10 ms resolution on the interrupt times is that all timer intervals must have 10 ms added and then rounded up to the nearest multiple of 10. Thus, if you wanted to wait for 1 millisecond, you must use an argument of 20 ms to be sure you wait at all! If you use an argument of 10, it won't help because you could get an interrupt right away and exit. If these routines are nios2 specific, you could add a local reset_timer, but I assume they are generic. . Note that if these routines are not nios2 specific, is there any harm in waiting "too long"?

I think in the context of this rework get_timer() should be changed to take no argumen. Actually noe is needed, and if used as is now it can only cuse harm.

I think this might break stand alone applications - I am sure get_timer() is exported.

There is no need to use get_timer() and reset_timer() and there are build breakages occuring because of them (specifically cfi_flash). Remove any usage outside arch/ to fix build breakages and to prepare for complete removal

Signed-off-by Graeme Russ <[email protected]>

---
checkpatch complains about long lines and brace usage in the board specific flash.c files - They are deprecated and not worth fixing for style

 board/BuS/EB+MCF-EV123/flash.c |   10 ++++++----

Am 2011-05-23 13:54, schrieb Graeme Russ:

Quoted 5 lines

There is no need to use get_timer() and reset_timer() and there are build breakages occuring because of them (specifically cfi_flash). Remove any usage outside arch/ to fix build breakages and to prepare for complete removal

Quoted 6 lines

Signed-off-by Graeme Russ <[email protected]>

---
checkpatch complains about long lines and brace usage in the board specific flash.c files - They are deprecated and not worth fixing for style

 board/BuS/EB+MCF-EV123/flash.c |   10 ++++++----

Ack, for EB+MCF-EV123 board

Can you please send a formal Acked-by: message?

Note that this is NOT nitpicking: Patchwork will automatically add such correct Acked-by: lines to the patch, so we don't have to track thse manually - this saves a LOT of time to the maintainers.

Thanks.

Wolfgang Denk

Quoted 4 lines

Note that this is NOT nitpicking: Patchwork will automatically add such correct Acked-by: lines to the patch, so we don't have to track thse manually - this saves a LOT of time to the maintainers.

One question: where is the information presented in patchwork, apart from coloring the ack line when displaying the patch discussion thread? I cannot find e.g. a summary list of all acks to a given patch.

Dear Albert ARIBAUD,

Quoted 8 lines · In message<[email protected]> you wrote:

Quoted 4 lines

Note that this is NOT nitpicking: Patchwork will automatically add such correct Acked-by: lines to the patch, so we don't have to track thse manually - this saves a LOT of time to the maintainers.

One question: where is the information presented in patchwork, apart from coloring the ack line when displaying the patch discussion thread? I cannot find e.g. a summary list of all acks to a given patch.

PW automatically inserts this into the patch if you click on the "Download: mbox" link, or is you access the patch through pwclient.

For example, instead of applying a patch directly from my mailbox I use this file only to get the hash value for the PW entry, and then use pwclient to apply it and to update it's state:

HASH=$(pwparser.py --hash<$PATCH)
if [ -z "$HASH" ]
then
echo "ERROR: $PATCH - no such entry in PatchWork">&2 exit 1
fi

if pwclient apply -h $HASH
then
pwclient update -s 'Accepted' -h $HASH
fi

This is extremely convenient as it automatically takes care of both the Acks and the state change.

Best regards,

Wolfgang Denk

Quoted 17 lines

For example, instead of applying a patch directly from my mailbox I use this file only to get the hash value for the PW entry, and then use pwclient to apply it and to update it's state:

HASH=$(pwparser.py --hash<$PATCH)
if [ -z "$HASH" ]
then
echo "ERROR: $PATCH - no such entry in PatchWork">&2 exit 1
fi

if pwclient apply -h $HASH
then
pwclient update -s 'Accepted' -h $HASH
fi

Quoted 2 lines

This is extremely convenient as it automatically takes care of both the Acks and the state change.

Thanks. Looks like this could be handy for many others. Maybe it should be 'backported' into pwclient itself as a new command.

Dear Albert ARIBAUD,

Quoted 17 lines · In message<[email protected]> you wrote:

Quoted 17 lines

For example, instead of applying a patch directly from my mailbox I use this file only to get the hash value for the PW entry, and then use pwclient to apply it and to update it's state:

HASH=$(pwparser.py --hash<$PATCH)
if [ -z "$HASH" ]
then
echo "ERROR: $PATCH - no such entry in PatchWork">&2 exit 1
fi

if pwclient apply -h $HASH
then
pwclient update -s 'Accepted' -h $HASH
fi

Quoted 5 lines

Quoted 2 lines

This is extremely convenient as it automatically takes care of both the Acks and the state change.

Thanks. Looks like this could be handy for many others. Maybe it should be 'backported' into pwclient itself as a new command.

Note that there is a caveat of this approach. Or call it bug in PW. We frequently see patch series, which get posted in several versions. Quite often not all patches are changed, so it happens that for example patch 3/8 is the same in all versions 2, 3 and 4 of the patch series. That means all versions of this patch have the same hash. Usually I apply the latest version, and want to change the status of this one, but the 'pwclient -h $HASH' will always reference the _oldest_ version of the patch. So watchout when you see it changing the state from "superseded" to "applied" - usually this means it changed the wrong version. [I reported this on the PW ML, but no response yet.]

I should mention that I have hacked pwclient a bit to use "git am" instead of plain "patch", and to report the status change:

(quoted diff) +2 -2

@@ -110,7 +110,7 @@
                          (os.path.basename(sys.argv[0])))
      sys.stderr.write("Where<action>  is one of:\n")
      sys.stderr.write(
-"""        apply<ID>     : Apply a patch (in the current dir, using -p1)
+"""        apply<ID>     : Apply a patch (using 'git-am -3 -u --whitespace=strip')
          get<ID>       : Download a patch and save it locally
          projects      : List all projects
          states        : Show list of potential patch states

@@ -260,7 +260,7 @@
      print "Description: %s" % patch['name']
      s = rpc.patch_get_mbox(patch_id)
      if len(s)>  0:
-        proc = subprocess.Popen(['patch', '-p1'], stdin = subprocess.PIPE)
+        proc = subprocess.Popen(['git', 'am', '-3', '-u', '--whitespace=strip'], stdin = subprocess.PIPE)
          proc.communicate(s)
      else:
          sys.stderr.write("Error: No patch content found\n")

Best regards,

Wolfgang Denk

There is no need to use get_timer() and reset_timer() and there are build breakages occuring because of them (specifically cfi_flash). Remove any usage outside arch/ to fix build breakages and to prepare for complete removal

Signed-off-by Graeme Russ <[email protected]>

Quoted 1 line · Graeme Russ

There is no need to use get_timer() and reset_timer() and there are build

shouldn't the above say set_timer() instead of get_timer()?

There is no need to use set_timer() and reset_timer() and there are build breakages occuring because of them (specifically cfi_flash). Remove any usage outside arch/ to fix build breakages and to prepare for complete removal

The Nios2 timer implentation updates every 10ms (increases in 10ms steps) and required a reset_timer() call before timing operations. For Nios2, call reset_timer() when get_timer() is called with a parameter value of 0

Signed-off-by Graeme Russ <[email protected]>

---
checkpatch complains about long lines and brace usage in the board specific flash.c files - They are deprecated and not worth fixing for style

Changes since V1:
- Fix typo in commit message
- Add reset_timer() to Nios2 get_timer(0)

---
 arch/nios2/cpu/interrupts.c    |    4 ++++
 board/BuS/EB+MCF-EV123/flash.c |   10 ++++++----
 board/cobra5272/flash.c        |   10 ++++++----
 board/idmr/flash.c             |   10 ++++++----
 drivers/block/mg_disk.c        |    1 -
 drivers/mtd/cfi_flash.c        |    2 --
 6 files changed, 22 insertions(+), 15 deletions(-)

There is no need to use set_timer() and reset_timer() and there are build breakages occuring because of them (specifically cfi_flash). Remove any usage outside arch/ to fix build breakages and to prepare for complete removal

The Nios2 timer implentation updates every 10ms (increases in 10ms steps) and required a reset_timer() call before timing operations. For Nios2, call reset_timer() when get_timer() is called with a parameter value of 0