Re: [U-Boot] [PATCH v1 (WIP) 00/16] [Timer]API Rewrite
Wolfgang, Bill This thread was getting a little long so I took the liberty to snip the lot ;) Now, the way I see things we are looking at two entirely different issues - udelay() and the Timer API. Unfortunately, they are somewhat intertwined because: a) Some Architectures/SoCs/Boards etc do not implement udelay() in a manner that is either 'available early' or 'inaccurate' (or in some cases both) and b) There is a not insignificant amount of code that uses a counter/udelay combination to implement timeout detection I think for the moment I would like to concentrate solely on the Timer API and leave udelay out of it. I know some architectures use the existing Timer API for udelay, but if you look at my patch series, I never actually touched the existing architecture timer implementations. To date, the series has mostly been a rename and tidy-up of the Timer API and it's usage. So, I think it will be easier to progress if we forget about udelay for the moment. We can identify where it is broken/being abused as a separate task. I can expand the scope of this patch series to look at those instances where an incrementing loop counter with a udelay in the loop is being used where get_time() and friend(s) should be used instead. So in future, any architecture that can both initialise the timer sub-system 'early' and has a timer resolution of = 1us can use the Timer API for udelay, otherwise, udelay will need an implementation (for that architecture) which is independent of the timer sub-system. As an aside, the x86 code _used_ to have a conditional udelay. If the timer sub-system was not initialised yet, udelay would be implemented as a NOP loop. After timers were available, they were used as they are more accurate. Regards, Graeme ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 (WIP) 00/16] [Timer]API Rewrite
Hi Wolfgang.
On 12/07/11 20:36, Graeme Russ wrote:
As I said, I will have a closer look at the Linux API...
OK, I've had a look at how the Linux API is used - in particular
time_after(). Here is a typical random example (from drivers/spi/ep93xx_spi.c):
timeout = jiffies + msecs_to_jiffies(SPI_TIMEOUT);
while (ep93xx_spi_read_u16(espi, SSPSR) SSPSR_RNE) {
if (time_after(jiffies, timeout)) {
dev_warn(espi-pdev-dev,
timeout while flushing RX FIFO\n);
msg-status = -ETIMEDOUT;
return;
}
ep93xx_spi_read_u16(espi, SSPDR);
}
Now I personally do not like the global 'jiffies' variable for two reasons:
a) It assumes there is some interrupt source updating jiffies which we
cannot guarantee. The discussions of the new API had the background counter
being update by both a background interrupt (if available) or a call to
get_timer()
b) It has no fixed time base
So it looks like the nanosecond clocksource code and the time_after et al
macros are not directly related (as evidenced by time_after being in jiffies.h)
So maybe we should look at something along the lines of:
timeout = get_ms_count() + SPI_TIMEOUT;
while (ep93xx_spi_read_u16(espi, SSPSR) SSPSR_RNE) {
if (time_after(get_ms_count(), timeout)) {
dev_warn(espi-pdev-dev,
timeout while flushing RX FIFO\n);
msg-status = -ETIMEDOUT;
return;
}
ep93xx_spi_read_u16(espi, SSPDR);
}
The get_ms_count() name is up for debate
So this would mean minimal timer related code conversion bringing drivers
from Linux, and a namespace which does not match Linux that will hence
generate obvious compile errors.
If you really wanted to we could
#define jiffies get_ms_count()
#define msecs_to_jiffies(x) x
But I think that might be dangerous
Now we can use the existing get_timer(0) call (as I already did with this
patch series) now and create the underlying architecture intrusive re-write
of the generic 'clocksource' API later.
And then phase 3 would be to revisit udelay
Regards,
Graeme
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 (WIP) 00/16] [Timer]API Rewrite
Dear J. William Campbell,
In message 4e1f8127.8030...@comcast.net you wrote:
I am pretty sure that is long enough for someone to notice. . I would be
interested in seeing an example of such code as you refer to. Could you
point me to one, because it seems to me that the only reason to code
such a delay is that for some reason the user didn't want to keep
looking at the termination condition so often. I think that that
arch/powerpc/cpu/mpc512x/i2c.c:
80 while (timeout-- (status I2C_BB)) {
...
88 udelay (1000);
89 status = mpc_reg_in (regs-msr);
90 }
103 while (timeout-- !(*status I2C_IF)) {
104 udelay (1000);
105 *status = mpc_reg_in (regs-msr);
106 }
arch/powerpc/cpu/mpc512x/pci.c:
87 /* We need to wait at least a 1sec based on PCI specs */
88 for (i = 0; i 1000; i++)
89 udelay(1000);
arch/powerpc/cpu/mpc5xx/spi.c:
258 for (i = 0; i 1000; i++) {
...
265 udelay(1000);
266 }
390 for (tm=0; tm1000; ++tm) {
...
394 udelay (1000);
395 }
arch/powerpc/cpu/mpc5xxx/i2c.c:
102 while (timeout-- (status I2C_BB)) {
...
111 udelay(15);
112 status = mpc_reg_in(regs-msr);
113 }
125 while (timeout-- !(*status I2C_IF)) {
126 udelay(15);
127 *status = mpc_reg_in(regs-msr);
128 }
And just that you don't think this is in a single CPU only:
arch/powerpc/cpu/mpc8260/pci.c:
343 for (i = 0; i 1000; ++i)
...
345 udelay (1000);
Or in board code:
board/altera/common/cfide.c:
29 /* wait 500 ms for power to stabilize */
30 for (i = 0; i 500; i++)
31 udelay(1000);
board/amcc/bamboo/bamboo.c:
1019 for (i=0; i500; i++)
1020 udelay(1000);
or common code:
common/cmd_fdc.c:
213 while((read_fdc_reg(FDC_SRA)0x80)==0) {
214 timeout--;
215 udelay(10);
216 if(timeout==0) /* timeout occured */
217 return FALSE;
218 }
228 while((read_fdc_reg(FDC_MSR)0xC0)!=0xC0) {
229 /* direction out and ready */
230 udelay(10);
231 timeout--;
232 if(timeout==0) /* timeout occured */
233 return -1;
234 }
etc.etc.
375 for(val=0;val255;val++)
376 udelay(500); /* wait some time to start motor */
418 for(i=0;i100;i++)
419 udelay(500); /* wait 500usec for fifo overrun */
600 for(i=0; i255; i++) /* then we wait some time */
601 udelay(500);
common/usb.c:
93 inline void wait_ms(unsigned long ms)
94 {
95 while (ms-- 0)
96 udelay(1000);
97 }
etc. etc. Note this last example which spreads the effect in a not
so nice way.
Note that there are even places where udelay() is part of the protocol
timing implementation - like in the soft-I2C and soft-SPI drivers.
equivalent operation can be produced by a pretty simple re-coding of
the loop. In any case, NIOS does not have the problem at present, so the
suggested new work-around would be no worse than the present situation.
I think it is not correct to state that NIOS does not have the
problem at present - it does, only this is not a blatant problem at
the moment. Which in turn might result from the fact that all
presently supported NIOS boards has any support enabled for I2C or
SPI or USB or MMC or watchdogs or ... you name it.
The first user who adds any of the currently unused (and thus
untested) standard features that work just fine on all other
architectures might run into issue...
It is also true that the hardware timer cannot be used in a
reasonable version of udelay, as most of the desired delays may be very
short relative to the timebase. A calibrated timing loop would be the
best approach to the udelay problem.
Just try using Soft-I2C on a NIOS board and you will immediately
realize how crucial a working version of udelay() is.
In general, that is true. There may be a few cases where a delay of less
than the resolution is essential to make something work. There are
These are not just a few cases. There are tons of it, all over the
place - from protocol implementations like soft-I2C or soft-I2C to
timing requirements for RAM initializations etc. etc. And there are
enough places that mandate that the timing is at least in the
requested oder, and not off by factors of tens or hundrets or worse.
probably lots of other cases where we can easily remove the restriction
on the resolution. We cannot fix the first kind, no matter what we do,
to work on a lower resolution timer. The second kind we can and probably
should fix, because
Re: [U-Boot] [PATCH v1 (WIP) 00/16] [Timer]API Rewrite
On 7/15/2011 12:17 AM, Wolfgang Denk wrote:
Dear J. William Campbell,
In message4e1f8127.8030...@comcast.net you wrote:
I am pretty sure that is long enough for someone to notice. . I would be
interested in seeing an example of such code as you refer to. Could you
point me to one, because it seems to me that the only reason to code
such a delay is that for some reason the user didn't want to keep
looking at the termination condition so often. I think that that
arch/powerpc/cpu/mpc512x/i2c.c:
80 while (timeout-- (status I2C_BB)) {
...
88 udelay (1000);
89 status = mpc_reg_in (regs-msr);
90 }
103 while (timeout-- !(*status I2C_IF)) {
104 udelay (1000);
105 *status = mpc_reg_in (regs-msr);
106 }
arch/powerpc/cpu/mpc512x/pci.c:
87 /* We need to wait at least a 1sec based on PCI specs */
88 for (i = 0; i 1000; i++)
89 udelay(1000);
arch/powerpc/cpu/mpc5xx/spi.c:
258 for (i = 0; i 1000; i++) {
...
265 udelay(1000);
266 }
390 for (tm=0; tm1000; ++tm) {
...
394 udelay (1000);
395 }
arch/powerpc/cpu/mpc5xxx/i2c.c:
102 while (timeout-- (status I2C_BB)) {
...
111 udelay(15);
112 status = mpc_reg_in(regs-msr);
113 }
125 while (timeout-- !(*status I2C_IF)) {
126 udelay(15);
127 *status = mpc_reg_in(regs-msr);
128 }
And just that you don't think this is in a single CPU only:
arch/powerpc/cpu/mpc8260/pci.c:
343 for (i = 0; i 1000; ++i)
...
345 udelay (1000);
Or in board code:
board/altera/common/cfide.c:
29 /* wait 500 ms for power to stabilize */
30 for (i = 0; i 500; i++)
31 udelay(1000);
board/amcc/bamboo/bamboo.c:
1019 for (i=0; i500; i++)
1020 udelay(1000);
or common code:
common/cmd_fdc.c:
213 while((read_fdc_reg(FDC_SRA)0x80)==0) {
214 timeout--;
215 udelay(10);
216 if(timeout==0) /* timeout occured */
217 return FALSE;
218 }
228 while((read_fdc_reg(FDC_MSR)0xC0)!=0xC0) {
229 /* direction out and ready */
230 udelay(10);
231 timeout--;
232 if(timeout==0) /* timeout occured */
233 return -1;
234 }
etc.etc.
375 for(val=0;val255;val++)
376 udelay(500); /* wait some time to start motor */
418 for(i=0;i100;i++)
419 udelay(500); /* wait 500usec for fifo overrun */
600 for(i=0; i255; i++) /* then we wait some time */
601 udelay(500);
common/usb.c:
93 inline void wait_ms(unsigned long ms)
94 {
95 while (ms-- 0)
96 udelay(1000);
97 }
etc. etc. Note this last example which spreads the effect in a not
so nice way.
Note that there are even places where udelay() is part of the protocol
timing implementation - like in the soft-I2C and soft-SPI drivers.
Hi All,
Thanks for the pointers Wolfgang. From your examples, it seems we
are talking exclusively about udelay. As I said in my previous post, on
NIOS2, udelay needs to be mechanized on NIOS2 by a timed loop, not by
using the same mechanism as the timer, as there is no way one can
approximate microsecond resolution by the hardware timer. It is my
opinion that there are many CPUs on which the udelay implementation must
not be/should not be based on the same timer code as the get_time()
operation, because in many cases the get_time() operation requires
interrupts to be functional. As you have also pointed out, udelay needs
to be available early, before interrupts are available. Therefore, the
udelay and get_timer must use the hardware at least somewhat
differently. The must also not interfere with each other, which on some
existing boards they did.
If the I2C protocol must be available before interrupts are
available, then udelay must be used. In the above examples, there are
some loops in i2c and spi that appear to be waiting a full second. I
assume they are using udelay because the get_timer feature is not yet
available to them. I also assume that the example in common/usb.c uses
udelay to wait for millisecond sized values for the same reason, i.e.
that it may run before get_time() is available. However, if you examine
the timer code on of the existing CPUs, you will find that udelay is NOT
available early ( before interrupts are enabled) on quite a few of them.
Therefore, none of the above code would work on these CPUs at present
either. These CPUs must not have I2C or spi busses on them that are
Re: [U-Boot] [PATCH v1 (WIP) 00/16] [Timer]API Rewrite
Dear J. William Campbell, In message 4e208227.6010...@comcast.net you wrote: If the I2C protocol must be available before interrupts are available, then udelay must be used. In the above examples, there are some loops in i2c and spi that appear to be waiting a full second. I assume they are using udelay because the get_timer feature is not yet available to them. I also assume that the example in common/usb.c uses No, this is usually not the case. This long delay is the error case, which most probably will never happen. For the normal case, you want a tight spinning loop that introduces as little additional delay as possible. True, although I expect you will find the statement on all the other architectures to be false. Many other architectures, yes, all, no. These other architectures just don't have spi or I2C yet, or if they do, they don't use it early. Are you aware of another architecture that cannot provide sub-microsecond timer resolution? Which is it? 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: w...@denx.de Make it right before you make it faster. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 (WIP) 00/16] [Timer]API Rewrite
On 7/15/2011 11:34 AM, Wolfgang Denk wrote: Dear J. William Campbell, In message4e208227.6010...@comcast.net you wrote: If the I2C protocol must be available before interrupts are available, then udelay must be used. In the above examples, there are some loops in i2c and spi that appear to be waiting a full second. I assume they are using udelay because the get_timer feature is not yet available to them. I also assume that the example in common/usb.c uses No, this is usually not the case. This long delay is the error case, which most probably will never happen. For the normal case, you want a tight spinning loop that introduces as little additional delay as possible. Hi All, I understand why one would want to use udelay in those cases where there are other conditions that should cause the loop to exit. However, in the 16 examples you cited, 7 of those usages are for delays of multiple milliseconds with no tests or escape conditions included. I therefore pointed out that I assume that it was being done that way (as opposed to using get_timer) because this code would be executed early, before get_timer was available. True, although I expect you will find the statement on all the other architectures to be false. Many other architectures, yes, all, no. These other architectures just don't have spi or I2C yet, or if they do, they don't use it early. Are you aware of another architecture that cannot provide sub-microsecond timer resolution? Which is it? No, but I am aware of some that do not provide udelay at all until after interrupts are enabled and the get_timer interface is available. Several ARM implementations have this property. There are one or two others (SPARC) that are similar as I recall. There were also some that called reset_timer, which would mess up nested timing loops, but this has already been fixed. There also quite a few udelay implementations that do not actually provide 1 µs resolution. For instance, the first thing that happens in arm720t/interrupts.c __udelay is that the input microseconds is divided by 1000 and the remainder discarded. A udelay(10) won't work so well here. The lh7a40x/timer.c uses a 508 kHz clock, so delays are accurate to at best 2 µs. There are other examples. For instance, I am pretty sure some u-boot based CPUs use a 32kHz clock as the timebase for udelay. The resolution in that case is 30 µs, which would be a problem for your example code. I am sure further digging will reveal other resolution issues. These udelay issues would need fixing if all u-boot options are to work out of the box. In some cases of low resolution, the fix is not easy, or maybe not even possible. However, this problem has not prevented the use of u-boot on these CPUs. It does mean that some things do not work out of the box, and these non-compliant udelays should be fixed wherever practical. In any case, this shouldn't stop progress on get_timer and the delay_at_least (or whatever it ends up being called) function being adopted. That is independent of the udelay issues. udelay must be implemented in a compliant manner, which may or may not use the same hardware as get_timer() does. It certainly can't use the same code (although some ARM version in effect do, resulting in a non-compliant, low resolution udelay). It might be interesting to utilize something like jiffies for the loop delays that are looking for error exits. In those cases, the error case should be infrequent. It may not matter that the total delay is quite a bit longer than the user requested, as long as it isn't vastly shorter. If a 10 µsec delay became a 30 µsec delay inside the loop examples where we expect to exit due to another cause, it probably wouldn't matter much. It would matter a lot though if the delay were setting a baud rate on a bit-banged output, so jiffies may not be worth it. I put it out there for consideration, as it is easier to produce than a udelay that has a guaranteed low absolute error. Best Regards, Bill Campbell Best regards, Wolfgang Denk ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 (WIP) 00/16] [Timer]API Rewrite
Dear Graeme Russ,
In message 4e1ce6ec.1030...@gmail.com you wrote:
Also remember that if we are looking to inherit nanosecond time base from
Linux, it will be extremely unlikely that every board will support a native
1ns clock source. Typical examples would be 33MHz (~30ns) or 66MHz (~15ns).
In any case, there will be a lot of variance between boards, so we will
still need to cater for arbitrary resolutions
Please note that Linux makes no assumption of a 1 ns clock source. We
should hav eno problems on this front.
It's just macros. And we don't need to use them all. Actually
time_after() is all that's needed to satisfy our current usage.
Oh please, macro, inline function, function - They are all API 'functions'
- As you stated before, the bigger the API, the more people will abuse is
by using the wrong functions. You stated that you want to keep the public
profile of the API as small and tight as possible by rejecting the
additional functions previously proposed.
OK, so let's start with time_after() only.
Agreed - As said before (for the time being) the return value of any
arbitrary call to time() means nothing. It _may_ mean the number of
nanoseconds since power-up, but this is by no means guaranteed.
Actually, I do agree will Bill - time() is probably not the best name -
get_current_ns() or similar may be more meaningful (I still have not had a
chance to look at the Linux code)
If you don't want to use time, then let's use get_time() please - this
is close enough to the existing name so everybody familiar with the
code recognizes it immediately.
void udelay(u32 usec)
{
u64 then = time() + (u64)usec * (u64)1000;
while (!time_after(time(), then))
... do something, like trigger watchdogs etc ...
}
Yes, that is how I always imagined udelay()
...except that udelay() is guaranteed to be available very early in
the initialization sequence, long before we have normal timer
services which may - for example - be interrupt based.
For 'elapsed time' it should be sufficient to store the start value of
time() as early as possible in the (architecture specific) code.
I did mean 'elapsed between two code locations' such as profiling the init
functions - That was what API function #3 was all about.
Sounds trivial: store the value of time() and the begin and the end
of the interval and compute the difference?
OK, this is a new philosophy for the mix. I think it is a good one for the
record. But since we will always use time_after(time(), then) why don't we
simplify it to:
u64 start = time();
...
if (time_elapsed_since(start, TIMEOUT)) {
/* handle timeout */
}
This does not fit. time_elapsed_since() suggests it returns the length
of the interval, i. e. a number of tiume units - but it does something
different.
I like the time_after() approach very much because it does exactly
what the name says, and nothing more - so you can use it in a number
of ways - I recognise good old UNIX philosophy here: do one simple
thing, and do it well.
Regarding the we will always use time_after(time() ...) - we might
want to check if we really have to use time() here (and in a number of
other places - or if we can manage to come up with a simple timestamp
variable, similar to what jiffies is in Linux. That would simplify
the code even further. [Maybe #define jiffies time()? ;-) }
Do we? What exactly is the needed resolution of the underlying
hardware timer? So far, it appears sufficient to have it ticking with
1000 Hz or more. Are there really systems that cannot provide that?
The only architecture I remember that seemed prolematic was NIOS - but
then the NIOS documentation suggests that this might actually be
solvable.
Yes, but as stated before, there is a FPGA gate count trade-off which may
not always be possible. Plus, you break existing boards
Understood. Well, how work the Linux timers on these boards, then?
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: w...@denx.de
At the source of every error which is blamed on the computer you will
find at least two human errors, including the error of blaming it on
the computer.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 (WIP) 00/16] [Timer]API Rewrite
Dear J. William Campbell, In message 4e1cf2e0.1030...@comcast.net you wrote: Yes, this is true. However, the time_elapsed_since routine can do this dynamically (i.e. add twice the timer resolution) . I think you had That would IMHO be a very bad idea. We have a number of places where we have to deal with pretty long timeouts (usually because of protocol specifications that require this - often in the order of several seconds), where the normal path is very fast. The typical approach is to break the timeout into a large number of very short loops. Sometimes we use udelay() for this, other places use get_timer(). So assume we have a timeout of 5 seconds, and implement this as 50,000 loops of 100 microseconds. If you silently turn each of these into 20 milliseconds on NIOS, the timeout would become 1,000 seconds instead of 5 - users would return boards as broken and report it just freezes because nobody expects that it will wake up again after some 16 minutes. another function name (at_least) involved, but you can define time_elapsed_since as always compensating for the resolution. That will fix any resolution questions in a processor-specific way. It is either that or the ifdefs. One way or another, the resolution must be addressed. Up to now, the implicit resolution has been 1 ms, but we now know that is not general enough. It's not as simple as this. You have to change a lot of code to make this work for such slow clock systems. 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: w...@denx.de Sometimes a man will tell his bartender things he'll never tell his doctor. -- Dr. Phillip Boyce, The Menagerie (The Cage), stardate unknown. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 (WIP) 00/16] [Timer]API Rewrite
On 7/14/2011 12:41 PM, Wolfgang Denk wrote: Dear J. William Campbell, In message4e1cf2e0.1030...@comcast.net you wrote: Yes, this is true. However, the time_elapsed_since routine can do this dynamically (i.e. add twice the timer resolution) . I think you had That would IMHO be a very bad idea. We have a number of places where we have to deal with pretty long timeouts (usually because of protocol specifications that require this - often in the order of several seconds), where the normal path is very fast. The typical approach is to break the timeout into a large number of very short loops. Sometimes we use udelay() for this, other places use get_timer(). So assume we have a timeout of 5 seconds, and implement this as 50,000 loops of 100 microseconds. If you silently turn each of these into 20 milliseconds on NIOS, the timeout would become 1,000 seconds instead of 5 - users would return boards as broken and report it just freezes because nobody expects that it will wake up again after some 16 minutes. Hi All, If such a condition existed, that is indeed what would happen. However, at present, such code is not being used on NIOS. We know this because the current work-around of resetting the timer at the start of any timeout operation extends the timeout to a minimum of 10 milliseconds. So we would be waiting 8 minutes currently, not 16, and I am pretty sure that is long enough for someone to notice. . I would be interested in seeing an example of such code as you refer to. Could you point me to one, because it seems to me that the only reason to code such a delay is that for some reason the user didn't want to keep looking at the termination condition so often. I think that that equivalent operation can be produced by a pretty simple re-coding of the loop. In any case, NIOS does not have the problem at present, so the suggested new work-around would be no worse than the present situation. It is also true that the hardware timer cannot be used in a reasonable version of udelay, as most of the desired delays may be very short relative to the timebase. A calibrated timing loop would be the best approach to the udelay problem. another function name (at_least) involved, but you can define time_elapsed_since as always compensating for the resolution. That will fix any resolution questions in a processor-specific way. It is either that or the ifdefs. One way or another, the resolution must be addressed. Up to now, the implicit resolution has been 1 ms, but we now know that is not general enough. It's not as simple as this. You have to change a lot of code to make this work for such slow clock systems. In general, that is true. There may be a few cases where a delay of less than the resolution is essential to make something work. There are probably lots of other cases where we can easily remove the restriction on the resolution. We cannot fix the first kind, no matter what we do, to work on a lower resolution timer. The second kind we can and probably should fix, because they are coded in an overly-restrictive manner. In any case, we don't absolutely have to fix them until somebody decides to use the code on a CPU with a low resolution timer. Practically speaking, the suggested solution will therefore work on all extant cases. Best Regards, Bill Campbell Best regards, Wolfgang Denk ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 (WIP) 00/16] [Timer]API Rewrite
Dear Graeme, I'm trying to summarize your last 3 postings here. In message 4e1b7e0c.8000...@gmail.com you wrote: First, I would very much like to get rid of this _ms thing. We should rather make very clear in the documentation which unit the time services are based on, and use this consequently. Only functions using a different unit should make this clean in their names. Ideally, I think this should be microseconds (a lot happens in a microsecond on modern platforms, and a millisecond can be an eternity) Then we might as well follow the example of the Linux generic TOD framework and use nanoseconds instead - we will need u64 data types anyway when going for microseconds, so we can just follow their example. Third: all this time_*_max(), ..._min() and ...raw() stuff. Aren't we over-engineering here? We have been successfully writing U-Boot code for 11 years now, and never needed these before. Neither does Linux Part of the problem came from the realisation that some architectures have really bad timer resolution (Nios2 in particular). In an ideal world, I think a uniform microsecond timer (millisecond is to coarse to cater for all needs such as udelay and profiling) would be sufficient to not need these functions but while ever we have platforms with 'poor' timers, I think you may find these functions have a place. No, I disagree here. We should not clutter up generic code with things that are only needed for a single (unusually restricted) architecture. Instead, we should try to come up with a design that hides such details in architecture specific code. As stated - This started from a suggestion made by yourself (albeit one you did not think was apparently necessary). The idea of the separate functions (rather than a parameter to a single function) was that if the functions were not used, they would be optimised out. You know how things go: if you offer the users a set of 20 functions so they can chose the one or two that fit their purposes best, they will end up with using all 20 of them, especially when diffferent parts of the code get maintained by different people with different preferences. Approaches like that always get out of hand quickly. We don't have any such stuff now, and U-Boot is working fine. Let's keep it with that, or make it even more simple. But not the other way. What would you like me to do with the clean-up patches that you have already ack'd which do not relate directly to the new API? Should I mark the current series as Rejected in patchwork and create a brand new smaller series which only has those specific patches? I suggest you create a new patch series from the independent clean-up patches and submit this as normal patches (i. e. without the WIP / RFC part). This would already be a great improvement, I think. Thanks a lot for the efforts! In message 4e1b88ee.9040...@gmail.com you wrote: - Looking at the low-level framework described in ols2006-hrtimers.pdf (Linux API), we are looking at implementing the same thing - An architecture specific free running, high speed, high resolution, high accuracy hardware counter/timer and a low speed interrupt which translates the hardware counter/timer to a common API. In this respect, we are not re-inventing that wheel at all Indeed. We should also avoid re-inventing the algorithms, and eventually re-implementing the code. In any case, I think it would be great to use a compatible API so we don;t have to change too many things when adapting code from Linux etc. - The rest of the Linux API is like hitting a thumb-tack with a sledgehammer - Timer Queues, NTP, Callbacks, Scheduling etc, etc, etc. We only want to do two things: Right. Linux is a full-blown OS with a lot of needs we don't have in U-Boot. 1. Monitor timeouts 2. Calculate code execution time (and even then, not everyone all the time) Actrually we need just timestamps. All the rest (like delays, timeouts, executin times etc.) can be derived from that. - The Linux API is nanosecond resolution. We are hard pressed to get the average hardware to support microsecond resolution On the other hand, it makes little difference to the code wether we use microseconds or nanoseconds. It's just slightly different values in the u64 variables. - The Linux API includes usage of 'clock events' - These are timer interrupts driven by programmable hardware and relieve the stress on the timer scheduler keeping track of every single timer - Overkill for a boot loader Agreed. Well, partially. We should still follow the example of keeping the generic code clean of architecture / timer specific code. This should be implemented in the respective arechitecture specific code. - The Linux API includes 'Time of Day' (Wall Time) - Again, I don't think we really need this in a boot loader (we have an RTC API we can use if we want to get the current Date and Time). We could also say this is all we
Re: [U-Boot] [PATCH v1 (WIP) 00/16] [Timer]API Rewrite
Hi Wolfgang, Thanks for the renewed feedback On 12/07/11 18:49, Wolfgang Denk wrote: Dear Graeme, I'm trying to summarize your last 3 postings here. In message 4e1b7e0c.8000...@gmail.com you wrote: First, I would very much like to get rid of this _ms thing. We should rather make very clear in the documentation which unit the time services are based on, and use this consequently. Only functions using a different unit should make this clean in their names. Ideally, I think this should be microseconds (a lot happens in a microsecond on modern platforms, and a millisecond can be an eternity) Then we might as well follow the example of the Linux generic TOD framework and use nanoseconds instead - we will need u64 data types anyway when going for microseconds, so we can just follow their example. Yes, I like the idea of using nanoseconds as a raw timebase Third: all this time_*_max(), ..._min() and ...raw() stuff. Aren't we over-engineering here? We have been successfully writing U-Boot code for 11 years now, and never needed these before. Neither does Linux Part of the problem came from the realisation that some architectures have really bad timer resolution (Nios2 in particular). In an ideal world, I think a uniform microsecond timer (millisecond is to coarse to cater for all needs such as udelay and profiling) would be sufficient to not need these functions but while ever we have platforms with 'poor' timers, I think you may find these functions have a place. No, I disagree here. We should not clutter up generic code with things that are only needed for a single (unusually restricted) architecture. Instead, we should try to come up with a design that hides such details in architecture specific code. So how do we deal with Nios2? It is what caused such a deep investigation into the timer API. We have three choices I can think of off the top of my head: 1. Move the whole timer API up to the architecture level and replicate code everywhere 2. Make the whole timer API weak 3. Fundamentally allow the timer API to handle arbitrary timer resolutions 1. Way ugly. We already have this, and that is why we are here today 2. Well, you know what will happen - Someone will be unhappy with the generic API and rewrite a completely different one (timer_masked anyone!) 3. Why is this so evil? I'm open to other options if you have any As stated - This started from a suggestion made by yourself (albeit one you did not think was apparently necessary). The idea of the separate functions (rather than a parameter to a single function) was that if the functions were not used, they would be optimised out. You know how things go: if you offer the users a set of 20 functions so they can chose the one or two that fit their purposes best, they will end up with using all 20 of them, especially when diffferent parts of the code get maintained by different people with different preferences. Approaches like that always get out of hand quickly. We don't have any such stuff now, and U-Boot is working fine. Let's keep it with that, or make it even more simple. But not the other way. Realistically we are looking at the following functionality: 1) Get the current time 2) Report the minimum time elapsed since an arbitrary epoch 3) Report the maximum time elapsed since an arbitrary epoch 4) Delay for an arbitrary period of time 4 is a derivative of 2 - Just loop until at least the required time has elapsed. And you then suggest bringing in no less than 6 functions from Linux What would you like me to do with the clean-up patches that you have already ack'd which do not relate directly to the new API? Should I mark the current series as Rejected in patchwork and create a brand new smaller series which only has those specific patches? I suggest you create a new patch series from the independent clean-up patches and submit this as normal patches (i. e. without the WIP / RFC part). This would already be a great improvement, I think. Thanks a lot for the efforts! Done - I will reject the current series and rebase/repost the patches you have already ack'd and assign them to you in patchwork - I'll leave it up to you to pull them in In message 4e1b88ee.9040...@gmail.com you wrote: - Looking at the low-level framework described in ols2006-hrtimers.pdf (Linux API), we are looking at implementing the same thing - An architecture specific free running, high speed, high resolution, high accuracy hardware counter/timer and a low speed interrupt which translates the hardware counter/timer to a common API. In this respect, we are not re-inventing that wheel at all Indeed. We should also avoid re-inventing the algorithms, and eventually re-implementing the code. In any case, I think it would be great to use a compatible API so we don;t have to change too many things when adapting code from Linux etc. OK, I'll spend some time digging into the Linux
Re: [U-Boot] [PATCH v1 (WIP) 00/16] [Timer]API Rewrite
Dear Graeme Russ, In message 4e1c23b8.6020...@gmail.com you wrote: So how do we deal with Nios2? It is what caused such a deep investigation into the timer API. We have three choices I can think of off the top of my head: 1. Move the whole timer API up to the architecture level and replicate code everywhere 2. Make the whole timer API weak 3. Fundamentally allow the timer API to handle arbitrary timer resolutions 1. Way ugly. We already have this, and that is why we are here today 2. Well, you know what will happen - Someone will be unhappy with the generic API and rewrite a completely different one (timer_masked anyone!) 3. Why is this so evil? The big disadvantage of 3) is that you cannot make any reasonable assumptions any more in the code. If I place a udelay(10) in some device driver, I am probably aware that we don't have exact times, and that the actual delay may be 10 or 12 or eventually even 20 micro- seconds. We should make sure that the delay never takes less than 10 us, but we also have to guarantee that it does not take - for example - 10 milliseconds. What exactly is the reason that we cannot have better timer resolutions in NIOS? I'm anything but a NIOS export, but skimming through the Altera Embedded Peripherals IP User Guide, section 28. Interval Timer Core I see that we can have 32-bit and 64-bit counters, Two count modes: count down once and continuous count-down, and the example in section Configuration: Timeout Period on p. 28-3 reads: For example, if the associated system clock has a frequency of 30 ns, and the specified Timeout Period value is 1 µs, the true timeout period will be 1.020 microseconds. Well, if I understand this correctly, we can have a continuously running 64 bit counter with microsecond resolution. There are other sections that describe configurations that might probably be used as well, for example in HAL System Library Support: Timestamp Driver The interval timer core may be used as a timestamp device ... Also, the Nios II Software Developer's Handbook says in Chapter 6: Developing Programs Using the Hardware Abstraction Layer - Using Timer Devices on o. 6-16: The HAL API provides two types of timer device drivers: - System clock driver--Supports alarms, such as you would use in a scheduler. - Timestamp driver--Supports high-resolution time measurement. ... You can obtain the rate at which the timestamp counter increments by calling the function alt_timestamp_freq(). This rate is typically the hardware frequency of the Nios II processor system--usually millions of cycles per second. The timestamp drivers are defined in the alt_timestamp.h header file. High-resolution time measurement? Isn't this what we are looking for? Or am I missing something? Scott, maybe you can comment here? I'm open to other options if you have any At the moment I'm trying to understand if we really have a problem on NIOS2 that cannot be fixed in a way that is compatible with our current plans. 1) Get the current time Agreed. That's time(). 2) Report the minimum time elapsed since an arbitrary epoch 3) Report the maximum time elapsed since an arbitrary epoch I don't understand why we would need this. 4) Delay for an arbitrary period of time 4 is a derivative of 2 - Just loop until at least the required time has elapsed. Right. Both delays and timeouts work like that, the difference being that delays are blocking, i. e. there is no other code running inbetween, and you can just sit in a tight spinning loop. I have not seen any requirement yet for 3. And you then suggest bringing in no less than 6 functions from Linux It's just macros. And we don't need to use them all. Actually time_after() is all that's needed to satisfy our current usage. Done - I will reject the current series and rebase/repost the patches you have already ack'd and assign them to you in patchwork - I'll leave it up to you to pull them in Don't reject them - just mark them as RFC. Provided you have access to an incrementing value which increments at a fixed rate and you know the rate, the rest is architecture independent We also have to deal with decrementing counters, but this is just aan unimportant detail. And it appears that we actually can have this, even on NIOS. We could also say this is all we need. If we have a working high precision TOD timestamp, we can derive all other things we need from that. So you want to look at bringing in the Linux TOD API as well? That means we No, I don't. See my previous comments. And the longer I think about it, the more I think we should just use u64 time(void) as core of this new code. Agreed - As said before (for the time being) the return value of any arbitrary call to time() means nothing. It _may_ mean the number of nanoseconds since
Re: [U-Boot] [PATCH v1 (WIP) 00/16] [Timer]API Rewrite
On 7/12/2011 6:10 AM, Wolfgang Denk wrote: Dear Graeme Russ, snip Do we? What exactly is the needed resolution of the underlying hardware timer? So far, it appears sufficient to have it ticking with 1000 Hz or more. Are there really systems that cannot provide that? The only architecture I remember that seemed prolematic was NIOS - but then the NIOS documentation suggests that this might actually be Hi All, The NIOS2 systems are a FPGA based architecture (soft core). They CAN have all different kinds of timers. However, there are many already out in the world that were built (generated) with a timer that has 10 ms resolution. We can't retro-actively change these. We can declare them not supported in future releases, but we can't fix the resolution problem retroactively. I have two comments regarding this discussion so far. First, I think using the time function name at all is a VERY BAD idea. People will confuse it with the normal c library function that returns the time of day since the epoch. One may say that they should RTFM, but it is lots easier to just use another name and avoid all such confusion. Second, I think forcing all systems to use 64 bit time stamps is not a good idea. The main purpose of the timer API is to provide hardware timeouts. 64 bits is vast overkill for such a purpose. Mandating 64 bit capability for all u-boots is I think counter-productive. A 32 bit timestamp in millisecond resolution plus udelay covers all practical cases. The 1 ms resolution is probably inadequate for performance measurement, but for every u-boot system that is sometimes used for performance measurement, there are 100 (probably even more, like 1000, but I don't really have the figures) systems that are just used to boot Linux or another stand alone program. So we may need a different API for performance measurement that uses higher resolution. On most larger CPUs, there are hardware timers that can already do this with minimal work. On some less-capable CPUs, it may be lots harder, but if you aren't doing performance measurement, you don't need it. So I suggest that mandating a 64 bit API for some of the processors (68K comes to mind) is not really a good idea. It seems counter to the elegant simplicity of the rest of u-boot to stick a 64 bit requirement on timestamps used to determine if the boot delay is expired. Best Regards, J. William Campbell Best regards, Wolfgang Denk ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 (WIP) 00/16] [Timer]API Rewrite
Dear Wolfgang Wolfgang Denk wrote: What exactly is the reason that we cannot have better timer resolutions in NIOS? You _can_ have better timer resolutions in Nios. However, there are legacy systems that implement timer(s) with a fixed period of 10 msec. The use of such implementations is very common. How do I know? Because my customers have such implementations. Why not upgrade? Because most sane engineers are loathe to change their rtl just so they can fix a simple software bug or add a simple custom feature. My obvious preference is to continue to use the latest u-boot in such systems ... without having to create a custom branch for customers with older Nios implementations, where I use the old timer interfaces ... simply because we improved the API. Scott, maybe you can comment here? I have only one comment: Out of pure frustration, I have been avoiding any discussions regarding this timer re-write effort. At the moment I'm trying to understand if we really have a problem on NIOS2 that cannot be fixed in a way that is compatible with our current plans. This is where my frustration begins. I've said this several times before, and I don't know how else to say this ... but I'll give it one more try: This is not a Nios problem. If I can't use a 10 msec timer interrupt to detect a simple timeout condition when I'm programming a flash memory, then the timer API design is garbage. And quite frankly, the enormous amount of discussion in this matter reminds me of an undergraduate science project. We also have to deal with decrementing counters, but this is just aan unimportant detail. And it appears that we actually can have this, even on NIOS. Wow! Even on Nios! Arrggghh! The only architecture I remember that seemed prolematic was NIOS Really? There have been occasional issues that have required a patch here and there. But I consider most of them far from problematic. This is exhausting. As I recall, this entire fuss was born out of an issue with nested calls to get_timer() ... and that begat removing reset_timer ... and that begat ... and so on. And we're still spilling lots of intellectual seed here. And now we have what? A jack-of-all-trades API that can do everything with exacting precision ... other than handling a 10 msec interrupt? Best Regards, --Scott ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 (WIP) 00/16] [Timer]API Rewrite
Dear J. William Campbell, All I have two comments regarding this discussion so far. First, I think using the time function name at all is a VERY BAD idea. People will confuse it with the normal c library function that returns the time of day since the epoch. One may say that they should RTFM, but it is lots easier to just use another name and avoid all such confusion. Second, I think forcing all systems to use 64 bit time stamps is not a good idea. The main purpose of the timer API is to provide hardware timeouts. 64 bits is vast overkill for such a purpose. Mandating 64 bit capability for all u-boots is I think counter-productive. A 32 bit timestamp in millisecond resolution plus udelay covers all practical cases. The 1 ms resolution is probably inadequate for performance measurement, but for every u-boot system that is sometimes used for performance measurement, there are 100 (probably even more, like 1000, but I don't really have the figures) systems that are just used to boot Linux or another stand alone program. So we may need a different API for performance measurement that uses higher resolution. On most larger CPUs, there are hardware timers that can already do this with minimal work. On some less-capable CPUs, it may be lots harder, but if you aren't doing performance measurement, you don't need it. So I suggest that mandating a 64 bit API for some of the processors (68K comes to mind) is not really a good idea. It seems counter to the elegant simplicity of the rest of u-boot to stick a 64 bit requirement on timestamps used to determine if the boot delay is expired. I can only FULLY agree here !!! Lets just keep the current functions udelay(us) and u32 get_timer(), the latter maybe without parameter. Remove all *masked() and *reset() functions and lets change the timeout values used in code that also NIOS uses to be 20ms more than the hardware requires. It does not really matter if a *timeout* of 20ms is blown up to 40ms. Reinhard ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 (WIP) 00/16] [Timer]API Rewrite
On 7/12/2011 8:23 AM, Scott McNutt wrote: Dear Wolfgang Wolfgang Denk wrote: What exactly is the reason that we cannot have better timer resolutions in NIOS? You _can_ have better timer resolutions in Nios. However, there are legacy systems that implement timer(s) with a fixed period of 10 msec. The use of such implementations is very common. How do I know? Because my customers have such implementations. Why not upgrade? Because most sane engineers are loathe to change their rtl just so they can fix a simple software bug or add a simple custom feature. My obvious preference is to continue to use the latest u-boot in such systems ... without having to create a custom branch for customers with older Nios implementations, where I use the old timer interfaces ... simply because we improved the API. Scott, maybe you can comment here? I have only one comment: Out of pure frustration, I have been avoiding any discussions regarding this timer re-write effort. At the moment I'm trying to understand if we really have a problem on NIOS2 that cannot be fixed in a way that is compatible with our current plans. This is where my frustration begins. I've said this several times before, and I don't know how else to say this ... but I'll give it one more try: This is not a Nios problem. If I can't use a 10 msec timer interrupt to detect a simple timeout condition when I'm programming a flash memory, then the timer API design is garbage. Hi All, I agree with the idea that mandating a particular timer resolution is an un-desired limitation. You may need it for some cases, but not for all. Proof of this concept is the number of legacy UNIX systems running on 10 ms timer ticks, or for that matter the number of systems running on line cycle interrupts (16.666 ms in the US). It is also however true that one cannot ignore the timer resolution. That is what produced the problem in the first place. Asking for a delay that is less than the resolution isn't going to work as expected. This problem can be easily fixed by always adding twice the timer resolution to any requested delay, to ensure you waited at least two ticks , or by having a routine that does this for you (like the suggested API does). Right now, there are many places in the u-boot code that wait a certain small number of milliseconds under the assumption that the timer resolution is either 1) not an issue at all, or 2) is 1 ms. Both cases are wrong, the first case more so than the second. I wonder how many delays in the program that say wait 5 ms are aware that you could exit the loop after 4ms plus epsilon every so often, even with 1 ms resolution? Perhaps the tolerance is built into the number 5, perhaps not. However, it is surely not if the timer resolution is 16.666 ms. And quite frankly, the enormous amount of discussion in this matter reminds me of an undergraduate science project. Or any government project. But when you will effect lots of people, you often must kill a few trees to make sure nobody (such as yourself and other NIOS users) somehow gets a bad deal. I am now returning to real work and will be offline for a while. Good Luck All. Best Regards, Bill Campbell We also have to deal with decrementing counters, but this is just aan unimportant detail. And it appears that we actually can have this, even on NIOS. Wow! Even on Nios! Arrggghh! The only architecture I remember that seemed prolematic was NIOS Really? There have been occasional issues that have required a patch here and there. But I consider most of them far from problematic. This is exhausting. As I recall, this entire fuss was born out of an issue with nested calls to get_timer() ... and that begat removing reset_timer ... and that begat ... and so on. And we're still spilling lots of intellectual seed here. And now we have what? A jack-of-all-trades API that can do everything with exacting precision ... other than handling a 10 msec interrupt? Best Regards, --Scott ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 (WIP) 00/16] [Timer]API Rewrite
Hi Wolfgang, On 12/07/11 23:10, Wolfgang Denk wrote: Dear Graeme Russ, In message 4e1c23b8.6020...@gmail.com you wrote: So how do we deal with Nios2? It is what caused such a deep investigation into the timer API. We have three choices I can think of off the top of my head: 1. Move the whole timer API up to the architecture level and replicate code everywhere 2. Make the whole timer API weak 3. Fundamentally allow the timer API to handle arbitrary timer resolutions 1. Way ugly. We already have this, and that is why we are here today 2. Well, you know what will happen - Someone will be unhappy with the generic API and rewrite a completely different one (timer_masked anyone!) 3. Why is this so evil? The big disadvantage of 3) is that you cannot make any reasonable assumptions any more in the code. If I place a udelay(10) in some device driver, I am probably aware that we don't have exact times, and that the actual delay may be 10 or 12 or eventually even 20 micro- seconds. We should make sure that the delay never takes less than 10 us, but we also have to guarantee that it does not take - for example - 10 milliseconds. OK, I will assume you agree with me that #1 and #2 are unacceptable In the case of Nios2 we cannot use the Timer API for udelay since any udelay will blow out to 20ms - Very Bad Indeed. Maybe udelay might need to be defined weak, but I fear this will be the thin end of the wedge... [snip pondering on NIOS] Also remember that if we are looking to inherit nanosecond time base from Linux, it will be extremely unlikely that every board will support a native 1ns clock source. Typical examples would be 33MHz (~30ns) or 66MHz (~15ns). In any case, there will be a lot of variance between boards, so we will still need to cater for arbitrary resolutions 1) Get the current time Agreed. That's time(). 2) Report the minimum time elapsed since an arbitrary epoch 3) Report the maximum time elapsed since an arbitrary epoch I don't understand why we would need this. Profiling - Lets say your hardware counter is ms accurate, if an operation takes 0.5ms then using #2 would give a 50/50 chance of reporting that the elapsed time was 0ms. Using method #3, the 50/50 split would be 1ms/2ms. Now as you low-level hardware clock becomes more accurate, those numbers move down to the real 0.5ms, but will never report that the operation took 0ms. I guess going to nanosecond time base, the need for #3 lessens as the hardware clock gets faster. Also, given that the architecture will need to provide a 'nanoseconds per counter increment', #3 can be easily derived if someone really wants it, but as soon as two people want it, you may as well move it into lib/ as it is a trivial function which is 100% architecture independent. 4) Delay for an arbitrary period of time 4 is a derivative of 2 - Just loop until at least the required time has elapsed. Right. Both delays and timeouts work like that, the difference being that delays are blocking, i. e. there is no other code running inbetween, and you can just sit in a tight spinning loop. I have not seen any requirement yet for 3. See above And you then suggest bringing in no less than 6 functions from Linux It's just macros. And we don't need to use them all. Actually time_after() is all that's needed to satisfy our current usage. Oh please, macro, inline function, function - They are all API 'functions' - As you stated before, the bigger the API, the more people will abuse is by using the wrong functions. You stated that you want to keep the public profile of the API as small and tight as possible by rejecting the additional functions previously proposed. Done - I will reject the current series and rebase/repost the patches you have already ack'd and assign them to you in patchwork - I'll leave it up to you to pull them in Don't reject them - just mark them as RFC. OK Provided you have access to an incrementing value which increments at a fixed rate and you know the rate, the rest is architecture independent We also have to deal with decrementing counters, but this is just aan unimportant detail. And it appears that we actually can have this, even on NIOS. Trivial in the proposed architecture with a #define in the config CONFIG_SYS_HW_COUNTER_DECREMENTS (or similar) We could also say this is all we need. If we have a working high precision TOD timestamp, we can derive all other things we need from that. So you want to look at bringing in the Linux TOD API as well? That means we No, I don't. See my previous comments. And the longer I think about it, the more I think we should just use u64 time(void) as core of this new code. Agreed - As said before (for the time being) the return value of any arbitrary call to time() means nothing. It _may_ mean the number of nanoseconds since power-up, but this is by no means guaranteed. Actually, I do agree will Bill - time() is
Re: [U-Boot] [PATCH v1 (WIP) 00/16] [Timer]API Rewrite
Hi Reinhard, On 13/07/11 02:08, Reinhard Meyer wrote: Dear J. William Campbell, All [snip] Lets just keep the current functions udelay(us) and u32 get_timer(), the latter maybe without parameter. Remove all *masked() and *reset() functions This is happening and has Wolfgang's 100% support. Patches are done, just need to be rebased and resent and lets change the timeout values used in code that also NIOS uses to be 20ms more than the hardware requires. It does not really matter if a *timeout* of 20ms is blown up to 40ms. Can't do that - That would mean winding out common code (CFI, Network, Serial, FPGA etc, etc) timeouts to global worst-case scenario unless you want to add a #define and start littering the code with macro's - Urgh! Regards, Graeme ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 (WIP) 00/16] [Timer]API Rewrite
On 7/12/2011 5:33 PM, Graeme Russ wrote: Hi Reinhard, On 13/07/11 02:08, Reinhard Meyer wrote: Dear J. William Campbell, All [snip] Lets just keep the current functions udelay(us) and u32 get_timer(), the latter maybe without parameter. Remove all *masked() and *reset() functions This is happening and has Wolfgang's 100% support. Patches are done, just need to be rebased and resent and lets change the timeout values used in code that also NIOS uses to be 20ms more than the hardware requires. It does not really matter if a *timeout* of 20ms is blown up to 40ms. Can't do that - That would mean winding out common code (CFI, Network, Serial, FPGA etc, etc) timeouts to global worst-case scenario unless you want to add a #define and start littering the code with macro's - Urgh! Hi All, Yes, this is true. However, the time_elapsed_since routine can do this dynamically (i.e. add twice the timer resolution) . I think you had another function name (at_least) involved, but you can define time_elapsed_since as always compensating for the resolution. That will fix any resolution questions in a processor-specific way. It is either that or the ifdefs. One way or another, the resolution must be addressed. Up to now, the implicit resolution has been 1 ms, but we now know that is not general enough. Best Regards, Bill Campbell Regards, Graeme ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 (WIP) 00/16] [Timer]API Rewrite
Dear Graeme Russ, In message 1309261269-4363-1-git-send-email-graeme.r...@gmail.com you wrote: The following series is a work-in-progress revamp of the timer API. The aim is to create a new userland API consisting of the following functions (along with a few arch level support functions): I have to apologize for not commenting on all of this for such a long time. There are a number of reeasons for this silence - lack of available time being one of them (and probably the most pressing one), but I also needed some time to lean back and think through all of this. One thing I always wanted to do in the previous discussion was to check what other projects (like Linux, barebox, etc.) are doing in this area. I think it is worth reading the Linux Documentation/timers/highres.txt document, especially the sections about John Stultz's Generic Time Of Day (GTOD) framework, please the documents linked there (i. e. the OLS slides [the link in Documentation/timers/highres.txt is stale; use http://www.kernel.org/pub/linux/kernel/people/tglx/hrtimers/ols2006-hrtimers.pdf instead] and the paper We Are Not Getting Any Younger: A New Approach to Time and Timers by J. Stultz et al. in http://www.linuxsymposium.org/2005/linuxsymposium_procv1.pdf p. 219ff). Having this still in mind, I took a look across the fence to what barebox is doing. Guess what? From common/clock.c: * clock.c - generic clocksource implementation * * This file contains the clocksource implementation from the Linux * kernel originally by John Stultz OK. Message received. What I'm asking myself (and now you) is: Should we really re-invent the wheel again? Regarding the function names: u32 time_now_ms(void); u32 time_since_ms(u32 from, u32 to); u32 time_max_since_ms(u32 from, u32 to); I don't like these. They appear wrong to me, and they are not in sync with the wiki page either. First, I would very much like to get rid of this _ms thing. We should rather make very clear in the documentation which unit the time services are based on, and use this consequently. Only functions using a different unit should make this clean in their names. Second, I don't feel well with time_now() - what is this, what does it do? Does it set or get a time? The get_time() suggestion we discussed earlier feels still much better to me (as it clearly says which operation the function performs). Or we could even follow the example of the Unix system call time(2) and just use time(). The signature of time_since(from, to) is broken. For time since it seems logically to expect a single argument only: time_since(when). The wiki page still lists this function as time_delta() which seems way more logical to me [although I have to admit that I don;t understand what the delta_type argument might be. Third: all this time_*_max(), ..._min() and ...raw() stuff. Aren't we over-engineering here? We have been successfully writing U-Boot code for 11 years now, and never needed these before. Neither does Linux need any of those, nor every other project I'm aware of. Come on, let's keep the code small and efficient and do without these bells and whistles. Quote Antoine de Saint-Exupery: Perfection is reached, not when there is no longer anything to add, but when there is no longer anything to take away. This current patch series migrates the users of the existing timer API consisting of get_timer() and reset_timer() to the new API while still retaining the arch specific framework in the background. I feel bad that you already have spent so much work, and only now I come and call everything into question again. Sorry. 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: w...@denx.de How many NASA managers does it take to screw in a lightbulb? That's a known problem... don't worry about it. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 (WIP) 00/16] [Timer]API Rewrite
Hi Wolfgang, On 12/07/11 07:56, Wolfgang Denk wrote: Dear Graeme Russ, In message 1309261269-4363-1-git-send-email-graeme.r...@gmail.com you wrote: The following series is a work-in-progress revamp of the timer API. The aim is to create a new userland API consisting of the following functions (along with a few arch level support functions): I have to apologize for not commenting on all of this for such a long time. There are a number of reeasons for this silence - lack of available time being one of them (and probably the most pressing one), but I also needed some time to lean back and think through all of this. I understand - I also would rather spend more time on this and get it right now rather than going back and fixing it again One thing I always wanted to do in the previous discussion was to check what other projects (like Linux, barebox, etc.) are doing in this area. I think it is worth reading the Linux Documentation/timers/highres.txt document, especially the sections about John Stultz's Generic Time Of Day (GTOD) framework, please the documents linked there (i. e. the OLS slides [the link in Documentation/timers/highres.txt is stale; use http://www.kernel.org/pub/linux/kernel/people/tglx/hrtimers/ols2006-hrtimers.pdf instead] and the paper We Are Not Getting Any Younger: A New Approach to Time and Timers by J. Stultz et al. in http://www.linuxsymposium.org/2005/linuxsymposium_procv1.pdf p. 219ff). Will do Having this still in mind, I took a look across the fence to what barebox is doing. Guess what? From common/clock.c: * clock.c - generic clocksource implementation * * This file contains the clocksource implementation from the Linux * kernel originally by John Stultz OK. Message received. What I'm asking myself (and now you) is: Should we really re-invent the wheel again? Regarding the function names: u32 time_now_ms(void); u32 time_since_ms(u32 from, u32 to); u32 time_max_since_ms(u32 from, u32 to); I don't like these. They appear wrong to me, and they are not in sync with the wiki page either. I know - there was some discussion about these names and they were in a little bit of flux (hence WIP). I was looking to get some community clarity on this first before updating the wiki. In hindsight, I'm glad I didn't update the wiki as it looks like there may be a better approach anyway First, I would very much like to get rid of this _ms thing. We should rather make very clear in the documentation which unit the time services are based on, and use this consequently. Only functions using a different unit should make this clean in their names. Ideally, I think this should be microseconds (a lot happens in a microsecond on modern platforms, and a millisecond can be an eternity) Second, I don't feel well with time_now() - what is this, what does it do? Does it set or get a time? The get_time() suggestion we discussed earlier feels still much better to me (as it clearly says which operation the function performs). Or we could even follow the example of the Unix system call time(2) and just use time(). Well time_ was the namespace for the new API and now is, well, now ;) The signature of time_since(from, to) is broken. For time since it seems logically to expect a single argument only: time_since(when). The wiki page still lists this function as time_delta() which seems way more logical to me [although I have to admit that I don;t understand what the delta_type argument might be. It was a refinement of your suggestion that sometimes we want to know that at least a specific period of time has passed (timeouts) and sometimes we want to know what the maximum amount of time that has passed is (profiling) Third: all this time_*_max(), ..._min() and ...raw() stuff. Aren't we over-engineering here? We have been successfully writing U-Boot code for 11 years now, and never needed these before. Neither does Linux Part of the problem came from the realisation that some architectures have really bad timer resolution (Nios2 in particular). In an ideal world, I think a uniform microsecond timer (millisecond is to coarse to cater for all needs such as udelay and profiling) would be sufficient to not need these functions but while ever we have platforms with 'poor' timers, I think you may find these functions have a place. need any of those, nor every other project I'm aware of. Come on, let's keep the code small and efficient and do without these bells and whistles. Quote Antoine de Saint-Exupery: Perfection is reached, not when there is no longer anything to add, but when there is no longer anything to take away. As stated - This started from a suggestion made by yourself (albeit one you did not think was apparently necessary). The idea of the separate functions (rather than a parameter to a single function) was that if the functions were not used, they would be optimised out. This
Re: [U-Boot] [PATCH v1 (WIP) 00/16] [Timer]API Rewrite
On 12/07/11 07:56, Wolfgang Denk wrote: Dear Graeme Russ, [snip] One thing I always wanted to do in the previous discussion was to check what other projects (like Linux, barebox, etc.) are doing in this area. I think it is worth reading the Linux Documentation/timers/highres.txt document, especially the sections about John Stultz's Generic Time Of Day (GTOD) framework, please the documents linked there (i. e. the OLS slides [the link in Documentation/timers/highres.txt is stale; use http://www.kernel.org/pub/linux/kernel/people/tglx/hrtimers/ols2006-hrtimers.pdf instead] and the paper We Are Not Getting Any Younger: A New Approach to Time and Timers by J. Stultz et al. in http://www.linuxsymposium.org/2005/linuxsymposium_procv1.pdf p. 219ff). Having this still in mind, I took a look across the fence to what barebox is doing. Guess what? From common/clock.c: * clock.c - generic clocksource implementation * * This file contains the clocksource implementation from the Linux * kernel originally by John Stultz OK. Message received. Yes, and barebox is also pulling in slabs of other Linux code such as driver framework etc. I think you will find barebox will ultimately have a very large code-base and binary image because of that. I think that U-Boot and barebox are heading in two different directions (and so they should, what would be the point otherwise) - barebox will become modular but larger, U-Boot will continue to be homogeneous and small. What I'm asking myself (and now you) is: Should we really re-invent the wheel again? OK, I've now had a brief look and I have the following comments: - Looking at the low-level framework described in ols2006-hrtimers.pdf (Linux API), we are looking at implementing the same thing - An architecture specific free running, high speed, high resolution, high accuracy hardware counter/timer and a low speed interrupt which translates the hardware counter/timer to a common API. In this respect, we are not re-inventing that wheel at all - The rest of the Linux API is like hitting a thumb-tack with a sledgehammer - Timer Queues, NTP, Callbacks, Scheduling etc, etc, etc. We only want to do two things: 1. Monitor timeouts 2. Calculate code execution time (and even then, not everyone all the time) - The Linux API is nanosecond resolution. We are hard pressed to get the average hardware to support microsecond resolution - The Linux API includes usage of 'clock events' - These are timer interrupts driven by programmable hardware and relieve the stress on the timer scheduler keeping track of every single timer - Overkill for a boot loader - The Linux API includes 'Time of Day' (Wall Time) - Again, I don't think we really need this in a boot loader (we have an RTC API we can use if we want to get the current Date and Time). So, all we need is a fixed resolution free running timer we can use to do simple (one at a time) timing operations. No callbacks, no scheduling, to 'Wall Time', no 'Clock Events', no NTP etc. The Linux API is 'Too Big' I don't think we are re-inventing the wheel with our underlying concept - Use a hardware counter to handle the accuracy and resolution and use a less frequent interrupt to map the hardware implementation to a common software API. In U-Boot, the interrupt can be as simple as the actual call into the API, but where the hardware timer does not have sufficient resolution (a 32-bit nanosecond timer of 16-bit millisecond timer for example) an additional hardware interrupt will be required. I personally think we are headed in the right direction 'for U-Boot' - small, tight, elegant, and fit-for-purpose Regards, Graeme ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 (WIP) 00/16] [Timer]API Rewrite
On 12/07/11 09:36, Graeme Russ wrote: On 12/07/11 07:56, Wolfgang Denk wrote: Dear Graeme Russ, [snip] Having this still in mind, I took a look across the fence to what barebox is doing. Guess what? From common/clock.c: * clock.c - generic clocksource implementation * * This file contains the clocksource implementation from the Linux * kernel originally by John Stultz OK. Message received. Yes, and barebox is also pulling in slabs of other Linux code such as driver framework etc. I think you will find barebox will ultimately have a very large code-base and binary image because of that. I think that U-Boot and barebox are heading in two different directions (and so they should, what would be the point otherwise) - barebox will become modular but larger, U-Boot will continue to be homogeneous and small. What I'm asking myself (and now you) is: Should we really re-invent the wheel again? OK, I've now had a brief look and I have the following comments: [snip] OK, now I've had a look at the Linux code (\kernel\time\*) and Barebox (http://git.pengutronix.de/?p=barebox.git;a=blob;f=common/clock.c;h=79c06c8ddc1b3f447f7c81bf8bb592458f895ab3;hb=HEAD) I think I can safely say that: a) We do not want to inherit the code from Linux. It's good code clean code, but it is just way too much for what we need. It handles registering and unregistering clock sources, change clock source 'rating' (what ever that is) and relies heavily on quite heavy (for U-Boot) struct's (which may need to be carted around in Global Data) b) Barebox doesn't really inherit that much from Linux anyway I think we have the basics right (and that is the same as Linux). We just need to settle on a few finer points such as: - Raw time interval. Linux uses nanoseconds. A 64-bit nanosecond clock source goes for 580 years so even if the highest resolution clock available to us right now is microsecond, it will not hurt to go with a nanosecond time-base as that will provide us with the greatest flexibility going forward. However, this will lead to a lot of integer divides and/or multiplies to scale to millisecond and microsecond intervals - Function naming - Performing time comparisons for timeouts - Barebox for example has a is_timeout function which takes a start time and a delay (in nanoseconds) an implements ndealy, udealy, and mdelay using is_timeout I think what has been proposed here recently and documented (slightly out-of-date) on the wiki is the way to go. Taking the Linux or Barebox source will be more effort than it's worth for the complexity it brings in. I recall an apt quote 'Good programmers know when to re-use, great programmers know when to re-write' Regards, Graeme ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 (WIP) 00/16] [Timer]API Rewrite
Hi Greame, Le 08/07/2011 02:25, Graeme Russ a écrit : 1) I understand that you would like each individual patch in the series to have the in-reply-to header set to the individual parent patch and not have the whole series in-reply-to the first (00/16) patch? (It will be a bit of a PITA to set in-reply-to for 16 individual patches, but I will do if that is the way you want it) Not sure what you mean here, but if it is make patch i/N of a series have be a reply-to patch i/N of the previous series, I agree that it would be a PITA which is not worth the effort considerong the benefit. (the only benefit I can see is to be able to find back the previous series and e.g. check that comments to patch i/N-1 were taken into account by patch i/N. This can be accomplished more easily by having series N-1 and N in a local branch and doing a git diff between commits i/N and i/N-1) Amicalement, -- Albert. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 (WIP) 00/16] [Timer]API Rewrite
On 28/06/11 21:40, Graeme Russ wrote: The following series is a work-in-progress revamp of the timer API. The aim is to create a new userland API consisting of the following functions (along with a few arch level support functions): [snip] Graeme Russ (16): [Timer]Fix misuse of ARM *timer_masked() functions outside arch/arm [Timer]Remove calls to set_timer outside arch/ [Timer]Remove calls to set_timer in arch/ [Timer]Allow reset_timer() only for Nios2 [Timer]Remove reset_timer() for non-Nios2 arches [Timer]Fix at91rm9200/spi.c timer usage [Timer]Remove reset_timer_masked() [Timer]Create new userland timer API [Timer]Replace get_timer() usage in drivers/block/ [Timer]Replace get_timer() usage in driver/mtd and driver/block [Timer]Remove reset_timer() completely [Timer]Replace get_timer() usage in drivers/ [Timer]Replace get_timer() usage in net/ [Timer]Replace get_timer() usage in common/ [Timer]Replace get_timer() usage in board/ [Timer]Replace get_timer() usage in arch/ OK, I'm back from holidays and there has only been a few minor comments so far. I take that as either a) the work is generally OK as is and there are no major objections or b) everyone is too busy to care ;) - I'll work with option a) and rebase/re-spin this series. Now although the board/ patch was too big for the list, it looks like it hit patchwork OK, so I will leave that as is. Wolfgang, a few quick question for you: 1) I understand that you would like each individual patch in the series to have the in-reply-to header set to the individual parent patch and not have the whole series in-reply-to the first (00/16) patch? (It will be a bit of a PITA to set in-reply-to for 16 individual patches, but I will do if that is the way you want it) 2) I will be changing the name of the series to a) drop the WIP and b) change [Timer] to Timer: as per Mike Frysinger's comment - Does this pose any issue to you provided the in-reply-to remains intact 3) The board/ series is a 'Big Patch' but it made it to Patchwork - Are you happy for me to leave it as-is? As it's in patchwork, can I forget about putting it on the Big Patches wiki? 4) I'm thinking about creating an Timer branch in my x86 repository and pushing the whole series through there - From there I can keep it rebased and simply issue a pull request when it's finished (and testers can just pull it if they want). Does this sound like a good plan? 5) Most importantly - Have you had a chance to look at this series? Is it to your liking? Regards, Graeme ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 (WIP) 00/16] [Timer]API Rewrite
Hi All, [snip] Graeme Russ (16): [Timer]Fix misuse of ARM *timer_masked() functions outside arch/arm [Timer]Remove calls to set_timer outside arch/ [Timer]Remove calls to set_timer in arch/ [Timer]Allow reset_timer() only for Nios2 [Timer]Remove reset_timer() for non-Nios2 arches [Timer]Fix at91rm9200/spi.c timer usage [Timer]Remove reset_timer_masked() [Timer]Create new userland timer API [Timer]Replace get_timer() usage in drivers/block/ [Timer]Replace get_timer() usage in driver/mtd and driver/block [Timer]Remove reset_timer() completely [Timer]Replace get_timer() usage in drivers/ [Timer]Replace get_timer() usage in net/ [Timer]Replace get_timer() usage in common/ [Timer]Replace get_timer() usage in board/ [Timer]Replace get_timer() usage in arch/ Patch 15 ([Timer]Replace get_timer() usage in board/) didn't make it through (too big) - I'll have to split it up (rather that that putting a lone patch on the wiki) Regards, Graeme ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 (WIP) 00/16] [Timer]API Rewrite
for future reference, could we use the foo: style in subjects instead of [foo]. git likes to eat [...] automatically and i find it hard to quickly parse. it's an abomination on my eyes. -[PATCH v1 (WIP) 00/16] [Timer]API Rewrite +[PATCH/WIP 00/16] timer: API Rewrite -mike signature.asc Description: This is a digitally signed message part. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v1 (WIP) 00/16] [Timer]API Rewrite
Hi Mike, On Wed, Jun 29, 2011 at 3:08 PM, Mike Frysinger vap...@gentoo.org wrote: for future reference, could we use the foo: style in subjects instead of [foo]. git likes to eat [...] automatically and i find it hard to quickly parse. it's an abomination on my eyes. -[PATCH v1 (WIP) 00/16] [Timer]API Rewrite +[PATCH/WIP 00/16] timer: API Rewrite -mike Will do Regards, Graeme ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
