thr3ads.net - freebsd stable - FreeBSD 4.9-RC3 and the ICH5 ATA controllers [Oct 2003]

If this information is useful, please help other people find it:
Share via:

Dan Strick

2003-Oct-21 00:40 UTC

FreeBSD 4.9-RC3 and the ICH5 ATA controllers

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

John Baldwin

2003-Oct-21 12:44 UTC

head link

FreeBSD 4.9-RC3 and the ICH5 ATA controllers

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/

freebsd stable - Oct 2003 - FreeBSD 4.9-RC3 and the ICH5 ATA controllers

FreeBSD 4.9-RC3 and the ICH5 ATA controllers

FreeBSD 4.9-RC3 and the ICH5 ATA controllers