Re: Patch to add support for SGI's IOC4 chipset

[Bartlomiej Zolnierkiewicz]

Hi,

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

> + *
> + *     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.
>
> 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.

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.

> +       return p - buffer;
> +}
>
> >Do you really need /proc/ide/sgiioc4?
> >You can print revision number during init.
>
> 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.

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

> +                                           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.
>
> The sgiioc4.h file has been removed in the new patch.

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

> Please merge this patch if there are no other issues.

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
--- linux-2.6.0-test6-bk2/drivers/ide/arm/icside.c~ide-dma-sgiioc4-exports	2003-10-04 18:43:13.746819240 +0200
+++ linux-2.6.0-test6-bk2-root/drivers/ide/arm/icside.c	2003-10-04 18:43:50.765191592 +0200
@@ -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
--- linux-2.6.0-test6-bk2/drivers/ide/ide-dma.c~ide-dma-sgiioc4-exports	2003-10-04 18:43:19.635923960 +0200
+++ linux-2.6.0-test6-bk2-root/drivers/ide/ide-dma.c	2003-10-04 18:45:28.323360496 +0200
@@ -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
--- linux-2.6.0-test6-bk2/include/linux/ide.h~ide-dma-sgiioc4-exports	2003-10-04 18:43:32.105028368 +0200
+++ linux-2.6.0-test6-bk2-root/include/linux/ide.h	2003-10-04 18:47:04.251777160 +0200
@@ -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 *);

_