From owner-freebsd-amd64@FreeBSD.ORG Fri Jun 10 10:42:45 2005 Return-Path: X-Original-To: amd64@freebsd.org Delivered-To: freebsd-amd64@FreeBSD.ORG Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 7315E16A41C; Fri, 10 Jun 2005 10:42:45 +0000 (GMT) (envelope-from bde@zeta.org.au) Received: from mailout2.pacific.net.au (mailout2.pacific.net.au [61.8.0.85]) by mx1.FreeBSD.org (Postfix) with ESMTP id 9534743D6D; Fri, 10 Jun 2005 10:42:40 +0000 (GMT) (envelope-from bde@zeta.org.au) Received: from mailproxy2.pacific.net.au (mailproxy2.pacific.net.au [61.8.0.87]) by mailout2.pacific.net.au (8.13.4/8.13.4/Debian-1) with ESMTP id j5AAgcdX013771; Fri, 10 Jun 2005 20:42:38 +1000 Received: from katana.zip.com.au (katana.zip.com.au [61.8.7.246]) by mailproxy2.pacific.net.au (8.13.4/8.13.4/Debian-1) with ESMTP id j5AAgZx6018991; Fri, 10 Jun 2005 20:42:36 +1000 Date: Fri, 10 Jun 2005 20:42:37 +1000 (EST) From: Bruce Evans X-X-Sender: bde@delplex.bde.org To: Ruslan Ermilov In-Reply-To: <20050609151212.GB31367@ip.net.ua> Message-ID: <20050610192451.F25104@delplex.bde.org> References: <20050603212555.GB36509@ip.net.ua> <20050605022447.GB26993@dragon.NUXI.org> <20050607072232.GC30490@ip.net.ua> <63165d15771f07f9778be0426dd4a211@FreeBSD.org> <20050609151212.GB31367@ip.net.ua> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: amd64@freebsd.org Subject: Re: device speaker X-BeenThere: freebsd-amd64@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Porting FreeBSD to the AMD64 platform List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 10 Jun 2005 10:42:45 -0000 On Thu, 9 Jun 2005, Ruslan Ermilov wrote: > On Wed, Jun 08, 2005 at 10:59:22PM -0700, John Baldwin wrote: >> On Jun 7, 2005, at 12:22 AM, Ruslan Ermilov wrote: >>> Fine. I'd like to repo-copy sys/i386/isa/spkr.c (and its header) >>> somewhere to sys/x86/ once the latter is ready. Do you have any >>> estimation when one gets ready? >> >> The correct thing to do is to go ahead and make a sys/x86/x86 to stick >> it in (I'm not sure it really belongs in an 'isa' subdirectory). The >> sys/x86 tree will probably grow as people start slowly moving things >> that are identical between i386 and amd64 over to it. >> > It sure is an ISA device, so I was thinking about sys/x86/isa/ or > sys/x86/dev/spkr/. Which one do you like more? He should like neither. The only x86 dependency of the spkr device is its implementation. Its hardware implementation just needs an 8254 and an 8255 wired up to a speaker in much the same way as the original IBM PC, and its software implementation currently has PIO hard-coded. Some alphas apparently have identical hardware, since alpha/isa/clock.c uses the same code (modulo bugs (*)) as i386/isa/clock.c to beep the speaker. The spkr device should just work on these alphas. (*) Bugs in sysbeep(): i386 version: (1) Broken locking. (2) Broken as designed interface: The "pitch" (frequency) is actually the period in cycles at the frequency of the i8253 on the original IBM PC. Non-broken callers invert the pitch to give a period under the assumption that the i8254 freqency is the same as on the IBM PC's i8253 frequency. This interface is broken as designed, but some callers understand it. sysbeep() should scale the period to match the actual frequency. alpha version: (2a) Gets the broken as designed interface backwards by inverting the "pitch" Diff of the sysbeep() part of alpha/isa/clock.c with i386/isa/clock.c (a full diff didn't produce useful context due to gratuitous differences in the files): % --- 0 Fri Jun 10 09:38:29 2005 % +++ 1 Fri Jun 10 09:38:58 2005 % @@ -2,24 +2,13 @@ % sysbeep(int pitch, int period) % { % - /* % - * XXX: TurboLaser doesn't have an i8254 counter. % - * XXX: A replacement is needed, and another method % - * XXX: of determining this would be nice. % - */ % - if (hwrpb->rpb_type == ST_DEC_21000) { % - return (0); % - } % - % - mtx_lock_spin(&clock_lock); % + int x = splclock(); Here is the start of the broken locking for i386's. splclock() used to work but is now null. clock_lock is acquired later for i386's, but there is nothing except Giant to lock the call to acquire_timer2() (this function requires callers to lock for it and the lock should strictly be at least clock_lock since an access to the shared TIMER_MODE register is made). Locking everything by abusing clock_lock for the PPI would work but isn't done in either version. In practice, the PPI is probably locked by Giant. Locking is similarly broken in spkr.c. % % if (acquire_timer2(TIMER_SQWAVE|TIMER_16BIT)) % if (!beeping) { % /* Something else owns it. */ % - mtx_unlock_spin(&clock_lock); % + splx(x); % return (-1); /* XXX Should be EBUSY, but nobody cares anyway. */ % } % - % - if (pitch) pitch = TIMER_DIV(pitch); % - Here is the inversion of the "pitch" on alpha's. The TIMER_DIV() used to cause a trap for division by zero when users asked for an impossibly high frequency and callers correctly inverted the frequency to give a not so correct period of 0. Then the code was unimproved further by only avoiding the division for the trapping case, complete with formatting and logic style bugs. % + mtx_lock_spin(&clock_lock); % outb(TIMER_CNTR2, pitch); % outb(TIMER_CNTR2, (pitch>>8)); This is a perfectly illogical place to acquire clock_lock (on i386's). We didn't acquire clock_lock for the call to acquire_timer2() which needs some locking, but we acquire it here when no more locking is needed since acquire_timer2() gave us a long-lived exlusive lock. (It happens that exclusivity gives us safe access to TIMER_CNTR2, since although the i8254 has a common control/status register, it has separate counter registers and we have exclusive access to the only register that we access diectly. % @@ -27,8 +16,9 @@ % if (!beeping) { % /* enable counter2 output to speaker */ % - if (pitch) outb(IO_PPI, inb(IO_PPI) | 3); % + outb(IO_PPI, inb(IO_PPI) | 3); This is the final part of the unimproved alpha code to avoid division by zero. Here "pitch" is the period in i8254 cycles and is not subject to further bogus inversions. A period of 0 is non-physical but is no worse than a period of 1 -- even speakers of quality thousands of times better than a PC speaker can't usefully oscillate at > 1MHz. The hardware doesn't seem to care whether we write 0, 1 or any other preposterously small value for the period, so there is little reason to handle the (pitch == 0) case specially (with 2 style bugs). (There is a tiny reason: a "pitch" of 0 can result in 2 ways: the original arg may be 0, or we may convert an very large arg to 0 when we bogusly invert the arg; if the inversion were non-bogus, then we should keep track of the original arg so as to special-case only the case where we didn't invert it.) % beeping = period; % timeout(sysbeepstop, (void *)NULL, period); % } % + splx(x); % return (0); % } Bruce