ultra5/cmd646 hang

Thomas Moestl t.moestl at tu-bs.de
Fri Nov 21 06:59:37 PST 2003


On Fri, 2003/11/21 at 08:46:13 +0100, Soren Schmidt wrote:
> It seems Thomas Moestl wrote:
> > I've played around some more with panther2, and managed to get it to work
> > seemingly stable in WDMA2 mode by tweaking the initialization code a bit.
> > I've attached the patch which I have used; the following changes in it
> > seem to all be required:
> > 
> > - Programming the timings before setting the transfer mode with
> >   ata_controlcmd(atadev, ATA_SETFEATURES, ATA_SF_SETXFER, ...);
> 
> Wierd, sounds like the machine doesn't set it up at all, which would
> make it hard to boot from ??
> 
> > - The added interrupt acking code in the chipset interrupt handler
> >   (cribbed from NetBSD)
> 
> That shouldn't be needed according to docs, but I'll look through the
> endless list of erratas for this..

This was a red herring, sorry. I inadvertently had did another change
when I was testing whether this is required.
 
> > - #if 0-ing out the code that sets the PIO timings. I have not
> >   yet investigated whether this is because of the PIO initialization
> >   of the disk before DMA is tried, or causes troubles when used
> >   for the secondary master, which is a PIO3 CD-ROM.
> 
> This all sounds screwed somehow, I've just upgraded my alpha to the
> latest -current and there the '646 works just fine as is...

It was indeed screwed; I have found the real reason for this behaviour
now, which was a simple operator precedence problem in ata_cmd_setmode():

	int treg = 0x54 + (devno < 3) ? (devno << 1) : 7;

This should, of course, be

	int treg = 0x54 + ((devno < 3) ? (devno << 1) : 7);

I had this right before my eyes for hours yesterday, and just didn't
notice it. I'm feeling quite stupid now.
The result was of course that the PIO/WDMA timings were written into
entirely different PCI registers. For primary master and slave (0
and 1) and secondary slave (3) this was mostly harmless (targets
were the PCI device, vendor and status registers), but for the
secondary master, the write went into the the PCI command register,
and for a PIO3 device the timing value was such that this write
cleared the port and busmaster enable bits, which the chip naturally
did not like.
Exchanging the setmode command and the timing register programming
is also not required any more with this fix, though I am not sure
why it helped at all before (in this case, I am positive that it was
needed).

The issue with ATA_BMSTAT_INTERRUPT that I reported remains. While
it seems to be set correctly to indicate completion of DMA operations,
it is clear for interrupts which pertain to non-DMA events.

I have attached an updated patch.

	- Thomas

-- 
Thomas Moestl <t.moestl at tu-bs.de>	http://www.tu-bs.de/~y0015675/
              <tmm at FreeBSD.org>		http://people.FreeBSD.org/~tmm/
PGP fingerprint: 1C97 A604 2BD0 E492 51D0  9C0F 1FE6 4F1D 419C 776C
-------------- next part --------------
Index: ata-chipset.c
===================================================================
RCS file: /vol/ncvs/src/sys/dev/ata/ata-chipset.c,v
retrieving revision 1.46
diff -u -r1.46 ata-chipset.c
--- ata-chipset.c	18 Nov 2003 15:27:28 -0000	1.46
+++ ata-chipset.c	21 Nov 2003 14:41:09 -0000
@@ -101,6 +101,7 @@
 static int ata_sii_mio_allocate(device_t, struct ata_channel *);
 static void ata_sii_intr(void *);
 static void ata_cmd_intr(void *);
+static void ata_nobmintr_intr(void *);
 static void ata_sii_setmode(struct ata_device *, int);
 static void ata_cmd_setmode(struct ata_device *, int);
 static int ata_sis_chipinit(device_t);
@@ -1581,7 +1582,7 @@
      { ATA_CMD649,    0x00, 0,	      SIIINTR,   ATA_UDMA5, "CMD 649" },
      { ATA_CMD648,    0x00, 0,	      SIIINTR,   ATA_UDMA4, "CMD 648" },
      { ATA_CMD646,    0x07, 0,	      0,	 ATA_UDMA2, "CMD 646U2" },
-     { ATA_CMD646,    0x00, 0,	      0,	 ATA_WDMA2, "CMD 646" },
+     { ATA_CMD646,    0x00, 0,	      SIINOBMI,	 ATA_WDMA2, "CMD 646" },
      { 0, 0, 0, 0, 0, 0}};
     char buffer[64];
 
@@ -1599,6 +1600,7 @@
 ata_sii_chipinit(device_t dev)
 {
     struct ata_pci_controller *ctlr = device_get_softc(dev);
+    void (*ih)(void *);
     int rid = ATA_IRQ_RID;
 
     if (!(ctlr->r_irq = bus_alloc_resource(dev, SYS_RES_IRQ, &rid, 0, ~0, 1,
@@ -1637,10 +1639,14 @@
 	    ctlr->setmode = ata_sii_setmode;
     }
     else {
+	if (ctlr->chip->cfg2 & SIIINTR)
+	    ih = ata_cmd_intr;
+	else if (ctlr->chip->cfg2 & SIINOBMI)
+	    ih = ata_nobmintr_intr;
+	else
+	    ih = ata_generic_intr;
 	if ((bus_setup_intr(dev, ctlr->r_irq, ATA_INTR_FLAGS,
-			    ctlr->chip->cfg2 & SIIINTR ? 
-			    ata_cmd_intr : ata_generic_intr,
-			    ctlr, &ctlr->handle))) {
+			    ih, ctlr, &ctlr->handle))) {
 	    device_printf(dev, "unable to setup interrupt\n");
 	    return ENXIO;
 	}
@@ -1743,6 +1749,34 @@
 }
 
 static void
+ata_nobmintr_intr(void *data)
+{
+    struct ata_pci_controller *ctlr = data;
+    struct ata_channel *ch;
+    int unit;
+
+    /* implement this as a toggle instead to balance load XXX */
+    for (unit = 0; unit < 2; unit++) {
+	if (!(ch = ctlr->interrupt[unit].argument))
+	    continue;
+	/*
+	 * The CMD646 does not always seem to set ATA_BMSTAT_INTERRUPT.
+	 * Only check it when a transfer is active.
+	 */
+	if (ch->dma && (ch->dma->flags & ATA_DMA_ACTIVE)) {
+	    int bmstat = ATA_IDX_INB(ch, ATA_BMSTAT_PORT) & ATA_BMSTAT_MASK;
+
+	    if ((bmstat & (ATA_BMSTAT_ACTIVE | ATA_BMSTAT_INTERRUPT)) !=
+		ATA_BMSTAT_INTERRUPT)
+		continue;
+	    ATA_IDX_OUTB(ch, ATA_BMSTAT_PORT, bmstat & ~ATA_BMSTAT_ERROR);
+	    DELAY(1);
+	}
+	ctlr->interrupt[unit].function(ch);
+    }
+}
+
+static void
 ata_sii_setmode(struct ata_device *atadev, int mode)
 {
     device_t parent = device_get_parent(atadev->channel->dev);
@@ -1823,7 +1857,7 @@
 		   (error) ? "FAILURE " : "",
 		   ata_mode2str(mode), ctlr->chip->text);
     if (!error) {
-	int treg = 0x54 + (devno < 3) ? (devno << 1) : 7;
+	int treg = 0x54 + ((devno < 3) ? (devno << 1) : 7);
 	int ureg = atadev->channel->unit ? 0x7b : 0x73;
 
 	if (mode >= ATA_UDMA0) {	
Index: ata-pci.h
===================================================================
RCS file: /vol/ncvs/src/sys/dev/ata/ata-pci.h,v
retrieving revision 1.18
diff -u -r1.18 ata-pci.h
--- ata-pci.h	18 Nov 2003 15:27:28 -0000	1.18
+++ ata-pci.h	21 Nov 2003 14:09:54 -0000
@@ -255,6 +255,7 @@
 #define SIIMEMIO	1
 #define SIIINTR		0x01
 #define SIISETCLK	0x02
+#define SIINOBMI	0x04
 
 #define SIS_SOUTH	1
 #define SISSATA		2
Index: ata-queue.c
===================================================================
RCS file: /vol/ncvs/src/sys/dev/ata/ata-queue.c,v
retrieving revision 1.11
diff -u -r1.11 ata-queue.c
--- ata-queue.c	20 Oct 2003 14:28:37 -0000	1.11
+++ ata-queue.c	21 Nov 2003 00:21:00 -0000
@@ -316,6 +316,8 @@
 ata_timeout(struct ata_request *request)
 {
     struct ata_channel *ch = request->device->channel;
+    struct ata_device *reqdev = request->device;
+    char *reqstr = ata_cmd2str(request);
     int quiet = request->flags & ATA_R_QUIET;
 
     /* clear timeout etc */
@@ -324,10 +326,11 @@
     /* call hw.interrupt to try finish up the command */
     ch->hw.interrupt(request->device->channel);
     if (ch->running != request) {
+	/* request might already be freed - use copies. */
 	if (!quiet)
-	    ata_prtdev(request->device,
+	    ata_prtdev(reqdev,
 		       "WARNING - %s recovered from missing interrupt\n",
-		       ata_cmd2str(request));
+		       reqstr);
 	return;
     }
 


More information about the freebsd-sparc64 mailing list