I have a couple of comments on this changeset. First, I think this has the potential to affect alpha, so you should run regressions. (MC146818 is used by alpha for the clock).
An alternative it to just increment the fields and add rollover code. It would a bunch of lines of code, but really easy and obviously correct. The only somewhat complicated code would be rolling over the month, but of course, that's not very difficult. In some ways, I think this would be preferred because the timezone code lead to hidden differences on platforms of stuff like leap seconds. I think we want to say that every day has 86400 seconds. Period. no surprises. I don't think we necessarily care about supporting a rollover at a year, but I don't know what other complicated stuff might exist in the timezone code and I'd rather not have to worry about it. Nate On Thu, Aug 20, 2009 at 9:50 AM, Steve Reinhardt<[email protected]> wrote: > Hi Gabe, > > I'm probably missing something here, but all the mucking about with TZ > caught my attention as something that would be nice to avoid. > Wouldn't it work to keep curTime as a time_t and just increment it on > every tick event, then call setTime(*gmtime(&curTime)) only when > you're about to touch clock_data in readData() or writeData()? > > In fact, you could get rid of the union altogether as a member (but > keep the type) and just create a local instance on each call to > readData() or writeData() that's dynamically populated from the time_t > curTime. (OK, on closer inspection that's not quite so clean since > you do need to store the *_alrm fields separately. So it probably is > easier just to keep the member, but treat all the fields other than > *_alrm as temporaries.) > > I think you're also missing the step of updating curTime if clock_data > is changed in writeData(), independent of whether curTime is a struct > tm or a time_t. > > BTW, I wanted to congratulate you on all the recent x86 progress,,, > this is looking awesome. I take it you're fixing the RTC because > you're now getting far enough to notice that it's not advancing? > > Steve > > On Thu, Aug 20, 2009 at 9:20 AM, Gabe Black<[email protected]> wrote: >> changeset ade9a088bb14 in /z/repo/m5 >> details: http://repo.m5sim.org/m5?cmd=changeset;node=ade9a088bb14 >> description: >> X86: Make the real time clock actually keep track of time. >> >> diffstat: >> >> 2 files changed, 103 insertions(+), 19 deletions(-) >> src/dev/mc146818.cc | 96 >> ++++++++++++++++++++++++++++++++++++++++----------- >> src/dev/mc146818.hh | 26 +++++++++++++ >> >> diffs (184 lines): >> >> diff -r de112a8ac3d8 -r ade9a088bb14 src/dev/mc146818.cc >> --- a/src/dev/mc146818.cc Thu Aug 20 00:42:14 2009 -0700 >> +++ b/src/dev/mc146818.cc Thu Aug 20 00:42:43 2009 -0700 >> @@ -43,28 +43,20 @@ >> >> using namespace std; >> >> -MC146818::MC146818(EventManager *em, const string &n, const struct tm time, >> - bool bcd, Tick frequency) >> - : EventManager(em), _name(n), event(this, frequency) >> +static uint8_t >> +bcdize(uint8_t val) >> { >> - memset(clock_data, 0, sizeof(clock_data)); >> - stat_regA = RTCA_32768HZ | RTCA_1024HZ; >> - stat_regB = RTCB_PRDC_IE | RTCB_24HR; >> - if (!bcd) >> - stat_regB |= RTCB_BIN; >> + uint8_t result; >> + result = val % 10; >> + result += (val / 10) << 4; >> + return result; >> +} >> >> +void >> +MC146818::setTime(const struct tm time) >> +{ >> + curTime = time; >> year = time.tm_year; >> - >> - if (bcd) { >> - // The datasheet says that the year field can be either BCD or >> - // years since 1900. Linux seems to be happy with years since >> - // 1900. >> - year = year % 100; >> - int tens = year / 10; >> - int ones = year % 10; >> - year = (tens << 4) + ones; >> - } >> - >> // Unix is 0-11 for month, data seet says start at 1 >> mon = time.tm_mon + 1; >> mday = time.tm_mday; >> @@ -75,6 +67,30 @@ >> // Datasheet says 1 is sunday >> wday = time.tm_wday + 1; >> >> + if (!(stat_regB & RTCB_BIN)) { >> + // The datasheet says that the year field can be either BCD or >> + // years since 1900. Linux seems to be happy with years since >> + // 1900. >> + year = bcdize(year % 100); >> + mon = bcdize(mon); >> + mday = bcdize(mday); >> + hour = bcdize(hour); >> + min = bcdize(min); >> + sec = bcdize(sec); >> + } >> +} >> + >> +MC146818::MC146818(EventManager *em, const string &n, const struct tm time, >> + bool bcd, Tick frequency) >> + : EventManager(em), _name(n), event(this, frequency), tickEvent(this) >> +{ >> + memset(clock_data, 0, sizeof(clock_data)); >> + stat_regA = RTCA_32768HZ | RTCA_1024HZ; >> + stat_regB = RTCB_PRDC_IE | RTCB_24HR; >> + if (!bcd) >> + stat_regB |= RTCB_BIN; >> + >> + setTime(time); >> DPRINTFN("Real-time clock set to %s", asctime(&time)); >> } >> >> @@ -141,6 +157,34 @@ >> } >> } >> >> +static time_t >> +mkutctime(struct tm *time) >> +{ >> + time_t ret; >> + char *tz; >> + >> + tz = getenv("TZ"); >> + setenv("TZ", "", 1); >> + tzset(); >> + ret = mktime(time); >> + if (tz) >> + setenv("TZ", tz, 1); >> + else >> + unsetenv("TZ"); >> + tzset(); >> + return ret; >> +} >> + >> +void >> +MC146818::tickClock() >> +{ >> + if (stat_regB & RTCB_NO_UPDT) >> + return; >> + time_t calTime = mkutctime(&curTime); >> + calTime++; >> + setTime(*gmtime(&calTime)); >> +} >> + >> void >> MC146818::serialize(const string &base, ostream &os) >> { >> @@ -190,3 +234,17 @@ >> { >> return "RTC interrupt"; >> } >> + >> +void >> +MC146818::RTCTickEvent::process() >> +{ >> + DPRINTF(MC146818, "RTC clock tick\n"); >> + parent->schedule(this, curTick + Clock::Int::s); >> + parent->tickClock(); >> +} >> + >> +const char * >> +MC146818::RTCTickEvent::description() const >> +{ >> + return "RTC clock tick"; >> +} >> diff -r de112a8ac3d8 -r ade9a088bb14 src/dev/mc146818.hh >> --- a/src/dev/mc146818.hh Thu Aug 20 00:42:14 2009 -0700 >> +++ b/src/dev/mc146818.hh Thu Aug 20 00:42:43 2009 -0700 >> @@ -64,6 +64,23 @@ >> virtual const char *description() const; >> }; >> >> + /** Event for RTC periodic interrupt */ >> + struct RTCTickEvent : public Event >> + { >> + MC146818 * parent; >> + >> + RTCTickEvent(MC146818 * _parent) : parent(_parent) >> + { >> + parent->schedule(this, curTick + Clock::Int::s); >> + } >> + >> + /** Event process to occur at interrupt*/ >> + void process(); >> + >> + /** Event description */ >> + const char *description() const; >> + }; >> + >> private: >> std::string _name; >> const std::string &name() const { return _name; } >> @@ -71,6 +88,9 @@ >> /** RTC periodic interrupt event */ >> RTCEvent event; >> >> + /** RTC tick event */ >> + RTCTickEvent tickEvent; >> + >> /** Data for real-time clock function */ >> union { >> uint8_t clock_data[10]; >> @@ -89,6 +109,10 @@ >> }; >> }; >> >> + struct tm curTime; >> + >> + void setTime(const struct tm time); >> + >> /** RTC status register A */ >> uint8_t stat_regA; >> >> @@ -106,6 +130,8 @@ >> /** RTC read data */ >> uint8_t readData(const uint8_t addr); >> >> + void tickClock(); >> + >> /** >> * Serialize this object to the given output stream. >> * @param base The base name of the counter object. >> _______________________________________________ >> m5-dev mailing list >> [email protected] >> http://m5sim.org/mailman/listinfo/m5-dev >> > _______________________________________________ > m5-dev mailing list > [email protected] > http://m5sim.org/mailman/listinfo/m5-dev > > _______________________________________________ m5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/m5-dev
