[PATCH 1/1] - sgiioc4: fixup use of mmio ops

[PATCH 1/1] - sgiioc4: fixup use of mmio ops

[John Keller]

sgiioc4.c had been recently converted to using mmio ops.
There are still a few issues to cleanup.

Signed-off-by: jpk@sgi.com

--- linux-2.6.orig/drivers/ide/pci/sgiioc4.c	2006-07-11 11:34:42.135934603 -0500
+++ linux-2.6/drivers/ide/pci/sgiioc4.c	2006-07-12 09:17:02.316590824 -0500
@@ -372,7 +372,7 @@ ide_dma_sgiioc4(ide_hwif_t * hwif, unsig
 	printk(KERN_INFO "%s: BM-DMA at 0x%04lx-0x%04lx\n", hwif->name,
 	       dma_base, dma_base + num_ports - 1);
 
-	if (!request_region(dma_base, num_ports, hwif->name)) {
+	if (!request_mem_region(dma_base, num_ports, hwif->name)) {
 		printk(KERN_ERR
 		       "%s(%s) -- ERROR, Addresses 0x%p to 0x%p "
 		       "ALREADY in use\n",
@@ -381,7 +381,7 @@ ide_dma_sgiioc4(ide_hwif_t * hwif, unsig
 		goto dma_alloc_failure;
 	}
 
-	hwif->dma_base = dma_base;
+	hwif->dma_base = (unsigned long) ioremap(dma_base, num_ports);
 	hwif->dmatable_cpu = pci_alloc_consistent(hwif->pci_dev,
 					  IOC4_PRD_ENTRIES * IOC4_PRD_BYTES,
 					  &hwif->dmatable_dma);
@@ -607,18 +607,14 @@ ide_init_sgiioc4(ide_hwif_t * hwif)
 	hwif->ide_dma_lostirq = &sgiioc4_ide_dma_lostirq;
 	hwif->ide_dma_timeout = &__ide_dma_timeout;
 
-	/*
-	 * The IOC4 uses MMIO rather than Port IO.
-	 * It also needs special workarounds for INB.
-	 */
-	default_hwif_mmiops(hwif);
 	hwif->INB = &sgiioc4_INB;
 }
 
 static int __devinit
 sgiioc4_ide_setup_pci_device(struct pci_dev *dev, ide_pci_device_t * d)
 {
-	unsigned long base, ctl, dma_base, irqport;
+	unsigned long ctl, dma_base, irqport;
+	unsigned long bar0, cmd_base, cmd_phys_base, virt_base;
 	ide_hwif_t *hwif;
 	int h;
 
@@ -636,23 +632,27 @@ sgiioc4_ide_setup_pci_device(struct pci_
 	}
 
 	/*  Get the CmdBlk and CtrlBlk Base Registers */
-	base = pci_resource_start(dev, 0) + IOC4_CMD_OFFSET;
-	ctl = pci_resource_start(dev, 0) + IOC4_CTRL_OFFSET;
-	irqport = pci_resource_start(dev, 0) + IOC4_INTR_OFFSET;
+	bar0 = pci_resource_start(dev, 0);
+	virt_base = (unsigned long) ioremap(bar0, pci_resource_len(dev, 0));
+	cmd_base = virt_base + IOC4_CMD_OFFSET;
+	ctl = virt_base + IOC4_CTRL_OFFSET;
+	irqport = virt_base + IOC4_INTR_OFFSET;
 	dma_base = pci_resource_start(dev, 0) + IOC4_DMA_OFFSET;
 
-	if (!request_region(base, IOC4_CMD_CTL_BLK_SIZE, hwif->name)) {
+	cmd_phys_base = bar0 + IOC4_CMD_OFFSET;
+	if (!request_mem_region(cmd_phys_base, IOC4_CMD_CTL_BLK_SIZE,
+	    hwif->name)) {
 		printk(KERN_ERR
-			"%s : %s -- ERROR, Port Addresses "
+			"%s : %s -- ERROR, Addresses "
 			"0x%p to 0x%p ALREADY in use\n",
-		       __FUNCTION__, hwif->name, (void *) base,
-		       (void *) base + IOC4_CMD_CTL_BLK_SIZE);
+		       __FUNCTION__, hwif->name, (void *) cmd_phys_base,
+		       (void *) cmd_phys_base + IOC4_CMD_CTL_BLK_SIZE);
 		return -ENOMEM;
 	}
 
-	if (hwif->io_ports[IDE_DATA_OFFSET] != base) {
+	if (hwif->io_ports[IDE_DATA_OFFSET] != cmd_base) {
 		/* Initialize the IO registers */
-		sgiioc4_init_hwif_ports(&hwif->hw, base, ctl, irqport);
+		sgiioc4_init_hwif_ports(&hwif->hw, cmd_base, ctl, irqport);
 		memcpy(hwif->io_ports, hwif->hw.io_ports,
 		       sizeof (hwif->io_ports));
 		hwif->noprobe = !hwif->io_ports[IDE_DATA_OFFSET];
@@ -665,6 +665,9 @@ sgiioc4_ide_setup_pci_device(struct pci_
 	hwif->cds = (struct ide_pci_device_s *) d;
 	hwif->gendev.parent = &dev->dev;/* setup proper ancestral information */
 
+	/* The IOC4 uses MMIO rather than Port IO. */
+	default_hwif_mmiops(hwif);
+
 	/* Initializing chipset IRQ Registers */
 	hwif->OUTL(0x03, irqport + IOC4_INTR_SET * 4);
 

Re: [PATCH 1/1] - sgiioc4: fixup use of mmio ops

[Jeremy Higdon]

On Wed, Jul 12, 2006 at 12:57:14PM -0500, John Keller wrote:
> sgiioc4.c had been recently converted to using mmio ops.
> There are still a few issues to cleanup.
> 
> Signed-off-by: jpk@sgi.com
Signed-off-by: jeremy@sgi.com

> 
> --- linux-2.6.orig/drivers/ide/pci/sgiioc4.c	2006-07-11 11:34:42.135934603 -0500
> +++ linux-2.6/drivers/ide/pci/sgiioc4.c	2006-07-12 09:17:02.316590824 -0500
> @@ -372,7 +372,7 @@ ide_dma_sgiioc4(ide_hwif_t * hwif, unsig
>  	printk(KERN_INFO "%s: BM-DMA at 0x%04lx-0x%04lx\n", hwif->name,
>  	       dma_base, dma_base + num_ports - 1);
>  
> -	if (!request_region(dma_base, num_ports, hwif->name)) {
> +	if (!request_mem_region(dma_base, num_ports, hwif->name)) {
>  		printk(KERN_ERR
>  		       "%s(%s) -- ERROR, Addresses 0x%p to 0x%p "
>  		       "ALREADY in use\n",
> @@ -381,7 +381,7 @@ ide_dma_sgiioc4(ide_hwif_t * hwif, unsig
>  		goto dma_alloc_failure;
>  	}
>  
> -	hwif->dma_base = dma_base;
> +	hwif->dma_base = (unsigned long) ioremap(dma_base, num_ports);
>  	hwif->dmatable_cpu = pci_alloc_consistent(hwif->pci_dev,
>  					  IOC4_PRD_ENTRIES * IOC4_PRD_BYTES,
>  					  &hwif->dmatable_dma);
> @@ -607,18 +607,14 @@ ide_init_sgiioc4(ide_hwif_t * hwif)
>  	hwif->ide_dma_lostirq = &sgiioc4_ide_dma_lostirq;
>  	hwif->ide_dma_timeout = &__ide_dma_timeout;
>  
> -	/*
> -	 * The IOC4 uses MMIO rather than Port IO.
> -	 * It also needs special workarounds for INB.
> -	 */
> -	default_hwif_mmiops(hwif);
>  	hwif->INB = &sgiioc4_INB;
>  }
>  
>  static int __devinit
>  sgiioc4_ide_setup_pci_device(struct pci_dev *dev, ide_pci_device_t * d)
>  {
> -	unsigned long base, ctl, dma_base, irqport;
> +	unsigned long ctl, dma_base, irqport;
> +	unsigned long bar0, cmd_base, cmd_phys_base, virt_base;
>  	ide_hwif_t *hwif;
>  	int h;
>  
> @@ -636,23 +632,27 @@ sgiioc4_ide_setup_pci_device(struct pci_
>  	}
>  
>  	/*  Get the CmdBlk and CtrlBlk Base Registers */
> -	base = pci_resource_start(dev, 0) + IOC4_CMD_OFFSET;
> -	ctl = pci_resource_start(dev, 0) + IOC4_CTRL_OFFSET;
> -	irqport = pci_resource_start(dev, 0) + IOC4_INTR_OFFSET;
> +	bar0 = pci_resource_start(dev, 0);
> +	virt_base = (unsigned long) ioremap(bar0, pci_resource_len(dev, 0));
> +	cmd_base = virt_base + IOC4_CMD_OFFSET;
> +	ctl = virt_base + IOC4_CTRL_OFFSET;
> +	irqport = virt_base + IOC4_INTR_OFFSET;
>  	dma_base = pci_resource_start(dev, 0) + IOC4_DMA_OFFSET;
>  
> -	if (!request_region(base, IOC4_CMD_CTL_BLK_SIZE, hwif->name)) {
> +	cmd_phys_base = bar0 + IOC4_CMD_OFFSET;
> +	if (!request_mem_region(cmd_phys_base, IOC4_CMD_CTL_BLK_SIZE,
> +	    hwif->name)) {
>  		printk(KERN_ERR
> -			"%s : %s -- ERROR, Port Addresses "
> +			"%s : %s -- ERROR, Addresses "
>  			"0x%p to 0x%p ALREADY in use\n",
> -		       __FUNCTION__, hwif->name, (void *) base,
> -		       (void *) base + IOC4_CMD_CTL_BLK_SIZE);
> +		       __FUNCTION__, hwif->name, (void *) cmd_phys_base,
> +		       (void *) cmd_phys_base + IOC4_CMD_CTL_BLK_SIZE);
>  		return -ENOMEM;
>  	}
>  
> -	if (hwif->io_ports[IDE_DATA_OFFSET] != base) {
> +	if (hwif->io_ports[IDE_DATA_OFFSET] != cmd_base) {
>  		/* Initialize the IO registers */
> -		sgiioc4_init_hwif_ports(&hwif->hw, base, ctl, irqport);
> +		sgiioc4_init_hwif_ports(&hwif->hw, cmd_base, ctl, irqport);
>  		memcpy(hwif->io_ports, hwif->hw.io_ports,
>  		       sizeof (hwif->io_ports));
>  		hwif->noprobe = !hwif->io_ports[IDE_DATA_OFFSET];
> @@ -665,6 +665,9 @@ sgiioc4_ide_setup_pci_device(struct pci_
>  	hwif->cds = (struct ide_pci_device_s *) d;
>  	hwif->gendev.parent = &dev->dev;/* setup proper ancestral information */
>  
> +	/* The IOC4 uses MMIO rather than Port IO. */
> +	default_hwif_mmiops(hwif);
> +
>  	/* Initializing chipset IRQ Registers */
>  	hwif->OUTL(0x03, irqport + IOC4_INTR_SET * 4);
>  

Re: [PATCH 1/1] - sgiioc4: fixup use of mmio ops

[Jeff Garzik]

Jeremy Higdon wrote:
>> @@ -381,7 +381,7 @@ ide_dma_sgiioc4(ide_hwif_t * hwif, unsig
>>  		goto dma_alloc_failure;
>>  	}
>>  
>> -	hwif->dma_base = dma_base;
>> +	hwif->dma_base = (unsigned long) ioremap(dma_base, num_ports);
>>  	hwif->dmatable_cpu = pci_alloc_consistent(hwif->pci_dev,
>>  					  IOC4_PRD_ENTRIES * IOC4_PRD_BYTES,
>>  					  &hwif->dmatable_dma);

adding a bug:  must check for NULL ioremap return value


>> @@ -607,18 +607,14 @@ ide_init_sgiioc4(ide_hwif_t * hwif)
>>  	hwif->ide_dma_lostirq = &sgiioc4_ide_dma_lostirq;
>>  	hwif->ide_dma_timeout = &__ide_dma_timeout;
>>  
>> -	/*
>> -	 * The IOC4 uses MMIO rather than Port IO.
>> -	 * It also needs special workarounds for INB.
>> -	 */
>> -	default_hwif_mmiops(hwif);
>>  	hwif->INB = &sgiioc4_INB;
>>  }
>>  
>>  static int __devinit
>>  sgiioc4_ide_setup_pci_device(struct pci_dev *dev, ide_pci_device_t * d)
>>  {
>> -	unsigned long base, ctl, dma_base, irqport;
>> +	unsigned long ctl, dma_base, irqport;
>> +	unsigned long bar0, cmd_base, cmd_phys_base, virt_base;

New code should use 'void __iomem *' for ioremap return values, wherever 
possible.


>>  	ide_hwif_t *hwif;
>>  	int h;
>>  
>> @@ -636,23 +632,27 @@ sgiioc4_ide_setup_pci_device(struct pci_
>>  	}
>>  
>>  	/*  Get the CmdBlk and CtrlBlk Base Registers */
>> -	base = pci_resource_start(dev, 0) + IOC4_CMD_OFFSET;
>> -	ctl = pci_resource_start(dev, 0) + IOC4_CTRL_OFFSET;
>> -	irqport = pci_resource_start(dev, 0) + IOC4_INTR_OFFSET;
>> +	bar0 = pci_resource_start(dev, 0);
>> +	virt_base = (unsigned long) ioremap(bar0, pci_resource_len(dev, 0));
>> +	cmd_base = virt_base + IOC4_CMD_OFFSET;
>> +	ctl = virt_base + IOC4_CTRL_OFFSET;
>> +	irqport = virt_base + IOC4_INTR_OFFSET;
>>  	dma_base = pci_resource_start(dev, 0) + IOC4_DMA_OFFSET;

ditto:  new bug here too


>> -	if (!request_region(base, IOC4_CMD_CTL_BLK_SIZE, hwif->name)) {
>> +	cmd_phys_base = bar0 + IOC4_CMD_OFFSET;
>> +	if (!request_mem_region(cmd_phys_base, IOC4_CMD_CTL_BLK_SIZE,
>> +	    hwif->name)) {
>>  		printk(KERN_ERR
>> -			"%s : %s -- ERROR, Port Addresses "
>> +			"%s : %s -- ERROR, Addresses "
>>  			"0x%p to 0x%p ALREADY in use\n",
>> -		       __FUNCTION__, hwif->name, (void *) base,
>> -		       (void *) base + IOC4_CMD_CTL_BLK_SIZE);
>> +		       __FUNCTION__, hwif->name, (void *) cmd_phys_base,
>> +		       (void *) cmd_phys_base + IOC4_CMD_CTL_BLK_SIZE);

If 'void __iomem *' were used, no casts would be needed here

Re: [PATCH 1/1] - sgiioc4: fixup use of mmio ops

[Brent Casavant]

On Wed, 12 Jul 2006, John Keller wrote:

> sgiioc4.c had been recently converted to using mmio ops.
> There are still a few issues to cleanup.

An issue I noticed the other day is that we really should be turning
on CONFIG_IDEPCI_SHARE_IRQ whenever CONFIG_BLK_DEV_SGIIOC4 is selected.
The IOC4 IDE part shares a PCI interrupt with the other IOC4 devices
(serial, external interrupts), so it is effectively always sharing an
IRQ.

I just haven't created the patch yet, and will be out of the office
until next Tuesday.  You might want to consider fixing this side-issue
as well as long as you're touching the IOC4 IDE code.

Brent

-- 
Brent Casavant                          All music is folk music.  I ain't
bcasavan@sgi.com                        never heard a horse sing a song.
Silicon Graphics, Inc.                    -- Louis Armstrong

Re: [PATCH 1/1] - sgiioc4: fixup use of mmio ops

[John Keller]

> 
> Jeremy Higdon wrote:
> >> @@ -381,7 +381,7 @@ ide_dma_sgiioc4(ide_hwif_t * hwif, unsig
> >>  		goto dma_alloc_failure;
> >>  	}
> >>  
> >> -	hwif->dma_base = dma_base;
> >> +	hwif->dma_base = (unsigned long) ioremap(dma_base, num_ports);
> >>  	hwif->dmatable_cpu = pci_alloc_consistent(hwif->pci_dev,
> >>  					  IOC4_PRD_ENTRIES * IOC4_PRD_BYTES,
> >>  					  &hwif->dmatable_dma);
> 
> adding a bug:  must check for NULL ioremap return value

Shall do.


> 
> 
> >> @@ -607,18 +607,14 @@ ide_init_sgiioc4(ide_hwif_t * hwif)
> >>  	hwif->ide_dma_lostirq = &sgiioc4_ide_dma_lostirq;
> >>  	hwif->ide_dma_timeout = &__ide_dma_timeout;
> >>  
> >> -	/*
> >> -	 * The IOC4 uses MMIO rather than Port IO.
> >> -	 * It also needs special workarounds for INB.
> >> -	 */
> >> -	default_hwif_mmiops(hwif);
> >>  	hwif->INB = &sgiioc4_INB;
> >>  }
> >>  
> >>  static int __devinit
> >>  sgiioc4_ide_setup_pci_device(struct pci_dev *dev, ide_pci_device_t * d)
> >>  {
> >> -	unsigned long base, ctl, dma_base, irqport;
> >> +	unsigned long ctl, dma_base, irqport;
> >> +	unsigned long bar0, cmd_base, cmd_phys_base, virt_base;
> 
> New code should use 'void __iomem *' for ioremap return values, wherever 
> possible.

OK.


> 
> 
> >>  	ide_hwif_t *hwif;
> >>  	int h;
> >>  
> >> @@ -636,23 +632,27 @@ sgiioc4_ide_setup_pci_device(struct pci_
> >>  	}
> >>  
> >>  	/*  Get the CmdBlk and CtrlBlk Base Registers */
> >> -	base = pci_resource_start(dev, 0) + IOC4_CMD_OFFSET;
> >> -	ctl = pci_resource_start(dev, 0) + IOC4_CTRL_OFFSET;
> >> -	irqport = pci_resource_start(dev, 0) + IOC4_INTR_OFFSET;
> >> +	bar0 = pci_resource_start(dev, 0);
> >> +	virt_base = (unsigned long) ioremap(bar0, pci_resource_len(dev, 0));
> >> +	cmd_base = virt_base + IOC4_CMD_OFFSET;
> >> +	ctl = virt_base + IOC4_CTRL_OFFSET;
> >> +	irqport = virt_base + IOC4_INTR_OFFSET;
> >>  	dma_base = pci_resource_start(dev, 0) + IOC4_DMA_OFFSET;
> 
> ditto:  new bug here too

I'll add the check.


> 
> 
> >> -	if (!request_region(base, IOC4_CMD_CTL_BLK_SIZE, hwif->name)) {
> >> +	cmd_phys_base = bar0 + IOC4_CMD_OFFSET;
> >> +	if (!request_mem_region(cmd_phys_base, IOC4_CMD_CTL_BLK_SIZE,
> >> +	    hwif->name)) {
> >>  		printk(KERN_ERR
> >> -			"%s : %s -- ERROR, Port Addresses "
> >> +			"%s : %s -- ERROR, Addresses "
> >>  			"0x%p to 0x%p ALREADY in use\n",
> >> -		       __FUNCTION__, hwif->name, (void *) base,
> >> -		       (void *) base + IOC4_CMD_CTL_BLK_SIZE);
> >> +		       __FUNCTION__, hwif->name, (void *) cmd_phys_base,
> >> +		       (void *) cmd_phys_base + IOC4_CMD_CTL_BLK_SIZE);
> 
> If 'void __iomem *' were used, no casts would be needed here

So, 'void __iomem *' should also be used for physical (non-mapped)
addresses, as in this case?

John

Re: [PATCH 1/1] - sgiioc4: fixup use of mmio ops

[Jeff Garzik]

John Keller wrote:
>>>> -	if (!request_region(base, IOC4_CMD_CTL_BLK_SIZE, hwif->name)) {
>>>> +	cmd_phys_base = bar0 + IOC4_CMD_OFFSET;
>>>> +	if (!request_mem_region(cmd_phys_base, IOC4_CMD_CTL_BLK_SIZE,
>>>> +	    hwif->name)) {
>>>>  		printk(KERN_ERR
>>>> -			"%s : %s -- ERROR, Port Addresses "
>>>> +			"%s : %s -- ERROR, Addresses "
>>>>  			"0x%p to 0x%p ALREADY in use\n",
>>>> -		       __FUNCTION__, hwif->name, (void *) base,
>>>> -		       (void *) base + IOC4_CMD_CTL_BLK_SIZE);
>>>> +		       __FUNCTION__, hwif->name, (void *) cmd_phys_base,
>>>> +		       (void *) cmd_phys_base + IOC4_CMD_CTL_BLK_SIZE);
>> If 'void __iomem *' were used, no casts would be needed here
> 
> So, 'void __iomem *' should also be used for physical (non-mapped)
> addresses, as in this case?


Ooops, no, just cookies returned from ioremap() and ioremap_nocache().

	Jeff

[PATCH 1/1] - sgiioc4: fixup use of mmio ops

[John Keller]

Resend #2 changes include:

- use 'void __iomem *' for ioremap() return values,
  and handle error cases.

sgiioc4.c had been recently converted to using mmio ops.
There are still a few issues to cleanup.

Signed-off-by: jpk@sgi.com


Index: linux-2.6/drivers/ide/pci/sgiioc4.c
===================================================================
--- linux-2.6.orig/drivers/ide/pci/sgiioc4.c	2006-07-17 08:53:58.107664466 -0500
+++ linux-2.6/drivers/ide/pci/sgiioc4.c	2006-07-17 09:03:40.402118344 -0500
@@ -367,12 +367,13 @@ sgiioc4_INB(unsigned long port)
 static void __devinit
 ide_dma_sgiioc4(ide_hwif_t * hwif, unsigned long dma_base)
 {
+	void __iomem *virt_dma_base;
 	int num_ports = sizeof (ioc4_dma_regs_t);
 
 	printk(KERN_INFO "%s: BM-DMA at 0x%04lx-0x%04lx\n", hwif->name,
 	       dma_base, dma_base + num_ports - 1);
 
-	if (!request_region(dma_base, num_ports, hwif->name)) {
+	if (!request_mem_region(dma_base, num_ports, hwif->name)) {
 		printk(KERN_ERR
 		       "%s(%s) -- ERROR, Addresses 0x%p to 0x%p "
 		       "ALREADY in use\n",
@@ -381,13 +382,21 @@ ide_dma_sgiioc4(ide_hwif_t * hwif, unsig
 		goto dma_alloc_failure;
 	}
 
-	hwif->dma_base = dma_base;
+	virt_dma_base = ioremap(dma_base, num_ports);
+	if (virt_dma_base == NULL) {
+		printk(KERN_ERR
+		       "%s(%s) -- ERROR, Unable to map addresses 0x%lx to 0x%lx\n",
+		       __FUNCTION__, hwif->name, dma_base, dma_base + num_ports - 1);
+		goto dma_remap_failure;
+	}
+	hwif->dma_base = (unsigned long) virt_dma_base;
+
 	hwif->dmatable_cpu = pci_alloc_consistent(hwif->pci_dev,
 					  IOC4_PRD_ENTRIES * IOC4_PRD_BYTES,
 					  &hwif->dmatable_dma);
 
 	if (!hwif->dmatable_cpu)
-		goto dma_alloc_failure;
+		goto dma_remap_failure;
 
 	hwif->sg_max_nents = IOC4_PRD_ENTRIES;
 
@@ -411,6 +420,9 @@ dma_base2alloc_failure:
 	printk(KERN_INFO
 	       "Changing from DMA to PIO mode for Drive %s\n", hwif->name);
 
+dma_remap_failure:
+	release_mem_region(dma_base, num_ports);
+
 dma_alloc_failure:
 	/* Disable DMA because we couldnot allocate any DMA maps */
 	hwif->autodma = 0;
@@ -607,18 +619,15 @@ ide_init_sgiioc4(ide_hwif_t * hwif)
 	hwif->ide_dma_lostirq = &sgiioc4_ide_dma_lostirq;
 	hwif->ide_dma_timeout = &__ide_dma_timeout;
 
-	/*
-	 * The IOC4 uses MMIO rather than Port IO.
-	 * It also needs special workarounds for INB.
-	 */
-	default_hwif_mmiops(hwif);
 	hwif->INB = &sgiioc4_INB;
 }
 
 static int __devinit
 sgiioc4_ide_setup_pci_device(struct pci_dev *dev, ide_pci_device_t * d)
 {
-	unsigned long base, ctl, dma_base, irqport;
+	unsigned long cmd_base, dma_base, irqport;
+	unsigned long bar0, cmd_phys_base, ctl;
+	void __iomem *virt_base;
 	ide_hwif_t *hwif;
 	int h;
 
@@ -636,23 +645,32 @@ sgiioc4_ide_setup_pci_device(struct pci_
 	}
 
 	/*  Get the CmdBlk and CtrlBlk Base Registers */
-	base = pci_resource_start(dev, 0) + IOC4_CMD_OFFSET;
-	ctl = pci_resource_start(dev, 0) + IOC4_CTRL_OFFSET;
-	irqport = pci_resource_start(dev, 0) + IOC4_INTR_OFFSET;
+	bar0 = pci_resource_start(dev, 0);
+	virt_base = ioremap(bar0, pci_resource_len(dev, 0));
+	if (virt_base == NULL) {
+		printk(KERN_ERR "%s: Unable to remap BAR 0 address: 0x%lx\n",
+			d->name, bar0);
+		return -ENOMEM;
+	}
+	cmd_base = (unsigned long) virt_base + IOC4_CMD_OFFSET;
+	ctl = (unsigned long) virt_base + IOC4_CTRL_OFFSET;
+	irqport = (unsigned long) virt_base + IOC4_INTR_OFFSET;
 	dma_base = pci_resource_start(dev, 0) + IOC4_DMA_OFFSET;
 
-	if (!request_region(base, IOC4_CMD_CTL_BLK_SIZE, hwif->name)) {
+	cmd_phys_base = bar0 + IOC4_CMD_OFFSET;
+	if (!request_mem_region(cmd_phys_base, IOC4_CMD_CTL_BLK_SIZE,
+	    hwif->name)) {
 		printk(KERN_ERR
-			"%s : %s -- ERROR, Port Addresses "
+			"%s : %s -- ERROR, Addresses "
 			"0x%p to 0x%p ALREADY in use\n",
-		       __FUNCTION__, hwif->name, (void *) base,
-		       (void *) base + IOC4_CMD_CTL_BLK_SIZE);
+		       __FUNCTION__, hwif->name, (void *) cmd_phys_base,
+		       (void *) cmd_phys_base + IOC4_CMD_CTL_BLK_SIZE);
 		return -ENOMEM;
 	}
 
-	if (hwif->io_ports[IDE_DATA_OFFSET] != base) {
+	if (hwif->io_ports[IDE_DATA_OFFSET] != cmd_base) {
 		/* Initialize the IO registers */
-		sgiioc4_init_hwif_ports(&hwif->hw, base, ctl, irqport);
+		sgiioc4_init_hwif_ports(&hwif->hw, cmd_base, ctl, irqport);
 		memcpy(hwif->io_ports, hwif->hw.io_ports,
 		       sizeof (hwif->io_ports));
 		hwif->noprobe = !hwif->io_ports[IDE_DATA_OFFSET];
@@ -665,6 +683,9 @@ sgiioc4_ide_setup_pci_device(struct pci_
 	hwif->cds = (struct ide_pci_device_s *) d;
 	hwif->gendev.parent = &dev->dev;/* setup proper ancestral information */
 
+	/* The IOC4 uses MMIO rather than Port IO. */
+	default_hwif_mmiops(hwif);
+
 	/* Initializing chipset IRQ Registers */
 	hwif->OUTL(0x03, irqport + IOC4_INTR_SET * 4);
 

Re: [PATCH 1/1] - sgiioc4: fixup use of mmio ops

[Jeremy Higdon]

On Wed, Jul 19, 2006 at 10:31:55AM -0500, John Keller wrote:
> Resend #2 changes include:
> 
> - use 'void __iomem *' for ioremap() return values,
>   and handle error cases.
> 
> sgiioc4.c had been recently converted to using mmio ops.
> There are still a few issues to cleanup.
> 
> Signed-off-by: jpk@sgi.com
Signed-off-by: jeremy@sgi.com

Re: [PATCH 1/1] - sgiioc4: fixup use of mmio ops

[Andrew Morton]

On Wed, 19 Jul 2006 22:19:29 -0700
Jeremy Higdon <jeremy@sgi.com> wrote:

> On Wed, Jul 19, 2006 at 10:31:55AM -0500, John Keller wrote:
> > Resend #2 changes include:
> > 
> > - use 'void __iomem *' for ioremap() return values,
> >   and handle error cases.
> > 
> > sgiioc4.c had been recently converted to using mmio ops.
> > There are still a few issues to cleanup.
> > 
> > Signed-off-by: jpk@sgi.com
> Signed-off-by: jeremy@sgi.com

hrm.  I think we're owed a couple of iounmap()s, please.  One in the
ide_dma_sgiioc4() error path and one in the takedown path.

Maybe that was one of the "few issues to cleanup", but it's a pretty simple
one.

[PATCH 1/1] - sgiioc4: fixup use of mmio ops

[John Keller]

Resend #3

 - Add iounmap() call for dma failure case.
   This driver cannot be unloaded, so no additional
   iounmap calls needed.

Resend #2 changes include:

- use 'void __iomem *' for ioremap() return values,
  and handle error cases.

sgiioc4.c had been recently converted to using mmio ops.
There are still a few issues to cleanup.

Signed-off-by: jpk@sgi.com


Index: linux-2.6/drivers/ide/pci/sgiioc4.c
===================================================================
--- linux-2.6.orig/drivers/ide/pci/sgiioc4.c	2006-07-21 13:03:29.120164007 -0500
+++ linux-2.6/drivers/ide/pci/sgiioc4.c	2006-07-21 13:03:48.058314086 -0500
@@ -367,12 +367,13 @@ sgiioc4_INB(unsigned long port)
 static void __devinit
 ide_dma_sgiioc4(ide_hwif_t * hwif, unsigned long dma_base)
 {
+	void __iomem *virt_dma_base;
 	int num_ports = sizeof (ioc4_dma_regs_t);
 
 	printk(KERN_INFO "%s: BM-DMA at 0x%04lx-0x%04lx\n", hwif->name,
 	       dma_base, dma_base + num_ports - 1);
 
-	if (!request_region(dma_base, num_ports, hwif->name)) {
+	if (!request_mem_region(dma_base, num_ports, hwif->name)) {
 		printk(KERN_ERR
 		       "%s(%s) -- ERROR, Addresses 0x%p to 0x%p "
 		       "ALREADY in use\n",
@@ -381,13 +382,21 @@ ide_dma_sgiioc4(ide_hwif_t * hwif, unsig
 		goto dma_alloc_failure;
 	}
 
-	hwif->dma_base = dma_base;
+	virt_dma_base = ioremap(dma_base, num_ports);
+	if (virt_dma_base == NULL) {
+		printk(KERN_ERR
+		       "%s(%s) -- ERROR, Unable to map addresses 0x%lx to 0x%lx\n",
+		       __FUNCTION__, hwif->name, dma_base, dma_base + num_ports - 1);
+		goto dma_remap_failure;
+	}
+	hwif->dma_base = (unsigned long) virt_dma_base;
+
 	hwif->dmatable_cpu = pci_alloc_consistent(hwif->pci_dev,
 					  IOC4_PRD_ENTRIES * IOC4_PRD_BYTES,
 					  &hwif->dmatable_dma);
 
 	if (!hwif->dmatable_cpu)
-		goto dma_alloc_failure;
+		goto dma_pci_alloc_failure;
 
 	hwif->sg_max_nents = IOC4_PRD_ENTRIES;
 
@@ -411,6 +420,12 @@ dma_base2alloc_failure:
 	printk(KERN_INFO
 	       "Changing from DMA to PIO mode for Drive %s\n", hwif->name);
 
+dma_pci_alloc_failure:
+	iounmap(virt_dma_base);
+
+dma_remap_failure:
+	release_mem_region(dma_base, num_ports);
+
 dma_alloc_failure:
 	/* Disable DMA because we couldnot allocate any DMA maps */
 	hwif->autodma = 0;
@@ -607,18 +622,15 @@ ide_init_sgiioc4(ide_hwif_t * hwif)
 	hwif->ide_dma_lostirq = &sgiioc4_ide_dma_lostirq;
 	hwif->ide_dma_timeout = &__ide_dma_timeout;
 
-	/*
-	 * The IOC4 uses MMIO rather than Port IO.
-	 * It also needs special workarounds for INB.
-	 */
-	default_hwif_mmiops(hwif);
 	hwif->INB = &sgiioc4_INB;
 }
 
 static int __devinit
 sgiioc4_ide_setup_pci_device(struct pci_dev *dev, ide_pci_device_t * d)
 {
-	unsigned long base, ctl, dma_base, irqport;
+	unsigned long cmd_base, dma_base, irqport;
+	unsigned long bar0, cmd_phys_base, ctl;
+	void __iomem *virt_base;
 	ide_hwif_t *hwif;
 	int h;
 
@@ -636,23 +648,32 @@ sgiioc4_ide_setup_pci_device(struct pci_
 	}
 
 	/*  Get the CmdBlk and CtrlBlk Base Registers */
-	base = pci_resource_start(dev, 0) + IOC4_CMD_OFFSET;
-	ctl = pci_resource_start(dev, 0) + IOC4_CTRL_OFFSET;
-	irqport = pci_resource_start(dev, 0) + IOC4_INTR_OFFSET;
+	bar0 = pci_resource_start(dev, 0);
+	virt_base = ioremap(bar0, pci_resource_len(dev, 0));
+	if (virt_base == NULL) {
+		printk(KERN_ERR "%s: Unable to remap BAR 0 address: 0x%lx\n",
+			d->name, bar0);
+		return -ENOMEM;
+	}
+	cmd_base = (unsigned long) virt_base + IOC4_CMD_OFFSET;
+	ctl = (unsigned long) virt_base + IOC4_CTRL_OFFSET;
+	irqport = (unsigned long) virt_base + IOC4_INTR_OFFSET;
 	dma_base = pci_resource_start(dev, 0) + IOC4_DMA_OFFSET;
 
-	if (!request_region(base, IOC4_CMD_CTL_BLK_SIZE, hwif->name)) {
+	cmd_phys_base = bar0 + IOC4_CMD_OFFSET;
+	if (!request_mem_region(cmd_phys_base, IOC4_CMD_CTL_BLK_SIZE,
+	    hwif->name)) {
 		printk(KERN_ERR
-			"%s : %s -- ERROR, Port Addresses "
+			"%s : %s -- ERROR, Addresses "
 			"0x%p to 0x%p ALREADY in use\n",
-		       __FUNCTION__, hwif->name, (void *) base,
-		       (void *) base + IOC4_CMD_CTL_BLK_SIZE);
+		       __FUNCTION__, hwif->name, (void *) cmd_phys_base,
+		       (void *) cmd_phys_base + IOC4_CMD_CTL_BLK_SIZE);
 		return -ENOMEM;
 	}
 
-	if (hwif->io_ports[IDE_DATA_OFFSET] != base) {
+	if (hwif->io_ports[IDE_DATA_OFFSET] != cmd_base) {
 		/* Initialize the IO registers */
-		sgiioc4_init_hwif_ports(&hwif->hw, base, ctl, irqport);
+		sgiioc4_init_hwif_ports(&hwif->hw, cmd_base, ctl, irqport);
 		memcpy(hwif->io_ports, hwif->hw.io_ports,
 		       sizeof (hwif->io_ports));
 		hwif->noprobe = !hwif->io_ports[IDE_DATA_OFFSET];
@@ -665,6 +686,9 @@ sgiioc4_ide_setup_pci_device(struct pci_
 	hwif->cds = (struct ide_pci_device_s *) d;
 	hwif->gendev.parent = &dev->dev;/* setup proper ancestral information */
 
+	/* The IOC4 uses MMIO rather than Port IO. */
+	default_hwif_mmiops(hwif);
+
 	/* Initializing chipset IRQ Registers */
 	hwif->OUTL(0x03, irqport + IOC4_INTR_SET * 4);