[PATCH] highmem I/O for ide-pmac.c

[PATCH] highmem I/O for ide-pmac.c

[Paul Mackerras]

Marcelo,

This patch fixes drivers/ide/ide-pmac.c to handle I/O to highmem
pages.  Please apply it to your tree.

Thanks,
Paul.

diff -urN linux-2.4/drivers/ide/ide-pmac.c linuxppc_2_4/drivers/ide/ide-pmac.c
--- linux-2.4/drivers/ide/ide-pmac.c	Wed Aug  7 18:09:29 2002
+++ linuxppc_2_4/drivers/ide/ide-pmac.c	Sun Aug 18 22:12:10 2002
@@ -1032,33 +1032,48 @@
 	struct pmac_ide_hwif *pmif = &pmac_ide[ix];
 	struct buffer_head *bh;
 	struct scatterlist *sg = pmif->sg_table;
+	unsigned long lastdataend = ~0UL;
 	int nents = 0;
 
 	if (hwif->sg_dma_active)
 		BUG();
-		
+
 	if (rq->cmd == READ)
 		pmif->sg_dma_direction = PCI_DMA_FROMDEVICE;
 	else
 		pmif->sg_dma_direction = PCI_DMA_TODEVICE;
+
 	bh = rq->bh;
 	do {
-		unsigned char *virt_addr = bh->b_data;
+		struct scatterlist *sge;
 		unsigned int size = bh->b_size;
 
+		/* continue segment from before? */
+		if (bh_phys(bh) == lastdataend) {
+			sg[nents-1].length += size;
+			lastdataend += size;
+			continue;
+		}
+
+		/* start new segment */
 		if (nents >= MAX_DCMDS)
 			return 0;
 
-		while ((bh = bh->b_reqnext) != NULL) {
-			if ((virt_addr + size) != (unsigned char *) bh->b_data)
-				break;
-			size += bh->b_size;
+		sge = &sg[nents];
+		memset(sge, 0, sizeof(*sge));
+
+		if (bh->b_page) {
+			sge->page = bh->b_page;
+			sge->offset = bh_offset(bh);
+		} else {
+			if ((unsigned long)bh->b_data < PAGE_SIZE)
+				BUG();
+			sge->address = bh->b_data;
 		}
-		memset(&sg[nents], 0, sizeof(*sg));
-		sg[nents].address = virt_addr;
-		sg[nents].length = size;
+		sge->length = size;
+		lastdataend = bh_phys(bh) + size;
 		nents++;
-	} while (bh != NULL);
+	} while ((bh = bh->b_reqnext) != NULL);
 
 	return pci_map_sg(hwif->pci_dev, sg, nents, pmif->sg_dma_direction);
 }
@@ -1330,6 +1345,23 @@
 	return 0;
 }
 
+static inline void pmac_ide_toggle_bounce(ide_drive_t *drive, int on)
+{
+	dma64_addr_t addr = BLK_BOUNCE_HIGH;
+
+	if (HWIF(drive)->no_highio || HWIF(drive)->pci_dev == NULL)
+		return;
+
+	if (on && drive->media == ide_disk) {
+		if (!PCI_DMA_BUS_IS_PHYS)
+			addr = BLK_BOUNCE_ANY;
+		else
+			addr = HWIF(drive)->pci_dev->dma_mask;
+	}
+
+	blk_queue_bounce_limit(&drive->queue, addr);
+}
+
 static int __pmac
 pmac_ide_dmaproc(ide_dma_action_t func, ide_drive_t *drive)
 {
@@ -1354,6 +1386,7 @@
 		printk(KERN_INFO "%s: DMA disabled\n", drive->name);
 	case ide_dma_off_quietly:
 		drive->using_dma = 0;
+		pmac_ide_toggle_bounce(drive, 0);
 		break;
 	case ide_dma_on:
 	case ide_dma_check:

Re: [PATCH] highmem I/O for ide-pmac.c

[Jens Axboe]

On Wed, Sep 11 2002, Paul Mackerras wrote:
> Marcelo,
> 
> This patch fixes drivers/ide/ide-pmac.c to handle I/O to highmem
> pages.  Please apply it to your tree.

Doesn't look like it's needed at all, at least you never turn on highmem
I/O with ide_toggle_bounce() :-)

BTW, it would be ok to export that from ide-dma.c instead of duplicating
it in ide-pmac.

Also, can you grow sg segments indefinitely?

-- 
Jens Axboe

Re: [PATCH] highmem I/O for ide-pmac.c

[Jens Axboe]

On Wed, Sep 11 2002, Jens Axboe wrote:
> Also, can you grow sg segments indefinitely?

Maybe you copied that from ide-dma? I think it would be safer to just
remove it, btw, there's no (if any) benefit to making the sg segments
bigger than a page since we'll much sooner hit the max sectors
limitation than the segment one.

Marcelo, please apply.

--- drivers/ide/ide-dma.c~	2002-09-11 15:06:48.000000000 +0200
+++ drivers/ide/ide-dma.c	2002-09-11 15:07:26.000000000 +0200
@@ -268,15 +268,6 @@
 		struct scatterlist *sge;
 
 		/*
-		 * continue segment from before?
-		 */
-		if (bh_phys(bh) == lastdataend) {
-			sg[nents - 1].length += bh->b_size;
-			lastdataend += bh->b_size;
-			continue;
-		}
-
-		/*
 		 * start new segment
 		 */
 		if (nents >= PRD_ENTRIES)

-- 
Jens Axboe

Re: [PATCH] highmem I/O for ide-pmac.c

[Jens Axboe]

On Wed, Sep 11 2002, Jens Axboe wrote:
> On Wed, Sep 11 2002, Jens Axboe wrote:
> > Also, can you grow sg segments indefinitely?
> 
> Maybe you copied that from ide-dma? I think it would be safer to just
> remove it, btw, there's no (if any) benefit to making the sg segments
> bigger than a page since we'll much sooner hit the max sectors
> limitation than the segment one.

ide_build_dmatable() makes sure that it is safe, so no extra checking is
needed. So we can keep current behaviour and its not a bug and no
changes are needed in either ide-dma nor ide-pmac.

-- 
Jens Axboe

Re: [PATCH] highmem I/O for ide-pmac.c

[Benjamin Herrenschmidt]

>BTW, it would be ok to export that from ide-dma.c instead of duplicating
>it in ide-pmac.

It isn't. ide-pmac doesn't use the sg_table & other DMA related
fields in HWIF, but it's own copies in the "pmif" which is a
parallel data structure.

It sucks, I know, but I have to do that with the current IDE code
as the common code would make assumption about the format of these
things and it's right to dispose them in ide_unregister.

Also, Jens is right, Paul, you never call pmac_ide_toggle_bounce()
to actually enable high IOs. Add that to the ide_dma_on/check: case,
with an if (drive->using_dma) (as enabling DMA may have failed)

Or just wait for me to finish fixing my 6xx power save on idle
stuff and I'll send an updated patch to Marcelo along with other
pmac stuffs. (I'm implementing your suggestion of comparing SRR0
content in transfer_to_handler, but so far, the comparison never
seem to match, I'll find that out soon enough though).

Ben.

Re: [PATCH] highmem I/O for ide-pmac.c

[Jens Axboe]

On Wed, Sep 11 2002, Benjamin Herrenschmidt wrote:
> >BTW, it would be ok to export that from ide-dma.c instead of duplicating
> >it in ide-pmac.
> 
> It isn't. ide-pmac doesn't use the sg_table & other DMA related
> fields in HWIF, but it's own copies in the "pmif" which is a
> parallel data structure.

The above refers to ide_toggle_bounce() export, pmac_* variant is
exactly the same. Sorry if that wasn't clear.

> It sucks, I know, but I have to do that with the current IDE code
> as the common code would make assumption about the format of these
> things and it's right to dispose them in ide_unregister.
> 
> Also, Jens is right, Paul, you never call pmac_ide_toggle_bounce()
> to actually enable high IOs. Add that to the ide_dma_on/check: case,
> with an if (drive->using_dma) (as enabling DMA may have failed)

Indeed

-- 
Jens Axboe

Re: [PATCH] highmem I/O for ide-pmac.c

[Paul Mackerras]

Jens Axboe writes:

> The above refers to ide_toggle_bounce() export, pmac_* variant is
> exactly the same. Sorry if that wasn't clear.

Why does ide_toggle_bounce assume we can only do DMA to highmem if
drive->media == ide_disk?  At the moment I can't see any reason why
the ide-pmac interface can't do DMA to highmem for a cdrom, for
instance.

Paul.

Re: [PATCH] highmem I/O for ide-pmac.c

[Jens Axboe]

On Thu, Sep 12 2002, Paul Mackerras wrote:
> Jens Axboe writes:
> 
> > The above refers to ide_toggle_bounce() export, pmac_* variant is
> > exactly the same. Sorry if that wasn't clear.
> 
> Why does ide_toggle_bounce assume we can only do DMA to highmem if
> drive->media == ide_disk?  At the moment I can't see any reason why
> the ide-pmac interface can't do DMA to highmem for a cdrom, for
> instance.

Because it requires changes to the personality driver (ie ide-disk,
ide-cd, etc). I only did those changes to ide-disk, didn't figure it was
worthwhile to do for ide-cd for instance. It's not exactly a
high-performance media :-)

If the device is always in dma mode, no changes are needed. It's when
you drop to pio the problem arises, and all the interrupt handlers need
to be fixed.

-- 
Jens Axboe

Re: [PATCH] highmem I/O for ide-pmac.c

[Paul Mackerras]

Jens Axboe writes:

> Doesn't look like it's needed at all, at least you never turn on highmem
> I/O with ide_toggle_bounce() :-)

Foo, neither I do. :(

> BTW, it would be ok to export that from ide-dma.c instead of duplicating
> it in ide-pmac.

Looking at it again, both ide_build_sglist and ide_raw_build_sglist do
*almost* what we want.  If ide-pmac used hwif->sg_table instead of
pmif->sg_table, and if ide_[raw_]build_sglist were exported and took
the maximum number of entries as a parameter instead of using the
PRD_ENTRIES constant, then ide-pmac wouldn't need to have its own
versions of those routines.  Would those changes be OK?

Ben, any reason why we have to use pmif->sg_table rather than
hwif->sg_table?

> Also, can you grow sg segments indefinitely?

Each DBDMA (descriptor-based DMA) command has a 16-byte length field,
so is limited to 65535 bytes.  There is code in pmac_ide_build_dmatable
to create multiple DBDMA commands from a single scatterlist element if
necessary.  We should limit the lengths of the segments instead, then
we could have a 1-1 correspondence of scatterlist elements to DBDMA
command blocks.

Paul.

Re: [PATCH] highmem I/O for ide-pmac.c

[Jens Axboe]

On Thu, Sep 12 2002, Paul Mackerras wrote:
> Jens Axboe writes:
> 
> > Doesn't look like it's needed at all, at least you never turn on highmem
> > I/O with ide_toggle_bounce() :-)
> 
> Foo, neither I do. :(
> 
> > BTW, it would be ok to export that from ide-dma.c instead of duplicating
> > it in ide-pmac.
> 
> Looking at it again, both ide_build_sglist and ide_raw_build_sglist do
> *almost* what we want.  If ide-pmac used hwif->sg_table instead of
> pmif->sg_table, and if ide_[raw_]build_sglist were exported and took
> the maximum number of entries as a parameter instead of using the
> PRD_ENTRIES constant, then ide-pmac wouldn't need to have its own
> versions of those routines.  Would those changes be OK?

Sounds like a perfectly fine change to me.

> Ben, any reason why we have to use pmif->sg_table rather than
> hwif->sg_table?

Looks identical to me. hwif->sg_table is kmalloc'ed sg list of
PRD_ENTRIES (256), pmif->sg_table is kmalloc'ed ditto of MAX_DCMDS (256)
entries.

-- 
Jens Axboe

Re: [PATCH] highmem I/O for ide-pmac.c

[Benjamin Herrenschmidt]

>> Looking at it again, both ide_build_sglist and ide_raw_build_sglist do
>> *almost* what we want.  If ide-pmac used hwif->sg_table instead of
>> pmif->sg_table, and if ide_[raw_]build_sglist were exported and took
>> the maximum number of entries as a parameter instead of using the
>> PRD_ENTRIES constant, then ide-pmac wouldn't need to have its own
>> versions of those routines.  Would those changes be OK?
>
>Sounds like a perfectly fine change to me.
>
>> Ben, any reason why we have to use pmif->sg_table rather than
>> hwif->sg_table?
>
>Looks identical to me. hwif->sg_table is kmalloc'ed sg list of
>PRD_ENTRIES (256), pmif->sg_table is kmalloc'ed ditto of MAX_DCMDS (256)
>entries.

Well, I decided to move all of those to pmif when I had the media
bay broken because ide_unregister calling ide_release_dma which
disposed of the tables behind my back.

Looking at ide.c in it's current incarnation (2.4.20pre), it seems
the common code will only play such tricks if hwif->dma_base is
non-NULL, in which case it assumes a PRD-style DMA.

So if we keep hwif->dma_base to 0, then we can probably go back
to using the hwif fields for sg_* and thus share the routines
with ide-dma.

I'd suggest you don't bother too much with that now. I'm working
with andre on his new IDE stuff in which I already did some
cleanup work on ide-pmac, I'll add that to it next week. That
code should ultimately move to both 2.4 and 2.5 (by 2.4.21 time
frame I beleive).

Ben.