Hi Marco, On Thu, 03 Jul 2008 20:52:53 +0200 Marco Gerards <[EMAIL PROTECTED]> wrote:
> Hi Colin, > > Colin D Bennett <[EMAIL PROTECTED]> writes: > > > I have implemented high resolution time support (through the > > new grub_get_time_ms() function) using the RDTSC instruction > > available on Pentium and higher x86 CPUs. The TSC value is simply > > a 64-bit block cycle counter that is zeroed at bootup, so > > grub_main() calls grub_time_init(), which is defined by each > > platform. If a platform links to kern/i386/tsc.c, then the > > grub_time_init() function from tsc.c is used, which calibrates the > > TSC rate and absolute zero reference using the RTC. > What if TSC is not available? I updated the changelog entry to indicate that running on a 386 or 486 will fail, since TSC is provided in Pentium+. Do we support running on 386 or 386? Should I check for this? If so, the code will have to change a bit, and be able to select between the generic implementation and the TSC implementation at runtime. I think this would be best done letting the "grub_get_time_ms" implementation be selected by setting a function pointer in grub_machine_init() depending on the machine's capabilities. I would have to significantly re-structure my patch, but it might be necessary (and could lead to more understandable code?). What do you think? >... > > + * kern/i386/pc/init.c (grub_millisleep): Likewise. > > + > > + * kern/ieee1275/init.c (grub_millisleep): Don't define > > + grub_millisleep() -- it just called > > grub_millisleep_generic() but now > > + we just need to link with kern/generic/millisleep.c to use > > the generic > > + implementation. > > No need to mention how it has to be linked. I just assume you made > this change already? Yes. I removed that part of the changelog comment. >... > > + (grub_time_init): Define this required function. Does > > nothing since > > + the generic RTC-based functions are used. > > No need to mention what a function does. Only describe the changes > you make. Other information should go into the file itself as > comments, if it is important enough. Here it is only noise... Ok. I tried to clean this up. > > + * kern/i386/linuxbios/init.c (grub_get_time_ms): > > + Define grub_get_time_ms() to always return 0. This should > > be fixed > > + when RTC functionality is implemented. > > + (grub_time_init): Define this required function as a > > no-op. Inserted > > + a comment to remind us delete this function when proper > > time support > > + is added to i386-linuxbios. > > + > > + * kern/main.c (grub_main): Call grub_time_init() right > > after > > + grub_machine_init(). > > I think this should go into grub_machine_init? Why didn't you just > add it there? I commented on this in a separate message a few minutes ago. > > +grub_uint64_t > > +grub_get_time_ms (void) > > +{ > > + /* FIXME: Delete this function and link to `kern/i386/tsc.c' > > once RTC > > + * is implemented. See also comment below in grub_time_init(). > > */ > > + return 0; > > +} > > Please do not use comments with *'s on each line. Sorry, it was a habit of mine. Fixed. >... > > +void > > +grub_time_init (void) > > +{ > > + /* FIXME: Delete this function and link with `kern/i386/tsc.c' > > once RTC > > + * is implemented. Until then, this dummy function simply > > defines > > + * grub_time_init(), which is called by grub_main() in > > `kern/main.c'. > > + * Without RTC, TSC calibration will hang waiting for an RTC > > edge. */ +} > > + > > Same here. Can you explain what is going on here? Since grub_main() calls grub_time_init(), it must be defined. However, we can't use the TSC implementation of grub_time_init() from kern/i386/tsc.c, since it will not be able to calibrate (it will hang) if the RTC always returns the same value, which the i386-linuxbios kernel does. >... > > +/* Reference for TSC=0. This defines the time since the epoch >in > > + * milliseconds that TSC=0 refers to. */ > > +static grub_uint64_t tsc_boot_time = 0; > > Please do not use a * on each line for comments. No need to set this > to zero explicitly. Ok. > > > +/* TSC rate. TSC ticks per millisecond. > > + * Begin with default (incorrect) value of assuming a 1 GHz > > machine. > > + * grub_tsc_calibrate() must be called to set this properly. */ > > +static grub_uint64_t tsc_ticks_per_ms = 1000000; > > Same as above. > > The value is not correct. Why can't we just set it to 0? I figured that in case calibration was not done, we could at least fake partial functionality by going with an incorrect value. However, I don't initialize tsc_boot_time, then there is no sense in initializing tsc_ticks_per_ms either, since without both of them set to a valid value, grub_get_time_ms() will not work. I just realize I should probably not have chopped up the patch above in my reply in an attempt to make it short. I hope I didn't make it too hard to follow. I'll send a new patch shortly for your comments -- it will include the minor changes you mentioned, but will not address the issue of supporting 386/486 machines (no TSC), as I discussed at the beginning of this message. I await your comments regarding that point. Regards, Colin
signature.asc
Description: PGP signature
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org http://lists.gnu.org/mailman/listinfo/grub-devel