From owner-freebsd-hackers@FreeBSD.ORG Mon Jan 9 23:38:45 2012 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 9F8321065672 for ; Mon, 9 Jan 2012 23:38:45 +0000 (UTC) (envelope-from freebsd@damnhippie.dyndns.org) Received: from qmta14.emeryville.ca.mail.comcast.net (qmta14.emeryville.ca.mail.comcast.net [76.96.27.212]) by mx1.freebsd.org (Postfix) with ESMTP id 810FE8FC12 for ; Mon, 9 Jan 2012 23:38:45 +0000 (UTC) Received: from omta20.emeryville.ca.mail.comcast.net ([76.96.30.87]) by qmta14.emeryville.ca.mail.comcast.net with comcast id KZmE1i00A1smiN4AEbelKJ; Mon, 09 Jan 2012 23:38:45 +0000 Received: from damnhippie.dyndns.org ([24.8.232.202]) by omta20.emeryville.ca.mail.comcast.net with comcast id Kbej1i01L4NgCEG8gbekDv; Mon, 09 Jan 2012 23:38:44 +0000 Received: from [172.22.42.240] (revolution.hippie.lan [172.22.42.240]) by damnhippie.dyndns.org (8.14.3/8.14.3) with ESMTP id q09Ncg66025723 for ; Mon, 9 Jan 2012 16:38:42 -0700 (MST) (envelope-from freebsd@damnhippie.dyndns.org) From: Ian Lepore To: freebsd-hackers In-Reply-To: <201201051033.53081.jhb@freebsd.org> References: <1325715749.25037.36.camel@revolution.hippie.lan> <201201051033.53081.jhb@freebsd.org> Content-Type: text/plain Date: Mon, 09 Jan 2012 16:38:42 -0700 Message-Id: <1326152322.2199.42.camel@revolution.hippie.lan> Mime-Version: 1.0 X-Mailer: Evolution 2.26.0 FreeBSD GNOME Team Port Content-Transfer-Encoding: 7bit Subject: Re: trouble with atrtc X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 09 Jan 2012 23:38:45 -0000 On Thu, 2012-01-05 at 10:33 -0500, John Baldwin wrote: > On Wednesday, January 04, 2012 5:22:29 pm Ian Lepore wrote: > > [...] > > Because atrtc.c has a long and rich history of modifcations, some of > > them fairly recent, I thought it would be a good idea to toss out my > > ideas for changes here and solicit feedback up front, rather than just > > blindly posting a PR with a patch... > > > > It turns out to be very easy to probe for the latched-read behavior with > > just a few lines of code in atrtc_start(), so I'd propose doing that and > > setting a flag that the in/out code can use to disable the caching of > > the current register number on hardware that needs it. > > > > I'd like to add a new public function, atrtc_nmi_enable(int enable) that > > drivers can use to manipulate the NMI flag safely under clock_lock and > > playing nicely with the register number caching code. > > > > Completely unrelated but nice to have: I'd like to add a tuneable to > > control the use of inb(0x84) ops to insert delays after writing to 0x70 > > and 0x71. Modern hardware doesn't need this, so I think it should > > default to not inserting delays. > > > > I've done all these things in our local 8.2 code base and tested them on > > all the hardware I've got on hand. If these changes sound acceptable > > I'll prepare patches to -current as well. > > These changes all sound good to me. > Here is the patch for -current and 9. I can provide a patch to 8-stable as well; it's essentially the same patch with small context differences. I've tested this using -current on several systems, recent and old hardware, including manually bumping up the quality score for the rtc event timer to force it to get used, and it seems to work without trouble (and of course I've been testing the same patch in 8.2 for a while on a bunch of different hardware). Index: sys/isa/rtc.h =================================================================== RCS file: /local/base/FreeBSD-CVS/src/sys/isa/rtc.h,v retrieving revision 1.16.2.1 diff -u -p -r1.16.2.1 rtc.h --- sys/isa/rtc.h 23 Sep 2011 00:51:37 -0000 1.16.2.1 +++ sys/isa/rtc.h 9 Jan 2012 22:04:12 -0000 @@ -117,6 +117,7 @@ extern int atrtcclock_disable; int rtcin(int reg); void atrtc_restore(void); void writertc(int reg, u_char val); +void atrtc_nmi_enable(int enable); #endif #endif /* _I386_ISA_RTC_H_ */ Index: sys/x86/isa/atrtc.c =================================================================== RCS file: /local/base/FreeBSD-CVS/src/sys/x86/isa/atrtc.c,v retrieving revision 1.13.2.1 diff -u -p -r1.13.2.1 atrtc.c --- sys/x86/isa/atrtc.c 23 Sep 2011 00:51:37 -0000 1.13.2.1 +++ sys/x86/isa/atrtc.c 9 Jan 2012 22:04:12 -0000 @@ -55,28 +55,59 @@ __FBSDID("$FreeBSD: src/sys/x86/isa/atrt #define RTC_LOCK mtx_lock_spin(&clock_lock) #define RTC_UNLOCK mtx_unlock_spin(&clock_lock) +/* atrtcclock_disable is set to 1 by apm_attach() or by hint.atrtc.0.clock=0 */ int atrtcclock_disable = 0; -static int rtc_reg = -1; -static u_char rtc_statusa = RTCSA_DIVIDER | RTCSA_NOPROF; -static u_char rtc_statusb = RTCSB_24HR; +static int use_iodelay = 0; /* set from hint.atrtc.0.use_iodelay */ + +#define RTC_REINDEX_REQUIRED 0xffU +#define NMI_ENABLE_BIT 0x80U + +static u_char nmi_enable; +static u_char rtc_reg = RTC_REINDEX_REQUIRED; +static u_char rtc_statusa = RTCSA_DIVIDER | RTCSA_NOPROF; +static u_char rtc_statusb = RTCSB_24HR; + +/* + * Delay after writing to IO_RTC[+1] registers. Modern hardware doesn't + * require this expensive delay, so it's a tuneable that's disabled by default. + */ +static __inline void +rtc_iodelay(void) +{ + if (use_iodelay) + inb(0x84); +} /* * RTC support routines + * + * Most rtc chipsets let you write a value into the index register and then each + * read of the IO register obtains a new value from the indexed location. Others + * behave as if they latch the indexed value when you write to the index, and + * repeated reads keep returning the same value until you write to the index + * register again. atrtc_start() probes for this behavior and leaves rtc_reg + * set to RTC_REINDEX_REQUIRED if reads keep returning the same value. */ +static __inline void +rtcindex(u_char reg) +{ + if (rtc_reg != reg) { + if (rtc_reg != RTC_REINDEX_REQUIRED) + rtc_reg = reg; + outb(IO_RTC, reg | nmi_enable); + rtc_iodelay(); + } +} + int rtcin(int reg) { u_char val; RTC_LOCK; - if (rtc_reg != reg) { - inb(0x84); - outb(IO_RTC, reg); - rtc_reg = reg; - inb(0x84); - } + rtcindex(reg); val = inb(IO_RTC + 1); RTC_UNLOCK; return (val); @@ -87,14 +118,9 @@ writertc(int reg, u_char val) { RTC_LOCK; - if (rtc_reg != reg) { - inb(0x84); - outb(IO_RTC, reg); - rtc_reg = reg; - inb(0x84); - } + rtcindex(reg); outb(IO_RTC + 1, val); - inb(0x84); + rtc_iodelay(); RTC_UNLOCK; } @@ -104,12 +130,31 @@ readrtc(int port) return(bcd2bin(rtcin(port))); } +/* + * At start, probe read-without-reindex behavior. Reading RTC_INTR clears it; + * read until it has a non-zero value, then read it again without re-writing the + * index register. If 2nd read returns a different value it's safe to cache the + * current index with this chipset; enable by changing rtc_reg to current index. + */ static void atrtc_start(void) { + int status; - writertc(RTC_STATUSA, rtc_statusa); + writertc(RTC_STATUSA, RTCSA_DIVIDER | RTCSA_8192); writertc(RTC_STATUSB, RTCSB_24HR); + + RTC_LOCK; + do { + rtcindex(RTC_INTR); + status = inb(IO_RTC+1); + } while (status == 0); + + if (status != inb(IO_RTC+1)) + rtc_reg = RTC_INTR; + RTC_UNLOCK; + + writertc(RTC_STATUSA, RTCSA_DIVIDER | RTCSA_NOPROF); } static void @@ -139,6 +184,17 @@ atrtc_disable_intr(void) } void +atrtc_nmi_enable(int enable) +{ + /* Must not write to 0x70 without a following read or write of 0x71 on + * some chipsets, so do a read. Also, must access a register other than + * the current rtc_reg to force rtcin to push a change out to 0x70. + */ + nmi_enable = enable ? NMI_ENABLE_BIT : 0x00; + rtcin((rtc_reg == RTC_STATUSA) ? RTC_STATUSB : RTC_STATUSA); +} + +void atrtc_restore(void) { @@ -249,6 +305,7 @@ atrtc_attach(device_t dev) IO_RTC, IO_RTC + 1, 2, RF_ACTIVE); if (sc->port_res == NULL) device_printf(dev, "Warning: Couldn't map I/O.\n"); + resource_int_value("atrtc", 0, "use_iodelay", &use_iodelay); atrtc_start(); clock_register(dev, 1000000); bzero(&sc->et, sizeof(struct eventtimer));