Hi This patch (against the 2.4 BK tree as of yesterday evening) adds support for SGI IOC4 IDE driver as found in SN2/Altix systems (http://sgi.com/altix for more details). No generic/non-sn2 code paths should be affected by this. Documentation/Configure.help | 6 MAINTAINERS | 5 drivers/ide/Config.in | 1 drivers/ide/pci/Makefile | 1 drivers/ide/pci/sgiioc4.c | 922 +++++++++++++++++++++++++++++++++++ drivers/ide/pci/sgiioc4.h | 177 ++++++ include/linux/pci_ids.h | 1 7 files changed, 1113 insertions The driver has been changed in various places in accordance with the suggestions on this list. A detailed explanation of which suggestions could and couldnot be implemented follows : > > +static inline void > > +IDE_DBG(int debuglevel, char *format, ...) > > +{ > > + va_list ap; > > + char tempstr[256]; > > + > > + if (sgiioc4_debug >= debuglevel) { > > + va_start(ap, format); > > + vsprintf(tempstr, format, ap); > > + printk(tempstr); > > + va_end(ap); > > + } > > +} > > I'm impressed that varargs static inline works. (not complaining, just > shocked :)) IDE_DBG() has been removed entirely from this version. > > +static inline void > > +xide_delay(long ticks) > > +{ > > + if (!ticks) > > + return; > > + > > + current->state = TASK_INTERRUPTIBLE; > > + schedule_timeout(ticks); > > + return; > > +} > > ITYM TASK_UNINTERRUPTIBLE, because you definitely don't handle being > interrupted... :) current->state has been set to TASK_UNINTERRUPTIBLE as has been suggested. > > +unsigned int __init > > +pci_init_sgiioc4(struct pci_dev *dev, const char *name) > > +{ > > + unsigned int class_rev; > > + > > + pci_read_config_dword(dev, PCI_CLASS_REVISION, &class_rev); > > + class_rev &= 0xff; > > this appears to be dead code. This code has been removed. > > + pci_enable_device(dev); > > you _definitely_ want to check for errors. Error check has been included. > > +void > > +sgiioc4_maskproc(ide_drive_t * drive, int mask) > > +{ > > + ide_hwif_t *hwif = HWIF(drive); > > + if (IDE_CONTROL_REG) > > + hwif->OUTB(mask ? (drive->ctl | 2) : (drive->ctl & ~2), > > + IDE_CONTROL_REG); > > +} > > I'm pretty sure you have a control reg :) Test should not be necessary. This test has been removed. > > +void __init > > +ide_init_sgiioc4(ide_hwif_t * hwif) > > +{ > > + hwif->autodma = 1; > > + hwif->index = 0; /* Channel 0 */ > > + hwif->channel = 0; > > + hwif->atapi_dma = 1; > > + hwif->ultra_mask = 0x0; /* Disable Ultra DMA */ > > + hwif->mwdma_mask = 0x2; /* Multimode-2 DMA */ > > + hwif->swdma_mask = 0x2; > > + hwif->identify = NULL; > > + hwif->tuneproc = NULL; /* Sets timing for PIO mode */ > > + hwif->speedproc = NULL; /* Sets timing for DMA &/or PIO modes */ > > + hwif->selectproc = NULL; /* Use the default selection routine to select drive */ > > + hwif->reset_poll = NULL; /* No HBA specific reset_poll needed */ > > + hwif->pre_reset = NULL; /* No HBA specific pre_set needed */ > > + hwif->resetproc = &sgiioc4_resetproc; /* Reset the IOC4 DMA engine, clear interrupts etc */ > > + hwif->intrproc = NULL; /* Enable or Disable interrupt from drive */ > > + hwif->maskproc = &sgiioc4_maskproc; /* Mask on/off NIEN register */ > > + hwif->quirkproc = NULL; > > + hwif->busproc = NULL; > > style: Under Linux we normally don't explicitly set to NULL. > > > +static int > > +sgiioc4_ide_dma_lostirq(ide_drive_t * drive) > > +{ > > + if (HWIF(drive)->resetproc != NULL) > > + HWIF(drive)->resetproc(drive); > > + return __ide_dma_lostirq(drive); > > +} > > don't you know if you have a resetproc? :) Removed the if condition in the new patch. > > +static u8 > > +sgiioc4_INB(unsigned long port) > > +{ > > + u8 reg = (u8) inb(port); > > + > > + if ((port & 0xFFF) == 0x11C) { /* Status register of IOC4 */ > > + if (reg & 0x50) { /* Not busy...check for interrupt */ > > hmmm, you probably want OK_STAT macro invocation that's used throughout > the IDE code. Here we just want to verify that the status is "not busy". The status could be GOOD (0x50) or Error (0x51) as long as its not busy. This check has been changed as : + if ((port & 0xFFF) == 0x11C) { /* Status register of IOC4 */ + if (reg & 0x51) { /* Not busy...check for interrupt */ in the new patch. > > +/* Creates a dma map for the scatter-gather list entries */ > > +void __init > > +ide_dma_sgiioc4(ide_hwif_t * hwif, unsigned long dma_base) > > +{ > > + int num_ports = sizeof (ioc4_dma_regs_t); > > + > > + IDE_DBG(0, "%s: BM-DMA at 0x%04lx-0x%04lx\n", hwif->name, dma_base, > > + dma_base + num_ports - 1); > > + if (check_region(dma_base, num_ports)) { > > + IDE_DBG(0, > > + "ide_dma_sgiioc4(%s) -- Error, Port Addresses 0x%p to 0x%p ALREADY in use\n", > > + hwif->name, dma_base, dma_base + num_ports - 1); > > + return; > > + } > > + request_region(dma_base, num_ports, hwif->name); > > deprecated from the get-go... eliminate check_region, and test > request_region return value. Check region has been eliminated and the code now tests the request_region value. > > + hwif->dma_base = dma_base; > > + hwif->dmatable_cpu = pci_alloc_consistent(hwif->pci_dev, > > + IOC4_PRD_ENTRIES * IOC4_PRD_BYTES, /* 1 Page */ > > + &hwif->dmatable_dma); > > + if (hwif->dmatable_cpu == NULL) > > + goto dma_alloc_failure; > > + > > + hwif->sg_table = kmalloc(sizeof (struct scatterlist) * IOC4_PRD_ENTRIES, > > + GFP_KERNEL); > > + if (hwif->sg_table == NULL) { > > + pci_free_consistent(hwif->pci_dev, > > + IOC4_PRD_ENTRIES * IOC4_PRD_BYTES, > > + hwif->dmatable_cpu, hwif->dmatable_dma); > > + goto dma_alloc_failure; > > + } > > + > > + > > + hwif->dma_base2 = (unsigned long) pci_alloc_consistent(hwif->pci_dev, > > + IOC4_IDE_CACHELINE_SIZE, > > + (dma_addr_t*)&(hwif->dma_status)); > > + if (!(hwif->dma_base2)) { > > + IDE_DBG(0, > > + "ide_dma_sgiioc4(%s) - Couldnot allocate map for Ending DMA \n", > > + hwif->name); > > + } > > no error handling for this last alloc, unlike the previous two? Error handling has been added for the last alloc as well. > > +/* Sets the ATAPI Drive to do DMA in Multimode-2 */ > > +void > > +sgiioc4_config_drive_for_dma(ide_drive_t * drive) > > +{ > > + u8 status_val; > > + ide_hwif_t *hwif = HWIF(drive); > > + > > + hwif->OUTB(drive->select.all, IDE_SELECT_REG); > > + hwif->OUTB(drive->ctl | 2, IDE_CONTROL_REG); > > + hwif->OUTB(0x0, IDE_DATA_REG); > > + hwif->OUTB(0x0, IDE_SECTOR_REG); > > + hwif->OUTB(0x0, IDE_LCYL_REG); > > + hwif->OUTB(0x0, IDE_HCYL_REG); > > + hwif->OUTB(0x22, IDE_NSECTOR_REG); > > + hwif->OUTB(0x03, IDE_FEATURE_REG); > > + hwif->OUTB(0xEF, IDE_COMMAND_REG); > > + > > + do { > > + xide_delay(HZ); > > + status_val = hwif->INB(IDE_STATUS_REG); > > + } while (status_val & 0x80); > > + > > + if (status_val == 0x50) { > > + IDE_DBG(1, "Successfully set %s in Multimode-2 DMA mode \n", > > + drive->name); > > + drive->using_dma = 1; > > + } else { /* Go to PIO mode if this occurs */ > > + IDE_DBG(0, > > + "Couldnot set %s in Multimode-2 DMA mode | status 0x%x | Drive %s using PIO \n", > > + drive->name, status_val, drive->name); > > + drive->using_dma = 0; > > + } > > + return; > > +} > > isn't there a standard function to do this? > This is the standard method of setting device transfer mode. This function has been removed and the standard function has been used as : + if (ide_config_drive_speed(drive,XFER_MW_DMA_2)!=0) { + printk(KERN_INFO "Couldnot set %s in Multimode-2 DMA mode | Drive %s using PIO instead\n", + drive->name, drive->name); + drive->using_dma = 0; + } else + drive->using_dma = 1; > > > +/* Creates the scatter gather list, DMA Table */ > > +unsigned int > > +sgiioc4_build_dma_table(ide_drive_t * drive, struct request *rq, int ddir) > > +{ > > + ide_hwif_t *hwif = HWIF(drive); > > + unsigned int *table = hwif->dmatable_cpu; > > + unsigned int count = 0, i = 1; > > + struct scatterlist *sg; > > + > > + if (rq->cmd == IDE_DRIVE_TASKFILE) > > + hwif->sg_nents = i = sgiioc4_ide_raw_build_sglist(hwif, rq); > > + else > > + hwif->sg_nents = i = sgiioc4_ide_build_sglist(hwif, rq, ddir); > > + if (!i) > > + return 0; /* sglist of length Zero */ > > + > > + sg = hwif->sg_table; > > + while (i && sg_dma_len(sg)) { > > + dma_addr_t cur_addr; > > + int cur_len; > > + cur_addr = sg_dma_address(sg); > > + cur_len = sg_dma_len(sg); > > + > > + while (cur_len) { > > + if (count++ >= IOC4_PRD_ENTRIES) { > > + printk("%s: DMA table too small\n", > > + drive->name); > > + goto use_pio_instead; > > + } else { > > + uint32_t xcount, bcount = > > + 0x10000 - (cur_addr & 0xffff); > > + > > + if (bcount > cur_len) > > + bcount = cur_len; > > + > > + /* put the addr, length in the IOC4 dma-table format */ > > + *table = 0x0; > > + table++; > > + *table = cpu_to_be32(cur_addr); > > + table++; > > + *table = 0x0; > > + table++; > > + > > + xcount = bcount & 0xffff; > > + *table = cpu_to_be32(xcount); > > + table++; > > + > > + cur_addr += bcount; > > + cur_len -= bcount; > > + } > > + } > > + > > + sg++; > > + i--; > > + } > > + > > + if (count) { > > + table--; > > + *table |= cpu_to_be32(0x80000000); > > + return count; > > + } > > + use_pio_instead: > > + pci_unmap_sg(hwif->pci_dev, > > + hwif->sg_table, hwif->sg_nents, hwif->sg_dma_direction); > > + hwif->sg_dma_active = 0; > > + return 0; /* revert to PIO for this request */ > > +} > > hrm... did all this code really need duplicating from ide-dma.c? The IOC4 doesnot support the generic IDE Multiword-2 DMA scatter-gather list format. IOC4 Features : a) Big Endian Card b) Scatter Gather List Entry is 128 bits. Bits 0-63 are present to support 64 bit addressing in the future. Currently only 32 bit addresses (in bit 0-31) are supported and hence bits 32-63 are 0. Bits 64-80 have the length of the data being transferred, bit 95 has the End of List marker(EOL) and bit 96-127 are 0. i.e while the generic IDE scatter-gather list format is a) Little Endian b) Of this form : ----------------------------------- | Address (bits 0-31) | ----------------------------------- |EOL| 0 | Data Length (bits 32-48)| ----------------------------------- The IOC4 scatter gather list format is a) Big Endian b) Of this form : --------------------------------------------------------------------------- | Bits 63-32 : Resv for 64 bit address | Bits 31-0 : 32 bit address | --------------------------------------------------------------------------- | Bit 127-96 : 0 |EOL| 0 | Data Length (bits 64-96) | --------------------------------------------------------------------------- Hence we cannot use the generic code to generate the IOC4's scatter-gather list. > > +static int > > +sgiioc4_clearirq(ide_drive_t * drive) > > +{ > > + u32 intr_reg; > > + ide_hwif_t *hwif = HWIF(drive); > > + ide_ioreg_t other_ir = > > + hwif->io_ports[IDE_IRQ_OFFSET] + (IOC4_INTR_REG << 2); > > + > > + /* Code to check for PCI error conditions */ > > + intr_reg = hwif->INL(other_ir); > > + if (intr_reg & 0x03) { > > + /* Valid IOC4-IDE interrupt */ > > + u8 stat = hwif->INB(IDE_STATUS_REG); > > + int count = 0; > > + do { > > + xide_delay(count); > > + stat = hwif->INB(IDE_STATUS_REG); /* Removes Interrupt from IDE Device */ > > + } while ((stat & 0x80) && (count++ < 1024)); > > + > > + if (intr_reg & 0x02) { > > + /* Error when transferring DMA data on PCI bus */ > > + uint32_t pci_err_addr_low, pci_err_addr_high, > > + pci_stat_cmd_reg; > > + > > + pci_err_addr_low = > > + hwif->INL(hwif->io_ports[IDE_IRQ_OFFSET]); > > + pci_err_addr_high = > > + hwif->INL(hwif->io_ports[IDE_IRQ_OFFSET] + 4); > > + pci_read_config_dword(hwif->pci_dev, PCI_COMMAND, > > + &pci_stat_cmd_reg); > > + IDE_DBG(0, > > + "sgiioc4_clearirq(%s) : PCI Bus Error when doing DMA : status-cmd reg is 0x%x \n", > > + drive->name, pci_stat_cmd_reg); > > + IDE_DBG(0, > > + "sgiioc4_clearirq(%s) : PCI Error Address is 0x%x%x \n", > > + drive->name, pci_err_addr_high, > > + pci_err_addr_low); > > + /* Clear the PCI Error indicator */ > > + pci_write_config_dword(hwif->pci_dev, PCI_COMMAND, > > + 0x00000146); > > + } > > + hwif->OUTL(0x03, other_ir); /* Clear the Interrupt, Error bits on the IOC4 */ > > + > > + intr_reg = hwif->INL(other_ir); > > + } > > + return intr_reg; > > +} > > nice... this code is actually applicable to several other PCI host > controllers, too. Thanks. > > +/* Weeds out non-IDE interrupts to the IOC4 */ > > +#define ide_ack_intr(hwif) ((hwif)->hw.ack_intr ? (hwif)->hw.ack_intr(hwif) : 1) > > does this deal with shared interrupts too? Yes. The IOC4 is a single PCI device having multiple independent functions. It incorporates : a) 4 Serial Ports b) Single Channel IDE c) keyboard, Mouse functions All the above functions share the same interrupt line (but have an interrupt register with different bits set depending upon which function's device has interrupted). The above code is used to return from the ide_intr() function without doing anything, since the interrupt is not for the IDE device. The IOC4 IDE part is different from most generic IDE cards in the following aspects : a) Big Endian PCI device b) All register offsets are from a single BAR register c) Different scattergather list format d) The interrupt from the card still stands (even after the interrupt from the ide device behind the card goes away). This interrupt from the card has to be explicitly turned off by writing a bit in 1 particular register. Please apply this patch if it seems satisfactory. Thanks Aniket