| Message ID | 1306151649-25640-1-git-send-email-graeme.russ@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
Hi Graeme,
Graeme Russ wrote:
> 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() ?
Regards,
--Scott
Am 2011-05-23 13:54, schrieb Graeme Russ: > 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 <graeme.russ@gmail.com> > --- > 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 Best regards Jens Scharsig
On 23/05/11 22:19, Scott McNutt wrote: > Hi Graeme, > > Graeme Russ wrote: >> 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 Regards, Graeme
Dear Graeme, Graeme Russ wrote: > On 23/05/11 22:19, Scott McNutt wrote: >> Hi Graeme, >> >> Graeme Russ wrote: >>> 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 Understood. However, removing reset_timer() from cfi_flash.c, will result in early timeouts for certain boards/archs. Your patch removes commit 22d6c8faac4e9fa43232b0cf4da427ec14d72ad3 (Thu Apr 1 2010). I'd rather not break something that has been working for over a year before I know how to make it work again. Fair enough? Regards, --Scott
Dear Jens Scharsig, In message <4DDA53B7.7020804@bus-elektronik.de> you wrote: > Am 2011-05-23 13:54, schrieb Graeme Russ: > > 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 <graeme.russ@gmail.com> > > --- > > 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
Am 2011-05-23 13:54, schrieb Graeme Russ: > 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 <graeme.russ@gmail.com> for EB+MCF-EV123 board Acked-by: Jens Scharsig <esw@bus-elektronik.de>
Le 23/05/2011 15:22, Wolfgang Denk a écrit : > 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. 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. > Thanks. > > Wolfgang Denk Amicalement,
Dear Albert ARIBAUD, In message <4DDA69A1.1070009@aribaud.net> you wrote: > > > 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
Le 23/05/2011 20:44, Wolfgang Denk a écrit : > Dear Albert ARIBAUD, > > In message<4DDA69A1.1070009@aribaud.net> you wrote: >> >>> 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. Thanks. Looks like this could be handy for many others. Maybe it should be 'backported' into pwclient itself as a new command. > Best regards, > > Wolfgang Denk Amicalement,
Dear Albert ARIBAUD, In message <4DDAB341.20006@aribaud.net> you wrote: > > > 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. > > 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: @@ -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") @@ -280,6 +280,8 @@ if state_id == 0: sys.stderr.write("Error: No State found matching %s*\n" % state) sys.exit(1) + old_id = patch['state'] + sys.stderr.write("## commit: %s state: %s ==> %s\n" % (commit, old_id, state)) params['state'] = state_id if commit: Best regards, Wolfgang Denk
On 24/05/11 00:00, Andrew Dyer wrote: > On Mon, May 23, 2011 at 06:54, Graeme Russ <graeme.russ@gmail.com> wrote: >> 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()? > Yes, thanks Regards, Graeme
On 24/05/11 04:29, Scott McNutt wrote: > Hi Bill, > > J. William Campbell wrote: >> On 5/23/2011 6:12 AM, Scott McNutt wrote: >>> Dear Graeme, >>> >>> Graeme Russ wrote: >>>> On 23/05/11 22:19, Scott McNutt wrote: >>>>> Hi Graeme, >>>>> >>>>> Graeme Russ wrote: >>>>>> 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() ? >> 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. > 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
Le 23/05/2011 21:42, Wolfgang Denk a écrit : > Dear Albert ARIBAUD, > > In message<4DDAB341.20006@aribaud.net> you wrote: >> >>> 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. >> >> 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: > > @@ -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") > @@ -280,6 +280,8 @@ > if state_id == 0: > sys.stderr.write("Error: No State found matching %s*\n" % state) > sys.exit(1) > + old_id = patch['state'] > + sys.stderr.write("## commit: %s state: %s ==> %s\n" % (commit, old_id, state)) > params['state'] = state_id > > if commit: I personally only added a "pwclient am" command to do a git am, the same way as "pwclient apply" does a git apply. > Best regards, > > Wolfgang Denk Amicalement,
On 5/23/2011 1:10 PM, Graeme Russ wrote: > On 24/05/11 04:29, Scott McNutt wrote: >> Hi Bill, >> >> J. William Campbell wrote: >>> On 5/23/2011 6:12 AM, Scott McNutt wrote: >>>> Dear Graeme, >>>> >>>> Graeme Russ wrote: >>>>> On 23/05/11 22:19, Scott McNutt wrote: >>>>>> Hi Graeme, >>>>>> >>>>>> Graeme Russ wrote: >>>>>>> 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() ? >>> 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. >> > 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"? Best Regards, Bill Campbell > Regards, > > Graeme > >
On 24/05/11 06:49, J. William Campbell wrote: > On 5/23/2011 1:10 PM, Graeme Russ wrote: >> On 24/05/11 04:29, Scott McNutt wrote: >>> Hi Bill, >>> >>> J. William Campbell wrote: >>>> On 5/23/2011 6:12 AM, Scott McNutt wrote: >>>>> Dear Graeme, >>>>> >>>>> Graeme Russ wrote: >>>>>> On 23/05/11 22:19, Scott McNutt wrote: >>>>>>> Hi Graeme, >>>>>>> >>>>>>> Graeme Russ wrote: >>>>>>>> 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() ? >>>> 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. >>> >> 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 ;) > 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 > 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
On 5/23/2011 2:02 PM, Graeme Russ wrote: > On 24/05/11 06:49, J. William Campbell wrote: >> On 5/23/2011 1:10 PM, Graeme Russ wrote: >>> On 24/05/11 04:29, Scott McNutt wrote: >>>> Hi Bill, >>>> >>>> J. William Campbell wrote: >>>>> On 5/23/2011 6:12 AM, Scott McNutt wrote: >>>>>> Dear Graeme, >>>>>> >>>>>> Graeme Russ wrote: >>>>>>> On 23/05/11 22:19, Scott McNutt wrote: >>>>>>>> Hi Graeme, >>>>>>>> >>>>>>>> Graeme Russ wrote: >>>>>>>>> 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() ? >>>>> 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. >>>> >>> 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 ;) >> 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 > >> 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? Hi All, That would probably be the best way to go for now. It might slow things down a bit though, if these delays are all desired to be "short", like 1 ms. We would expand the 1 ms delay to 15 ms (average) while the current (illegal) solution would expand a 1 ms delay to 10 ms always. It is worth trying I think. It is also true that any other delays in the program will suffer from the 10 ms resolution problem, so your idea is I think a good one. Best Regards, Bill Campbell > Regards, > > Graeme > > >
Dear "J. William Campbell", In message <4DDAC866.1050508@comcast.net> you wrote: > > > 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. Best regards, Wolfgang Denk
Hi Wolfgang, On Tue, May 24, 2011 at 7:53 AM, Wolfgang Denk <wd@denx.de> wrote: > Dear "J. William Campbell", > > In message <4DDAC866.1050508@comcast.net> you wrote: >> >> > 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. If not, sure, we can remove the argument, but that will be a huge patch. If we do, we should change it from get_timer() to get_ms() while we are at it Regards, Graeme
On Monday, May 23, 2011 18:44:33 Graeme Russ wrote: > I think this might break stand alone applications - I am sure get_timer() > is exported. the export list has a rev associated with it. see XF_VERSION in include/exports.h. u-boot will detect the mismatch at runtime and bail forcing people to rebuild their stand alone apps. we've done this in the past (6 times even!) and no one has complained (at least with any visibility on the mailing list). i dont have a problem with it. -mike
On Tue, May 24, 2011 at 7:02 AM, Graeme Russ <graeme.russ@gmail.com> wrote: [snip] > > 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
On 5/23/2011 10:13 PM, Graeme Russ wrote: > On Tue, May 24, 2011 at 7:02 AM, Graeme Russ<graeme.russ@gmail.com> wrote: > > [snip] >> 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 Hi Graeme, No, you were not on drugs. If base != 0 then { if (timer - base < 20) return 0; return timer - base -10} return timer. That will make the subsequent comparison do what you wanted. Differences of less than 20 are unreliable because of quantization, so we return 0. If the difference was > 20, return a conservative number, the difference -10. If the input base was 0, return the timer, the user is just trying to read it. Best Regards Bill Campbell > So I think adding reset_timer() to the NIOS get_timer() when the parameter > is zero is the only option > > Regards, > > Graeme > >
diff --git a/board/BuS/EB+MCF-EV123/flash.c b/board/BuS/EB+MCF-EV123/flash.c index 3c36367..8b7f957 100644 --- a/board/BuS/EB+MCF-EV123/flash.c +++ b/board/BuS/EB+MCF-EV123/flash.c @@ -157,6 +157,7 @@ int amd_flash_erase_sector(flash_info_t * info, int sector) { int state; ulong result; + ulong start; volatile u16 *addr = (volatile u16 *) (info->start[sector]); @@ -171,13 +172,13 @@ int amd_flash_erase_sector(flash_info_t * info, int sector) /* wait until flash is ready */ state = 0; - set_timer (0); + start = get_timer(0); do { result = *addr; /* check timeout */ - if (get_timer (0) > CONFIG_SYS_FLASH_ERASE_TOUT) { + if (get_timer(start) > CONFIG_SYS_FLASH_ERASE_TOUT) { MEM_FLASH_ADDR1 = CMD_READ_ARRAY; state = ERR_TIMOUT; } @@ -267,6 +268,7 @@ volatile static int amd_write_word (flash_info_t * info, ulong dest, u16 data) ulong result; int cflag, iflag; int state; + ulong start; /* * Check if Flash is (sufficiently) erased @@ -295,7 +297,7 @@ volatile static int amd_write_word (flash_info_t * info, ulong dest, u16 data) *addr = data; /* arm simple, non interrupt dependent timer */ - set_timer (0); + start = get_timer(0); /* wait until flash is ready */ state = 0; @@ -303,7 +305,7 @@ volatile static int amd_write_word (flash_info_t * info, ulong dest, u16 data) result = *addr; /* check timeout */ - if (get_timer (0) > CONFIG_SYS_FLASH_ERASE_TOUT) { + if (get_timer(start) > CONFIG_SYS_FLASH_ERASE_TOUT) { state = ERR_TIMOUT; } if (!state && ((result & BIT_RDY_MASK) == (data & BIT_RDY_MASK))) diff --git a/board/cobra5272/flash.c b/board/cobra5272/flash.c index 33c9361..e8f02eb 100644 --- a/board/cobra5272/flash.c +++ b/board/cobra5272/flash.c @@ -147,6 +147,7 @@ int flash_erase (flash_info_t * info, int s_first, int s_last) int iflag, cflag, prot, sect; int rc = ERR_OK; int chip1; + ulong start; /* first look for protection bits */ @@ -190,7 +191,7 @@ int flash_erase (flash_info_t * info, int s_first, int s_last) printf ("Erasing sector %2d ... ", sect); /* arm simple, non interrupt dependent timer */ - set_timer (0); + start = get_timer(0); if (info->protect[sect] == 0) { /* not protected */ volatile u16 *addr = @@ -211,7 +212,7 @@ int flash_erase (flash_info_t * info, int s_first, int s_last) result = *addr; /* check timeout */ - if (get_timer (0) > CONFIG_SYS_FLASH_ERASE_TOUT) { + if (get_timer(start) > CONFIG_SYS_FLASH_ERASE_TOUT) { MEM_FLASH_ADDR1 = CMD_READ_ARRAY; chip1 = TMO; break; @@ -264,6 +265,7 @@ static int write_word (flash_info_t * info, ulong dest, ulong data) int rc = ERR_OK; int cflag, iflag; int chip1; + ulong start; /* * Check if Flash is (sufficiently) erased @@ -291,7 +293,7 @@ static int write_word (flash_info_t * info, ulong dest, ulong data) *addr = data; /* arm simple, non interrupt dependent timer */ - set_timer (0); + start = get_timer(0); /* wait until flash is ready */ chip1 = 0; @@ -299,7 +301,7 @@ static int write_word (flash_info_t * info, ulong dest, ulong data) result = *addr; /* check timeout */ - if (get_timer (0) > CONFIG_SYS_FLASH_ERASE_TOUT) { + if (get_timer(start) > CONFIG_SYS_FLASH_ERASE_TOUT) { chip1 = ERR | TMO; break; } diff --git a/board/idmr/flash.c b/board/idmr/flash.c index 57c9948..9f4ff2b 100644 --- a/board/idmr/flash.c +++ b/board/idmr/flash.c @@ -130,6 +130,7 @@ int flash_erase (flash_info_t * info, int s_first, int s_last) int iflag, prot, sect; int rc = ERR_OK; int chip1; + ulong start; /* first look for protection bits */ @@ -170,7 +171,7 @@ int flash_erase (flash_info_t * info, int s_first, int s_last) printf ("Erasing sector %2d ... ", sect); /* arm simple, non interrupt dependent timer */ - set_timer (0); + start = get_timer(0); if (info->protect[sect] == 0) { /* not protected */ volatile u16 *addr = @@ -191,7 +192,7 @@ int flash_erase (flash_info_t * info, int s_first, int s_last) result = *addr; /* check timeout */ - if (get_timer (0) > CONFIG_SYS_FLASH_ERASE_TOUT * CONFIG_SYS_HZ / 1000) { + if (get_timer(start) > CONFIG_SYS_FLASH_ERASE_TOUT * CONFIG_SYS_HZ / 1000) { MEM_FLASH_ADDR1 = CMD_READ_ARRAY; chip1 = TMO; break; @@ -248,6 +249,7 @@ static int write_word (flash_info_t * info, ulong dest, ulong data) int rc = ERR_OK; int iflag; int chip1; + ulong start; /* * Check if Flash is (sufficiently) erased @@ -272,7 +274,7 @@ static int write_word (flash_info_t * info, ulong dest, ulong data) *addr = data; /* arm simple, non interrupt dependent timer */ - set_timer (0); + start = get_timer(0); /* wait until flash is ready */ chip1 = 0; @@ -280,7 +282,7 @@ static int write_word (flash_info_t * info, ulong dest, ulong data) result = *addr; /* check timeout */ - if (get_timer (0) > CONFIG_SYS_FLASH_ERASE_TOUT * CONFIG_SYS_HZ / 1000) { + if (get_timer(start) > CONFIG_SYS_FLASH_ERASE_TOUT * CONFIG_SYS_HZ / 1000) { chip1 = ERR | TMO; break; } diff --git a/drivers/block/mg_disk.c b/drivers/block/mg_disk.c index b74307a..7609557 100644 --- a/drivers/block/mg_disk.c +++ b/drivers/block/mg_disk.c @@ -91,7 +91,6 @@ static unsigned int mg_wait (u32 expect, u32 msec) u32 from, cur, err; err = MG_ERR_NONE; - reset_timer(); from = get_timer(0); status = readb(mg_base() + MG_REG_STATUS); diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c index 5788328..ea5e1f2 100644 --- a/drivers/mtd/cfi_flash.c +++ b/drivers/mtd/cfi_flash.c @@ -573,7 +573,6 @@ static int flash_status_check (flash_info_t * info, flash_sect_t sector, #endif /* Wait for command completion */ - reset_timer(); start = get_timer (0); while (flash_is_busy (info, sector)) { if (get_timer (start) > tout) { @@ -660,7 +659,6 @@ static int flash_status_poll(flash_info_t *info, void *src, void *dst, #endif /* Wait for command completion */ - reset_timer(); start = get_timer(0); while (1) { switch (info->portwidth) {
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 <graeme.russ@gmail.com> --- 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 ++++++---- board/cobra5272/flash.c | 10 ++++++---- board/idmr/flash.c | 10 ++++++---- drivers/block/mg_disk.c | 1 - drivers/mtd/cfi_flash.c | 2 -- 5 files changed, 18 insertions(+), 15 deletions(-) -- 1.7.5.2.317.g391b14