(2.4.22-pre6 BK) new & changed (IDE) driver: SGI IOC4]
A message that is not in the archive (sent off-list or not yet received): [email protected]
PATCH 1/1 (2.4.22-pre6 BK) new & changed (IDE) driver: SGI IOC4]
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 :
Quoted 8 lines
Quoted 8 lines
(quoted diff) +8
-0+static inline void +IDE_DBG(int debuglevel, char *format, ...) +{ + va_list ap; + char tempstr[256]; + + if (sgiioc4_debug >= debuglevel) { + va_start(ap, format);
Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+ 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.
Quoted 5 lines
Quoted 5 lines
(quoted diff) +5
-0+static inline void +xide_delay(long ticks) +{ + if (!ticks) + return;
Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+ + 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.
Quoted 9 lines
Review
Quoted 7 lines
(quoted diff) +7
-0+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.
Quoted 3 lines
Quoted 1 line
+
pci_enable_device(dev);you _definitely_ want to check for errors.
Error check has been included.
Quoted 10 lines
Review
Quoted 8 lines
(quoted diff) +8
-0+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.
Quoted 24 lines
Quoted 11 lines
(quoted diff) +11
-0+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;Review
Quoted 10 lines
(quoted diff) +10
-0+ 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.
Quoted 9 lines
Review
Quoted 7 lines
(quoted diff) +7
-0+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.
Quoted 10 lines
Review
Quoted 7 lines
(quoted diff) +7
-0+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_STATmacro 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.
Quoted 10 lines
Quoted 10 lines
(quoted diff) +10
-0+/* 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,
Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +5
-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 testrequest_regionreturn value.
Check region has been eliminated and the code now tests the request_region value.
Quoted 19 lines
Quoted 19 lines
(quoted diff) +19
-0+ 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,
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ (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.
Quoted 29 lines
Quoted 29 lines
(quoted diff) +29
-0+/* 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",
Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+ 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;
Quoted 62 lines
Quoted 62 lines
(quoted diff) +61
-0+/* 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; + }
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ 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.
Quoted 42 lines
Quoted 42 lines
(quoted diff) +42
-0+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 */
Quoted 8 lines
Review
Quoted 5 lines
(quoted diff) +5
-0+ + intr_reg = hwif->INL(other_ir); + } + return intr_reg; +}nice... this code is actually applicable to several other PCI host controllers, too.
Thanks.
Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+/* 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
# This is a BitKeeper generated patch for the following project:
# Project Name: Linux kernel tree
# This patch format is intended for GNU patch command version 2.5 or higher. # This patch includes the following deltas:
# ChangeSet 1.1016 -> 1.1017
# drivers/ide/pci/Makefile 1.4 -> 1.5
# include/linux/pci_ids.h 1.66 -> 1.67
# MAINTAINERS 1.102 -> 1.103
# drivers/ide/Config.in 1.34 -> 1.35
# Documentation/Configure.help 1.171 -> 1.172
# (new) -> 1.1 drivers/ide/pci/sgiioc4.h # (new) -> 1.2 drivers/ide/pci/sgiioc4.c
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/07/17 [email protected] 1.1017
# foo # -------------------------------------------- # diff -Nru a/Documentation/Configure.help b/Documentation/Configure.help --- a/Documentation/Configure.help Thu Jul 17 17:43:20 2003 +++ b/Documentation/Configure.help Thu Jul 17 17:43:20 2003 @@ -1203,6 +1203,12 @@ This driver adds PIO/(U)DMA support for the ServerWorks OSB4/CSB5 chipsets.
+SGI IOC4 chipset support
+CONFIG_BLK_DEV_SGIIOC4
+ This driver adds PIO & MultiMode DMA-2 support for the SGI IOC4 + chipset. Please say Y here, if you have an Altix System from + Silicon Graphics Inc.
+
Intel PIIXn chipsets support
CONFIG_BLK_DEV_PIIX
This driver adds PIO mode setting and tuning for all PIIX IDE
MAINTAINERS +5 -0
@@ -950,6 +950,11 @@ L: [email protected] S: Maintained +IOC4 IDE DRIVER +P: Aniket Malatpure +M: [email protected] +S: Maintained + IP MASQUERADING: P: Juanjo Ciarlante M: [email protected]
drivers/ide/Config.in +1 -0
@@ -71,6 +71,7 @@ dep_tristate ' RZ1000 chipset bugfix/support' CONFIG_BLK_DEV_RZ1000 $CONFIG_X86 dep_tristate ' SCx200 chipset support' CONFIG_BLK_DEV_SC1200 $CONFIG_BLK_DEV_IDEDMA_PCI dep_tristate ' ServerWorks OSB4/CSB5/CSB6 chipsets support' CONFIG_BLK_DEV_SVWKS $CONFIG_BLK_DEV_IDEDMA_PCI + dep_tristate ' SGI IOC4 chipset support' CONFIG_BLK_DEV_SGIIOC4 $CONFIG_BLK_DEV_IDEDMA_PCI dep_tristate ' Silicon Image chipset support' CONFIG_BLK_DEV_SIIMAGE $CONFIG_BLK_DEV_IDEDMA_PCI dep_tristate ' SiS5513 chipset support' CONFIG_BLK_DEV_SIS5513 $CONFIG_BLK_DEV_IDEDMA_PCI $CONFIG_X86 dep_tristate ' SLC90E66 chipset support' CONFIG_BLK_DEV_SLC90E66 $CONFIG_BLK_DEV_IDEDMA_PCI
drivers/ide/pci/Makefile +1 -0
@@ -25,6 +25,7 @@ obj-$(CONFIG_BLK_DEV_RZ1000) += rz1000.o obj-$(CONFIG_BLK_DEV_SVWKS) += serverworks.o obj-$(CONFIG_BLK_DEV_SC1200) += sc1200.o +obj-$(CONFIG_BLK_DEV_SGIIOC4) += sgiioc4.o obj-$(CONFIG_BLK_DEV_SIIMAGE) += siimage.o obj-$(CONFIG_BLK_DEV_SIS5513) += sis5513.o obj-$(CONFIG_BLK_DEV_SL82C105) += sl82c105.o
drivers/ide/pci/sgiioc4.c new file +922 -0
922 lines not on this page · raw message
drivers/ide/pci/sgiioc4.h new file +177 -0
177 lines not on this page · raw message
include/linux/pci_ids.h +1 -0
@@ -839,6 +839,7 @@#define PCI_VENDOR_ID_SGI 0x10a9#define PCI_DEVICE_ID_SGI_IOC3 0x0003 +#define PCI_DEVICE_ID_SGI_IOC4 0x100a#define PCI_VENDOR_ID_ACC 0x10aa#define PCI_DEVICE_ID_ACC_2056 0x0000
Quoted 6 lines · Aniket Malatpure
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).
Ok by me. There are sokme things that want dealing with but they affect general paths so this is best for 2.4.22. I'll take a look at abstracting the sglist format properly. SI3xxx also supports some extended formats and could benefit.
Quoted 7 lines · Aniket Malatpure
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.
A few comments:
+ dep_tristate ' SGI IOC4 chipset support' CONFIG_BLK_DEV_SGIIOC4 $CONFIG_BLK_DEV_IDEDMA_PCI
This probably wants a depency on CONFIG_IA64_SGI_SN2?
+#include <linux/config.h>
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/pci.h>
+#include <linux/delay.h>
+#include <linux/hdreg.h>
+#include <linux/init.h>
+#include <asm/io.h>
<asm/*> includes after <linux/*> ones please.
+#ifdef CONFIG_PROC_FS
+static u8 sgiioc4_proc = 0;
+#endif /* CONFIG_PROC_FS */
As all procfs code is properly stubbed out you can probably nuke the ifdef.
+
+static int n_sgiioc4_devs = 0;
Also both of these don't need zero initialization, Linux takes care of that for you.
+#define SGIIOC4_HD_SUPPORT 0
Please make this either a config �ption or nuke it.
+
+static inline void
+xide_delay(long ticks)
Documentation/CodingStyle says
static inline void xide_delay(long ticks)
The same is true for many other function in this file.
+ printk(KERN_INFO "%s: %s Bus-Master DMA disabled \n", hwif->name, name);
Please linewrap after 80 chars.
+static unsigned int __init +pci_init_sgiioc4(struct pci_dev *dev, const char *name) +{
+ extern pciio_endian_t snia_pciio_endian_set(struct pci_dev *pci_dev,
+ pciio_endian_t device_end,
+ pciio_endian_t desired_end);
Shouldn't this be in a header?
+#if defined(CONFIG_IA64_SGI_SN2) || defined(CONFIG_IA64_GENERIC)
+ /* Enable Byte Swapping in the PIC */
+ snia_pciio_endian_set(dev, PCIDMA_ENDIAN_LITTLE, PCIDMA_ENDIAN_BIG);
+#endif
Currentl 2.4 mainline doesn't seem to include SN2 in IA64_GENERIC so compilation will fail for this case.
+static 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 */
Use a memset here so you only have to initialize the non-NULL fields?