Patch to add support for SGI's IOC4 chipset

Hi

This patch adds support for the ATAPI part of SGI's IOC4 chipset.
A version of this patch for the 2.4 series has been accepted and is present in the tree. This patch is a slight modification of the earlier patch for the 2.4 series.

Please merge this patch if there are no outstanding issues.

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.1418 -> 1.1419
# drivers/ide/pci/Makefile 1.8 -> 1.9
# include/linux/pci_ids.h 1.123 -> 1.124

#	 drivers/ide/Kconfig	1.29    -> 1.30   
#	               (new)	        -> 1.1     drivers/ide/pci/sgiioc4.c
#	               (new)	        -> 1.1     drivers/ide/pci/sgiioc4.h

#
# The following is the BitKeeper ChangeSet Log # --------------------------------------------
# 03/10/02 [email protected] 1.1419
# Adds support for the ATAPI part of the IOC4 chipset. # -------------------------------------------- #

diff -Nru a/drivers/ide/Kconfig b/drivers/ide/Kconfig
@@ -740,6 +740,13 @@
 	  This driver adds PIO/(U)DMA support for the ServerWorks OSB4/CSB5
 	  chipsets.
 
+config BLK_DEV_SGIIOC4
+	tristate "Silicon Graphics IOC4 chipset support"
+	help
+	  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.
+		
 config BLK_DEV_SIIMAGE
 	tristate "Silicon Image chipset support"
 	help

diff -Nru a/drivers/ide/pci/Makefile b/drivers/ide/pci/Makefile
@@ -22,6 +22,7 @@
 obj-$(CONFIG_BLK_DEV_PIIX)		+= piix.o
 obj-$(CONFIG_BLK_DEV_RZ1000)		+= rz1000.o
 obj-$(CONFIG_BLK_DEV_SVWKS)		+= serverworks.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

diff -Nru a/include/linux/pci_ids.h b/include/linux/pci_ids.h
@@ -896,6 +896,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_SGI_LITHIUM	0x1002
 
 #define PCI_VENDOR_ID_ACC		0x10aa

Perhaps Bart could take a look over this sometime please?

This cannot be __devinitdata because the PCI table walking will look at it even after __init code has been dropped. We've had oopses from this.

Please follow Documentation/CodingStyle and respect 80-columns limit.

+/**
+ * "Copied from drivers/ide/ide-dma.c"
+ * sgiioc4_ide_build_sglist - map IDE scatter gather for DMA I/O
+ * @hwif: the interface to build the DMA table for
+ * @rq: the request holding the sg list
+ * @ddir: data direction
+ *
+ * Perform the PCI mapping magic neccessary to access the source
+ * or target buffers of a request via PCI DMA. The lower layers
+ * of the kernel provide the neccessary cache management so that
+ * we can operate in a portable fashion.
+ *
+ * This code is identical to ide_build_sglist in ide-dma.c
+ * however that it not exported and even if it were would create
+ * dependancy problems for modular drivers.
+ */

+/**
+ * Copied from drivers/ide/ide-dma.c
+ * sgiioc4_ide_raw_build_sglist - map IDE scatter gather for DMA
+ * @hwif: the interface to build the DMA table for
+ * @rq: the request holding the sg list
+ *
+ * Perform the PCI mapping magic neccessary to access the source or
+ * target buffers of a taskfile request via PCI DMA. The lower layers
+ * of the kernel provide the neccessary cache management so that we can
+ * operate in a portable fashion
+ *
+ * This code is identical to ide_raw_build_sglist in ide-dma.c
+ * however that it not exported and even if it were would create
+ * dependancy problems for modular drivers.
+ */

What problems?
BTW during coping tabs were replaced by spaces in these functions.

+static int
+sgiioc4_get_info(char *buffer, char **addr, off_t offset, int count) +{
+ char *p = buffer;
+ unsigned int class_rev;
+ int i = 0;
+
+ while (i < n_sgiioc4_devs) {
+ pci_read_config_dword(sgiioc4_devs[i], PCI_CLASS_REVISION,
+ &class_rev);
+ class_rev &= 0xff;
+
+ if (sgiioc4_devs[i]->device == PCI_DEVICE_ID_SGI_IOC4) {
+ p += sprintf(p, "\n SGI IOC4 Chipset rev %d. ", class_rev);
+ p += sprintf(p, "\n Chipset has 1 IDE channel and supports 2 devices on that channel.");
+ p += sprintf(p, "\n Chipset supports DMA in MultiMode-2 data transfer protocol.\n");
+ /* Do we need more info. here? */
+ }
+ i++;
+ }
+
+ return p - buffer;
+}

Do you really need /proc/ide/sgiioc4?
You can print revision number during init.

+static void sgiioc4_init_hwif_ports(hw_regs_t * hw, unsigned long data_port,
+ unsigned long ctrl_port, unsigned long irq_port);
+static void sgiioc4_ide_setup_pci_device(struct pci_dev *dev, ide_pci_device_t *d); +static void sgiioc4_resetproc(ide_drive_t * drive);
+static void sgiioc4_maskproc(ide_drive_t * drive, int mask);
+static void sgiioc4_configure_for_dma(int dma_direction, ide_drive_t * drive); +static void __init ide_init_sgiioc4(ide_hwif_t * hwif);
+static void __init ide_dma_sgiioc4(ide_hwif_t * hwif, unsigned long dma_base); +static int sgiioc4_checkirq(ide_hwif_t * hwif);
+static int sgiioc4_clearirq(ide_drive_t * drive);
+static int sgiioc4_get_info(char *buffer, char **addr, off_t offset, int count); +static int sgiioc4_ide_dma_read(ide_drive_t * drive);
+static int sgiioc4_ide_dma_write(ide_drive_t * drive);
+static int sgiioc4_ide_dma_begin(ide_drive_t * drive);
+static int sgiioc4_ide_dma_end(ide_drive_t * drive);
+static int sgiioc4_ide_dma_check(ide_drive_t * drive);
+static int sgiioc4_ide_dma_on(ide_drive_t * drive);
+static int sgiioc4_ide_dma_off(ide_drive_t * drive);
+static int sgiioc4_ide_dma_off_quietly(ide_drive_t * drive);
+static int sgiioc4_ide_dma_test_irq(ide_drive_t * drive);
+static int sgiioc4_ide_dma_host_on(ide_drive_t * drive);
+static int sgiioc4_ide_dma_host_off(ide_drive_t * drive);
+static int sgiioc4_ide_dma_count(ide_drive_t * drive);
+static int sgiioc4_ide_dma_verbose(ide_drive_t * drive);
+static int sgiioc4_ide_dma_lostirq(ide_drive_t * drive);
+static int sgiioc4_ide_dma_timeout(ide_drive_t * drive);
+static int sgiioc4_ide_build_sglist(ide_drive_t *drive, struct request *rq); +static int sgiioc4_ide_raw_build_sglist(ide_drive_t *drive, struct request *rq); +
+static u8 sgiioc4_INB(unsigned long port);
+static inline void xide_delay(long ticks);
+extern int (*sgiioc4_display_info) (char *, char **, off_t, int); /* ide-proc.c */
+static unsigned int sgiioc4_build_dma_table(ide_drive_t * drive, struct request *rq,
+ int ddir);
+static unsigned int __init pci_init_sgiioc4(struct pci_dev *dev,ide_pci_device_t *d);

Most of this declarations are not needed as sgiioc4.h is only included from shiioc4.c.

Otherwise it looks okay.

Thanks.
--bartlomiej

I agree... but if you look at other PCI IDE drivers like piix.c,
you see the same thing. Maybe we should blame Alan... ;-)

Jeff

Not exactly...
in piix.h you have only declarations which are later used in piix.h.

Maybe all these PCI IDE *.h files should die, I find them really annoying while going through PCI IDE *.c files.

--bartlomiej

Hi

Attached is a new patch which takes into account the comments on the earlier patch.
I have changed the patch as per the comments below :

--------------------------------------------------------------------------------------------------
a) Comments from Andrew Morton :

This line has been removed.

The __devinitdata attribute has been removed from this declaration.

This file has been merged with sgiioc4.c as was suggested.

This structure is no longer volatile in the new patch.

--------------------------------------------------------------------------------------------------
b) Comments from Bartlomiej Zolnierkiewicz

Lindent has been used to format the original file. Some parts have been reformatted by hand.
The 80 column limit is respected for a majority of the patch.

+ *

+ *     This code is identical to ide_raw_build_sglist in ide-dma.c
+ *     however that it not exported and even if it were would create
+ *     dependancy problems for modular drivers.

+ */

Actually what I meant by problems was that, when this driver is compiled as a module and the earlier functions are not exported, the driver fails to find them. I have removed the earlier line from the new patch.

+ return p - buffer;
+}

It has been helpful to be able to see the firmware revision num anytime during system operation.
So the new patch still creates the above entry.

+ int ddir);
+static unsigned int __init pci_init_sgiioc4(struct pci_dev *dev,ide_pci_device_t *d);

The sgiioc4.h file has been removed in the new patch.
--------------------------------------------------------------------------------------------------

Please merge this patch if there are no other issues.

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.1433 -> 1.1434
# drivers/ide/pci/Makefile 1.8 -> 1.9
# include/linux/pci_ids.h 1.123 -> 1.124
# drivers/ide/Kconfig 1.29 -> 1.30
# (new) -> 1.1 drivers/ide/pci/sgiioc4.c
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/10/03 [email protected] 1.1434
# Adds support for the IOC4 IDE part.
# --------------------------------------------
#

diff -Nru a/drivers/ide/Kconfig b/drivers/ide/Kconfig
@@ -740,6 +740,13 @@
 	  This driver adds PIO/(U)DMA support for the ServerWorks OSB4/CSB5
 	  chipsets.
 
+config BLK_DEV_SGIIOC4
+	tristate "Silicon Graphics IOC4 chipset support"
+	help
+	  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.
+		
 config BLK_DEV_SIIMAGE
 	tristate "Silicon Image chipset support"
 	help

diff -Nru a/drivers/ide/pci/Makefile b/drivers/ide/pci/Makefile
@@ -22,6 +22,7 @@
 obj-$(CONFIG_BLK_DEV_PIIX)		+= piix.o
 obj-$(CONFIG_BLK_DEV_RZ1000)		+= rz1000.o
 obj-$(CONFIG_BLK_DEV_SVWKS)		+= serverworks.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

diff -Nru a/include/linux/pci_ids.h b/include/linux/pci_ids.h
@@ -896,6 +896,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_SGI_LITHIUM	0x1002
 
 #define PCI_VENDOR_ID_ACC		0x10aa

Hi

Please find attached a newer version of the patch I sent out earlier today. This patch has exactly the same code as earlier, it is just formatted in a neater way.
(We realized this after an internal review of my code).

Please merge this patch if there are no other issues.

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.1433 -> 1.1435
# drivers/ide/pci/Makefile 1.8 -> 1.9
# include/linux/pci_ids.h 1.123 -> 1.124
# drivers/ide/Kconfig 1.29 -> 1.30
# (new) -> 1.2 drivers/ide/pci/sgiioc4.c
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/10/03 [email protected] 1.1434
# Adds support for the IOC4 IDE part.
# --------------------------------------------
# 03/10/03 [email protected] 1.1435
# Formatted the driver to make it neat
# --------------------------------------------
#

diff -Nru a/drivers/ide/Kconfig b/drivers/ide/Kconfig
@@ -740,6 +740,13 @@
 	  This driver adds PIO/(U)DMA support for the ServerWorks OSB4/CSB5
 	  chipsets.
 
+config BLK_DEV_SGIIOC4
+	tristate "Silicon Graphics IOC4 chipset support"
+	help
+	  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.
+		
 config BLK_DEV_SIIMAGE
 	tristate "Silicon Image chipset support"
 	help

diff -Nru a/drivers/ide/pci/Makefile b/drivers/ide/pci/Makefile
@@ -22,6 +22,7 @@
 obj-$(CONFIG_BLK_DEV_PIIX)		+= piix.o
 obj-$(CONFIG_BLK_DEV_RZ1000)		+= rz1000.o
 obj-$(CONFIG_BLK_DEV_SVWKS)		+= serverworks.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

diff -Nru a/include/linux/pci_ids.h b/include/linux/pci_ids.h
@@ -896,6 +896,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_SGI_LITHIUM	0x1002
 
 #define PCI_VENDOR_ID_ACC		0x10aa

Hi,

Patch looks better, but there are still some issues to be resolved.

Okay, please apply attached patch first.
It exports ide_build_sglist() and ide_raw_build_sglist().

I've noticed that descriptions of these functions are slightly incorrect in your patch, there is no hwif argument etc.

I don't buy this, lspci can be used :-).

sgiioc4.h was removed, but declarations weren't.
You can shuffle code around to get rid of them.

ide_dma_sgiioc4():
+ if (!request_region(dma_base, num_ports, hwif->name)) {
+ printk(KERN_ERR

+		       "%s(%s) -- WARNING, Addresses 0x%p to 0x%p ALREADY in use\n",
+		       __FUNCTION__, hwif->name, (void *) dma_base,
+		       (void *) dma_base + num_ports - 1);

+ }
+

Driver can't just warn if addresses are already in use, it should fail. There is one more request_region() with this problem in the driver.

+ hwif->sg_table =
+ kmalloc(sizeof (struct scatterlist) * IOC4_PRD_ENTRIES, GFP_KERNEL);
+
+ if (!hwif->sg_table) {
+ 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) {
+ pci_free_consistent(hwif->pci_dev,
+ IOC4_PRD_ENTRIES * IOC4_PRD_BYTES,
+ hwif->dmatable_cpu, hwif->dmatable_dma);
+ kfree(hwif->sg_table);
+ goto dma_alloc_failure;
+ }
+
+ return;
+
+ dma_alloc_failure:

Minor issue: you can move pci_free_consisten() after dma_alloc_failure label.

+static ide_pci_device_t sgiioc4_chipsets[] __devinitdata = {
+ {
+ /* Channel 0 */
+ .vendor = PCI_VENDOR_ID_SGI,
+ .device = PCI_DEVICE_ID_SGI_IOC4,
+ .name = "SGIIOC4",
+ .init_chipset = NULL,
+ .init_iops = NULL,
+ .init_hwif = ide_init_sgiioc4,
+ .init_dma = ide_dma_sgiioc4,
+ .channels = 1,
+ .autodma = AUTODMA,
+ .enablebits = {{0x00, 0x00, 0x00}, {0x00, 0x00, 0x00}},
+ .bootable = ON_BOARD,
+ .extra = 0,
+ }
+};

Minor issue: static variables are initialized to 0 by kernel, so .init_{chipset, iops}, .enablebits and .extra lines should go away.

There are no .enablebits on SGI IOC4? Please add a comment about it.

sgiioc4_init_one():
+ pci_read_config_dword(dev, PCI_CLASS_REVISION, &class_rev);
+ class_rev &= 0xff;

Access to PCI devices before pci_enable_device()
(it is called later in pci_init_sgiioc4).

+static int
+sgiioc4_ide_dma_count(ide_drive_t * drive)
+{
+ return HWIF(drive)->ide_dma_begin(drive);
+}

This is identical to __ide_dma_count().

+static int
+sgiioc4_ide_dma_timeout(ide_drive_t * drive)
+{
+ printk(KERN_ERR "%s: timeout waiting for DMA\n", drive->name);
+ if (HWIF(drive)->ide_dma_test_irq(drive))
+ return 0;
+
+ return HWIF(drive)->ide_dma_end(drive);
+}

This is identical to __ide_dma_timeout().

+ hwif->OUTL(IOC4_S_DMA_STOP, dma_base + IOC4_DMA_CTRL * 4);
+ count = 0;
+ do {
+ xide_delay(count);
+ ioc4_dma = hwif->INL(dma_base + IOC4_DMA_CTRL * 4);
+ count += 10;
+ } while ((ioc4_dma & IOC4_S_DMA_STOP) && (count < 100));

This is duplicated 3x (one time with "xide_delay()" and "hwif->INL()" lines interchanged, maybe by a mistake?).

Can you make it a helper function, i.e. sgiioc4_ide_dma_stop_engine() ?

Thanks,
--bartlomiej

 drivers/ide/arm/icside.c |    4 ++--
 drivers/ide/ide-dma.c    |   12 ++++++++----
 include/linux/ide.h      |    2 ++
 3 files changed, 12 insertions(+), 6 deletions(-)

diff -puN drivers/ide/arm/icside.c~ide-dma-sgiioc4-exports drivers/ide/arm/icside.c
@@ -214,7 +214,7 @@ static void icside_maskproc(ide_drive_t 
 #define NR_ENTRIES 256
 #define TABLE_SIZE (NR_ENTRIES * 8)
 
-static void ide_build_sglist(ide_drive_t *drive, struct request *rq)
+static void icside_build_sglist(ide_drive_t *drive, struct request *rq)
 {
 	ide_hwif_t *hwif = drive->hwif;
 	struct icside_state *state = hwif->hwif_data;

@@ -543,7 +543,7 @@ icside_dma_common(ide_drive_t *drive, st
 	BUG_ON(hwif->sg_dma_active);
 	BUG_ON(dma_channel_active(hwif->hw.dma));
 
-	ide_build_sglist(drive, rq);
+	icside_build_sglist(drive, rq);
 
 	/*
 	 * Ensure that we have the right interrupt routed.

diff -puN drivers/ide/ide-dma.c~ide-dma-sgiioc4-exports drivers/ide/ide-dma.c
@@ -200,8 +200,8 @@ EXPORT_SYMBOL_GPL(ide_dma_intr);
  *	kernel provide the necessary cache management so that we can
  *	operate in a portable fashion
  */
- 
-static int ide_build_sglist (ide_drive_t *drive, struct request *rq)
+
+int ide_build_sglist(ide_drive_t *drive, struct request *rq)
 {
 	ide_hwif_t *hwif = HWIF(drive);
 	struct scatterlist *sg = hwif->sg_table;

@@ -220,6 +220,8 @@ static int ide_build_sglist (ide_drive_t
 	return pci_map_sg(hwif->pci_dev, sg, nents, hwif->sg_dma_direction);
 }
 
+EXPORT_SYMBOL_GPL(ide_build_sglist);
+
 /**
  *	ide_raw_build_sglist	-	map IDE scatter gather for DMA
  *	@drive: the drive to build the DMA table for

@@ -230,8 +232,8 @@ static int ide_build_sglist (ide_drive_t
  *	of the  kernel provide the necessary cache management so that we can
  *	operate in a portable fashion
  */
- 
-static int ide_raw_build_sglist (ide_drive_t *drive, struct request *rq)
+
+int ide_raw_build_sglist(ide_drive_t *drive, struct request *rq)
 {
 	ide_hwif_t *hwif = HWIF(drive);
 	struct scatterlist *sg = hwif->sg_table;

@@ -270,6 +272,8 @@ static int ide_raw_build_sglist (ide_dri
 	return pci_map_sg(hwif->pci_dev, sg, nents, hwif->sg_dma_direction);
 }
 
+EXPORT_SYMBOL_GPL(ide_raw_build_sglist);
+
 /**
  *	ide_build_dmatable	-	build IDE DMA table
  *

diff -puN include/linux/ide.h~ide-dma-sgiioc4-exports include/linux/ide.h
@@ -1695,6 +1695,8 @@ extern void ide_setup_pci_devices(struct
 #define GOOD_DMA_DRIVE		1
 
 #ifdef CONFIG_BLK_DEV_IDEDMA_PCI
+extern int ide_build_sglist(ide_drive_t *, struct request *);
+extern int ide_raw_build_sglist(ide_drive_t *, struct request *);
 extern int ide_build_dmatable(ide_drive_t *, struct request *);
 extern void ide_destroy_dmatable(ide_drive_t *);
 extern ide_startstop_t ide_dma_intr(ide_drive_t *);

_

Hello Bartlomiej,

I have a few questions. I'm going to be submitting the rest of the patches that Aniket was working on, as he started at a new company today (he was attempting to finish the submission before he left). Please forgive a lack of expertise on my part.

lspci gives the version number.
/proc/ide/sgiioc4 gives you:

SGI IOC4 Chipset rev 79.
Chipset has 1 IDE channel and supports 2 devices on that channel. Chipset supports DMA in MultiMode-2 data transfer protocol.

Is the # of IDE channels/devices and the DMA mode also available elsewhere?

How important is this to you? It seems more a style issue. I agree with you, by the way. When I write code, I try to minimize forward declarations. If I can get rid of some easily, will that be good enough?

What are they used for (i.e. what are you looking for in the comment)?

Is pci_enable_device required before a config space access?

I will make changes in accordance with the other comments and based on your responses to this.

Thanks

jeremy

Hi,

Okay.

Channels/devices is only available indirectly by /proc/ide/ device symlinks and mate entry in (ie.) ide0 proc directory. Controller DMA mode info is not available. However /proc/ide/sgiioc4 is useless for programs (parsing it to obtain needed info is a really bad idea) and info for users should be provided by drivers/ide/Kconfig (and optionally by init time message).

Okay, I can clear the rest.

It is good for simplicity (people reading this code) and maintainability (you don't have to update declaration if you change corresponding function).

It is mainly x86 thing (if channel is disabled in BIOS we don't probe it). You can just put "/* SGI IOC4 doesn't have enablebits. */ somewhere.

Yes, please read Documentation/pci.txt chapter 3.

Thanks,
--bartlomiej

Okay, I've removed this /proc entry (which was the only /proc entry in the driver).

Once I got started, I found that I was able to get rid of all of them.

done

Fixed.

Attached is the latest patch, which also includes your patch sent previously. Thanks for your review and assistance.

I will be on a vacation starting tomorrow, so I won't be able to reply until Oct 19th or 20th, in case there are any more issues. Hopefully this one will be okay :-)

thanks

jeremy

# 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.1490 -> 1.1492
# drivers/ide/pci/Makefile 1.9 -> 1.10
# include/linux/ide.h 1.75 -> 1.76
# include/linux/pci_ids.h 1.123 -> 1.124
# drivers/ide/Kconfig 1.30 -> 1.32
# drivers/ide/arm/icside.c 1.10 -> 1.11
# drivers/ide/ide-dma.c 1.21 -> 1.22
# (new) -> 1.2 drivers/ide/pci/sgiioc4.c
#
# The following is the BitKeeper ChangeSet Log
# --------------------------------------------
# 03/10/07 [email protected] 1.1491
# Add SGI IOC4.
# --------------------------------------------
# 03/10/07 [email protected] 1.1492
# Changes based on comments from Bartlomiej.
# --------------------------------------------
#

diff -Nru a/drivers/ide/Kconfig b/drivers/ide/Kconfig
@@ -740,6 +740,13 @@
 	  This driver adds PIO/(U)DMA support for the ServerWorks OSB4/CSB5
 	  chipsets.
 
+config BLK_DEV_SGIIOC4
+	tristate "Silicon Graphics IOC4 chipset support"
+	help
+	  This driver adds PIO & MultiMode DMA-2 support for the SGI IOC4
+	  chipset, which has one channel and can support two devices.
+	  Please say Y here if you have an Altix System from SGI.
+		
 config BLK_DEV_SIIMAGE
 	tristate "Silicon Image chipset support"
 	help

diff -Nru a/drivers/ide/arm/icside.c b/drivers/ide/arm/icside.c
@@ -214,7 +214,7 @@
 #define NR_ENTRIES 256
 #define TABLE_SIZE (NR_ENTRIES * 8)
 
-static void ide_build_sglist(ide_drive_t *drive, struct request *rq)
+static void icside_build_sglist(ide_drive_t *drive, struct request *rq)
 {
 	ide_hwif_t *hwif = drive->hwif;
 	struct icside_state *state = hwif->hwif_data;

@@ -543,7 +543,7 @@
 	BUG_ON(hwif->sg_dma_active);
 	BUG_ON(dma_channel_active(hwif->hw.dma));
 
-	ide_build_sglist(drive, rq);
+	icside_build_sglist(drive, rq);
 
 	/*
 	 * Ensure that we have the right interrupt routed.

diff -Nru a/drivers/ide/ide-dma.c b/drivers/ide/ide-dma.c
@@ -200,8 +200,8 @@
  *	kernel provide the necessary cache management so that we can
  *	operate in a portable fashion
  */
- 
-static int ide_build_sglist (ide_drive_t *drive, struct request *rq)
+
+int ide_build_sglist(ide_drive_t *drive, struct request *rq)
 {
 	ide_hwif_t *hwif = HWIF(drive);
 	struct scatterlist *sg = hwif->sg_table;

@@ -220,6 +220,8 @@
 	return pci_map_sg(hwif->pci_dev, sg, nents, hwif->sg_dma_direction);
 }
 
+EXPORT_SYMBOL_GPL(ide_build_sglist);
+
 /**
  *	ide_raw_build_sglist	-	map IDE scatter gather for DMA
  *	@drive: the drive to build the DMA table for

@@ -230,8 +232,8 @@
  *	of the  kernel provide the necessary cache management so that we can
  *	operate in a portable fashion
  */
- 
-static int ide_raw_build_sglist (ide_drive_t *drive, struct request *rq)
+
+int ide_raw_build_sglist(ide_drive_t *drive, struct request *rq)
 {
 	ide_hwif_t *hwif = HWIF(drive);
 	struct scatterlist *sg = hwif->sg_table;

@@ -269,6 +271,8 @@
 
 	return pci_map_sg(hwif->pci_dev, sg, nents, hwif->sg_dma_direction);
 }
+
+EXPORT_SYMBOL_GPL(ide_raw_build_sglist);
 
 /**
  *	ide_build_dmatable	-	build IDE DMA table

diff -Nru a/drivers/ide/pci/Makefile b/drivers/ide/pci/Makefile
@@ -21,6 +21,7 @@
 obj-$(CONFIG_BLK_DEV_PIIX)		+= piix.o
 obj-$(CONFIG_BLK_DEV_RZ1000)		+= rz1000.o
 obj-$(CONFIG_BLK_DEV_SVWKS)		+= serverworks.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

diff -Nru a/include/linux/ide.h b/include/linux/ide.h
@@ -1695,6 +1695,8 @@
 #define GOOD_DMA_DRIVE		1
 
 #ifdef CONFIG_BLK_DEV_IDEDMA_PCI
+extern int ide_build_sglist(ide_drive_t *, struct request *);
+extern int ide_raw_build_sglist(ide_drive_t *, struct request *);
 extern int ide_build_dmatable(ide_drive_t *, struct request *);
 extern void ide_destroy_dmatable(ide_drive_t *);
 extern ide_startstop_t ide_dma_intr(ide_drive_t *);

diff -Nru a/include/linux/pci_ids.h b/include/linux/pci_ids.h
@@ -896,6 +896,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_SGI_LITHIUM	0x1002
 
 #define PCI_VENDOR_ID_ACC		0x10aa

Thanks.

I think that after applying attached incremental patch it can go in.

- defining IDE_ARCH_ACK_INTR and ide_ack_intr() in sgiioc4.c is a no-op, it should be done <asm/ide.h> to make it work
(I think the same problem is present in 2.4.x)
- fix NULL pointer dereference (accessing hwif->name while hwif is NULL) in sgiioc4_ide_setup_pci_device() (was this driver ever tested?)
- make config option SN2 specific
- replace uint{8,32,64}_t by u{8,32,64}

--bartlomiej

 drivers/ide/Kconfig       |    3 ++-
 drivers/ide/pci/sgiioc4.c |   44 ++++++++++++++++----------------------------
 include/asm-ia64/ide.h    |    6 ++++++
 3 files changed, 24 insertions(+), 29 deletions(-)

diff -puN drivers/ide/Kconfig~ide-sgiioc4-fixes drivers/ide/Kconfig
@@ -742,11 +742,12 @@ config BLK_DEV_SVWKS
 
 config BLK_DEV_SGIIOC4
 	tristate "Silicon Graphics IOC4 chipset support"
+	depends on IA64_SGI_SN2
 	help
 	  This driver adds PIO & MultiMode DMA-2 support for the SGI IOC4
 	  chipset, which has one channel and can support two devices.
 	  Please say Y here if you have an Altix System from SGI.
-		
+
 config BLK_DEV_SIIMAGE
 	tristate "Silicon Image chipset support"
 	help

diff -puN include/asm-ia64/ide.h~ide-sgiioc4-fixes include/asm-ia64/ide.h
@@ -91,6 +91,12 @@ ide_init_default_hwifs (void)
 #endif
 }
 
+#ifdef CONFIG_BLK_DEV_SGIIOC4
+#define IDE_ARCH_ACK_INTR
+/* Weeds out non-IDE interrupts to the IOC4 */
+#define ide_ack_intr(hwif) ((hwif)->hw.ack_intr ? (hwif)->hw.ack_intr(hwif) : 1)
+#endif
+
 #include <asm-generic/ide_iops.h>
 
 #endif /* __KERNEL__ */

_

Thanks. I'll test this and reply with the results.

The definition in <include/linux/ide.h> is only used if IDE_ARCH_ACK_INTR is not defined. sgiioc4.c defines IDE_ARCH_ACK_INTR before including that file, so I believe we get the definition we want without touching ide.h, don't we?

Yes. It may be that since name is an array, the value passed to request_region was the offset of name in the structure. Since that was never dereferenced, things worked okay. In any case, the code was obviously incorrect.

These look fine.

I'll await a response on the IDE_ARCH_ACK_INTR issue. Do you want me to send another patch, or is the previous with your update sufficient?

thanks

jeremy

ide_ack_intr() is used by ide-io.c. If IDE_ARCH_ACK_INTR is not defined in ide.h (and it won't be cause you are doing this only in sgiioc4.c /sgiioc4.h in 2.4.x case/ about which ide-io.c has abolutely no idea) ide_ack_intr() will turn into no-op and hwif->ack_intr() won't be called.

It is sufficient.

thanks,
--bartlomiej

I see what you mean. Thanks for spotting and fixing this.

I've run into a problem in testing. For some reason, I've started to get ide timeouts, and the error recovery is not working correctly, due to a problem in the driver.

In sgiioc4_ide_dma_stop(), sgiioc4_ide_dma_end(), and sgiioc4_clearirq(), there are calls to xide_delay(), which uses schedule_timeout() to sleep. Since all of these sgiioc4_ functions can be called from interrupt context, that's an obvious problem.

In sgiioc4_clearirq(), the delay function is while we're waiting for the interrupt to clear.

In sgiioc4_ide_dma_stop(), we're waiting for the DMA bit to clear.

In sgiioc4_ide_dma_end(), we're waiting for another DMA to finish.

I believe that the right answer is to use udelay() and give up after a short period of time. My question is what does the ide layer expect? That is, if you call the dma_end function and the hardware driver can't succeed, what would you like us to do? Is there a way to return error, or should we just fail and the ide infrastructure will pick it up later and reset things?

I am new to Linux IDE, so forgive these questions if the answers should be obvious.

thanks

jeremy

I think there is another bug:

...

	hwif->hw.ack_intr = &sgiioc4_checkirq;	/* MultiFunction Chip */

...

sgiioc4_clearirq() should be used instead of sgiioc4_checkirq() here, because otherwise IRQ won't be cleared.

In order to do this you must modify sgiioc4_clearirq() slightly, just change "return intr_reg;" to "return intr_reg & 0x03;".

If you wonder why, please look at ide_ack_intr() use in ide-io.c:ide_intr().

Yes, just use udelay().

The latter, sgiioc4_ide_dma_end() already returns error value to higher layer. dma_intr() checks it and handle errors (at least in theory).

No problem.

thanks,
--bartlomiej

Thanks. I've taken a look at it and have become puzzled.

It looks as though ide_ack_intr normally just "returns" 1 and has no other effect. But then I see some ide drivers that also have an ack_intr routine. On some (most?) architechtures, it would appear that the ack_intr routine is not used, since the ide_ack_intr macro will not call it.

In gayle_ack_intr_a4000(), it looks as though all it does is read a register and return something. Is that register read supposed to clear interrupt as a side effect.

So maybe an explicit clear is not needed on most implementations?

jeremy

Bartlomiej and Andrew,

Here is the new IOC4 patch relative to Linus's BK tree.

Bartlomiej, can you review and give your comment? I took out the ack_intr code, since it seems to be redundant, because the IOC4 is a PCI device and the interrupt is cleared in sgiioc4_INB().

thanks

jeremy

 drivers/ide/Kconfig       |    8 
 drivers/ide/arm/icside.c  |    4 
 drivers/ide/ide-dma.c     |   12 
 drivers/ide/pci/Makefile  |    1 
 drivers/ide/pci/sgiioc4.c |  833 ++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/ide.h       |    2 
 include/linux/pci_ids.h   |    1 
 7 files changed, 855 insertions(+), 6 deletions(-)

diff -Nru a/drivers/ide/Kconfig b/drivers/ide/Kconfig
@@ -740,6 +740,14 @@
 	  This driver adds PIO/(U)DMA support for the ServerWorks OSB4/CSB5
 	  chipsets.
 
+config BLK_DEV_SGIIOC4
+	tristate "Silicon Graphics IOC4 chipset support"
+	depends on IA64_SGI_SN2
+	help
+	  This driver adds PIO & MultiMode DMA-2 support for the SGI IOC4
+	  chipset, which has one channel and can support two devices.
+	  Please say Y here if you have an Altix System from SGI.
+
 config BLK_DEV_SIIMAGE
 	tristate "Silicon Image chipset support"
 	help

diff -Nru a/drivers/ide/arm/icside.c b/drivers/ide/arm/icside.c
@@ -214,7 +214,7 @@
 #define NR_ENTRIES 256
 #define TABLE_SIZE (NR_ENTRIES * 8)
 
-static void ide_build_sglist(ide_drive_t *drive, struct request *rq)
+static void icside_build_sglist(ide_drive_t *drive, struct request *rq)
 {
 	ide_hwif_t *hwif = drive->hwif;
 	struct icside_state *state = hwif->hwif_data;

@@ -543,7 +543,7 @@
 	BUG_ON(hwif->sg_dma_active);
 	BUG_ON(dma_channel_active(hwif->hw.dma));
 
-	ide_build_sglist(drive, rq);
+	icside_build_sglist(drive, rq);
 
 	/*
 	 * Ensure that we have the right interrupt routed.

diff -Nru a/drivers/ide/ide-dma.c b/drivers/ide/ide-dma.c
@@ -200,8 +200,8 @@
  *	kernel provide the necessary cache management so that we can
  *	operate in a portable fashion
  */
- 
-static int ide_build_sglist (ide_drive_t *drive, struct request *rq)
+
+int ide_build_sglist(ide_drive_t *drive, struct request *rq)
 {
 	ide_hwif_t *hwif = HWIF(drive);
 	struct scatterlist *sg = hwif->sg_table;

@@ -220,6 +220,8 @@
 	return pci_map_sg(hwif->pci_dev, sg, nents, hwif->sg_dma_direction);
 }
 
+EXPORT_SYMBOL_GPL(ide_build_sglist);
+
 /**
  *	ide_raw_build_sglist	-	map IDE scatter gather for DMA
  *	@drive: the drive to build the DMA table for

@@ -230,8 +232,8 @@
  *	of the  kernel provide the necessary cache management so that we can
  *	operate in a portable fashion
  */
- 
-static int ide_raw_build_sglist (ide_drive_t *drive, struct request *rq)
+
+int ide_raw_build_sglist(ide_drive_t *drive, struct request *rq)
 {
 	ide_hwif_t *hwif = HWIF(drive);
 	struct scatterlist *sg = hwif->sg_table;

@@ -269,6 +271,8 @@
 
 	return pci_map_sg(hwif->pci_dev, sg, nents, hwif->sg_dma_direction);
 }
+
+EXPORT_SYMBOL_GPL(ide_raw_build_sglist);
 
 /**
  *	ide_build_dmatable	-	build IDE DMA table

diff -Nru a/drivers/ide/pci/Makefile b/drivers/ide/pci/Makefile
@@ -21,6 +21,7 @@
 obj-$(CONFIG_BLK_DEV_PIIX)		+= piix.o
 obj-$(CONFIG_BLK_DEV_RZ1000)		+= rz1000.o
 obj-$(CONFIG_BLK_DEV_SVWKS)		+= serverworks.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

diff -Nru a/include/linux/ide.h b/include/linux/ide.h
@@ -1693,6 +1693,8 @@
 #define GOOD_DMA_DRIVE		1
 
 #ifdef CONFIG_BLK_DEV_IDEDMA_PCI
+extern int ide_build_sglist(ide_drive_t *, struct request *);
+extern int ide_raw_build_sglist(ide_drive_t *, struct request *);
 extern int ide_build_dmatable(ide_drive_t *, struct request *);
 extern void ide_destroy_dmatable(ide_drive_t *);
 extern ide_startstop_t ide_dma_intr(ide_drive_t *);

diff -Nru a/include/linux/pci_ids.h b/include/linux/pci_ids.h
@@ -896,6 +896,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_SGI_LITHIUM	0x1002
 
 #define PCI_VENDOR_ID_ACC		0x10aa