2.6.32: Promise UDMA33 card refuses to work in UDMA mode
I tried upgrading my main machine from an old 2.6.23 kernel to 2.6.32. In doing so, I switched from IDE to ATA support.
However, I find that the Promise UDMA33 card fails to work in DMA mode under PATA on 2.6.32, where as the 2.6.23 IDE driver works fine. Reads on PATA seem fine, it's just writes that seem to be causing grief.
Messages from 2.6.32 kernel (a little edited to remove the interleaving with other kernel messages):
scsi0 : pata_pdc202xx_old
scsi1 : pata_pdc202xx_old
ata1: PATA max UDMA/33 cmd 0x10b0 ctl 0x10c0 bmdma 0x1080 irq 24
ata2: PATA max UDMA/33 cmd 0x10b8 ctl 0x10c4 bmdma 0x1088 irq 24
...
ata1.00: ATA-6: ST340015A, 3.01, max UDMA/100
ata1.00: 78165360 sectors, multi 16: LBA
ata1.00: configured for UDMA/33
scsi 0:0:0:0: Direct-Access ATA ST340015A 3.01 PQ: 0 ANSI: 5
sd 0:0:0:0: [sda] 78165360 512-byte logical blocks: (40.0 GB/37.2 GiB) sd 0:0:0:0: [sda] Write Protect is off
sd 0:0:0:0: [sda] Mode Sense: 00 3a 00 00
sd 0:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO
or FUA
sda: sda1 sda2 sda3 sda4 < sda5 sda6 sda7 sda8 sda9 >
sd 0:0:0:0: [sda] Attached SCSI disk
ata2.00: ATA-6: ST340015A, 3.01, max UDMA/100
ata2.00: 78165360 sectors, multi 16: LBA
ata2.00: configured for UDMA/33
scsi 1:0:0:0: Direct-Access ATA ST340015A 3.01 PQ: 0 ANSI: 5
sd 1:0:0:0: [sdb] 78165360 512-byte logical blocks: (40.0 GB/37.2 GiB) sd 1:0:0:0: [sdb] Write Protect is off
sd 1:0:0:0: [sdb] Mode Sense: 00 3a 00 00
sd 1:0:0:0: [sdb] Write cache: enabled, read cache: enabled, doesn't support DPO
or FUA
sdb: sdb1 sdb2 sdb3 sdb4 < sdb5 sdb6 sdb7 sdb8 sdb9 >
sd 1:0:0:0: [sdb] Attached SCSI disk
...
md1: detected capacity change from 0 to 2147876864 md: considering sdb1 ... md: adding sdb1 ... md: adding sda1 ... md: created md0 md: bind<sda1> md: bind<sdb1> md: running: <sdb1><sda1> raid1: raid set md0 active with 2 out of 2 mirrors md0: detected capacity change from 0 to 367329280 md: ... autorun DONE.
md0: unknown partition table
kjournald starting. Commit interval 5 seconds
EXT3-fs: mounted filesystem with writeback data mode.
VFS: Mounted root (ext3 filesystem) readonly on device 9:0. Freeing init memory: 144K
ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 ata1.00: BMDMA stat 0x24
ata1.00: failed command: WRITE DMA
ata1.00: cmd ca/00:08:bf:f2:0a/00:00:00:00:00/e0 tag 0 dma 4096 out
res 51/84:00:bf:f2:0a/00:00:00:00:00/e0 Emask 0x10 (ATA bus error)
ata1.00: status: { DRDY ERR }
ata1.00: error: { ICRC ABRT }
ata1: soft resetting link
ata1.00: configured for UDMA/33
ata1: EH complete
ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 ata1.00: BMDMA stat 0x24
ata1.00: failed command: WRITE DMA
ata1.00: cmd ca/00:08:bf:f2:0a/00:00:00:00:00/e0 tag 0 dma 4096 out
res 51/84:00:bf:f2:0a/00:00:00:00:00/e0 Emask 0x10 (ATA bus error)
ata1.00: status: { DRDY ERR }
ata1.00: error: { ICRC ABRT }
ata1: soft resetting link
ata1.00: configured for UDMA/33
ata1: EH complete
ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 ata1.00: BMDMA stat 0x24
ata1.00: failed command: WRITE DMA
ata1.00: cmd ca/00:08:bf:f2:0a/00:00:00:00:00/e0 tag 0 dma 4096 out
res 51/84:00:bf:f2:0a/00:00:00:00:00/e0 Emask 0x10 (ATA bus error)
ata1.00: status: { DRDY ERR }
ata1.00: error: { ICRC ABRT }
ata1: soft resetting link
ata1.00: configured for UDMA/33
ata1: EH complete
ata1.00: limiting speed to UDMA/25:PIO4
ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 ata1.00: BMDMA stat 0x24
ata1.00: failed command: WRITE DMA
ata1.00: cmd ca/00:08:bf:f2:0a/00:00:00:00:00/e0 tag 0 dma 4096 out
res 51/84:00:bf:f2:0a/00:00:00:00:00/e0 Emask 0x10 (ATA bus error)
ata1.00: status: { DRDY ERR }
ata1.00: error: { ICRC ABRT }
ata1: soft resetting link
ata1.00: configured for UDMA/25
ata1: EH complete
ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 ata1.00: BMDMA stat 0x24
ata1.00: failed command: WRITE DMA
ata1.00: cmd ca/00:08:bf:f2:0a/00:00:00:00:00/e0 tag 0 dma 4096 out
res 51/84:00:bf:f2:0a/00:00:00:00:00/e0 Emask 0x10 (ATA bus error)
ata1.00: status: { DRDY ERR }
ata1.00: error: { ICRC ABRT }
ata1: soft resetting link
ata1.00: configured for UDMA/25
ata1: EH complete
ata1.00: limiting speed to PIO4
ata1.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 ata1.00: BMDMA stat 0x24
ata1.00: failed command: WRITE DMA
ata1.00: cmd ca/00:08:bf:f2:0a/00:00:00:00:00/e0 tag 0 dma 4096 out
res 51/84:00:bf:f2:0a/00:00:00:00:00/e0 Emask 0x10 (ATA bus error)
ata1.00: status: { DRDY ERR }
ata1.00: error: { ICRC ABRT }
ata1: soft resetting link
ata1.00: configured for PIO4
sd 0:0:0:0: [sda] Result: hostbyte=0x00 driverbyte=0x08 sd 0:0:0:0: [sda] Sense Key : 0xb [current] [descriptor] Descriptor sense data with sense descriptors (in hex):
72 0b 47 00 00 00 00 0c 00 0a 80 00 00 00 00 00
00 0a f2 bf sd 0:0:0:0: [sda] ASC=0x47 ASCQ=0x0
sd 0:0:0:0: [sda] CDB: cdb[0]=0x2a: 2a 00 00 0a f2 bf 00 00 08 00end_request: I/O error, dev sda, sector 717503
ata1: EH completeend_request: I/O error, dev sda, sector 717503
md: super_written gets error=-5, uptodate=0
raid1: Disk failure on sda1, disabling device.
raid1: Operation continuing on 1 devices.
ata2: lost interrupt (Status 0x51)
ata2.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 frozen ata2.00: failed command: WRITE DMA
ata2.00: cmd ca/00:08:bf:f2:0a/00:00:00:00:00/e0 tag 0 dma 4096 out
res 40/00:00:00:00:00/00:00:00:00:00/00 Emask 0x4 (timeout)
ata2.00: status: { DRDY }
ata2: soft resetting link
ata2.00: configured for UDMA/33
ata2.00: device reported invalid CHS sector 0
ata2: EH complete
ata2.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 ata2.00: BMDMA stat 0x44
ata2.00: failed command: WRITE DMA
ata2.00: cmd ca/00:08:bf:f2:0a/00:00:00:00:00/e0 tag 0 dma 4096 out
res 51/84:00:bf:f2:0a/00:00:00:00:00/e0 Emask 0x10 (ATA bus error)
ata2.00: status: { DRDY ERR }
ata2.00: error: { ICRC ABRT }
ata2: soft resetting link
ata2.00: configured for UDMA/33
ata2: EH complete
ata2.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 ata2.00: BMDMA stat 0x44
ata2.00: failed command: WRITE DMA
ata2.00: cmd ca/00:08:bf:f2:0a/00:00:00:00:00/e0 tag 0 dma 4096 out
res 51/84:00:bf:f2:0a/00:00:00:00:00/e0 Emask 0x10 (ATA bus error)
ata2.00: status: { DRDY ERR }
ata2.00: error: { ICRC ABRT }
ata2: soft resetting link
ata2.00: configured for UDMA/33
ata2: EH complete
ata2.00: limiting speed to UDMA/25:PIO4
ata2.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 ata2.00: BMDMA stat 0x44
ata2.00: failed command: WRITE DMA
ata2.00: cmd ca/00:08:bf:f2:0a/00:00:00:00:00/e0 tag 0 dma 4096 out
res 51/84:00:bf:f2:0a/00:00:00:00:00/e0 Emask 0x10 (ATA bus error)
ata2.00: status: { DRDY ERR }
ata2.00: error: { ICRC ABRT }
ata2: soft resetting link
ata2.00: configured for UDMA/25
ata2: EH complete
ata2.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 ata2.00: BMDMA stat 0x44
ata2.00: failed command: WRITE DMA
ata2.00: cmd ca/00:08:bf:f2:0a/00:00:00:00:00/e0 tag 0 dma 4096 out
res 51/84:00:bf:f2:0a/00:00:00:00:00/e0 Emask 0x10 (ATA bus error)
ata2.00: status: { DRDY ERR }
ata2.00: error: { ICRC ABRT }
ata2: soft resetting link
ata2.00: configured for UDMA/25
ata2: EH complete
ata2.00: limiting speed to PIO4
ata2.00: exception Emask 0x0 SAct 0x0 SErr 0x0 action 0x6 ata2.00: BMDMA stat 0x44
ata2.00: failed command: WRITE DMA
ata2.00: cmd ca/00:08:bf:f2:0a/00:00:00:00:00/e0 tag 0 dma 4096 out
res 51/84:00:bf:f2:0a/00:00:00:00:00/e0 Emask 0x10 (ATA bus error)
ata2.00: status: { DRDY ERR }
ata2.00: error: { ICRC ABRT }
ata2: soft resetting link
ata2.00: configured for PIO4
sd 1:0:0:0: [sdb] Result: hostbyte=0x00 driverbyte=0x08 sd 1:0:0:0: [sdb] Sense Key : 0xb [current] [descriptor] Descriptor sense data with sense descriptors (in hex):
72 0b 47 00 00 00 00 0c 00 0a 80 00 00 00 00 00
00 0a f2 bf sd 1:0:0:0: [sdb] ASC=0x47 ASCQ=0x0
sd 1:0:0:0: [sdb] CDB: cdb[0]=0x2a: 2a 00 00 0a f2 bf 00 00 08 00end_request: I/O error, dev sdb, sector 717503
ata2: EH completeend_request: I/O error, dev sdb, sector 717503
md: super_written gets error=-5, uptodate=0
RAID1 conf printout:
--- wd:1 rd:2
disk 0, wo:0, o:1, dev:sdb1
disk 1, wo:1, o:0, dev:sda1
Diffing the PCI space, and the BMDMA space between the 2.6.23 and 2.6.32 kernels reveals nothing unexpected given the results from above:
promise-2.6.32-dump renamed from promise-2.6.23-dump +3 -3
@@ -5,7 +5,7 @@ 30: 00 00 01 04 00 00 00 00 00 00 00 00 18 01 00 00 40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 50: ee 03 00 00 00 00 00 00 00 00 00 00 00 00 00 00 -60: f1 24 41 00 c4 f3 4f 00 f1 24 41 00 c4 f3 4f 00 +60: f1 44 42 00 c9 f3 4f 00 f1 44 42 00 c9 f3 4f 00 70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 80: 5a 10 33 4d 07 00 00 02 01 00 04 01 00 00 00 00 90: b1 10 00 00 c1 10 00 00 b9 10 00 00 c5 10 00 00
@@ -13,9 +13,9 @@ b0: 00 00 01 04 00 00 00 00 00 00 00 00 18 01 00 00 c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 d0: ee 03 00 00 00 00 00 00 00 00 00 00 00 00 00 00 -e0: f1 24 41 00 c4 f3 4f 00 f1 24 41 00 c4 f3 4f 00 +e0: f1 44 42 00 c9 f3 4f 00 f1 44 42 00 c9 f3 4f 00 f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 -1080: 00240000 e051b008 00640000 e051c008 +1080: 00200000 e7942008 00400000 e7943008 1090: 00000000 00000000 00000000 01d2110c
Could this be another case where the kernel should always need to write out the full task file to the drive? I don't see any form of hardware control on the PDC20247 (which is the UDMA add-on to the PDC20246 chip) to control its mode, other than it snooping the taskfile writes.
(I'm referring there to my old 380XD thinkpad where the drive regularly goes into lemon mode because the APM bios touches it behind the kernel's back. Alan said that was caused by PATA not writing the full taskfile out, resulting in some registers being left in unexpected states by the APM bios.)
Signature · 3 lines
Russell King Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of:
Some additions due to further testing:
Quoted 2 lines · Russell King
I tried upgrading my main machine from an old 2.6.23 kernel to 2.6.32. In doing so, I switched from IDE to ATA support.
IDE on 2.6.32 results in "irq 24: nobody cared" for the IDE interrupt. Booting back to 2.6.23 results in everything working normally again.
Quoted 4 lines
Could this be another case where the kernel should always need to write out the full task file to the drive? I don't see any form of hardware control on the PDC20247 (which is the UDMA add-on to the PDC20246 chip) to control its mode, other than it snooping the taskfile writes.
I tried commenting out the caching of the drive control register:
drivers/ata/libata-sff.c +2 -2
@@ -574,12 +574,12 @@ void ata_sff_tf_load(struct ata_port *ap, const struct ata_taskfile *tf)struct ata_ioports *ioaddr = &ap->ioaddr;unsigned int is_addr = tf->flags &ATA_TFLAG_ISADDR ; -if (tf->ctl != ap->last_ctl) { +// if (tf->ctl != ap->last_ctl) { if (ioaddr->ctl_addr)iowrite8 (tf->ctl, ioaddr->ctl_addr); ap->last_ctl = tf->ctl;ata_wait_idle (ap); - } +// } if (is_addr && (tf->flags &ATA_TFLAG_LBA48 )) {WARN_ON_ONCE (!ioaddr->ctl_addr);
This resulted in the same failure, but without the 'timeout' for the second drive. So there seems to be a separate problem in that when the Promise host gets reset, the host state isn't properly updated on all channels, resulting in the drive control register not being written for the 'other' channel.
Signature · 3 lines
Russell King Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of:
Okay, things have moved on, but not very much.
I've pulled the card out of the ARM machine, and its now in an x86 desktop, where, with 2.6.33-rc2 it behaves in exactly the same way. That is:
- with libata
- UDMA reads work fine, every time.
- UDMA writes always fail with a CRC error.
Note: there appears to be an additional libata bug here: when the speed gets knocked down to PIO mode, the block IO request is failed with *no* PIO retries. Subsequent IO requests succeed. This means that if you're using MD raid, your drive unnecessarily gets marked as failed - not nice when things are actually still operational, which can be seen by the other MD raid partitions on the drive behaving just fine in PIO mode.
An additional note: it seems that libata reads the ATA taskfile back from the drive on every command completion, successful or not - there are at least 30 IO accesses between any two ATA commands. Is there any reason for that?
- with IDE
- locks the interrupt line, and makes the machine extremely painful - about an hour to get to the point of being able to unload the pdc202xx_old module.
Having spent a lot of time today looking at the ATA drive, SFF DMA, promise IO configuration and PCI configuration registers, I can't see anything obviously wrong.
Nevertheless, the fact is that this card does work with previous kernels just fine, and continues to work if I boot back to old kernels. So, there is something going on which the old kernels do, which makes this card behave.
I'm going to drop investigation of libata for the time being, and possibly switch to the old IDE driver to find out why its getting a stuck PCI interrupt. Once the old IDE stuff works again, I'll be able to compare the differences between libata and IDE.
Quoted 46 lines · Russell King
Some additions due to further testing:
Quoted 2 lines · Russell King
I tried upgrading my main machine from an old 2.6.23 kernel to 2.6.32. In doing so, I switched from IDE to ATA support.
IDE on 2.6.32 results in "irq 24: nobody cared" for the IDE interrupt. Booting back to 2.6.23 results in everything working normally again.
Quoted 4 lines
Could this be another case where the kernel should always need to write out the full task file to the drive? I don't see any form of hardware control on the PDC20247 (which is the UDMA add-on to the PDC20246 chip) to control its mode, other than it snooping the taskfile writes.
I tried commenting out the caching of the drive control register:
drivers/ata/libata-sff.c +2
-2@@ -574,12 +574,12 @@ void ata_sff_tf_load(struct ata_port *ap, const struct ata_taskfile *tf)struct ata_ioports *ioaddr = &ap->ioaddr;unsigned int is_addr = tf->flags &ATA_TFLAG_ISADDR ; -if (tf->ctl != ap->last_ctl) { +// if (tf->ctl != ap->last_ctl) { if (ioaddr->ctl_addr)iowrite8 (tf->ctl, ioaddr->ctl_addr); ap->last_ctl = tf->ctl;ata_wait_idle (ap); - } +// } if (is_addr && (tf->flags &ATA_TFLAG_LBA48 )) {WARN_ON_ONCE (!ioaddr->ctl_addr);This resulted in the same failure, but without the 'timeout' for the second drive. So there seems to be a separate problem in that when the Promise host gets reset, the host state isn't properly updated on all channels, resulting in the drive control register not being written for the 'other' channel.
Signature · 3 lines
Russell King Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of:
Signature · 3 lines
Russell King Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of:
Quoted 10 lines · Russell King
Okay, things have moved on, but not very much.
I've pulled the card out of the ARM machine, and its now in an x86 desktop, where, with 2.6.33-rc2 it behaves in exactly the same way. That is:
- with libata
- UDMA reads work fine, every time.
- UDMA writes always fail with a CRC error.
Quoted 7 lines
Note: there appears to be an additional libata bug here: when the speed gets knocked down to PIO mode, the block IO request is failed with *no* PIO retries. Subsequent IO requests succeed. This means that if you're using MD raid, your drive unnecessarily gets marked as failed - not nice when things are actually still operational, which can be seen by the other MD raid partitions on the drive behaving just fine in PIO mode.
libata doesn't attempt any retries internally, that's up to the upper layers. If they're marking the array as failed after one failed request without any retries, that's presumably either intentional or a bug in the MD layer..
Quoted 5 lines
An additional note: it seems that libata reads the ATA taskfile back from the drive on every command completion, successful or not - there are at least 30 IO accesses between any two ATA commands. Is there any reason for that?
It shouldn't be reading the entire taskfile back after completion unless it failed or it's marked as requiring result taskfile (ATA passthru commands or internal commands, i.e. during probing). You're seeing this on normal read/write commands?
Quoted 68 lines
- with IDE
- locks the interrupt line, and makes the machine extremely painful - about an hour to get to the point of being able to unload thepdc202xx_oldmodule.Having spent a lot of time today looking at the ATA drive, SFF DMA, promise IO configuration and PCI configuration registers, I can't see anything obviously wrong.
Nevertheless, the fact is that this card does work with previous kernels just fine, and continues to work if I boot back to old kernels. So, there is something going on which the old kernels do, which makes this card behave.
I'm going to drop investigation of libata for the time being, and possibly switch to the old IDE driver to find out why its getting a stuck PCI interrupt. Once the old IDE stuff works again, I'll be able to compare the differences between libata and IDE.
Quoted 46 lines · Russell King
Some additions due to further testing:
Quoted 2 lines · Russell King
I tried upgrading my main machine from an old 2.6.23 kernel to 2.6.32. In doing so, I switched from IDE to ATA support.
IDE on 2.6.32 results in "irq 24: nobody cared" for the IDE interrupt. Booting back to 2.6.23 results in everything working normally again.
Quoted 4 lines
Could this be another case where the kernel should always need to write out the full task file to the drive? I don't see any form of hardware control on the PDC20247 (which is the UDMA add-on to the PDC20246 chip) to control its mode, other than it snooping the taskfile writes.
I tried commenting out the caching of the drive control register:
drivers/ata/libata-sff.c +2
-2@@ -574,12 +574,12 @@ void ata_sff_tf_load(struct ata_port *ap, const struct ata_taskfile *tf)struct ata_ioports *ioaddr =&ap->ioaddr;unsigned int is_addr = tf->flags&ATA_TFLAG_ISADDR ; -if (tf->ctl != ap->last_ctl) { +// if (tf->ctl != ap->last_ctl) { if (ioaddr->ctl_addr)iowrite8 (tf->ctl, ioaddr->ctl_addr); ap->last_ctl = tf->ctl;ata_wait_idle (ap); - } +// } if (is_addr&& (tf->flags&ATA_TFLAG_LBA48 )) {WARN_ON_ONCE (!ioaddr->ctl_addr);This resulted in the same failure, but without the 'timeout' for the second drive. So there seems to be a separate problem in that when the Promise host gets reset, the host state isn't properly updated on all channels, resulting in the drive control register not being written for the 'other' channel.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of:
Quoted 4 lines · Robert Hancock
libata doesn't attempt any retries internally, that's up to the upper layers. If they're marking the array as failed after one failed request without any retries, that's presumably either intentional or a bug in the MD layer..
Why should MD care? All that MD knows is that the IO request failed, it isn't told why. If a request fails, then it has to assume disk failure.
Maybe it's not a libata problem, but it _is_ bad system behaviour that needs to be resolved in some way.
However, it's only libata which knows that a UDMA CRC error can't happen with PIO. If its getting UDMA CRC errors, backing the speed down eventually to PIO is guaranteed to fix it. The SCSI layer can't know this, and shouldn't know this. The MD layer certainly doesn't get to know why requests are failing, nor can it know about these kinds of interface behaviour, so it can't be fixed there. That only leaves libata which knows this, knows what's happening and knows how to fix it.
Quoted 9 lines
Quoted 4 lines
An additional note: it seems that libata reads the ATA taskfile back from the drive on every command completion, successful or not - there are at least 30 IO accesses between any two ATA commands. Is there any reason for that?
It shouldn't be reading the entire taskfile back after completion unless it failed or it's marked as requiring result taskfile (ATA passthru commands or internal commands, i.e. during probing). You're seeing this on normal read/write commands?
Hmm, yes, you're right. For normal DMA read/write commands which are successful, no readback occurs. However, it appears for non-data and PIO commands during probe/error recovery - eg, this is what the accesses for a set features, xfer mode command looks like:
ioread8(0001b407) = 50 read statusiowrite8(a0, 0001b406) write drive
ioread8(0001b006) = 50 read alt status
ioread8(0001b407) = 50 read statusiowrite8(0a, 0001b006) write ctrl
ioread8(0001b407) = 50 read statusiowrite8(03, 0001b401) write taskfileiowrite8(42, 0001b402)iowrite8(00, 0001b403)iowrite8(00, 0001b404)iowrite8(00, 0001b405)iowrite8(a0, 0001b406)
ioread8(0001b407) = 50 read statusiowrite8(ef, 0001b407) write command
ioread8(0001b006) = d0 read altstatus
ioread8(0001b407) = d0 poll status
ioread8(0001b407) = d0
ioread8(0001b407) = d0
ioread8(0001b407) = d0
ioread8(0001b407) = d0
ioread8(0001b407) = 50iowrite8(08, 0001b006) write ctrl
ioread8(0001b407) = 50 read status
ioread8(0001ac0a) = 60 read sff dma statusiowrite8(60, 0001ac0a) write sff dma status
ioread8(0001ac0a) = 60 read sff dma status
ioread8(0001b407) = 50 read status
ioread8(0001b401) = 00 read taskfile
ioread8(0001b402) = 42
ioread8(0001b403) = 00
ioread8(0001b404) = 00
ioread8(0001b405) = 00
ioread8(0001b406) = a0
ioread8(0001ac08) = 00 read sff dma controliowrite8(00, 0001ac08) write sff dma control
ioread8(0001ac08) = 00 read sff dma control
Identify (ec) gets the same treatment. I don't have a trace for a PIO read/write to hand at the moment to check what their behaviour is, but I suspect the same on the grounds that I didn't notice a "please read back the taskfile" flag being set for the set features command.
Signature · 3 lines
Russell King Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of:
Quoted 2 lines
eventually to PIO is guaranteed to fix it. The SCSI layer can't know this, and shouldn't know this. The MD layer certainly doesn't get to
That depends which bit you mean - at the lower levels SCSI most certainly knows this kind of stuff as it too must deal with speed shifting and of course has to handle domain validation.
PATCH 1/1 Fix Promise UDMA33 IDE driver (pdc202xx_old)
In reply to Russell KingQuoted 4 lines · Russell King
- with IDE
- locks the interrupt line, and makes the machine extremely painful - about an hour to get to the point of being able to unload thepdc202xx_oldmodule.
Having manually bisected kernel versions, I've narrowed it down to some change between 2.6.30 and 2.6.31. There's not much which has changed between the two kernels, but one change stands out like a sore thumb:
+static int pdc202xx_test_irq(ide_hwif_t *hwif)
+{
+ struct pci_dev *dev = to_pci_dev(hwif->dev);
+ unsigned long high_16 = pci_resource_start(dev, 4);
+ u8 sc1d = inb(high_16 + 0x1d);
+
+ if (hwif->channel) {
+ /*
+ * bit 7: error, bit 6: interrupting,
+ * bit 5: FIFO full, bit 4: FIFO empty
+ */
+ return ((sc1d & 0x50) == 0x40) ? 1 : 0;
+ } else {
+ /*
+ * bit 3: error, bit 2: interrupting,
+ * bit 1: FIFO full, bit 0: FIFO empty
+ */+ return ((sc1d & 0x05) == 0x04) ? 1 : 0;
+ }
+}
Reading the (documented as a 32-bit) system control register when the interface is idle gives: 0x01da110c
So, the byte at 0x1d is 0x11, which is documented as meaning that the primary and secondary FIFOs are empty.
The code above, which is trying to see whether an IRQ is pending, checks for the IRQ bit to be one, and the FIFO bit to be zero - or in English, to be non-empty.
Since during a BM-DMA read, the FIFOs will naturally be drained to the PCI bus, the chance of us getting to the interface before this happens are extremely small - and if we don't, it means we decide not to service the interrupt. Hence, the screaming interrupt problem with drivers/ide.
Fix this by only indicating an interrupt is ready if both the interrupt and FIFO empty bits are at '1'.
This bug only affects PDC20246/PDC20247 (Promise Ultra33) based cards, and has been tested on 2.6.31 and 2.6.33-rc2.
--- drivers/ide/pdc202xx_old.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/ide/pdc202xx_old.c +2 -2
@@ -100,13 +100,13 @@ static int pdc202xx_test_irq(ide_hwif_t *hwif)* bit 7: error, bit 6: interrupting, * bit 5: FIFO full, bit 4: FIFO empty */ -return ((sc1d &0x50 ) ==0x40 ) ?1 :0 ; +return ((sc1d &0x50 ) ==0x50 ) ?1 :0 ; }else {/* * bit 3: error, bit 2: interrupting, * bit 1: FIFO full, bit 0: FIFO empty */ -return ((sc1d &0x05 ) ==0x04 ) ?1 :0 ; +return ((sc1d &0x05 ) ==0x05 ) ?1 :0 ; } }
Signature · 3 lines
Russell King Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of:
Re: [PATCH] Fix Promise UDMA33 IDE driver (pdc202xx_old)
Hello.
Quoted 19 lines · Russell King
Quoted 4 lines
- with IDE
- locks the interrupt line, and makes the machine extremely painful - about an hour to get to the point of being able to unload thepdc202xx_oldmodule.Having manually bisected kernel versions, I've narrowed it down to some change between 2.6.30 and 2.6.31. There's not much which has changed between the two kernels, but one change stands out like a sore thumb:
(quoted diff) +10
-0+static int pdc202xx_test_irq(ide_hwif_t *hwif) +{ + struct pci_dev *dev = to_pci_dev(hwif->dev); + unsigned long high_16 = pci_resource_start(dev, 4); + u8 sc1d = inb(high_16 + 0x1d); + + if (hwif->channel) { + /* + * bit 7: error, bit 6: interrupting, + * bit 5: FIFO full, bit 4: FIFO empty
Review
Quoted 10 lines
(quoted diff) +10
-0+ */ + return ((sc1d & 0x50) == 0x40) ? 1 : 0; + } else { + /* + * bit 3: error, bit 2: interrupting, + * bit 1: FIFO full, bit 0: FIFO empty + */ + return ((sc1d & 0x05) == 0x04) ? 1 : 0; + } +}
Sigh, it was mine... :-/
Quoted 2 lines
Reading the (documented as a 32-bit) system control register when the interface is idle gives:
0x01da110c
Oh, you even have the docs... :-)
Quoted 6 lines
So, the byte at
0x1dis0x11, which is documented as meaning that the primary and secondary FIFOs are empty.The code above, which is trying to see whether an IRQ is pending, checks for the IRQ bit to be one, and the FIFO bit to be zero - or in English, to be non-empty.
Yeah, here's the older driver code that I got inspiration from:
static int pdc202xx_dma_test_irq(ide_drive_t *drive)
{
ide_hwif_t *hwif = drive->hwif;
unsigned long high_16 = hwif->extra_base - 16;
u8 dma_stat = inb(hwif->dma_base + ATA_DMA_STATUS);
u8 sc1d = inb(high_16 + 0x001d);
if (hwif->channel) {
/* bit7: Error, bit6: Interrupting, bit5: FIFO Full, bit4:
FIFO Empty */
if ((sc1d & 0x50) == 0x50)
goto somebody_else;
else if ((sc1d & 0x40) == 0x40)
return (dma_stat & 4) == 4;
} else {
/* bit3: Error, bit2: Interrupting, bit1: FIFO Full, bit0:
FIFO Empty */
if ((sc1d & 0x05) == 0x05)
goto somebody_else;
else if ((sc1d & 0x04) == 0x04)
return (dma_stat & 4) == 4;
}
somebody_else:
return (dma_stat & 4) == 4; /* return 1 if INTR asserted */
} Now you can see the source of the confusion -- interrupts with FIFO
empty were regarded as coming from something else. The 'sc1d' value was effectively ignored at that time though, that's why this went unnoticed so far.
Quoted 7 lines
Since during a BM-DMA read, the FIFOs will naturally be drained to the PCI bus, the chance of us getting to the interface before this happens are extremely small - and if we don't, it means we decide not to service the interrupt. Hence, the screaming interrupt problem with
drivers/ide.Fix this by only indicating an interrupt is ready if both the interrupt and FIFO empty bits are at '1'.
I suggest that we completely ignore the "FIFO empty" bit. dma_test_irq()
method which is called eventually when doing FMA should yield the needed result WRT FIFO flushing, filtering out .
Quoted 14 lines
This bug only affects PDC20246/PDC20247 (Promise Ultra33) based cards, and has been tested on 2.6.31 and 2.6.33-rc2.
Signed-off-by Russell King <[email protected]>Tested-by Russell King <[email protected]>--- drivers/ide/pdc202xx_old.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)drivers/ide/pdc202xx_old.c +0
-0
drivers/ide/pdc202xx_old.c static int pdc202xx_test_irq(ide_hwif_t *hwif) · line 103
Quoted 5 lines
drivers/ide/pdc202xx_old.c +1
-1@@ -100,13 +100,13 @@ static int pdc202xx_test_irq(ide_hwif_t *hwif)* bit 7: error, bit 6: interrupting, * bit 5: FIFO full, bit 4: FIFO empty */ -return ((sc1d &0x50 ) ==0x40 ) ?1 :0 ; +return ((sc1d &0x50 ) ==0x50 ) ?1 :0 ;
+ return (sc1d & 0x40) ? 1 : 0;
drivers/ide/pdc202xx_old.c static int pdc202xx_test_irq(ide_hwif_t *hwif) · line 109
Quoted 7 lines
drivers/ide/pdc202xx_old.c +1
-1}else {/* * bit 3: error, bit 2: interrupting, * bit 1: FIFO full, bit 0: FIFO empty */ -return ((sc1d &0x05 ) ==0x04 ) ?1 :0 ; +return ((sc1d &0x05 ) ==0x05 ) ?1 :0 ;
+ return (sc1d & 0x04) ? 1 : 0;
MBR, Sergei
Re: [PATCH] Fix Promise UDMA33 IDE driver (pdc202xx_old)
From: Sergei Shtylyov <[email protected]>
Date: Mon, 04 Jan 2010 22:14:55 +0300
Quoted 3 lines
I suggest that we completely ignore the "FIFO empty"
bit.dma_test_irq()method which is called eventually when doing FMA should yield the needed result WRT FIFO flushing, filtering out .
Can someone implement and test such a revised patch?
Thanks!
Re: [PATCH] Fix Promise UDMA33 IDE driver (pdc202xx_old)
Quoted 8 lines · David Miller
From: Sergei Shtylyov <[email protected]>
Date: Mon, 04 Jan 2010 22:14:55 +0300Quoted 3 lines
I suggest that we completely ignore the "FIFO empty"
bit.dma_test_irq()method which is called eventually when doing FMA should yield the needed result WRT FIFO flushing, filtering out .Can someone implement and test such a revised patch?
Do we have any guarantee that the interrupt bit in this register will only be set when the DMA FIFOs on the card have emptied?
Unlike the SFF status register's interrupt bit, which is defined by SFF to only be asserted when the drive signals an interrupt _and_ the FIFOs have emptied, there seems to be no such words in the PDC20246 data which suggests that the same is true of the interrupt bit in this alternative register.
Therefore, I would suggest that using this alternative bit could be unsafe with shared interrupts without also qualifying it with the FIFO empty bit - we could end up disabling the DMA before the transfer has completed, thereby corrupting the last few words of a transfer from the drive.
What was the reason for checking this alternate promise-special register in the first place, rather than the SFF BM-DMA status register?
Signature · 3 lines
Russell King Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of:
Re: [PATCH] Fix Promise UDMA33 IDE driver (pdc202xx_old)
Hello.
Quoted 9 lines · Russell King
Quoted 8 lines
From: Sergei Shtylyov <[email protected]>
Date: Mon, 04 Jan 2010 22:14:55 +0300Quoted 3 lines
I suggest that we completely ignore the "FIFO empty"
bit.dma_test_irq()method which is called eventually when doing FMA should yield the needed result WRT FIFO flushing, filtering out .Can someone implement and test such a revised patch?
Quoted 8 lines
Do we have any guarantee that the interrupt bit in this register will only be set when the DMA FIFOs on the card have emptied?
Unlike the SFF status register's interrupt bit, which is defined by SFF to only be asserted when the drive signals an interrupt _and_ the FIFOs have emptied, there seems to be no such words in the PDC20246 data which suggests that the same is true of the interrupt bit in this alternative register.
So what? The SFF status register will be read eventually.
Quoted 4 lines
Therefore, I would suggest that using this alternative bit could be unsafe with shared interrupts without also qualifying it with the FIFO empty bit - we could end up disabling the DMA before the transfer has completed, thereby corrupting the last few words of a transfer from the drive.
How so?
Quoted 2 lines
What was the reason for checking this alternate promise-special register in the first place, rather than the SFF BM-DMA status register?
It's not "rather then" -- we're checking both.
MBR, Sergei
Quoted 18 lines · Russell King
Okay, things have moved on, but not very much.
I've pulled the card out of the ARM machine, and its now in an x86 desktop, where, with 2.6.33-rc2 it behaves in exactly the same way. That is:
- with libata
- UDMA reads work fine, every time.
- UDMA writes always fail with a CRC error.Note: there appears to be an additional libata bug here: when the speed gets knocked down to PIO mode, the block IO request is failed with *no* PIO retries. Subsequent IO requests succeed. This means that if you're using MD raid, your drive unnecessarily gets marked as failed - not nice when things are actually still operational, which can be seen by the other MD raid partitions on the drive behaving just fine in PIO mode.
Quoted 4 lines
An additional note: it seems that libata reads the ATA taskfile back from the drive on every command completion, successful or not - there are at least 30 IO accesses between any two ATA commands. Is there any reason for that?
Now that the old IDE driver is working, comparing 2.6.33-rc2 setup. Timing registers are at 0x60, each of 32-bits for primary master, slave, secondary master, slave. As can be seen, the setup is identical, so it's not a card configuration issue.
ATA:
00: 5a 10 33 4d 07 00 00 02 01 00 04 01 00 20 00 00 10: 01 bc 00 00 05 b8 00 00 01 b4 00 00 05 b0 00 00 20: 01 ac 00 00 00 00 00 00 00 00 00 00 00 00 00 00 30: 00 00 0f ff 00 00 00 00 00 00 00 00 0b 01 00 00 40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 50: ee 03 00 00 00 00 00 00 00 00 00 00 00 00 00 00 60: f1 24 41 00 c9 f3 4f 00 f1 24 41 00 c9 f3 4f 00
IDE:
00: 5a 10 33 4d 07 00 00 02 01 00 04 01 00 20 00 00 10: 01 bc 00 00 05 b8 00 00 01 b4 00 00 05 b0 00 00 20: 01 ac 00 00 00 00 00 00 00 00 00 00 00 00 00 00 30: 00 00 0f ff 00 00 00 00 00 00 00 00 0b 01 00 00 40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 50: ee 03 00 00 00 00 00 00 00 00 00 00 00 00 00 00 60: f1 24 41 00 c4 f3 4f 00 f1 24 41 00 c4 f3 4f 00
The system control register is almost same - ATA 01d2110c vs 01da110c. Programming it to the value it has under ATA doesn't cause any ill effects with IDE.
This leaves one thing - the way the ATA taskfile is accessed. Those who don't know the Promise Ultra33 hardware, it consists of two devices - the PDC20246, which is a PIO/SWDMA/MWDMA host, and a PDC20247 add-on, which partly sits between the host and the drives providing the UDMA protocol. The 247 intercepts INTRQ, DIOR, DIOW, IORDY, DMARQ, DMACK and DD0:15 signals, but monitors the DA0:2 and two CS signals to the drive.
The documentation doesn't specify how the 247 works, except specifying that it has a UDMA mode and a pass-through mode. However, we can make some assumptions from the wiring:
1. There is no way for the 247 to see any configuration settings; the only thing it can see are the taskfile reads and writes, and the timing of the DMA signals from the 246.
2. The 246 timings for MWDMA2 and UDMA0 are identical; there is no programming difference between these two modes. The only way the 246 can know that UDMA is selected is by looking for the SET FEATURES command to the drive.
So, taking all this together, I'm now down to the way libata drives the ATA task file, which seems to be *extremely* noisy. What is the best way to cut down that noise, down to the bare minimum that's actually required, while still keeping compatibility for my ICH5 SATA controller (which I need for my x86 machine's boot drive.)
I've tried changing the set xfermode code to use a version of ata_exec_internal() which doesn't return the taskfile, but this makes no difference to the promise exploding with CRC errors with UDMA writes. Is it possible to do a similar thing with IDENTIFY?
Also, is it possible to get rid of the additional identify and read native max address commands which seem to be repeated (command register writes listed):
iowrite8(ec, 0001b407) identifyiowrite8(f8, 0001b407) read native maxiowrite8(ef, 0001b407) set featuresiowrite8(ec, 0001b407) identifyiowrite8(f8, 0001b407) read native maxiowrite8(c8, 0001b407) read dma, succeedsiowrite8(c8, 0001b407)
...iowrite8(c8, 0001b407)
repeat {iowrite8(ca, 0001b407) write dma, failsiowrite8(ec, 0001b407) identifyiowrite8(f8, 0001b407) read native maxiowrite8(ef, 0001b407) set featuresiowrite8(ec, 0001b407) identifyiowrite8(f8, 0001b407) read native max
} until PIO
Signature · 3 lines
Russell King Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of:
Quoted 8 lines
1. There is no way for the 247 to see any configuration settings; the only thing it can see are the taskfile reads and writes, and the timing of the DMA signals from the 246.
2. The 246 timings for MWDMA2 and UDMA0 are identical; there is no programming difference between these two modes. The only way the 246 can know that UDMA is selected is by looking for the SET FEATURES command to the drive.
The 2026x certainly snoops SET FEATURES so that would be a reasonable assumption.
Quoted 4 lines
I've tried changing the set xfermode code to use a version of
ata_exec_internal()which doesn't return the taskfile, but this makes no difference to the promise exploding with CRC errors with UDMA writes. Is it possible to do a similar thing with IDENTIFY?
No because you need to know if it worked.
Quoted 3 lines
Also, is it possible to get rid of the additional identify and read native max address commands which seem to be repeated (command register writes listed):
You can turn off Host Protected Area support for this. You could also btw turn *on* host protected area for the IDE stack and see what occurs but I imagine its a red herring anyway. If the snoop is failing then it is more likely to be that the chip has some limitations on the taskfile snooping and perhaps requires that the device register is always written or that the registers are written in a specific order when writing the set features command.
Another thing to try if that fails is using a polled set features in case the chip has problems in that area. We've seen a similar bug on some older VIA devices.
Quoted 31 lines · Alan Cox
Quoted 8 lines
1. There is no way for the 247 to see any configuration settings; the only thing it can see are the taskfile reads and writes, and the timing of the DMA signals from the 246.
2. The 246 timings for MWDMA2 and UDMA0 are identical; there is no programming difference between these two modes. The only way the 246 can know that UDMA is selected is by looking for the SET FEATURES command to the drive.
The 2026x certainly snoops SET FEATURES so that would be a reasonable assumption.
Quoted 4 lines
I've tried changing the set xfermode code to use a version of
ata_exec_internal()which doesn't return the taskfile, but this makes no difference to the promise exploding with CRC errors with UDMA writes. Is it possible to do a similar thing with IDENTIFY?No because you need to know if it worked.
Quoted 3 lines
Also, is it possible to get rid of the additional identify and read native max address commands which seem to be repeated (command register writes listed):
You can turn off Host Protected Area support for this. You could also btw turn *on* host protected area for the IDE stack and see what occurs but I imagine its a red herring anyway. If the snoop is failing then it is more likely to be that the chip has some limitations on the taskfile snooping and perhaps requires that the device register is always written or that the registers are written in a specific order when writing the set features command.
Quoted 3 lines
Another thing to try if that fails is using a polled set features in case the chip has problems in that area. We've seen a similar bug on some older VIA devices.
Found the problem - getting rid of the read of the alt status register after the command has been written fixes the UDMA CRC errors on write:
@@ -676,7 +676,8 @@ void ata_sff_exec_command(struct ata_port *ap, const struct ata_taskfile *tf)
DPRINTK("ata%u: cmd 0x%X\n", ap->print_id, tf->command);
iowrite8(tf->command, ap->ioaddr.command_addr);
- ata_sff_pause(ap);
+ ndelay(400);
+// ata_sff_pause(ap); }
EXPORT_SYMBOL_GPL(ata_sff_exec_command);
This rather makes sense. The PDC20247 handles the UDMA part of the protocol. It has no way to tell the PDC20246 to wait while it suspends UDMA, so that a normal register access can take place - the 246 ploughs on with the register access without any regard to the state of the 247.
If the drive immediately starts the UDMA protocol after a write to the command register (as it probably will for the DMA WRITE command), then we'll be accessing the taskfile in the middle of the UDMA setup, which can't be good. It's certainly a violation of the ATA specs.
Signature · 3 lines
Russell King Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of:
Quoted 4 lines
If the drive immediately starts the UDMA protocol after a write to the command register (as it probably will for the DMA WRITE command), then we'll be accessing the taskfile in the middle of the UDMA setup, which can't be good. It's certainly a violation of the ATA specs.
I concur entirely - that wants pushing into the upstream kernel ASAP and if its ok backporting. May also be worth seeing if that fixes the funny in the ALi setup.
Quoted 8 lines · Alan Cox
Quoted 4 lines
If the drive immediately starts the UDMA protocol after a write to the command register (as it probably will for the DMA WRITE command), then we'll be accessing the taskfile in the middle of the UDMA setup, which can't be good. It's certainly a violation of the ATA specs.
I concur entirely - that wants pushing into the upstream kernel ASAP and if its ok backporting. May also be worth seeing if that fixes the funny in the ALi setup.
How do you think one tests for command completion? :) There does not seem to be a general problem reading AltStatus(BSY) during a DMA transfer. IDE driver and other well known codebases have been doing that for decades.
I think the undefined behavior is only where the AltStatus read occurs before the 400ns period specified by ATA... excising the AltStatus read there (ata_sff_pause) should be fine for PIO, but problematic for MMIO chipsets that need posted writes flushed prior to the 400ns delay.
Jeff
Quoted 15 lines · Jeff Garzik
Quoted 8 lines · Alan Cox
Quoted 4 lines
If the drive immediately starts the UDMA protocol after a write to the command register (as it probably will for the DMA WRITE command), then we'll be accessing the taskfile in the middle of the UDMA setup, which can't be good. It's certainly a violation of the ATA specs.
I concur entirely - that wants pushing into the upstream kernel ASAP and if its ok backporting. May also be worth seeing if that fixes the funny in the ALi setup.
How do you think one tests for command completion? :) There does not seem to be a general problem reading AltStatus(BSY) during a DMA transfer. IDE driver and other well known codebases have been doing that for decades.
Quoted 4 lines
I think the undefined behavior is only where the AltStatus read occurs before the 400ns period specified by ATA... excising the AltStatus read there (
ata_sff_pause) should be fine for PIO, but problematic for MMIO chipsets that need posted writes flushed prior to the 400ns delay.
That's not the problem here - with the printk's I've added, there's no way for the IO read/writes to come that fast. Slowing down the IO accesses made no difference what so ever.
Signature · 3 lines
Russell King Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of:
Quoted 3 lines
That's not the problem here - with the printk's I've added, there's no way for the IO read/writes to come that fast. Slowing down the IO accesses made no difference what so ever.
Doing it within 400nS of command setup is suspect at the ATA level and can trivially fixed.
Sane PCI controllers (and quite a few less than sane ones) all implement an AltStatus which can be checked for busy bits with DMA running or stall that access until the DMA completes (wrongly in some CMD64x cases).
The 20246 implements its own magic 'am I busy' flag which old IDE supports and libata doesn't. So as well as the ndelay(400) fix for PIO controllers it needs pdc202xx_test_irq porting over to complete the fix.
Alan
Quoted 7 lines · Alan Cox
Quoted 3 lines
That's not the problem here - with the printk's I've added, there's no way for the IO read/writes to come that fast. Slowing down the IO accesses made no difference what so ever.
Doing it within 400nS of command setup is suspect at the ATA level and can trivially fixed.
Quoted 7 lines
Sane PCI controllers (and quite a few less than sane ones) all implement an AltStatus which can be checked for busy bits with DMA running or stall that access until the DMA completes (wrongly in some CMD64x cases).
The 20246 implements its own magic 'am I busy' flag which old IDE supports and libata doesn't. So as well as the ndelay(400) fix for PIO controllers it needs
pdc202xx_test_irqporting over to complete the fix.
Yes, but please use a non-buggy version of pdc202xx_test_irq() as per my recent patch. Testing for the DMA FIFOs being non-empty to qualify an interrupt is Very Bad News too.
It's interesting to note that SFF 8038 suggests an approach to dealing with IDE interrupts connected to a SFF compatible host interface:
3) Software issues the appropriate DMA transfer command to the disk device.
4) Engage the bus master function by writing a '1' to the Start bit in the Bus Master IDE Command Register for the appropriate channel.
5) The controller transfers data to/from memory responding to DMA requests from the IDE device.
6) at the end of the transfer the IDE device signals an interrupt .
7) In response to the interrupt, software resets the Start/Stop bit in the command register. It then reads the controller status and then the drive status to determine if the transfer completed successfully.
It goes on to say that the interrupt status bit in the BM-DMA status register will only be set after buffered data has been transferred - which means reading the altstatus register to determine when the device has finished would give a premature indication and doesn't take any account of the BM-DMA buffering.
So I think Jeff is not right on the "must read altstatus" point.
I would not be surprised if lots of host controllers were actually buggy in respect of reading any ATA device register mid-transfer - firstly a started UDMA operation can't be stopped before at least one word has been transferred, nor can it be stopped without first transferring the CRC bytes. That's an awful lot of logic (and overhead) to be randomly interacting with due to other shared devices interrupt activity.
Reading the BM-DMA status register (or equivalent on hosts) seems to be the right way to tell if an interrupt is pending.
Signature · 3 lines
Russell King Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of:
Quoted 15 lines · Russell King
It goes on to say that the interrupt status bit in the BM-DMA status register will only be set after buffered data has been transferred - which means reading the altstatus register to determine when the device has finished would give a premature indication and doesn't take any account of the BM-DMA buffering.
So I think Jeff is not right on the "must read altstatus" point.
I would not be surprised if lots of host controllers were actually buggy in respect of reading any ATA device register mid-transfer - firstly a started UDMA operation can't be stopped before at least one word has been transferred, nor can it be stopped without first transferring the CRC bytes. That's an awful lot of logic (and overhead) to be randomly interacting with due to other shared devices interrupt activity.
Quoted 2 lines
Reading the BM-DMA status register (or equivalent on hosts) seems to be the right way to tell if an interrupt is pending.
libata already does that, as does IDE:
case HSM_ST_LAST:
if (qc->tf.protocol == ATA_PROT_DMA ||
qc->tf.protocol == ATAPI_PROT_DMA) {
/* check status of DMA engine */
host_stat = ap->ops->bmdma_status(ap);
VPRINTK("ata%u: host_stat 0x%X\n",
ap->print_id, host_stat);
/* if it's not our irq... */
if (!(host_stat & ATA_DMA_INTR))
goto idle_irq;[...]
/* check main status, clearing INTRQ if needed */
status = ata_sff_irq_status(ap); <-- reads AltStatus here if (status & ATA_BUSY)
goto idle_irq;
IDE driver has basically the same logic in drive_is_ready()
I never said "must" read, I was only responding to the notion that touching AltStatus at all, in general, was a violation of the ATA spec.
IFF there is a DMA status register available to be checked, and IFF
the transfer is DMA, then we do check that first.
Jeff
Hello.
Quoted 11 lines · Russell King
Quoted 10 lines
Quoted 3 lines
That's not the problem here - with the printk's I've added, there's no way for the IO read/writes to come that fast. Slowing down the IO accesses made no difference what so ever.
Doing it within 400nS of command setup is suspect at the ATA level and can trivially fixed.
Sane PCI controllers (and quite a few less than sane ones) all implement an AltStatus which can be checked for busy bits with DMA running or stall that access until the DMA completes (wrongly in some CMD64x cases).
Quoted 7 lines
Quoted 3 lines
The 20246 implements its own magic 'am I busy' flag which old IDE supports and libata doesn't. So as well as the ndelay(400) fix for PIO controllers it needs
pdc202xx_test_irqporting over to complete the fix.Yes, but please use a non-buggy version of
pdc202xx_test_irq()as per my recent patch. Testing for the DMA FIFOs being non-empty to qualify an interrupt is Very Bad News too.
Sorry about that -- I was the author of pdc202xx_test_irq()...
Quoted 13 lines
It's interesting to note that SFF 8038 suggests an approach to dealing with IDE interrupts connected to a SFF compatible host interface:
3) Software issues the appropriate DMA transfer command to the disk device.
4) Engage the bus master function by writing a '1' to the Start bit in the Bus Master IDE Command Register for the appropriate channel.
5) The controller transfers data to/from memory responding to DMA requests from the IDE device.
6) at the end of the transfer the IDE device signals an interrupt .
7) In response to the interrupt, software resets the Start/Stop bit in the command register. It then reads the controller status and then the drive status to determine if the transfer completed successfully.
Quoted 5 lines
It goes on to say that the interrupt status bit in the BM-DMA status register will only be set after buffered data has been transferred - which means reading the altstatus register to determine when the device has finished would give a premature indication and doesn't take any account of the BM-DMA buffering.
Yes.
Quoted 1 line
So I think Jeff is not right on the "must read altstatus" point.
Frankly speaking, I'm not sure why Jeff insisted on that, at least for
the DMA case...
Quoted 9 lines
I would not be surprised if lots of host controllers were actually buggy in respect of reading any ATA device register mid-transfer - firstly a started UDMA operation can't be stopped before at least one word has been transferred, nor can it be stopped without first transferring the CRC bytes. That's an awful lot of logic (and overhead) to be randomly interacting with due to other shared devices interrupt activity.
Reading the BM-DMA status register (or equivalent on hosts) seems to be the right way to tell if an interrupt is pending.
Except the BMDMA interrupt bit (generally) doesn't get set for the PIO
transfers. Anyway, the IDE core eventually reads that register for the DMA transfers. I'm sure libata also does eventually read it; looking at the code in ata_sff_host_intr(), it even reads the BMDMA status first, before touching any of the IDE status registers.
MBR, Sergei
Quoted 4 lines
How do you think one tests for command completion? :) There does not seem to be a general problem reading AltStatus(BSY) during a DMA transfer. IDE driver and other well known codebases have been doing that for decades.
You use altstatus if available but you can't do so for 400nS after command issue.
Quoted 4 lines
I think the undefined behavior is only where the AltStatus read occurs before the 400ns period specified by ATA... excising the AltStatus read there (
ata_sff_pause) should be fine for PIO, but problematic for MMIO chipsets that need posted writes flushed prior to the 400ns delay.
Old IDE says no such chipsets exist. Probably because their own designers would have been lynched by the in house software team ;)
Quoted 56 lines · Russell King
Quoted 31 lines · Alan Cox
Quoted 8 lines
1. There is no way for the 247 to see any configuration settings; the only thing it can see are the taskfile reads and writes, and the timing of the DMA signals from the 246.
2. The 246 timings for MWDMA2 and UDMA0 are identical; there is no programming difference between these two modes. The only way the 246 can know that UDMA is selected is by looking for the SET FEATURES command to the drive.
The 2026x certainly snoops SET FEATURES so that would be a reasonable assumption.
Quoted 4 lines
I've tried changing the set xfermode code to use a version of
ata_exec_internal()which doesn't return the taskfile, but this makes no difference to the promise exploding with CRC errors with UDMA writes. Is it possible to do a similar thing with IDENTIFY?No because you need to know if it worked.
Quoted 3 lines
Also, is it possible to get rid of the additional identify and read native max address commands which seem to be repeated (command register writes listed):
You can turn off Host Protected Area support for this. You could also btw turn *on* host protected area for the IDE stack and see what occurs but I imagine its a red herring anyway. If the snoop is failing then it is more likely to be that the chip has some limitations on the taskfile snooping and perhaps requires that the device register is always written or that the registers are written in a specific order when writing the set features command.
Quoted 3 lines
Another thing to try if that fails is using a polled set features in case the chip has problems in that area. We've seen a similar bug on some older VIA devices.
Found the problem - getting rid of the read of the alt status register after the command has been written fixes the UDMA CRC errors on write:
(quoted diff) +2
-1@@ -676,7 +676,8 @@ void ata_sff_exec_command(struct ata_port *ap, const struct ata_taskfile *tf) DPRINTK("ata%u: cmd 0x%X\n", ap->print_id, tf->command); iowrite8(tf->command, ap->ioaddr.command_addr); - ata_sff_pause(ap); + ndelay(400); +// ata_sff_pause(ap); } EXPORT_SYMBOL_GPL(ata_sff_exec_command);This rather makes sense. The PDC20247 handles the UDMA part of the protocol. It has no way to tell the PDC20246 to wait while it suspends UDMA, so that a normal register access can take place - the 246 ploughs on with the register access without any regard to the state of the 247.
Quoted 4 lines
If the drive immediately starts the UDMA protocol after a write to the command register (as it probably will for the DMA WRITE command), then we'll be accessing the taskfile in the middle of the UDMA setup, which can't be good. It's certainly a violation of the ATA specs.
Well...
Without AltStatus, you would not be able to check and see if a command is complete... pretty much by definition you should able to access that at any time. If BSY=1, none of the other bits are valid, but nonetheless it is axiomatic that you cannot determine BSY state without reading AltStatus.
It is certainly conceivable that this is a problem on a rare controller or two, but in general, AltStatus is how you poll for DMA completion. You are allowed to hit it during a DMA transfer.
Well known BIOS code, ATADRVR, the IDE driver and libata all read AltStatus __in the middle of a transfer__ in the case of PCI shared interrupts, or in the case of interrupt-free polling for DMA completion.
HOWEVER, accessing AltStatus prior to the 400ns post-exec-command period is potentially an area of undefined behavior. Changing this code is only a problem for MMIO-based controllers, which use the AltStatus read to guarantee that the 400ns delay occurs outside any posted writes.
Jeff
Quoted 11 lines · Jeff Garzik
Quoted 9 lines · Russell King
This rather makes sense. The PDC20247 handles the UDMA part of the protocol. It has no way to tell the PDC20246 to wait while it suspends UDMA, so that a normal register access can take place - the 246 ploughs on with the register access without any regard to the state of the 247.
If the drive immediately starts the UDMA protocol after a write to the command register (as it probably will for the DMA WRITE command), then we'll be accessing the taskfile in the middle of the UDMA setup, which can't be good. It's certainly a violation of the ATA specs.
Quoted 4 lines
Well...
Without AltStatus, you would not be able to check and see if a command is complete...
I wasn't inferring that reading altstatus was a bad idea. I was saying that causing altstatus to be read in the middle of an active UDMA transfer (in other words, while the hosts DMACK is active) is a violation of the spec.
There's no way for the 20247 (UDMA add-on) to know that the IO access is coming; the first the 247 knows about it is when the CS/address lines to the drive have been activated - in violation of the ATA specification (which calls for the CS lines to be inactive and address lines to be zero while UDMA is occuring.)
That means there's absolutely no way for the 20247 to stop the UDMA protocol to allow a read of any registers on the drive until UDMA has completed.
Yes, technically a buggy host design...
Signature · 3 lines
Russell King Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of:
Quoted 14 lines · Russell King
Quoted 13 lines · Jeff Garzik
Quoted 9 lines · Russell King
This rather makes sense. The PDC20247 handles the UDMA part of the protocol. It has no way to tell the PDC20246 to wait while it suspends UDMA, so that a normal register access can take place - the 246 ploughs on with the register access without any regard to the state of the 247.
If the drive immediately starts the UDMA protocol after a write to the command register (as it probably will for the DMA WRITE command), then we'll be accessing the taskfile in the middle of the UDMA setup, which can't be good. It's certainly a violation of the ATA specs.
Well...
Quoted 7 lines
Quoted 2 lines
Without AltStatus, you would not be able to check and see if a command is complete...
I wasn't inferring that reading altstatus was a bad idea. I was saying that causing altstatus to be read in the middle of an active UDMA transfer (in other words, while the hosts DMACK is active) is a violation of the spec.
I wouldn't say that's the case, as I don't see any such prohibition in the ATA or SFF host adapter specs. The host doesn't necessarily know whether or not a UDMA transfer is active or not. It's supposed to be the controller's job to either terminate the UDMA burst or stall the access until it can handle the request. It seems a bit of a deficiency of this controller that it can't.
The problem with just taking the altstatus read out is that in certain cases, the ATA spec requires that we wait for not 400ns, but one PIO cycle time before reading the status register (which may be up to 600ns). The altstatus read inherently accomplishes this since it causes a bus cycle. The spec says nothing about not being allowed to access altstatus during this period, and in fact ATA-6 and up explictly suggest a dummy altstatus read to do this.
One alternative might be to increase the delay to 600ns and change it to read the BMDMA status register instead (to ensure PCI posted write flushing with MMIO). The trick in doing this in generic code is that the register might not exist (BMDMA controllers are a subset of SFF controllers, which is what this code deals with).
Quoted 11 lines
There's no way for the 20247 (UDMA add-on) to know that the IO access is coming; the first the 247 knows about it is when the CS/address lines to the drive have been activated - in violation of the ATA specification (which calls for the CS lines to be inactive and address lines to be zero while UDMA is occuring.)
That means there's absolutely no way for the 20247 to stop the UDMA protocol to allow a read of any registers on the drive until UDMA has completed.
Yes, technically a buggy host design...
Indeed, which is why I suspect that a global change to handle this controller may not be a good idea. Overriding sff_exec_command for this driver to not do the altstatus read (just wait 600ns, maybe) might be the best solution for now.
Quoted 4 lines
Indeed, which is why I suspect that a global change to handle this controller may not be a good idea. Overriding
sff_exec_commandfor this driver to not do the altstatus read (just wait 600ns, maybe) might be the best solution for now.
Old IDE does 400nS and no read. We know that works. At least for all the standard PATA controllers. Thats an incredibly strong argument for doing command issue that way for non MMIO controllers.
The MMIO case is muddy and probably simply does not matter because no sane hardware implementation is not going to handle it internally.
Alan
Quoted 8 lines · Alan Cox
Quoted 4 lines
Indeed, which is why I suspect that a global change to handle this controller may not be a good idea. Overriding
sff_exec_commandfor this driver to not do the altstatus read (just wait 600ns, maybe) might be the best solution for now.Old IDE does 400nS and no read. We know that works. At least for all the standard PATA controllers. Thats an incredibly strong argument for doing command issue that way for non MMIO controllers.
To which old-IDE do you refer? :) This specific area of functionality varies from 2.4.x to 2.6.x. Old-Old-IDE even picks and chooses whether or not to wait depending on protocol (PIO or not), something that has changed in the more recent old-IDE code.
Quoted 2 lines
The MMIO case is muddy and probably simply does not matter because no sane hardware implementation is not going to handle it internally.
It's not muddy, the rules for MMIO are quite clear, as is kernel practice for drivers driving MMIO-based hardware.
Any PATA MMIO controller on a PCI card -- pata_pdc2027x comes to mind -- must handle this situation, because the PCI posting situation varies depending on where you slot in your PCI card. That's not something a controller can really handle internally.
Jeff
Quoted 4 lines
To which old-IDE do you refer? :) This specific area of functionality varies from 2.4.x to 2.6.x. Old-Old-IDE even picks and chooses whether or not to wait depending on protocol (PIO or not), something that has changed in the more recent old-IDE code.
Which is irrelevant to the discussion.
Quoted 2 lines
It's not muddy, the rules for MMIO are quite clear, as is kernel practice for drivers driving MMIO-based hardware.
The IDE layer shows the SI3112 doesn't have this problem so we shouldn't cripple it with Jeff paranoia.
Quoted 4 lines
Any PATA MMIO controller on a PCI card --
pata_pdc2027xcomes to mind -- must handle this situation, because the PCI posting situation varies depending on where you slot in your PCI card. That's not something a controller can really handle internally.
Of course it can. The controller can implement a 400nS delay internally if it gets a back to back command write and a read of a tf register state. Nobody implementing an MMIO controller is going to not do that because avoiding an artificial posting stall is a big performance win. Compared to the big chunk of logic they all have for fifo draining interlocks its nothing. Without that its going to be cheaper to operate PIO cycles not MMIO cycles !
My SI controllers at least don't seem to need any kind of posting protection in MMIO mode. They seem to be handling it all internally somehow - Try rdtsc/write to cmd/rdtsc/read status/rdtsc
Alan
Quoted 4 lines · Robert Hancock
Indeed, which is why I suspect that a global change to handle this controller may not be a good idea. Overriding
sff_exec_commandfor this driver to not do the altstatus read (just wait 600ns, maybe) might be the best solution for now.
Well, I think it's fair to say that this should be applied more generally than just one driver. It is definitely conceivable that reading AltStatus causes a problem with more than one host controller, even though the ATA7 and ATA8 state diagrams recommends doing so.
One solution could be taken from the old-IDE driver: handle things at the other end of the transaction. Audit the paths that check status at the end of a drive command, rather than focusing on the start of the transaction.
That is where the "hard requirements" exist, anyway. ATA requires us to
* wait 400ns before reading Status, and/or
* wait one PIO cycle before reading Status
And logically, we only need to read Status at the end of the transaction, or when handling a non-DMA PCI shared interrupt.
Jeff
Quoted 2 lines
Without AltStatus, you would not be able to check and see if a command is complete... pretty much by definition you should able to access that
Correct - and you may not make that check for 400nS after the command is issued. The old IDE code enforces this (I know because I added it to fix real problems as PC's got fast enough to go from write to IRQ in 400nS)
Quoted 3 lines
It is certainly conceivable that this is a problem on a rare controller or two, but in general, AltStatus is how you poll for DMA completion. You are allowed to hit it during a DMA transfer.
But not for 400nS after a command is issued, or on certain old controllers during a DMA transfer before you halt it.
Quoted 4 lines
HOWEVER, accessing AltStatus prior to the 400ns post-exec-command period is potentially an area of undefined behavior. Changing this code is only a problem for MMIO-based controllers, which use the AltStatus read to guarantee that the 400ns delay occurs outside any posted writes.
Certainly for pio interfaces we should use ndelay(400). All our MMIO controllers are SATA and that's a different world anyway, including the fact that an altstatus read isn't going to produce a timing delay ! Basically we know ndelay(400) works.
SIL680 is MMIO in the old IDE stack but the hardware appears to be doing the required magic itself. In fact as it can stack two commands in non LBA48 mode (a feature we don't use) it would seem to have a fairly complete internal state machine.
Alan
Quoted 19 lines · Alan Cox
Quoted 2 lines
Without AltStatus, you would not be able to check and see if a command is complete... pretty much by definition you should able to access that
Correct - and you may not make that check for 400nS after the command is issued. The old IDE code enforces this (I know because I added it to fix real problems as PC's got fast enough to go from write to IRQ in 400nS)
Quoted 3 lines
It is certainly conceivable that this is a problem on a rare controller or two, but in general, AltStatus is how you poll for DMA completion. You are allowed to hit it during a DMA transfer.
But not for 400nS after a command is issued, or on certain old controllers during a DMA transfer before you halt it.
Quoted 4 lines
HOWEVER, accessing AltStatus prior to the 400ns post-exec-command period is potentially an area of undefined behavior. Changing this code is only a problem for MMIO-based controllers, which use the AltStatus read to guarantee that the 400ns delay occurs outside any posted writes.
Quoted 4 lines
Certainly for pio interfaces we should use ndelay(400). All our MMIO controllers are SATA and that's a different world anyway, including the fact that an altstatus read isn't going to produce a timing delay ! Basically we know ndelay(400) works.
I think you misunderstood the "Changing this code..." sentence. I make no claim, nor is there code, that says AltStatus produces the necessary 400ns delay.
To rephrase/repeat, the AltStatus read in ata_sff_pause() exists to ensure that any previous posted MMIO writes -- most notably the taskfile registers we just wrote -- are flushed from CPU and PCI bridges etc. to the ATA host controller chip.
After the AltStatus read is executed, the delay is executed. Here is the libata implementation, in its entirety:
void ata_sff_pause(struct ata_port *ap)
{
ata_sff_sync(ap); /* dummy AltStatus register read */
ndelay(400);
}On MMIO, you cannot guarantee that ndelay(400) occurs after the ATA Command register is written, without some method of ensuring that all posted writes are flushed. Typically, this is done by a dummy register read of some worthless register -- AltStatus was used in this case.
Absent that, ndelay(400) could occur in _parallel_ with the posted taskfile writes, essentially eliminating the spec-required delay.
Thus,
(1a) The solution for non-MMIO controllers is obvious and easy: remove the dummy AltStatus register read, as Russell has done in his test patch.
(1b) The solution for MMIO controllers is a bit more complex: replace the dummy AltStatus register read with something else.
Quoted 2 lines
SIL680 is MMIO in the old IDE stack but the hardware appears to be doing the required magic itself.
You cannot draw such a conclusion from looking only at the SIL680 chip.
SIL680 might easily be behind a PCI bridge.
Jeff
Quoted 2 lines
(1b) The solution for MMIO controllers is a bit more complex: replace the dummy AltStatus register read with something else.
If we had any SFF PATA controllers using MMIO. I can't find any. SATA is different anyway. In fact we probably want to avoid such delays on a pure SATA controller.
Quoted 5 lines
Quoted 2 lines
SIL680 is MMIO in the old IDE stack but the hardware appears to be doing the required magic itself.
You cannot draw such a conclusion from looking only at the SIL680 chip.
SIL680 might easily be behind a PCI bridge.
Not the one I have, nor it appears have there ever been any reports of the problem on any system in years. That problem would show up if it exists. It's also necessary that the chip can do its own timings in some of its fancier modes.
The person who has the problem to worry about is Ben, but the older PMAC stuff has its own implementation of all the DMA/IRQ/Command stuff as far as I can see so its not relevant there.
Several years and zillions of users say there isn't a problem. The fact we only enable the SIL680 mmio on libata also means its a rather irrelevant discussion in the first place.
Quoted 6 lines · Alan Cox
Quoted 2 lines
(1b) The solution for MMIO controllers is a bit more complex: replace the dummy AltStatus register read with something else.
If we had any SFF PATA controllers using MMIO. I can't find any. SATA is different anyway. In fact we probably want to avoid such delays on a pure SATA controller.
Early SATA controllers are just PATA controllers in disguise. All SFF controllers want that 400ns delay. The 400ns delay should -not- be avoided.
Because several SATA controllers are SFF and use the code in question, the MMIO issue is relevant for the code change, even if it is irrelevant to drivers/ata/pata_*.c.
Jeff
Quoted 11 lines · Jeff Garzik
Quoted 6 lines · Alan Cox
Quoted 2 lines
(1b) The solution for MMIO controllers is a bit more complex: replace the dummy AltStatus register read with something else.
If we had any SFF PATA controllers using MMIO. I can't find any. SATA is different anyway. In fact we probably want to avoid such delays on a pure SATA controller.
Early SATA controllers are just PATA controllers in disguise. All SFF controllers want that 400ns delay. The 400ns delay should -not- be avoided.
Note that ICH5 SATA is SFF, which only offers non-MMIO addressing; the change I made is running there just fine:
libata version 3.00 loaded.pata_acpi 0000:00:1f.1: PCI INT A -> GSI 18 (level, low) -> IRQ 18 pata_acpi 0000:00:1f.1: setting latency timer to 64pata_acpi 0000:00:1f.1: PCI INT A disabledpata_acpi 0000:00:1f.2: PCI INT A -> GSI 18 (level, low) -> IRQ 18 pata_acpi 0000:00:1f.2: setting latency timer to 64pata_acpi 0000:00:1f.2: PCI INT A disabled
input: PS/2 Logitech Mouse as /devices/platform/i8042/serio1/input/input3 ata_piix 0000:00:1f.1: version 2.13ata_piix 0000:00:1f.1: PCI INT A -> GSI 18 (level, low) -> IRQ 18 ata_piix 0000:00:1f.1: setting latency timer to 64
scsi0 : ata_piix
scsi1 : ata_piix
ata1: PATA max UDMA/100 cmd 0x1f0 ctl 0x3f6 bmdma 0xfc00 irq 14
ata2: PATA max UDMA/100 cmd 0x170 ctl 0x376 bmdma 0xfc08 irq 15ata_piix 0000:00:1f.2: PCI INT A -> GSI 18 (level, low) -> IRQ 18 ata_piix 0000:00:1f.2: MAP [ P0 -- P1 -- ]ata_piix 0000:00:1f.2: setting latency timer to 64
scsi2 : ata_piix
scsi3 : ata_piix
ata3: SATA max UDMA/133 cmd 0xd800 ctl 0xd400 bmdma 0xc800 irq 18
ata4: SATA max UDMA/133 cmd 0xd000 ctl 0xcc00 bmdma 0xc808 irq 18 ata4.00: ATA-7: MAXTOR STM3160811AS, 3.AAE, max UDMA/133
ata4.00: 312581808 sectors, multi 16: LBA48 NCQ (depth 0/32)
ata4.00: configured for UDMA/133
scsi 3:0:0:0: Direct-Access ATA MAXTOR STM316081 3.AA PQ: 0 ANSI: 5
sd 3:0:0:0: [sda] 312581808 512-byte logical blocks: (160 GB/149 GiB)
sd 3:0:0:0: [sda] Write Protect is off
sd 3:0:0:0: [sda] Mode Sense: 00 3a 00 00
sd 3:0:0:0: [sda] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA
sda: sda1 sda2 sda3
sd 3:0:0:0: [sda] Attached SCSI disk
Signature · 3 lines
Russell King Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of:
Quoted 9 lines · Russell King
Quoted 8 lines · Jeff Garzik
Quoted 6 lines · Alan Cox
Quoted 2 lines
(1b) The solution for MMIO controllers is a bit more complex: replace the dummy AltStatus register read with something else.
If we had any SFF PATA controllers using MMIO. I can't find any. SATA is different anyway. In fact we probably want to avoid such delays on a pure SATA controller.
Quoted 6 lines
Quoted 3 lines
Early SATA controllers are just PATA controllers in disguise. All SFF controllers want that 400ns delay. The 400ns delay should -not- be avoided.
Note that ICH5 SATA is SFF, which only offers non-MMIO addressing; the change I made is running there just fine:
Yes, your change should be fine for all non-MMIO SFF controllers, SATA or PATA.
The AltStatus read is simply a dummy read, for MMIO controllers, to ensure the previously-written taskfile registers make it to the controller before the ndelay() begins execution.
Jeff
Quoted 17 lines · Jeff Garzik
Quoted 9 lines · Russell King
Quoted 8 lines · Jeff Garzik
Quoted 6 lines · Alan Cox
Quoted 2 lines
(1b) The solution for MMIO controllers is a bit more complex: replace the dummy AltStatus register read with something else.
If we had any SFF PATA controllers using MMIO. I can't find any. SATA is different anyway. In fact we probably want to avoid such delays on a pure SATA controller.
Quoted 6 lines
Quoted 3 lines
Early SATA controllers are just PATA controllers in disguise. All SFF controllers want that 400ns delay. The 400ns delay should -not- be avoided.
Note that ICH5 SATA is SFF, which only offers non-MMIO addressing; the change I made is running there just fine:
Quoted 6 lines
Yes, your change should be fine for all non-MMIO SFF controllers, SATA or PATA.
The AltStatus read is simply a dummy read, for MMIO controllers, to ensure the previously-written taskfile registers make it to the controller before the
ndelay()begins execution.
Can we solve the problem by doing a read of the BMDMA status register?
As it is, the command register and alt status registers are in different BARs on the controller, so reading from BAR 4 instead of 1/3 should have the same effect?
Signature · 3 lines
Russell King Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of:
Quoted 25 lines · Russell King
Quoted 17 lines · Jeff Garzik
Quoted 9 lines · Russell King
Quoted 8 lines · Jeff Garzik
Quoted 6 lines · Alan Cox
Quoted 2 lines
(1b) The solution for MMIO controllers is a bit more complex: replace the dummy AltStatus register read with something else.
If we had any SFF PATA controllers using MMIO. I can't find any. SATA is different anyway. In fact we probably want to avoid such delays on a pure SATA controller.
Quoted 6 lines
Quoted 3 lines
Early SATA controllers are just PATA controllers in disguise. All SFF controllers want that 400ns delay. The 400ns delay should -not- be avoided.
Note that ICH5 SATA is SFF, which only offers non-MMIO addressing; the change I made is running there just fine:
Quoted 6 lines
Yes, your change should be fine for all non-MMIO SFF controllers, SATA or PATA.
The AltStatus read is simply a dummy read, for MMIO controllers, to ensure the previously-written taskfile registers make it to the controller before the
ndelay()begins execution.
Quoted 5 lines
Can we solve the problem by doing a read of the BMDMA status register?
As it is, the command register and alt status registers are in different BARs on the controller, so reading from BAR 4 instead of 1/3 should have the same effect?
Correct -- it does not matter which controller register is read, to ensure the MMIO flush.
Standard MMIO flush / PCI posting rules, nothing unique to PATA at all really. You can see examples of MMIO flushes all over the tree, such as the cpw8_f() and cpw32_f() macros in drivers/net/8139cp.c.
Jeff
Quoted 2 lines
Early SATA controllers are just PATA controllers in disguise. All SFF controllers want that 400ns delay. The 400ns delay should -not- be avoided.
Its a signalling protocol requirement for PATA or a PATA controller with a PATA/SATA bridge. What makes you think a SATA controller cares one iota ?
Quoted 3 lines
Because several SATA controllers are SFF and use the code in question, the MMIO issue is relevant for the code change, even if it is irrelevant to
drivers/ata/pata_*.c.
and to SATA. It's simply not relevant at a protocol level to SATA which is message passing anyway.
Quoted 6 lines · Alan Cox
Quoted 2 lines
Early SATA controllers are just PATA controllers in disguise. All SFF controllers want that 400ns delay. The 400ns delay should -not- be avoided.
Its a signalling protocol requirement for PATA or a PATA controller with a PATA/SATA bridge. What makes you think a SATA controller cares one iota ?
Because first gen SATA controllers, notably the Promise ones, were often just the same old PATA silicon logic with additional PATA<->SATA translation glue logic, rather than a pure SATA SFF implementation.
Quoted 6 lines
Quoted 3 lines
Because several SATA controllers are SFF and use the code in question, the MMIO issue is relevant for the code change, even if it is irrelevant to
drivers/ata/pata_*.c.and to SATA. It's simply not relevant at a protocol level to SATA which is message passing anyway.
It is not relevant to FIS-based SATA (SiI 3124, AHCI), sure. But SFF is not FIS-based SATA.
Jeff
Quoted 1 line · Alan Cox
If we had any SFF PATA controllers using MMIO. I can't find any.
Well,
[jgarzik@bd linux-2.6]$ grep -l ATA_FLAG_MMIO drivers/ata/pata_*.c drivers/ata/pata_at32.cdrivers/ata/pata_bf54x.cdrivers/ata/pata_ixp4xx_cf.cdrivers/ata/pata_macio.cdrivers/ata/pata_octeon_cf.cdrivers/ata/pata_palmld.cdrivers/ata/pata_pdc2027x.cdrivers/ata/pata_rb532_cf.cdrivers/ata/pata_scc.c
plus drivers/ata/pata_platform.c
Quoted 56 lines · Russell King
Quoted 31 lines · Alan Cox
Quoted 8 lines
1. There is no way for the 247 to see any configuration settings; the only thing it can see are the taskfile reads and writes, and the timing of the DMA signals from the 246.
2. The 246 timings for MWDMA2 and UDMA0 are identical; there is no programming difference between these two modes. The only way the 246 can know that UDMA is selected is by looking for the SET FEATURES command to the drive.
The 2026x certainly snoops SET FEATURES so that would be a reasonable assumption.
Quoted 4 lines
I've tried changing the set xfermode code to use a version of
ata_exec_internal()which doesn't return the taskfile, but this makes no difference to the promise exploding with CRC errors with UDMA writes. Is it possible to do a similar thing with IDENTIFY?No because you need to know if it worked.
Quoted 3 lines
Also, is it possible to get rid of the additional identify and read native max address commands which seem to be repeated (command register writes listed):
You can turn off Host Protected Area support for this. You could also btw turn *on* host protected area for the IDE stack and see what occurs but I imagine its a red herring anyway. If the snoop is failing then it is more likely to be that the chip has some limitations on the taskfile snooping and perhaps requires that the device register is always written or that the registers are written in a specific order when writing the set features command.
Quoted 3 lines
Another thing to try if that fails is using a polled set features in case the chip has problems in that area. We've seen a similar bug on some older VIA devices.
Found the problem - getting rid of the read of the alt status register after the command has been written fixes the UDMA CRC errors on write:
(quoted diff) +2
-1@@ -676,7 +676,8 @@ void ata_sff_exec_command(struct ata_port *ap, const struct ata_taskfile *tf) DPRINTK("ata%u: cmd 0x%X\n", ap->print_id, tf->command); iowrite8(tf->command, ap->ioaddr.command_addr); - ata_sff_pause(ap); + ndelay(400); +// ata_sff_pause(ap); } EXPORT_SYMBOL_GPL(ata_sff_exec_command);This rather makes sense. The PDC20247 handles the UDMA part of the protocol. It has no way to tell the PDC20246 to wait while it suspends UDMA, so that a normal register access can take place - the 246 ploughs on with the register access without any regard to the state of the 247.
Quoted 4 lines
If the drive immediately starts the UDMA protocol after a write to the command register (as it probably will for the DMA WRITE command), then we'll be accessing the taskfile in the middle of the UDMA setup, which can't be good. It's certainly a violation of the ATA specs.
Great debugging work!
It is very likely that your fix is also valid for UDMA problems reported for later PDC2026x UDMA66 chips and libata..
--
Bartlomiej Zolnierkiewicz
Quoted 4 lines · Bartlomiej Zolnierkiewicz
Great debugging work!
It is very likely that your fix is also valid for UDMA problems reported for later PDC2026x UDMA66 chips and libata..
Possibly...
But, unrelated to this thread, I have a question for you. While digging through the changes to find out why drivers/ide broke on PDC20246, I noticed dff8817.
This change is bogus.
writeb(0, base + ICS_ARCIN_V6_INTROFFSET_1);
readb(base + ICS_ARCIN_V6_INTROFFSET_2);
writeb(0, base + ICS_ARCIN_V6_INTROFFSET_2);
readb(base + ICS_ARCIN_V6_INTROFFSET_1);This sequence of code does:
1. enable interrupt 1
2. disable interrupt 2
3. enable interrupt 2
4. disable interrupt 1
which results in the interrupt for the second channel being enabled - leaving channel 1 blocked.
Firstly, icside shares its two IDE channels with one DMA engine - so it's a simplex interface. IDE supports those (or did when the code was written) serializing requests between the two interfaces. libata does not.
Secondly, the interrupt lines on icside float when there's no drive connected or when the drive has its NIEN bit set, which means that you get spurious screaming interrupts which can kill off all expansion card interrupts on the machine unless you disable the channel interrupt on the card.
Since libata can not serialize the operation of the two channels like IDE can, the libata version of the icside driver does not contain the interrupt stearing logic. Instead, it looks at the status after reset, and if nothing was found on that channel, it masks the interrupt from that channel.
In this respect, the IDE driver is more fully featured than the libata driver - the IDE driver supports DMA on both channels, whereas the libata version only supports DMA on one channel.
So, your change which was "inspired by pata_icside" is plain wrong. Please revert it.
Signature · 3 lines
Russell King Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of:
Quoted 3 lines
Firstly, icside shares its two IDE channels with one DMA engine - so it's a simplex interface. IDE supports those (or did when the code was written) serializing requests between the two interfaces. libata does not.
Libata does the driver may not.
Quoted 5 lines · Alan Cox
Quoted 3 lines
Firstly, icside shares its two IDE channels with one DMA engine - so it's a simplex interface. IDE supports those (or did when the code was written) serializing requests between the two interfaces. libata does not.
Libata does the driver may not.
libata certainly didn't when I wrote the driver in 2007.
int ata_do_set_mode(struct ata_link *link, struct ata_device **r_failed_dev)
{
/* Record simplex status. If we selected DMA then the other
* host channels are not permitted to do so.
*/
if (used_dma && (ap->host->flags & ATA_HOST_SIMPLEX))
ap->host->simplex_claimed = ap;...
static void ata_dev_xfermask(struct ata_device *dev) {
...
if ((host->flags & ATA_HOST_SIMPLEX) &&
host->simplex_claimed && host->simplex_claimed != ap) {
xfer_mask &= ~(ATA_MASK_MWDMA | ATA_MASK_UDMA);
ata_dev_printk(dev, KERN_WARNING, "simplex DMA is claimed by "
"other device, disabling DMA\n");
}and apparantly still doesn't. Only one channel of a simplex device is permitted to be configured for DMA at any one time.
Signature · 3 lines
Russell King Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of:
Quoted 21 lines
libata certainly didn't when I wrote the driver in 2007.
int
ata_do_set_mode(struct ata_link *link, struct ata_device **r_failed_dev){ /* Record simplex status. If we selected DMA then the other * host channels are not permitted to do so. */ if (used_dma && (ap->host->flags & ATA_HOST_SIMPLEX)) ap->host->simplex_claimed = ap;...
static voidata_dev_xfermask(struct ata_device *dev){
...if ((host->flags & ATA_HOST_SIMPLEX) && host->simplex_claimed && host->simplex_claimed != ap) { xfer_mask &= ~(ATA_MASK_MWDMA | ATA_MASK_UDMA); ata_dev_printk(dev, KERN_WARNING, "simplex DMA is claimed by " "other device, disabling DMA\n"); }
Quoted 2 lines
and apparantly still doesn't. Only one channel of a simplex device is permitted to be configured for DMA at any one time.
Simplex has a specific meaning and we implement it to the meaning of the standard PCI config bits, which is that you have a single channel that can be configured for DMA. There is nothing in the standard that says you can play fast and loose flipping them back and forth.
If your controller simply can't handle two DMA transactions (or two transactions in general) at the same time you can provide a private qc_defer function and ask the midlayer to defer commands that don't fit your needs.
Ie the simplex handling is like the Simplex bit handling in drivers/ide, and the qc_defer method is akin to the ->serialize flag in drivers/ide, but a bit more flexible.
Alan
Quoted 5 lines · Alan Cox
Quoted 3 lines
Firstly, icside shares its two IDE channels with one DMA engine - so it's a simplex interface. IDE supports those (or did when the code was written) serializing requests between the two interfaces. libata does not.
Libata does the driver may not.
IIRC it didn't at the time pata_icside was written.
--
Bartlomiej Zolnierkiewicz
Quoted 56 lines · Russell King
Quoted 31 lines · Alan Cox
Quoted 8 lines
1. There is no way for the 247 to see any configuration settings; the only thing it can see are the taskfile reads and writes, and the timing of the DMA signals from the 246.
2. The 246 timings for MWDMA2 and UDMA0 are identical; there is no programming difference between these two modes. The only way the 246 can know that UDMA is selected is by looking for the SET FEATURES command to the drive.
The 2026x certainly snoops SET FEATURES so that would be a reasonable assumption.
Quoted 4 lines
I've tried changing the set xfermode code to use a version of
ata_exec_internal()which doesn't return the taskfile, but this makes no difference to the promise exploding with CRC errors with UDMA writes. Is it possible to do a similar thing with IDENTIFY?No because you need to know if it worked.
Quoted 3 lines
Also, is it possible to get rid of the additional identify and read native max address commands which seem to be repeated (command register writes listed):
You can turn off Host Protected Area support for this. You could also btw turn *on* host protected area for the IDE stack and see what occurs but I imagine its a red herring anyway. If the snoop is failing then it is more likely to be that the chip has some limitations on the taskfile snooping and perhaps requires that the device register is always written or that the registers are written in a specific order when writing the set features command.
Quoted 3 lines
Another thing to try if that fails is using a polled set features in case the chip has problems in that area. We've seen a similar bug on some older VIA devices.
Found the problem - getting rid of the read of the alt status register after the command has been written fixes the UDMA CRC errors on write:
(quoted diff) +2
-1@@ -676,7 +676,8 @@ void ata_sff_exec_command(struct ata_port *ap, const struct ata_taskfile *tf) DPRINTK("ata%u: cmd 0x%X\n", ap->print_id, tf->command); iowrite8(tf->command, ap->ioaddr.command_addr); - ata_sff_pause(ap); + ndelay(400); +// ata_sff_pause(ap); } EXPORT_SYMBOL_GPL(ata_sff_exec_command);This rather makes sense. The PDC20247 handles the UDMA part of the protocol. It has no way to tell the PDC20246 to wait while it suspends UDMA, so that a normal register access can take place - the 246 ploughs on with the register access without any regard to the state of the 247.
Quoted 4 lines
If the drive immediately starts the UDMA protocol after a write to the command register (as it probably will for the DMA WRITE command), then we'll be accessing the taskfile in the middle of the UDMA setup, which can't be good. It's certainly a violation of the ATA specs.
I don't think it is. From ATA-8 APT:
"HDMA0: Check_Status State: This state is entered when the host has written a DMA command to the device; when all data for the command has been transferred and nIEN is set to one; or when all data for the command has been transferred, nIEN is cleared zero, and INTRQ has been asserted.
When in this state, the host shall read the device Status register. When entering this state from the HI4 state, the host shall wait 400 ns before reading the Status register. When entering this state from the HDMA1 state, the host shall wait one PIO transfer cycle time before reading the Status register. The wait may be accomplished by reading the Alternate Status register and ignoring the result."
The last sentence is basically what the code in ata_sff_pause is doing..
Quoted 8 lines · Robert Hancock
rom ATA-8 APT:
"HDMA0:
Check_StatusState: This state is entered when the host has written a DMA command to the device; when all data for the command has been transferred and nIEN is set to one; or when all data for the command has been transferred, nIEN is cleared zero, and INTRQ has been asserted.
Quoted 7 lines
When in this state, the host shall read the device Status register. When entering this state from the HI4 state, the host shall wait 400 ns before reading the Status register. When entering this state from the HDMA1 state,
the host shall wait one PIO transfer cycle time before reading the Status register. The wait may be accomplished by reading the Alternate Status register and ignoring the result."
Indeed... I had forgotten about that bit.
Jeff
Quoted 1 line · Robert Hancock
I don't think it is. From ATA-8 APT:
Now go and look at the drive state diagram rather than the host state diagram.
Signature · 3 lines
Russell King Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/ maintainer of:
Quoted 5 lines · Russell King
Quoted 1 line · Robert Hancock
I don't think it is. From ATA-8 APT:
Now go and look at the drive state diagram rather than the host state diagram.
What about it are you referring to?