Re: ATA Patch #7 (Re: ATA Patch #6)

[Matthew Dillon <dillon@xxxxxxxxxxxxxxxxxxxx>]

:Ok, it may be too early to speak, but if I understand the documents
:correctly, the device control register should not be accessed when
:DMACK is assert -- that is, you can't disable device interrupt after
:ata_dmastart(), right? Here's a small modification to ATA Patch #7 
:that just worked(but slightly worse numbers from dd command compared
:to unpatched kernel) on Dynabook SS3500.
:- reduced DELAY()'s you added down to 1
:- move up ata_clear_interlock() before ata_dmastart()
:- add missing ata_clear_interlock() after calls to ata_command()
:  with ATA_WAIT_READY flag.
    
    Ooh.  I think you are onto something.  That makes a lot of sense.

    You need to add one thing, and that is to put a critical section
    around the clearing of the interlock and the DMA start so no interrupt
    can occur inbetween the two.

    Also, the command delay is mandatory... it's either in the specs
    (somewhere), or it had to be done some time in the past to support
    slow devices.  The selection DELAY (the first one) below can be reduced
    to 1us, but the command-start DELAY (the second one) cannot.

    Maybe we should make the command-start delay programmable with a
    sysctl.

					-Matt
					Matthew Dillon 
					<dillon@xxxxxxxxxxxxx>

:diff -u ata-7/ata-all.c ata/ata-all.c
:--- ata-7/ata-all.c	2004-11-28 21:59:02.000000000 +0900
:+++ ata/ata-all.c	2004-11-30 01:51:32.000000000 +0900
:@@ -1189,7 +1189,7 @@
:     crit_enter();
:     atadev->channel->flags |= ATA_INTERRUPT_INTERLOCK;
:     ATA_OUTB(atadev->channel->r_io, ATA_DRIVE, ATA_D_IBM | atadev->unit);
:-    DELAY(10);
:+    DELAY(1);
:     ATA_OUTB(atadev->channel->r_altio, ATA_ALTSTAT, ATA_A_IDS | ATA_A_4BIT);
:     crit_exit();
: 
:@@ -1269,7 +1269,7 @@
:      */
:     atadev->channel->active |= flags & (ATA_WAIT_INTR | ATA_WAIT_READY);
:     ATA_OUTB(atadev->channel->r_io, ATA_CMD, command);
:-    DELAY(10);
:+    DELAY(1);
: 
:     /*
:      * ATA_IMMEDIATE means that the caller is going to do more setup, do
:diff -u ata-7/ata-disk.c ata/ata-disk.c
:--- ata-7/ata-disk.c	2004-11-28 21:59:02.000000000 +0900
:+++ ata/ata-disk.c	2004-11-30 01:52:21.000000000 +0900
:@@ -265,6 +265,7 @@
:     if (flush) {
: 	if (ata_command(atadev, ATA_C_FLUSHCACHE, 0, 0, 0, ATA_WAIT_READY))
: 	    ata_prtdev(atadev, "flushing cache on detach failed\n");
:+	ata_clear_interlock(atadev);
:     }
:     if (adp->flags & AD_F_RAID_SUBDISK)
: 	ata_raiddisk_detach(adp);
:@@ -295,6 +296,7 @@
:     ATA_SLEEPLOCK_CH(adp->device->channel, ATA_CONTROL);
:     if (ata_command(adp->device, ATA_C_FLUSHCACHE, 0, 0, 0, ATA_WAIT_READY))
: 	ata_prtdev(adp->device, "flushing cache on close failed\n");
:+    ata_clear_interlock(adp->device);
:     ATA_UNLOCK_CH(adp->device->channel);
:     splx(s);
:     return 0;
:@@ -571,10 +573,10 @@
: 	     * clear the interrupt interlock and reenable interrupts.  It
: 	     * is possible that this will generate an immediate interrupt.
: 	     */
:+	    ata_clear_interlock(adp->device);
: 	    ata_dmastart(adp->device, request->data, request->bytecount,
: 			request->flags & ADR_F_READ);
: 	    ad_start_timeout(request);
:-	    ata_clear_interlock(adp->device);
: 	    return ATA_OP_CONTINUES;
: 	}
: 
:@@ -843,9 +845,9 @@
: 	    ad_invalidatequeue(adp, NULL);
: 	    return ATA_OP_FINISHED;
: 	}
:+	ata_clear_interlock(adp->device);
: 	ata_dmastart(adp->device, request->data, request->bytecount,
: 		    request->flags & ADR_F_READ);
:-	ata_clear_interlock(adp->device);
: 	return ATA_OP_CONTINUES;
:     }
:     return ATA_OP_FINISHED;
:@@ -881,6 +883,7 @@
: 	if (ata_command(adp->device, ATA_C_NOP,
: 			0, 0, ATA_C_F_FLUSHQUEUE, ATA_WAIT_READY))
: 	    ata_prtdev(adp->device, "flush queue failed\n");
:+	ata_clear_interlock(adp->device);
: 	adp->outstanding = 0;
:     }
: }
:@@ -993,6 +996,7 @@
: 		    ata_umode(adp->device->param));
:     else
: 	ata_dmainit(atadev, ata_pmode(adp->device->param), -1, -1);
:+    ata_clear_interlock(atadev);
: }
: 
: void
: