Skip site navigation (1)Skip section navigation (2)
Date:      Tue, 21 Oct 2003 00:38:46 -0700 (PDT)
From:      Dan Strick <strick@covad.net>
To:        freebsd-stable@freebsd.org
Cc:        jhb@freefall.freebsd.org
Subject:   FreeBSD 4.9-RC3 and the ICH5 ATA controllers
Message-ID:  <200310210738.h9L7ckYQ000811@ice.nodomain>

next in thread | raw e-mail | index | archive | help
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?

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.

(I have not yet figured out how to discover the correct device
configurations when the SATA controller is in legacy mode.  The BIOS
changes the subsystem device ID in the SATA function PCI configuration
space, but I can't find any documentation on this and I don't understand
how one should determine which ATA channel is really SATA and which is
really PATA.  Perhaps the information required for correct device
configuration can be obtained with the ATA IDENTIFY commands.
The problem may be beyond the scope of a quick bug fix for release 4.9.
The patch as you have it now is probably the best fix for the moment.
It may incorrectly report IDE devices as being attached to a SATA
controller, but that is merely a cosmetic error.  At least the devices
are configured more or less correctly.)

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.

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...

Dan Strick
strick@covad.net



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200310210738.h9L7ckYQ000811>