Re: [coreboot] libpayload: add gettimeofday()

[Uwe Hermann]

On Wed, Apr 16, 2008 at 05:44:45PM -0600, Jordan Crouse wrote:
> libpayload:  Add gettimeofday() and friends
>
> Add a gettimeofday() implementation - it works pretty well, but it
> drifts a little bit so its not very suitable for keeping time.  It
> works best to track changes in time over small periods of time.
>
> Signed-off-by: Jordan Crouse 
> Index: libpayload/drivers/nvram.c
> ===================================================================
> --- libpayload.orig/drivers/nvram.c	2008-04-18 04:16:35.000000000 -0600
> +++ libpayload/drivers/nvram.c	2008-04-18 04:20:20.000000000 -0600
> @@ -93,3 +93,42 @@
>    outb(addr, rtc_port);
>    outb(val, rtc_port + 1);
>  }
>
> +/**
> Return 1 if the NVRAM is currently updating
> */

... "and zero otherwise."

Also, please add a "@return" item for doxygen.
> +int nvram_updating(void)
> +{
>   return (nvram_read(NVRAM_RTC_FREQ_SELECT) & NVRAM_RTC_UIP) ? 1 : 0;
>
> +}
>
> +/**
> Get the current time and date from the RTC
> @param time A pointer to a broken-down time structure
> */
>
> +void rtc_read_clock(struct tm *time)
> +{
> memset(time, 0, sizeof(*time));
> while(nvram_updating());
> time->tm_mon = bcd2dec(nvram_read(NVRAM_RTC_MONTH)) - 1;
> time->tm_sec = bcd2dec(nvram_read(NVRAM_RTC_SECONDS));
> time->tm_min = bcd2dec(nvram_read(NVRAM_RTC_MINUTES));
> time->tm_mday = bcd2dec(nvram_read(NVRAM_RTC_DAY));
> time->tm_hour = bcd2dec(nvram_read(NVRAM_RTC_HOURS));
> /* Instead of finding the century register,
>   we just make an assumption that if the year value is
>   less then 80, then it is 2000+
> */

I'd add a FIXME or TODO here :)
> time->tm_year = bcd2dec(nvram_read(NVRAM_RTC_YEAR));
> if (time->tm_year < 80)
> time->tm_year += 100;
>
> +}
> Index: libpayload/include/arch/rdtsc.h
>
> --- libpayload.orig/include/arch/rdtsc.h	2008-04-18 04:16:34.000000000 -0600
> +++ libpayload/include/arch/rdtsc.h	2008-04-18 04:17:09.000000000 -0600
> @@ -30,9 +30,9 @@
>  #ifndef _ARCH_RDTSC_H
>  #define _ARCH_RDTSC_H
>
> -static inline unsigned long long rdtsc(void)
> +static inline u64 rdtsc(void)
>  {
> unsigned long long val;
> u64 val;

Yep.

Can the inline be removed as well or is it required here?
> +static unsigned int day_of_year(int mon, int day, int year)
> +{
> static u8 mdays[12] = { 31, 28, 31, 30, 31, 30,
> 		31, 31, 30, 31, 30, 31 };
> int ret = 0, i;

Maybe this?

int i, ret = 0;

Looks a bit less confusing. Or only "int ret, i" and put the
"ret = 0" in the for-loop:

for(i = 0, ret = 0; i < mon; i++)
> f(r(i = 0; i < mon; i++) {
> ret += mdays[i];
> if (i == 1 && (year % 4))
> 	ret++;
> }
> return (ret + day);
>
> +}
>
> +static void gtod_init(void)

Nonovious abbreviation, use gettimeofday_init() for better readability
please. Ditto for other "gtod" occurences.

A test on real hardware is pending, I'll report back.

Uwe.