Date: Tue, 21 Oct 2003 15:44:33 -0400 (EDT) From: John Baldwin <jhb@FreeBSD.org> To: Dan Strick <strick@covad.net> Cc: sos@FreeBSD.org Subject: RE: FreeBSD 4.9-RC3 and the ICH5 ATA controllers Message-ID: <XFMail.20031021154433.jhb@FreeBSD.org> In-Reply-To: <200310210738.h9L7ckYQ000811@ice.nodomain>
next in thread | previous in thread | raw e-mail | index | archive | help
On 21-Oct-2003 Dan Strick wrote: > On Sat, 18 Oct 2003, Murray Stokely wrote in freebsd-stable: >> ... >> release candidate (RC3) is now available for i386 >> ... >> The firewire module loading issue has been resolved, but the >> SATA issue has not. John Baldwin has a patch posted at >> http://people.freebsd.org/~jhb/patches/sata.patch >> ... > > I fetched the SATA patch and examined it. It seems to be identical > to the changes I suggested in my Oct 13 post to freebsd-stable (modulo > the order of cases in switch statements) with one minor exception. > > I was wondering if the patch is based on my suggested changes. > (I have received no feedback on my email to freebsd-stable.) > If so, did someone test them or did you just "take my word" for > the correctness of the changes? I did use your suggested change to the interrupt handler. You should have been cc'd on my reply to re@ that said as much. The one exception I noticed is that I didn't and dmastat with 0x7f but or'd it with INTERRUPT like other code in that function does. > Is the patch likely to make it into release 4.9? I have discovered > that on my system with the SATA controller configured for "legacy" > mode and *without* the patch, RC3 fails to configure my CD drive for > DMA even though I set hw.ata.atapi_dma=1 in /boot/loader.conf.local. > > The problem is that when the ICH5 SATA controller is configured in > legacy mode, the ICH5 IDE controller disappears from PCI configuration > space. Then the IDE devices appear to be connected to the SATA controller, > which is an unknown controller if the patch is not installed. Thus some > sort of patch is needed even if the SATA controller is configured in > legacy mode. Yes. > I was also wondering about the minor difference between the patch and > my suggested changes. In function ata_pci_match() in ata-pci.c, the > patch does: > ATA_OUTB(ch->r_bmio, ATA_BMSTAT_PORT, dmastat | ATA_BMSTAT_INTERRUPT); > where my code does: > ATA_OUTB(ch->r_bmio, ATA_BMSTAT_PORT, dmastat & 0x7c); > Setting the ATA_BMSTAT_INTERRUPT bit in dmastat is non functional since > we know from context that the bit is already set. True. It's what other code in that function does though, so I figured Søren had a reason for doing it that way. > The ICH5 SATA Bus Master Status Register, offset ATA_BMSTAT_PORT in the > bus master i/o control block, is described in section 11.2.2 of the Intel > ICH5 datasheet. Bits 1, 2, 7 are R/WC. In order to clear just the > ATA_BMSTAT_INTERRUPT (bit 2) and leave the other bits alone we should > clear the other R/WC bits in the status register value before writing > it back into the register (i.e. write back the value (dmastat & 0x7d)). > Bit 0 happens to be RO for the ICH5 ATA controllers, so (dmastat & 0x7c) > does the same thing, but on reconsidering the code I favor writing the > value (dmastat & 0x7d). Using cpp symbols, that would be: > (dmastat & ~(ATA_BMSTAT_DMA_SIMPLEX|ATA_BMSTAT_ERROR)) > > Just to add to the confusion, different PCI ATA controllers treat bit 7 > differently. The ICH5 SATA controller uses it to say something obscure > about the current/previous DMA operation. The bit is reserved for the > ICH5 IDE/PATA controller. The so far unpublished INCITS T13 ATA/ATAPI > Host Adapters Standard says the bit is normally RO and means two ATA > channels cannot be simultaneously active (hence the cpp macro name > ATA_BMSTAT_DMA_SIMPLEX) but some ATA vendors use it differently. I read > somewhere that PCI-SIG also has a document that expresses an opinion on > this bit, but I don't have access to PCI-SIG standards so I can't check > that out. In any case, the above code would only be executed for the > ICH5 ATA controllers, so only the ICH5 usage really matters here. > > The upshot is that the current version of the patch might clear the > ATA_BMSTAT_DMA_SIMPLEX and ATA_BMSTAT_ERROR error bits inappropriately. > The ATA_BMSTAT_DMA_SIMPLEX bit (which actually means something different > for the ICH5) does not seem to be used by the ATA driver in a way that > would make a difference. The driver might be using the ATA_BMSTAT_ERROR > bit but I don't know if the patch interferes with this usage. It may be > a waste to spend too much effort beating this to death since there may > be a whole new ATA driver in FreeBSD CURRENT. > Have I thoroughly confused you? Sorry about that... Whatever Søren says to use for the OUTB value is fine. I'd like his input though. -- John Baldwin <jhb@FreeBSD.org> <>< http://www.FreeBSD.org/~jhb/ "Power Users Use the Power to Serve!" - http://www.FreeBSD.org/
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?XFMail.20031021154433.jhb>