2.6.32: Promise UDMA33 card refuses to work in UDMA mode

2.6.32: Promise UDMA33 card refuses to work in UDMA mode

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

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 00
end_request: I/O error, dev sda, sector 717503
ata1: EH complete
end_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 00
end_request: I/O error, dev sdb, sector 717503
ata2: EH complete
end_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.23-dump	2009-12-24 17:43:29.000000000 +0000
+++ promise-2.6.32-dump	2009-12-24 17:15:00.000000000 +0000
@@ -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.)

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Russell King]

Some additions due to further testing:

On Thu, Dec 24, 2009 at 06:13:00PM +0000, Russell King wrote:
> 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.

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

diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index bbbb1fa..ddd275a 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -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:

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

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

  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.

On Thu, Dec 24, 2009 at 09:54:51PM +0000, Russell King wrote:
> Some additions due to further testing:
> 
> On Thu, Dec 24, 2009 at 06:13:00PM +0000, Russell King wrote:
> > 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.
> 
> > 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:
> 
> diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
> index bbbb1fa..ddd275a 100644
> --- a/drivers/ata/libata-sff.c
> +++ b/drivers/ata/libata-sff.c
> @@ -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:

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Robert Hancock]

On 01/02/2010 06:23 PM, Russell King wrote:
> 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.

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

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

>
> - 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.
>
> On Thu, Dec 24, 2009 at 09:54:51PM +0000, Russell King wrote:
>> Some additions due to further testing:
>>
>> On Thu, Dec 24, 2009 at 06:13:00PM +0000, Russell King wrote:
>>> 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.
>>
>>> 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:
>>
>> diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
>> index bbbb1fa..ddd275a 100644
>> --- a/drivers/ata/libata-sff.c
>> +++ b/drivers/ata/libata-sff.c
>> @@ -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:
>

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Russell King]

On Sat, Jan 02, 2010 at 09:08:00PM -0600, Robert Hancock wrote:
> 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.

>>    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 status
iowrite8(a0, 0001b406)		write drive
ioread8(0001b006) = 50		read alt status
ioread8(0001b407) = 50		read status
iowrite8(0a, 0001b006)		write ctrl
ioread8(0001b407) = 50		read status
iowrite8(03, 0001b401)		write taskfile
iowrite8(42, 0001b402)
iowrite8(00, 0001b403)
iowrite8(00, 0001b404)
iowrite8(00, 0001b405)
iowrite8(a0, 0001b406)
ioread8(0001b407) = 50		read status
iowrite8(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) = 50
iowrite8(08, 0001b006)		write ctrl
ioread8(0001b407) = 50		read status
ioread8(0001ac0a) = 60		read sff dma status
iowrite8(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 control
iowrite8(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.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Alan Cox]

> 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] Fix Promise UDMA33 IDE driver (pdc202xx_old)

[Russell King]

On Sun, Jan 03, 2010 at 12:23:14AM +0000, Russell King wrote:
> - 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 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.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
Tested-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
 drivers/ide/pdc202xx_old.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ide/pdc202xx_old.c b/drivers/ide/pdc202xx_old.c
index cb812f3..f5c08f4 100644
--- a/drivers/ide/pdc202xx_old.c
+++ b/drivers/ide/pdc202xx_old.c
@@ -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;
 	}
 }
 


-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Russell King]

On Sun, Jan 03, 2010 at 12:23:14AM +0000, Russell King wrote:
> 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?

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)	identify
iowrite8(f8, 0001b407)	read native max
iowrite8(ef, 0001b407)	set features
iowrite8(ec, 0001b407)	identify
iowrite8(f8, 0001b407)	read native max
iowrite8(c8, 0001b407)	read dma, succeeds
iowrite8(c8, 0001b407)
...
iowrite8(c8, 0001b407)
repeat {
iowrite8(ca, 0001b407)	write dma, fails
iowrite8(ec, 0001b407)	identify
iowrite8(f8, 0001b407)	read native max
iowrite8(ef, 0001b407)	set features
iowrite8(ec, 0001b407)	identify
iowrite8(f8, 0001b407)	read native max
} until PIO

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Alan Cox]

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

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

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

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Russell King]

On Mon, Jan 04, 2010 at 10:37:56AM +0000, Alan Cox wrote:
> > 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.
> 
> > 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.
> 
> > 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.

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.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Alan Cox]

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

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Jeff Garzik]

On 01/04/2010 08:30 AM, Russell King wrote:
> On Mon, Jan 04, 2010 at 10:37:56AM +0000, Alan Cox wrote:
>>> 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.
>>
>>> 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.
>>
>>> 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.
>
> 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.

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

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Jeff Garzik]

On 01/04/2010 10:16 AM, Alan Cox wrote:
>> 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

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Russell King]

On Mon, Jan 04, 2010 at 10:25:37AM -0500, Jeff Garzik wrote:
> On 01/04/2010 08:30 AM, Russell King wrote:
>> 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...
>
> 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...

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Russell King]

On Mon, Jan 04, 2010 at 10:32:55AM -0500, Jeff Garzik wrote:
> On 01/04/2010 10:16 AM, Alan Cox wrote:
>>> 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.

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.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Alan Cox]

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

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

> 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

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Alan Cox]

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

> 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 ;)

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Alan Cox]

> 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

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Russell King]

On Mon, Jan 04, 2010 at 03:55:44PM +0000, Alan Cox wrote:
> > 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.

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.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Bartlomiej Zolnierkiewicz]

On Monday 04 January 2010 02:30:24 pm Russell King wrote:
> On Mon, Jan 04, 2010 at 10:37:56AM +0000, Alan Cox wrote:
> > > 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.
> > 
> > > 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.
> > 
> > > 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.
> 
> 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.

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

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Jeff Garzik]

On 01/04/2010 10:46 AM, Alan Cox wrote:
>> 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)
>
>> 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.
>
>> 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.

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.


> 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

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Jeff Garzik]

On 01/04/2010 11:15 AM, Russell King wrote:
> 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.


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

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Alan Cox]

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

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

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Sergei Shtylyov]

Hello.

Russell King wrote:

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

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

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

    Yes.

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

> 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

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Jeff Garzik]

On 01/04/2010 12:02 PM, Alan Cox wrote:
>> (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

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Russell King]

On Mon, Jan 04, 2010 at 05:31:35PM +0100, Bartlomiej Zolnierkiewicz wrote:
> 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.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Russell King]

On Mon, Jan 04, 2010 at 12:27:54PM -0500, Jeff Garzik wrote:
> On 01/04/2010 12:02 PM, Alan Cox wrote:
>>> (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 64
pata_acpi 0000:00:1f.1: PCI INT A disabled
pata_acpi 0000:00:1f.2: PCI INT A -> GSI 18 (level, low) -> IRQ 18
pata_acpi 0000:00:1f.2: setting latency timer to 64
pata_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.13
ata_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 15
ata_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


-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Alan Cox]

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

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Alan Cox]

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

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Russell King]

On Mon, Jan 04, 2010 at 05:39:58PM +0000, Alan Cox wrote:
> > 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.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Bartlomiej Zolnierkiewicz]

On Monday 04 January 2010 06:39:58 pm Alan Cox wrote:
> > 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

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Jeff Garzik]

On 01/04/2010 12:30 PM, Russell King wrote:
> On Mon, Jan 04, 2010 at 12:27:54PM -0500, Jeff Garzik wrote:
>> On 01/04/2010 12:02 PM, Alan Cox wrote:
>>>> (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:

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

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Russell King]

On Mon, Jan 04, 2010 at 01:03:36PM -0500, Jeff Garzik wrote:
> On 01/04/2010 12:30 PM, Russell King wrote:
>> On Mon, Jan 04, 2010 at 12:27:54PM -0500, Jeff Garzik wrote:
>>> On 01/04/2010 12:02 PM, Alan Cox wrote:
>>>>> (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:
>
> 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?

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Jeff Garzik]

On 01/04/2010 12:38 PM, Alan Cox wrote:
>> 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.


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

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Alan Cox]

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

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

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Jeff Garzik]

On 01/04/2010 12:02 PM, Alan Cox wrote:
> 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.c
drivers/ata/pata_bf54x.c
drivers/ata/pata_ixp4xx_cf.c
drivers/ata/pata_macio.c
drivers/ata/pata_octeon_cf.c
drivers/ata/pata_palmld.c
drivers/ata/pata_pdc2027x.c
drivers/ata/pata_rb532_cf.c
drivers/ata/pata_scc.c

plus drivers/ata/pata_platform.c

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Jeff Garzik]

On 01/04/2010 01:06 PM, Russell King wrote:
> On Mon, Jan 04, 2010 at 01:03:36PM -0500, Jeff Garzik wrote:
>> On 01/04/2010 12:30 PM, Russell King wrote:
>>> On Mon, Jan 04, 2010 at 12:27:54PM -0500, Jeff Garzik wrote:
>>>> On 01/04/2010 12:02 PM, Alan Cox wrote:
>>>>>> (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:
>>
>> 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?

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

Re: [PATCH] Fix Promise UDMA33 IDE driver (pdc202xx_old)

[Sergei Shtylyov]

Hello.

Russell King wrote:

>>- 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 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;
> +       }
> +}

    Sigh, it was mine... :-/

> Reading the (documented as a 32-bit) system control register when the
> interface is idle gives: 0x01da110c

    Oh, you even have the docs... :-)

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

    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.

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

> 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 <rmk+kernel@arm.linux.org.uk>
> Tested-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
>  drivers/ide/pdc202xx_old.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ide/pdc202xx_old.c b/drivers/ide/pdc202xx_old.c
> index cb812f3..f5c08f4 100644
> --- a/drivers/ide/pdc202xx_old.c
> +++ b/drivers/ide/pdc202xx_old.c
> @@ -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;

>  	} 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: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Robert Hancock]

On 01/04/2010 07:30 AM, Russell King wrote:
> On Mon, Jan 04, 2010 at 10:37:56AM +0000, Alan Cox wrote:
>>> 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.
>>
>>> 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.
>>
>>> 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.
>
> 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.

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

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Robert Hancock]

On 01/04/2010 09:42 AM, Russell King wrote:
> On Mon, Jan 04, 2010 at 10:25:37AM -0500, Jeff Garzik wrote:
>> On 01/04/2010 08:30 AM, Russell King wrote:
>>> 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...
>>
>> 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).

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

Re: [PATCH] Fix Promise UDMA33 IDE driver (pdc202xx_old)

[David Miller]

From: Sergei Shtylyov <sshtylyov@ru.mvista.com>
Date: Mon, 04 Jan 2010 22:14:55 +0300

>    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: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Jeff Garzik]

On 01/04/2010 08:03 PM, Robert Hancock wrote:
> rom 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."


Indeed...  I had forgotten about that bit.

	Jeff

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Alan Cox]

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

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

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Jeff Garzik]

On 01/05/2010 06:25 AM, Alan Cox wrote:
>> 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.
>
> 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.


> 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

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Jeff Garzik]

On 01/04/2010 09:06 PM, Robert Hancock wrote:
> 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.


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

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Alan Cox]

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

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

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

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

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Russell King]

On Mon, Jan 04, 2010 at 07:03:49PM -0600, Robert Hancock wrote:
> 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.

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

[Russell King]

On Mon, Jan 04, 2010 at 10:26:58PM -0800, David Miller wrote:
> From: Sergei Shtylyov <sshtylyov@ru.mvista.com>
> Date: Mon, 04 Jan 2010 22:14:55 +0300
> 
> >    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?

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

[Sergei Shtylyov]

Hello.

Russell King wrote:

>>From: Sergei Shtylyov <sshtylyov@ru.mvista.com>
>>Date: Mon, 04 Jan 2010 22:14:55 +0300

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

    So what? The SFF status register will be read eventually.

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

> 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

Re: 2.6.32: Promise UDMA33 card refuses to work in UDMA mode

[Robert Hancock]

On Tue, Jan 5, 2010 at 11:44 AM, Russell King <rmk@arm.linux.org.uk> wrote:
> On Mon, Jan 04, 2010 at 07:03:49PM -0600, Robert Hancock wrote:
>> 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?