Re: [PATCH] IDE 58

Re: [PATCH] IDE 58

[Benjamin Herrenschmidt]

(resent, I had the date screwed up previously, sorry about the
inconvenience).

>I don't see why all IDE-interfaces in the world have to be I/O-mapped just 
>because the first PC implementations used that. Sure it was an extended 
>ISA-bus but the ISA bus is long gone and we don't all run PC's anymore 
>either.
>
>So the simple abstraction we need to hit IDE-bus registers is a macro or 
>inline, instead of a call of an I/O-primitive. It was too much work to 
>abstract this when I inserted the CRIS-arch IDE-driver in the first place 
>so I found a workaround but now seems like a better time..

No, not a macro. There are cases where you want different access methods
on the same machine. For example, pmacs can have the "mac-io" (ide-pmac)
controller, which is MMIO based, _and_ a PCI-based legacy IDE controller
using inx/outx like IOs. (A typical example is the Blue&White G3 who has
both on the motherboard).

Ultimately, you want the hwif (or what it becomes in 2.5) provide a set
of functions for accessing taskfile registers and doing the PIO data
stream read/writes (that is replace inb/outb and insw/outsw).

Re: [PATCH] IDE 58

[Martin Dalecki]

Uz.ytkownik Benjamin Herrenschmidt napisa?:
> (resent, I had the date screwed up previously, sorry about the
> inconvenience).
> 
> 
>>I don't see why all IDE-interfaces in the world have to be I/O-mapped just 
>>because the first PC implementations used that. Sure it was an extended 
>>ISA-bus but the ISA bus is long gone and we don't all run PC's anymore 
>>either.
>>
>>So the simple abstraction we need to hit IDE-bus registers is a macro or 
>>inline, instead of a call of an I/O-primitive. It was too much work to 
>>abstract this when I inserted the CRIS-arch IDE-driver in the first place 
>>so I found a workaround but now seems like a better time..
> 
> 
> No, not a macro. There are cases where you want different access methods
> on the same machine. For example, pmacs can have the "mac-io" (ide-pmac)
> controller, which is MMIO based, _and_ a PCI-based legacy IDE controller
> using inx/outx like IOs. (A typical example is the Blue&White G3 who has
> both on the motherboard).
> 
> Ultimately, you want the hwif (or what it becomes in 2.5) provide a set
> of functions for accessing taskfile registers and doing the PIO data
> stream read/writes (that is replace inb/outb and insw/outsw).

Terminology in 2.5:
We have a host chip set or shortly a host chip. This is implementing the
ATA interface on the side of the motherboard.
The host chip is providing two channels. A primary and a secondary
one. To a channel we can attach two devices, however we use the term
drive instead in code becouse the termi device is quite overloaded with
meaning already. The devices are enumerated as units. That's it.
Far more natural then hwif hwgrp and so on. IDE is the Integrated Device
Electronic - the microcontroller stuff I don't care that much about.

Re: [PATCH] IDE 58

[Anton Altaparmakov]

At 11:25 08/05/02, Martin Dalecki wrote:
>Terminology in 2.5:
>We have a host chip set or shortly a host chip. This is implementing the
>ATA interface on the side of the motherboard.
>The host chip is providing two channels. A primary and a secondary
>one. To a channel we can attach two devices, however we use the term
>drive instead in code becouse the termi device is quite overloaded with
>meaning already. The devices are enumerated as units. That's it.
>Far more natural then hwif hwgrp and so on. IDE is the Integrated Device
>Electronic - the microcontroller stuff I don't care that much about.

</me ignorant>Um, what about the IDE PCI cards which have 4 channels on 
them? Like these two:

Adaptec 2400 4Ch IDE Raid Controller
RocketRaid 404 4Ch ATA133 Raid Host Adaptor

Best regards,

         Anton

ps Sorry about the outburst yesterday, I was tired and just flipped...


-- 
   "I've not lost my mind. It's backed up on tape somewhere." - Unknown
-- 
Anton Altaparmakov <aia21 at cantab.net> (replace at with @)
Linux NTFS Maintainer / IRC: #ntfs on irc.openprojects.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/

Re: [PATCH] IDE 58

[Andre Hedrick]

On Wed, 8 May 2002, Anton Altaparmakov wrote:

> </me ignorant>Um, what about the IDE PCI cards which have 4 channels on 
> them? Like these two:
> 
> Adaptec 2400 4Ch IDE Raid Controller
> RocketRaid 404 4Ch ATA133 Raid Host Adaptor

It is not an issue since they broadcast as single channel pairs per host.
Martin is winning the argument hands down.

Andre Hedrick
LAD Storage Consulting Group

Re: [PATCH] IDE 58

[Martin Dalecki]

Uz.ytkownik Anton Altaparmakov napisa?:
> At 11:25 08/05/02, Martin Dalecki wrote:
> 
>> Terminology in 2.5:
>> We have a host chip set or shortly a host chip. This is implementing the
>> ATA interface on the side of the motherboard.
>> The host chip is providing two channels. A primary and a secondary
>> one. To a channel we can attach two devices, however we use the term
>> drive instead in code becouse the termi device is quite overloaded with
>> meaning already. The devices are enumerated as units. That's it.
>> Far more natural then hwif hwgrp and so on. IDE is the Integrated Device
>> Electronic - the microcontroller stuff I don't care that much about.
> 
> 
> </me ignorant>Um, what about the IDE PCI cards which have 4 channels on 
> them? Like these two:
> 
> Adaptec 2400 4Ch IDE Raid Controller
> RocketRaid 404 4Ch ATA133 Raid Host Adaptor

They appear as SCSI on the host side.

Re: [PATCH] IDE 58

[Alan Cox]

> > Adaptec 2400 4Ch IDE Raid Controller
> > RocketRaid 404 4Ch ATA133 Raid Host Adaptor
> 
> They appear as SCSI on the host side.

Actually the adaptec appears as a block device.

Re: [PATCH] IDE 58

[Anton Altaparmakov]

At 19:55 08/05/02, Andre Hedrick wrote:
>On Wed, 8 May 2002, Anton Altaparmakov wrote:
>
> > </me ignorant>Um, what about the IDE PCI cards which have 4 channels on
> > them? Like these two:
> >
> > Adaptec 2400 4Ch IDE Raid Controller
> > RocketRaid 404 4Ch ATA133 Raid Host Adaptor
>
>It is not an issue since they broadcast as single channel pairs per host.
>Martin is winning the argument hands down.

Thanks, I was just wondering not trying to argument...

Best regards,

         Anton


-- 
   "I've not lost my mind. It's backed up on tape somewhere." - Unknown
-- 
Anton Altaparmakov <aia21 at cantab.net> (replace at with @)
Linux NTFS Maintainer / IRC: #ntfs on irc.openprojects.net
WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/

Re: [PATCH] IDE 58

[Andre Hedrick]

Martin,

You have a practical model that is coming togather nicely (compliments
from the old man), there is a change in the industry to use MMIO based ATA
HBA's.  We are currently in a transistion state of affairs, so the problem
Benjamin address that everyone has overlooked is going to bite hard and
soon.  There will even be HBA's whose channels will be split between IOMIO
and MMIO, thus being able to select between access calls is urgent.

Cheers,


Andre Hedrick
LAD Storage Consulting Group

On Wed, 8 May 2002, Anton Altaparmakov wrote:

> At 19:55 08/05/02, Andre Hedrick wrote:
> >On Wed, 8 May 2002, Anton Altaparmakov wrote:
> >
> > > </me ignorant>Um, what about the IDE PCI cards which have 4 channels on
> > > them? Like these two:
> > >
> > > Adaptec 2400 4Ch IDE Raid Controller
> > > RocketRaid 404 4Ch ATA133 Raid Host Adaptor
> >
> >It is not an issue since they broadcast as single channel pairs per host.
> >Martin is winning the argument hands down.
> 
> Thanks, I was just wondering not trying to argument...
> 
> Best regards,
> 
>          Anton
> 
> 
> -- 
>    "I've not lost my mind. It's backed up on tape somewhere." - Unknown
> -- 
> Anton Altaparmakov <aia21 at cantab.net> (replace at with @)
> Linux NTFS Maintainer / IRC: #ntfs on irc.openprojects.net
> WWW: http://linux-ntfs.sf.net/ & http://www-stu.christs.cam.ac.uk/~aia21/
> 

Linux-2.5.14..

[Linus Torvalds]

There's a lot of stuff that has happened in the 2.5.x series lately, and
you can see the gory details in the ChangeLog files that accompany
releases these days, but I thought I'd point out 2.5.14, since it has some
interesting fundamental changes to how dirty state is maintained in the
VM.

(The big changes were actually in 2.5.12, but 2.5.13 contained various
minor fixes and tweaks, and 2.5.14 contains a number of fixes especially
wrt truncate, so hopefully it's fairly _stable_ as of 2.5.14.)

Credit goes to Andrew Morton, and not only does it clean up the code a
lot, it also seems to perform a lot better in many circumstances.

There's a lot of other stuff in the 2.5.x tree too, but few things are so
fundamental. Please test (but also, please be careful - backups are always
a good idea).

		Linus

[PATCH] IDE 58

[Martin Dalecki]

[-- Attachment #1: Type: text/plain, Size: 629 bytes --]

Tue May  7 14:28:47 CEST 2002 ide-clean-58

- Apply m68k fixes by Roman Zippel.

- Apply CDROM PIO mode fix by Osamu Tamita.
   (You are true "Hawk-eye" hovering over my head! Respect - and many Thanks.)

- Virtualize the udma_enable method as well to help ARM and PPC people.  Please
   please if you would like to have some other methods virtualized in a similar
   way - just tell me or even better do it yourself at the end of ide-dma.c.
   I *don't mind* patches.

- Fix the pmac code to adhere to the new API. It's supposed to work again.
   However this is blind coding... I give myself 80% chances for it to work ;-).




[-- Attachment #2: ide-clean-58.diff --]
[-- Type: text/plain, Size: 18516 bytes --]

diff -urN linux-2.5.14/drivers/ide/buddha.c linux/drivers/ide/buddha.c
--- linux-2.5.14/drivers/ide/buddha.c	2002-05-06 05:38:01.000000000 +0200
+++ linux/drivers/ide/buddha.c	2002-05-07 15:41:02.000000000 +0200
@@ -1,10 +1,10 @@
 /*
  *  linux/drivers/ide/buddha.c -- Amiga Buddha, Catweasel and X-Surf IDE Driver
  *
- *	Copyright (C) 1997 by Geert Uytterhoeven
+ *	Copyright (C) 1997, 2001 by Geert Uytterhoeven and others
  *
- *  This driver was written by based on the specifications in README.buddha and
- *  the X-Surf info from Inside_XSurf.txt available at 
+ *  This driver was written based on the specifications in README.buddha and
+ *  the X-Surf info from Inside_XSurf.txt available at
  *  http://www.jschoenfeld.com
  *
  *  This file is subject to the terms and conditions of the GNU General Public
@@ -52,7 +52,7 @@
     BUDDHA_BASE1, BUDDHA_BASE2, BUDDHA_BASE3
 };
 
-static const u_int xsurf_bases[XSURF_NUM_HWIFS] __initdata = {
+static u_int xsurf_bases[XSURF_NUM_HWIFS] __initdata = {
      XSURF_BASE1, XSURF_BASE2
 };
 
@@ -97,7 +97,7 @@
     BUDDHA_IRQ1, BUDDHA_IRQ2, BUDDHA_IRQ3
 };
 
-static const int xsurf_irqports[XSURF_NUM_HWIFS] __initdata = {
+static int xsurf_irqports[XSURF_NUM_HWIFS] __initdata = {
     XSURF_IRQ1, XSURF_IRQ2
 };
 
@@ -108,8 +108,9 @@
      *  Board information
      */
 
-enum BuddhaType_Enum {BOARD_BUDDHA, BOARD_CATWEASEL, BOARD_XSURF};
-typedef enum BuddhaType_Enum BuddhaType;
+typedef enum BuddhaType_Enum {
+    BOARD_BUDDHA, BOARD_CATWEASEL, BOARD_XSURF
+} BuddhaType;
 
 
     /*
@@ -175,15 +176,20 @@
 			if (!request_mem_region(board+XSURF_BASE1, 0x1000, "IDE"))
 				continue;
 			if (!request_mem_region(board+XSURF_BASE2, 0x1000, "IDE"))
+				goto fail_base2;
+			if (!request_mem_region(board+XSURF_IRQ1, 0x8, "IDE")) {
+				release_mem_region(board+XSURF_BASE2, 0x1000);
+fail_base2:
+				release_mem_region(board+XSURF_BASE1, 0x1000);
 				continue;
-			if (!request_mem_region(board+XSURF_IRQ1, 0x8, "IDE"))
-				continue;
+			}
 		}	  
 		buddha_board = ZTWO_VADDR(board);
 		
 		/* write to BUDDHA_IRQ_MR to enable the board IRQ */
 		/* X-Surf doesn't have this.  IRQs are always on */
-		if(type != BOARD_XSURF) *(char *)(buddha_board+BUDDHA_IRQ_MR) = 0;
+		if (type != BOARD_XSURF)
+			z_writeb(0, buddha_board+BUDDHA_IRQ_MR);
 		
 		for(i=0;i<buddha_num_hwifs;i++) {
 			if(type != BOARD_XSURF) {
diff -urN linux-2.5.14/drivers/ide/Config.in linux/drivers/ide/Config.in
--- linux-2.5.14/drivers/ide/Config.in	2002-05-07 17:56:57.000000000 +0200
+++ linux/drivers/ide/Config.in	2002-05-07 15:41:02.000000000 +0200
@@ -103,7 +103,7 @@
       dep_mbool '    Amiga IDE Doubler support (EXPERIMENTAL)' CONFIG_BLK_DEV_IDEDOUBLER $CONFIG_BLK_DEV_GAYLE $CONFIG_EXPERIMENTAL
    fi
    if [ "$CONFIG_ZORRO" = "y" -a "$CONFIG_EXPERIMENTAL" = "y" ]; then
-      dep_mbool '  Buddha/Catweasel IDE interface support (EXPERIMENTAL)' CONFIG_BLK_DEV_BUDDHA $CONFIG_ZORRO $CONFIG_EXPERIMENTAL
+      dep_mbool '  Buddha/Catweasel/X-Surf IDE interface support (EXPERIMENTAL)' CONFIG_BLK_DEV_BUDDHA $CONFIG_ZORRO $CONFIG_EXPERIMENTAL
    fi
    if [ "$CONFIG_ATARI" = "y" ]; then
       dep_bool '  Falcon IDE interface support' CONFIG_BLK_DEV_FALCON_IDE $CONFIG_ATARI
diff -urN linux-2.5.14/drivers/ide/falconide.c linux/drivers/ide/falconide.c
--- linux-2.5.14/drivers/ide/falconide.c	2002-05-06 05:38:01.000000000 +0200
+++ linux/drivers/ide/falconide.c	2002-05-07 15:41:02.000000000 +0200
@@ -7,7 +7,7 @@
  *  License.  See the file COPYING in the main directory of this archive for
  *  more details.
  */
-#include <linux/config.h>
+
 #include <linux/types.h>
 #include <linux/mm.h>
 #include <linux/interrupt.h>
diff -urN linux-2.5.14/drivers/ide/ide.c linux/drivers/ide/ide.c
--- linux-2.5.14/drivers/ide/ide.c	2002-05-07 17:57:04.000000000 +0200
+++ linux/drivers/ide/ide.c	2002-05-07 15:48:53.000000000 +0200
@@ -2097,6 +2097,7 @@
 	ch->atapi_read = old.atapi_read;
 	ch->atapi_write = old.atapi_write;
 	ch->XXX_udma = old.XXX_udma;
+	ch->udma_enable = old.udma_enable;
 	ch->udma_start = old.udma_start;
 	ch->udma_stop = old.udma_stop;
 	ch->udma_read = old.udma_read;
diff -urN linux-2.5.14/drivers/ide/ide-cd.c linux/drivers/ide/ide-cd.c
--- linux-2.5.14/drivers/ide/ide-cd.c	2002-05-07 17:57:04.000000000 +0200
+++ linux/drivers/ide/ide-cd.c	2002-05-07 15:43:27.000000000 +0200
@@ -962,7 +962,7 @@
 
 	/* First, figure out if we need to bit-bucket
 	   any of the leading sectors. */
-	nskip = MIN(rq->current_nr_sectors - bio_sectors(rq->bio), sectors_to_transfer);
+	nskip = MIN((int)(rq->current_nr_sectors - bio_sectors(rq->bio)), sectors_to_transfer);
 
 	while (nskip > 0) {
 		/* We need to throw away a sector. */
diff -urN linux-2.5.14/drivers/ide/ide-dma.c linux/drivers/ide/ide-dma.c
--- linux-2.5.14/drivers/ide/ide-dma.c	2002-05-07 17:56:57.000000000 +0200
+++ linux/drivers/ide/ide-dma.c	2002-05-07 15:49:00.000000000 +0200
@@ -533,8 +533,20 @@
 {
 	struct ata_channel *ch = drive->channel;
 	int set_high = 1;
-	u8 unit = (drive->select.b.unit & 0x01);
-	u64 addr = BLK_BOUNCE_HIGH;
+	u8 unit;
+	u64 addr;
+
+
+	/* Method overloaded by host chip specific code. */
+	if (ch->udma_enable) {
+		ch->udma_enable(drive, on, verbose);
+
+		return;
+	}
+
+	/* Fall back to the default implementation. */
+	unit = (drive->select.b.unit & 0x01);
+	addr = BLK_BOUNCE_HIGH;
 
 	if (!on) {
 		if (verbose)
diff -urN linux-2.5.14/drivers/ide/ide-pmac.c linux/drivers/ide/ide-pmac.c
--- linux-2.5.14/drivers/ide/ide-pmac.c	2002-05-06 05:38:04.000000000 +0200
+++ linux/drivers/ide/ide-pmac.c	2002-05-07 17:52:35.000000000 +0200
@@ -256,7 +256,15 @@
 #define IDE_WAKEUP_DELAY_MS	2000
 
 static void pmac_ide_setup_dma(struct device_node *np, int ix);
-static int pmac_ide_dmaproc(ide_dma_action_t func, struct ata_device *drive, struct request *rq);
+
+static void pmac_udma_enable(struct ata_device *drive, int on, int verbose);
+static int pmac_udma_start(struct ata_device *drive, struct request *rq);
+static int pmac_udma_stop(struct ata_device *drive);
+static int pmac_do_udma(unsigned int reading, struct ata_device *drive, struct request *rq);
+static int pmac_udma_read(struct ata_device *drive, struct request *rq);
+static int pmac_udma_write(struct ata_device *drive, struct request *rq);
+static int pmac_udma_irq_status(struct ata_device *drive);
+static int pmac_ide_dmaproc(struct ata_device *drive);
 static int pmac_ide_build_dmatable(struct ata_device *drive, struct request *rq, int ix, int wr);
 static int pmac_ide_tune_chipset(struct ata_device *drive, byte speed);
 static void pmac_ide_tuneproc(struct ata_device *drive, byte pio);
@@ -323,7 +331,13 @@
 	ide_hwifs[ix].selectproc = pmac_ide_selectproc;
 	ide_hwifs[ix].speedproc = &pmac_ide_tune_chipset;
 	if (pmac_ide[ix].dma_regs && pmac_ide[ix].dma_table_cpu) {
-		ide_hwifs[ix].udma = pmac_ide_dmaproc;
+		ide_hwifs[ix].udma_enable = pmac_udma_enable;
+		ide_hwifs[ix].udma_start = pmac_udma_start;
+		ide_hwifs[ix].udma_stop = pmac_udma_stop;
+		ide_hwifs[ix].udma_read = pmac_udma_read;
+		ide_hwifs[ix].udma_write = pmac_udma_write;
+		ide_hwifs[ix].udma_irq_status = pmac_udma_irq_status;
+		ide_hwifs[ix].XXX_udma = pmac_ide_dmaproc;
 #ifdef CONFIG_BLK_DEV_IDEDMA_PMAC_AUTO
 		if (!noautodma)
 			ide_hwifs[ix].autodma = 1;
@@ -1025,7 +1039,13 @@
 				    	pmif->dma_table_cpu, pmif->dma_table_dma);
 		return;
 	}
-	ide_hwifs[ix].udma = pmac_ide_dmaproc;
+	ide_hwifs[ix].udma_enable = pmac_udma_enable;
+	ide_hwifs[ix].udma_start = pmac_udma_start;
+	ide_hwifs[ix].udma_stop = pmac_udma_stop;
+	ide_hwifs[ix].udma_read = pmac_udma_read;
+	ide_hwifs[ix].udma_write = pmac_udma_write;
+	ide_hwifs[ix].udma_irq_status = pmac_udma_irq_status;
+	ide_hwifs[ix].XXX_udma = pmac_ide_dmaproc;
 #ifdef CONFIG_BLK_DEV_IDEDMA_PMAC_AUTO
 	if (!noautodma)
 		ide_hwifs[ix].autodma = 1;
@@ -1336,130 +1356,178 @@
 	blk_queue_bounce_limit(&drive->queue, addr);
 }
 
-static int pmac_ide_dmaproc(ide_dma_action_t func, struct ata_device *drive, struct request *rq)
+static void pmac_udma_enable(struct ata_device *drive, int on, int verbose)
+{
+	if (verbose) {
+		printk(KERN_INFO "%s: DMA disabled\n", drive->name);
+	}
+
+	drive->using_dma = 0;
+	ide_toggle_bounce(drive, 0);
+}
+
+static int pmac_udma_start(struct ata_device *drive, struct request *rq)
 {
-	int ix, dstat, reading, ata4;
+	int ix, ata4;
+	volatile struct dbdma_regs *dma;
+
+	/* Can we stuff a pointer to our intf structure in config_data
+	 * or select_data in hwif ?
+	 */
+	ix = pmac_ide_find(drive);
+	if (ix < 0)
+		return 0;
+	dma = pmac_ide[ix].dma_regs;
+	ata4 = (pmac_ide[ix].kind == controller_kl_ata4 ||
+		pmac_ide[ix].kind == controller_kl_ata4_80);
+
+	out_le32(&dma->control, (RUN << 16) | RUN);
+	/* Make sure it gets to the controller right now */
+	(void)in_le32(&dma->control);
+	return 0;
+}
+
+static int pmac_udma_stop(struct ata_device *drive)
+{
+	int ix, dstat, ata4;
+	volatile struct dbdma_regs *dma;
+
+	/* Can we stuff a pointer to our intf structure in config_data
+	 * or select_data in hwif ?
+	 */
+	ix = pmac_ide_find(drive);
+	if (ix < 0)
+		return 0;
+	dma = pmac_ide[ix].dma_regs;
+	ata4 = (pmac_ide[ix].kind == controller_kl_ata4 ||
+		pmac_ide[ix].kind == controller_kl_ata4_80);
+
+	drive->waiting_for_dma = 0;
+	dstat = in_le32(&dma->status);
+	out_le32(&dma->control, ((RUN|WAKE|DEAD) << 16));
+	pmac_ide_destroy_dmatable(drive->channel, ix);
+	/* verify good dma status */
+	return (dstat & (RUN|DEAD|ACTIVE)) != RUN;
+}
+
+static int pmac_do_udma(unsigned int reading, struct ata_device *drive, struct request *rq)
+{
+	int ix, ata4;
 	volatile struct dbdma_regs *dma;
 	byte unit = (drive->select.b.unit & 0x01);
-	
+
 	/* Can we stuff a pointer to our intf structure in config_data
 	 * or select_data in hwif ?
 	 */
 	ix = pmac_ide_find(drive);
 	if (ix < 0)
-		return 0;		
+		return 0;
 	dma = pmac_ide[ix].dma_regs;
 	ata4 = (pmac_ide[ix].kind == controller_kl_ata4 ||
 		pmac_ide[ix].kind == controller_kl_ata4_80);
-	
-	switch (func) {
-	case ide_dma_off:
-		printk(KERN_INFO "%s: DMA disabled\n", drive->name);
-	case ide_dma_off_quietly:
-		drive->using_dma = 0;
-		ide_toggle_bounce(drive, 0);
-		break;
-	case ide_dma_on:
-	case ide_dma_check:
-		/* Change this to better match ide-dma.c */
-		pmac_ide_check_dma(drive);
-		ide_toggle_bounce(drive, drive->using_dma);
-		break;
-	case ide_dma_read:
-	case ide_dma_write:
-		reading = (func == ide_dma_read);
-		if (!pmac_ide_build_dmatable(drive, rq, ix, !reading))
-			return 1;
-		/* Apple adds 60ns to wrDataSetup on reads */
-		if (ata4 && (pmac_ide[ix].timings[unit] & TR_66_UDMA_EN)) {
-			out_le32((unsigned *)(IDE_DATA_REG + IDE_TIMING_CONFIG + _IO_BASE),
-				pmac_ide[ix].timings[unit] + 
-				((func == ide_dma_read) ? 0x00800000UL : 0));
-			(void)in_le32((unsigned *)(IDE_DATA_REG + IDE_TIMING_CONFIG + _IO_BASE));
-		}
-		drive->waiting_for_dma = 1;
-		if (drive->type != ATA_DISK)
-			return 0;
-		ide_set_handler(drive, ide_dma_intr, WAIT_CMD, NULL);
-		if ((rq->flags & REQ_DRIVE_ACB) &&
-		    (drive->addressing == 1)) {
-			struct ata_taskfile *args = rq->special;
-			OUT_BYTE(args->taskfile.command, IDE_COMMAND_REG);
-		} else if (drive->addressing) {
-			OUT_BYTE(reading ? WIN_READDMA_EXT : WIN_WRITEDMA_EXT, IDE_COMMAND_REG);
-		} else {
-			OUT_BYTE(reading ? WIN_READDMA : WIN_WRITEDMA, IDE_COMMAND_REG);
-		}
-		/* fall through */
-	case ide_dma_begin:
-		out_le32(&dma->control, (RUN << 16) | RUN);
-		/* Make sure it gets to the controller right now */
-		(void)in_le32(&dma->control);
-		break;
-	case ide_dma_end: /* returns 1 on error, 0 otherwise */
-		drive->waiting_for_dma = 0;
-		dstat = in_le32(&dma->status);
-		out_le32(&dma->control, ((RUN|WAKE|DEAD) << 16));
-		pmac_ide_destroy_dmatable(drive->channel, ix);
-		/* verify good dma status */
-		return (dstat & (RUN|DEAD|ACTIVE)) != RUN;
-	case ide_dma_test_irq: /* returns 1 if dma irq issued, 0 otherwise */
-		/* We have to things to deal with here:
-		 * 
-		 * - The dbdma won't stop if the command was started
-		 * but completed with an error without transfering all
-		 * datas. This happens when bad blocks are met during
-		 * a multi-block transfer.
-		 * 
-		 * - The dbdma fifo hasn't yet finished flushing to
-		 * to system memory when the disk interrupt occurs.
-		 * 
-		 * The trick here is to increment drive->waiting_for_dma,
-		 * and return as if no interrupt occured. If the counter
-		 * reach a certain timeout value, we then return 1. If
-		 * we really got the interrupt, it will happen right away
-		 * again.
-		 * Apple's solution here may be more elegant. They issue
-		 * a DMA channel interrupt (a separate irq line) via a DBDMA
-		 * NOP command just before the STOP, and wait for both the
-		 * disk and DBDMA interrupts to have completed.
-		 */
-		 
-		/* If ACTIVE is cleared, the STOP command have passed and
-		 * transfer is complete.
-		 */
-		if (!(in_le32(&dma->status) & ACTIVE))
-			return 1;
-		if (!drive->waiting_for_dma)
-			printk(KERN_WARNING "ide%d, ide_dma_test_irq \
-				called while not waiting\n", ix);
 
-		/* If dbdma didn't execute the STOP command yet, the
-		 * active bit is still set */
-		drive->waiting_for_dma++;
-		if (drive->waiting_for_dma >= DMA_WAIT_TIMEOUT) {
-			printk(KERN_WARNING "ide%d, timeout waiting \
-				for dbdma command stop\n", ix);
-			return 1;
-		}
-		udelay(1);
+	if (!pmac_ide_build_dmatable(drive, rq, ix, !reading))
+		return 1;
+	/* Apple adds 60ns to wrDataSetup on reads */
+	if (ata4 && (pmac_ide[ix].timings[unit] & TR_66_UDMA_EN)) {
+		out_le32((unsigned *)(IDE_DATA_REG + IDE_TIMING_CONFIG + _IO_BASE),
+			pmac_ide[ix].timings[unit] + 
+			((reading) ? 0x00800000UL : 0));
+		(void)in_le32((unsigned *)(IDE_DATA_REG + IDE_TIMING_CONFIG + _IO_BASE));
+	}
+	drive->waiting_for_dma = 1;
+	if (drive->type != ATA_DISK)
+		return 0;
+	ide_set_handler(drive, ide_dma_intr, WAIT_CMD, NULL);
+	if ((rq->flags & REQ_DRIVE_ACB) &&
+		(drive->addressing == 1)) {
+		struct ata_taskfile *args = rq->special;
+		OUT_BYTE(args->taskfile.command, IDE_COMMAND_REG);
+	} else if (drive->addressing) {
+		OUT_BYTE(reading ? WIN_READDMA_EXT : WIN_WRITEDMA_EXT, IDE_COMMAND_REG);
+	} else {
+		OUT_BYTE(reading ? WIN_READDMA : WIN_WRITEDMA, IDE_COMMAND_REG);
+	}
+
+	return udma_start(drive, rq);
+}
+
+static int pmac_udma_read(struct ata_device *drive, struct request *rq)
+{
+	return pmac_do_udma(1, drive, rq);
+}
+
+static int pmac_udma_write(struct ata_device *drive, struct request *rq)
+{
+	return pmac_do_udma(0, drive, rq);
+}
+
+/*
+ * FIXME: This should be attached to a channel as we can see now!
+ */
+static int pmac_udma_irq_status(struct ata_device *drive)
+{
+	int ix, ata4;
+	volatile struct dbdma_regs *dma;
+
+	/* Can we stuff a pointer to our intf structure in config_data
+	 * or select_data in hwif ?
+	 */
+	ix = pmac_ide_find(drive);
+	if (ix < 0)
 		return 0;
+	dma = pmac_ide[ix].dma_regs;
+	ata4 = (pmac_ide[ix].kind == controller_kl_ata4 ||
+		pmac_ide[ix].kind == controller_kl_ata4_80);
+
+	/* We have to things to deal with here:
+	 *
+	 * - The dbdma won't stop if the command was started but completed with
+	 * an error without transfering all datas. This happens when bad blocks
+	 * are met during a multi-block transfer.
+	 *
+	 * - The dbdma fifo hasn't yet finished flushing to to system memory
+	 * when the disk interrupt occurs.
+	 *
+	 * The trick here is to increment drive->waiting_for_dma, and return as
+	 * if no interrupt occured. If the counter reach a certain timeout
+	 * value, we then return 1. If we really got the interrupt, it will
+	 * happen right away again.  Apple's solution here may be more elegant.
+	 * They issue a DMA channel interrupt (a separate irq line) via a DBDMA
+	 * NOP command just before the STOP, and wait for both the disk and
+	 * DBDMA interrupts to have completed.
+	 */
 
-		/* Let's implement tose just in case someone wants them */
-	case ide_dma_bad_drive:
-	case ide_dma_good_drive:
-		return check_drive_lists(drive, (func == ide_dma_good_drive));
-	case ide_dma_lostirq:
-	case ide_dma_timeout:
-		printk(KERN_WARNING "ide_pmac_dmaproc: chipset supported func only: %d\n", func);
+	/* If ACTIVE is cleared, the STOP command have passed and
+	 * transfer is complete.
+	 */
+	if (!(in_le32(&dma->status) & ACTIVE))
 		return 1;
-	default:
-		printk(KERN_WARNING "ide_pmac_dmaproc: unsupported func: %d\n", func);
+	if (!drive->waiting_for_dma)
+		printk(KERN_WARNING "ide%d, ide_dma_test_irq \
+				called while not waiting\n", ix);
+
+	/* If dbdma didn't execute the STOP command yet, the
+	 * active bit is still set */
+	drive->waiting_for_dma++;
+	if (drive->waiting_for_dma >= DMA_WAIT_TIMEOUT) {
+		printk(KERN_WARNING "ide%d, timeout waiting \
+				for dbdma command stop\n", ix);
 		return 1;
 	}
+	udelay(1);
 	return 0;
 }
-#endif /* CONFIG_BLK_DEV_IDEDMA_PMAC */
+
+static int pmac_ide_dmaproc(struct ata_device *drive)
+{
+	/* Change this to better match ide-dma.c */
+	pmac_ide_check_dma(drive);
+	ide_toggle_bounce(drive, drive->using_dma);
+
+	return 0;
+}
+#endif
 
 static void idepmac_sleep_device(ide_drive_t *drive, int i, unsigned base)
 {
diff -urN linux-2.5.14/drivers/ide/ide-taskfile.c linux/drivers/ide/ide-taskfile.c
--- linux-2.5.14/drivers/ide/ide-taskfile.c	2002-05-07 17:57:01.000000000 +0200
+++ linux/drivers/ide/ide-taskfile.c	2002-05-07 15:41:02.000000000 +0200
@@ -39,8 +39,6 @@
 #define DTF(x...)
 #endif
 
-#define SUPPORT_VLB_SYNC 1
-
 /*
  * for now, taskfile requests are special :/
  */
diff -urN linux-2.5.14/include/asm-m68k/ide.h linux/include/asm-m68k/ide.h
--- linux-2.5.14/include/asm-m68k/ide.h	2002-05-07 17:56:58.000000000 +0200
+++ linux/include/asm-m68k/ide.h	2002-05-07 15:41:02.000000000 +0200
@@ -121,6 +121,7 @@
 #define inb(p)     in_8(ADDR_TRANS_B(p))
 #define inb_p(p)     in_8(ADDR_TRANS_B(p))
 #define inw(p)     in_be16(ADDR_TRANS_W(p))
+#define inw_p(p)     in_be16(ADDR_TRANS_W(p))
 #define outb(v,p)  out_8(ADDR_TRANS_B(p),v)
 #define outb_p(v,p)  out_8(ADDR_TRANS_B(p),v)
 #define outw(v,p)  out_be16(ADDR_TRANS_W(p),v)
diff -urN linux-2.5.14/include/linux/ide.h linux/include/linux/ide.h
--- linux-2.5.14/include/linux/ide.h	2002-05-07 17:57:04.000000000 +0200
+++ linux/include/linux/ide.h	2002-05-07 15:48:57.000000000 +0200
@@ -459,6 +459,8 @@
 
 	int (*XXX_udma)(struct ata_device *);
 
+	void (*udma_enable)(struct ata_device *, int, int);
+
 	int (*udma_start) (struct ata_device *, struct request *rq);
 	int (*udma_stop) (struct ata_device *);
 

Re: [PATCH] IDE 58

[Paul Mackerras]

Martin Dalecki writes:

> - Virtualize the udma_enable method as well to help ARM and PPC people.  Please
>    please if you would like to have some other methods virtualized in a similar
>    way - just tell me or even better do it yourself at the end of ide-dma.c.
>    I *don't mind* patches.
> 
> - Fix the pmac code to adhere to the new API. It's supposed to work again.
>    However this is blind coding... I give myself 80% chances for it to work ;-).

OK, now I am truly impressed.  Not only does it compile cleanly, it
works first go!

I am using the tiny patch below, it sets the unmask flag so interrupts
will be unmasked by default (which is safe on powermacs).

Thanks,
Paul.

diff -urN linux-2.5/drivers/ide/ide-pmac.c pmac-2.5/drivers/ide/ide-pmac.c
--- linux-2.5/drivers/ide/ide-pmac.c	Wed May  8 16:40:17 2002
+++ pmac-2.5/drivers/ide/ide-pmac.c	Wed May  8 08:26:48 2002
@@ -343,6 +343,7 @@
 			ide_hwifs[ix].autodma = 1;
 #endif
 	}
+	ide_hwifs[ix].unmask = 1;
 }
 
 #if 0

Re: [PATCH] IDE 58

[Martin Dalecki]

Uz.ytkownik Paul Mackerras napisa?:
> Martin Dalecki writes:
> 
> 
>>- Virtualize the udma_enable method as well to help ARM and PPC people.  Please
>>   please if you would like to have some other methods virtualized in a similar
>>   way - just tell me or even better do it yourself at the end of ide-dma.c.
>>   I *don't mind* patches.
>>
>>- Fix the pmac code to adhere to the new API. It's supposed to work again.
>>   However this is blind coding... I give myself 80% chances for it to work ;-).
> 
> 
> OK, now I am truly impressed.  Not only does it compile cleanly, it
> works first go!

Thank you.

BTW> I would really love it if the cris architecture people could
"lend me" some small developement system for they interresting CPU.
In return I could give them what's certainly worth "several weeks of
developers time". (If you hear me: this is a hint if you need an argument for
your management.)

This unfortunately is the somehow most wired ATA interface
around. Which is due to the fact that the interface cell is directly mapped to
some CPU registers. As a CPU design I think it's a fine approach. Don't
take me wrong. You save yourself the whole silicon which is needed
for BM access arbitration and general handling and so on... Very nice tought
out. But on the software side this is a bit wired, since you can't use
the generic I/O primitives of the arch in question.

This makes my  cleanup of the portability layer a bit hard
to finish on the software side.

> I am using the tiny patch below, it sets the unmask flag so interrupts
> will be unmasked by default (which is safe on powermacs).

And on every other fscking PCI based system... (modulo the "problematic"
cmd640 and RZ1000). Should have been done a long time ago this way... I will 
adjust the others as well.

> Thanks,
> Paul.
> 
> diff -urN linux-2.5/drivers/ide/ide-pmac.c pmac-2.5/drivers/ide/ide-pmac.c
> --- linux-2.5/drivers/ide/ide-pmac.c	Wed May  8 16:40:17 2002
> +++ pmac-2.5/drivers/ide/ide-pmac.c	Wed May  8 08:26:48 2002
> @@ -343,6 +343,7 @@
>  			ide_hwifs[ix].autodma = 1;
>  #endif
>  	}
> +	ide_hwifs[ix].unmask = 1;
>  }
>  
>  #if 0
> 

Re: [PATCH] IDE 58

[Bjorn Wesen]

On Wed, 8 May 2002, Martin Dalecki wrote:
> BTW> I would really love it if the cris architecture people could
> "lend me" some small developement system for they interresting CPU.

We'll consider it :) However,

> This unfortunately is the somehow most wired ATA interface
> around. Which is due to the fact that the interface cell is directly mapped to
> some CPU registers. As a CPU design I think it's a fine approach. Don't
> take me wrong. You save yourself the whole silicon which is needed
> for BM access arbitration and general handling and so on... Very nice tought
> out. But on the software side this is a bit wired, since you can't use
> the generic I/O primitives of the arch in question.

I don't see why all IDE-interfaces in the world have to be I/O-mapped just 
because the first PC implementations used that. Sure it was an extended 
ISA-bus but the ISA bus is long gone and we don't all run PC's anymore 
either.

So the simple abstraction we need to hit IDE-bus registers is a macro or 
inline, instead of a call of an I/O-primitive. It was too much work to 
abstract this when I inserted the CRIS-arch IDE-driver in the first place 
so I found a workaround but now seems like a better time..

Similarily, there is no reason at all why the CPU has to do _polling_ just 
because the IDE _bus_ is using a PIO-mode. It probably does that on legacy 
PC's but HW designed, hrm, more optimally can use DMA. Hence the hooks for 
the ide_func_t.

So I'd figure the software side really would be _easier_ to implement with 
those assumptions about how an IDE-interface is supposed to work gone.

> This makes my  cleanup of the portability layer a bit hard
> to finish on the software side.

I understand that, so lets keep the discussion going and I'll check over 
your current cleanup.

/Bjorn

Re: [PATCH] IDE 58

[Martin Dalecki]

Uz.ytkownik Bjorn Wesen napisa?:
> On Wed, 8 May 2002, Martin Dalecki wrote:
> 
>>BTW> I would really love it if the cris architecture people could
>>"lend me" some small developement system for they interresting CPU.
> 
> 
> We'll consider it :) However,
> 
> 
>>This unfortunately is the somehow most wired ATA interface
>>around. Which is due to the fact that the interface cell is directly mapped to
>>some CPU registers. As a CPU design I think it's a fine approach. Don't
>>take me wrong. You save yourself the whole silicon which is needed
>>for BM access arbitration and general handling and so on... Very nice tought
>>out. But on the software side this is a bit wired, since you can't use
>>the generic I/O primitives of the arch in question.
> 
> 
> I don't see why all IDE-interfaces in the world have to be I/O-mapped just 
> because the first PC implementations used that. Sure it was an extended 
> ISA-bus but the ISA bus is long gone and we don't all run PC's anymore 
> either.

Hey I agree and anticipate the design decisions for the Cris CPU
as good and surprisingly refreshing. Like for example the whole
concept of the compacted command set and so on. They are just *cute*...
It's about a year ago I did study the public available documentation
on it.

> So the simple abstraction we need to hit IDE-bus registers is a macro or 
> inline, instead of a call of an I/O-primitive. It was too much work to 
> abstract this when I inserted the CRIS-arch IDE-driver in the first place 
> so I found a workaround but now seems like a better time..

I don't think that it's always the proper aproach for hardware
portability to do it on the "micro operation" level. That's good
for generics like inb outb. In the case of the ATA interface it's
better to do it on the "functional" level above... Just like you did
with ata_read() and ata_write() as they are called now. You can
see I picked it up and when I sort the transport method detecion/setting
out I will apply it to the other friends from the ata_read_xxx family as well.
And then we have the same aproach in the udma_ familiy I just
introduced.

> Similarily, there is no reason at all why the CPU has to do _polling_ just 
> because the IDE _bus_ is using a PIO-mode. It probably does that on legacy 
> PC's but HW designed, hrm, more optimally can use DMA. Hence the hooks for 
> the ide_func_t.

Well right now I think if you look at the IDE 58 patch you will see
that ide_func_t is a 'bit ugly', simple becouse it is introducing
just another entity to the game. We don't need it.
struct ata_chanell *is* the central entitiy for operations from
the host view. In my whole expierence as a programmer it always turned
out to be most sane to make the software design be a homological mapping
of the generalized hardware design on this level of coding.
It's just natural functions are there to serve a specific purpose.

> So I'd figure the software side really would be _easier_ to implement with 
> those assumptions about how an IDE-interface is supposed to work gone.
> 
> 
>>This makes my  cleanup of the portability layer a bit hard
>>to finish on the software side.
> 
> 
> I understand that, so lets keep the discussion going and I'll check over 
> your current cleanup.

Well please consider: iff I had access to the hardware it would possibly save
you  a lot of reading through bad english ;-).

> /Bjorn

Regards.

Re: [PATCH] IDE 58

[Linus Torvalds]

On Wed, 8 May 2002, Martin Dalecki wrote:
>
> I don't think that it's always the proper aproach for hardware
> portability to do it on the "micro operation" level. That's good
> for generics like inb outb. In the case of the ATA interface it's
> better to do it on the "functional" level above...

Amen.

Helleluja.

Listen to the man.

Please don't play games with "ide_outb()" etc, which cause 99% of the
architectures to have to make the 1:1 translation to just "outb()", and
which also makes it incredibly cumbersome to handle multiple _different_
controllers that just happen to use different schemes.

Instead, making the virtualization at a higher point means that you can
have _one_ set of common operations for traditional PCI/ATA controllers
(and that one set uses inx/outx/readx/writex), and then you have a few
others for the "strange" cases.

And done properly with per-controller (or drive - you may want to
virtualize at the drive level just because you could separate out
different kinds of drive accesses that way too) function pointers you can
then _mix_ access methods, without getting completely idiotic run-time
checks inside "ide_out()".

			Linus

Re: [PATCH] IDE 58

[Benjamin Herrenschmidt]

>And done properly with per-controller (or drive - you may want to
>virtualize at the drive level just because you could separate out
>different kinds of drive accesses that way too) function pointers you can
>then _mix_ access methods, without getting completely idiotic run-time
>checks inside "ide_out()".

Which ends up basically into having function pointers in the
ata_channel (or ata_drive, but I doubt that would be really
necessary) a set of 4 access functions: taskfile_in/out for
access to taskfile registers (8 bits), and data_in/out for
steaming datas in/out of the data reg (16 bits).

That would cleanly solve my problem of mixing MMIO and PIO
controllers in the same machine, that would solve the crazy
byteswapping needed by some controllers for PIO at least,
etc...

I would even suggest not caring about the taskfile register
address at all (that is kill the array of port addresses) but
just pass the taskfile_in/out functions the register number
(cyl_hi, cyl_lo, select, ....) as a nice symbolic constant,
and let the channel specific implementation figure it out.
I haven't checked if you already killed all of the request/release
region crap done by the common ide code, that is matter is completely
internal to the host controller driver, etc...

Now, andre may tell us we need one more set for "slow IO"
versions for some HW, I don't know the details for these so
I'll let the old man speak up here.

Ben.

Re: [PATCH] IDE 58

[Alan Cox]

> ata_channel (or ata_drive, but I doubt that would be really
> necessary) a set of 4 access functions: taskfile_in/out for
> access to taskfile registers (8 bits), and data_in/out for
> steaming datas in/out of the data reg (16 bits).

Please push it higher level than that. Load the taskfile as a set in
each method. Remember its 1 potentially paired instruction to do an MMIO
write, its a whole mess of synchronziation and stalls to do a function 
pointer.

> address at all (that is kill the array of port addresses) but
> just pass the taskfile_in/out functions the register number
> (cyl_hi, cyl_lo, select, ....) as a nice symbolic constant,
> and let the channel specific implementation figure it out.

Pass  dev->taskfile_load() a struct at least for the common paths. Make the
PIO block transfers also single callbacks for each block not word.

Alan

Re: [PATCH] IDE 58

[Benjamin Herrenschmidt]

>
>Please push it higher level than that. Load the taskfile as a set in
>each method. Remember its 1 potentially paired instruction to do an MMIO
>write, its a whole mess of synchronziation and stalls to do a function 
>pointer.

I though about that, but what about corner cases where only a single
register can be accessed ? (typically alt status). Provide specific
routines ? Also, how does the extended addressing works ? by writing
several times to the cyl registers ? That would have to be dealt with
as well in each host driver then.

>> address at all (that is kill the array of port addresses) but
>> just pass the taskfile_in/out functions the register number
>> (cyl_hi, cyl_lo, select, ....) as a nice symbolic constant,
>> and let the channel specific implementation figure it out.
>
>Pass  dev->taskfile_load() a struct at least for the common paths. Make the
>PIO block transfers also single callbacks for each block not word.

Right. We could go the darwin (apple) way and have taskfile_load/store
functions doing the entire registers controlled by a bitmask of which
registers has to be touched. it has a cost (testing each bit and
conditionally branching, which can suck hard) but probably less than
an indirect function call which isn't predictable.

Ben.

Re: [PATCH] IDE 58

[Alan Cox]

> I though about that, but what about corner cases where only a single
> register can be accessed ? (typically alt status). Provide specific
> routines ? Also, how does the extended addressing works ? by writing
> several times to the cyl registers ? That would have to be dealt with
> as well in each host driver then.

There are lots of cases we don't care about speed - things like setup of
the controller, changing UDMA mode etc. 

> Right. We could go the darwin (apple) way and have taskfile_load/store
> functions doing the entire registers controlled by a bitmask of which
> registers has to be touched. it has a cost (testing each bit and
> conditionally branching, which can suck hard) but probably less than

Get yourself a conditional move instruction 8)

> an indirect function call which isn't predictable.

Or you have a small set of such functions for the critical paths - ie doing
actual block I/O which pass the set of values required to do that operation
and do the stores. What are the performance critical paths

	Begin a disk write
	Begin a disk read
	PIO transfer in
	PIO transfer out
	End a disk I/O fastpaths (no error case)

	Maybe ATAPI command writes ?

beyond that I doubt the rest are critical 

Alan

Re: [PATCH] IDE 58

[Benjamin Herrenschmidt]

>> I though about that, but what about corner cases where only a single
>> register can be accessed ? (typically alt status). Provide specific
>> routines ? Also, how does the extended addressing works ? by writing
>> several times to the cyl registers ? That would have to be dealt with
>> as well in each host driver then.
>
>There are lots of cases we don't care about speed - things like setup of
>the controller, changing UDMA mode etc. 

Right, so we keep the basic indiret access functions, and add the taskfile
ones on top for performances, that's what you mean ?

>> Right. We could go the darwin (apple) way and have taskfile_load/store
>> functions doing the entire registers controlled by a bitmask of which
>> registers has to be touched. it has a cost (testing each bit and
>> conditionally branching, which can suck hard) but probably less than
>
>Get yourself a conditional move instruction 8)

Hehe, let's make an ARM/PPC hybrid ;)

>> an indirect function call which isn't predictable.
>
>Or you have a small set of such functions for the critical paths - ie doing
>actual block I/O which pass the set of values required to do that operation
>and do the stores. What are the performance critical paths
>
>	Begin a disk write
>	Begin a disk read
>	PIO transfer in
>	PIO transfer out
>	End a disk I/O fastpaths (no error case)
>
>	Maybe ATAPI command writes ?
>
>beyond that I doubt the rest are critical 

Well, I would normally agree with the above... except that IDE is so full of
corner cases that I don't want to see dealt with in each host controller
driver...

Ben.

Re: [PATCH] IDE 58

[Ian Molton]

On Wed, 8 May 2002 22:04:57 +0200
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> 
> Hehe, let's make an ARM/PPC hybrid ;)

Me wantee :)

Re: [PATCH] IDE 58

[Andre Hedrick]

On Wed, 8 May 2002, Alan Cox wrote:

> > I though about that, but what about corner cases where only a single
> > register can be accessed ? (typically alt status). Provide specific
> > routines ? Also, how does the extended addressing works ? by writing
> > several times to the cyl registers ? That would have to be dealt with
> > as well in each host driver then.
> 
> There are lots of cases we don't care about speed - things like setup of
> the controller, changing UDMA mode etc. 

Erm, think about the state diagram for testing the acceptance and command
migration if TAG is going to be  standard.

> 	Begin a disk write
> 	Begin a disk read

test command accept/reject

repeat 'til done:
check_status

> 	PIO transfer in
> 	PIO transfer out

io hardware atomic segment or burst

(for linus, "atomic" is what is needed to complete, not just linusism")

goto repeat if !done with entire request

The above is where data integrity is lost if error happens.

> 	End a disk I/O fastpaths (no error case)
> 
> 	Maybe ATAPI command writes ?
> 
> beyond that I doubt the rest are critical 

There are several other cases, but 95% is the command block execution.
The rest is sense data.

Cheers,

Andre Hedrick
LAD Storage Consulting Group

Re: [PATCH] IDE 58

[Andre Hedrick]

Alan, we talked about this and the driver/hardware has a flaw.
If you count the total number of single IO operations to check
status/error et al., it is out right fugly.  Preprocessing will kill us
like today.


On Wed, 8 May 2002, Alan Cox wrote:

> > ata_channel (or ata_drive, but I doubt that would be really
> > necessary) a set of 4 access functions: taskfile_in/out for
> > access to taskfile registers (8 bits), and data_in/out for
> > steaming datas in/out of the data reg (16 bits).
> 
> Please push it higher level than that. Load the taskfile as a set in
> each method. Remember its 1 potentially paired instruction to do an MMIO
> write, its a whole mess of synchronziation and stalls to do a function 
> pointer.
> 
> > address at all (that is kill the array of port addresses) but
> > just pass the taskfile_in/out functions the register number
> > (cyl_hi, cyl_lo, select, ....) as a nice symbolic constant,
> > and let the channel specific implementation figure it out.
> 
> Pass  dev->taskfile_load() a struct at least for the common paths. Make the
> PIO block transfers also single callbacks for each block not word.
> 
> Alan
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Andre Hedrick
LAD Storage Consulting Group

Re: [PATCH] IDE 58

[Benjamin Herrenschmidt]

>Alan, we talked about this and the driver/hardware has a flaw.
>If you count the total number of single IO operations to check
>status/error et al., it is out right fugly.  Preprocessing will kill us
>like today.

So we can still end up having both

 - The single ops indirected like I first proposed (with maybe the
addition of slow versions but that could be a parameter, and 32 bits
versions)
 - A couple of "apply this taskfile" versions with well known semantics
used only in normal cases for perfs.

Ben.

Re: [PATCH] IDE 58

[Martin Dalecki]

Uz.ytkownik Andre Hedrick napisa?:
> Alan, we talked about this and the driver/hardware has a flaw.
> If you count the total number of single IO operations to check
> status/error et al., it is out right fugly.  Preprocessing will kill us
> like today.

You mean the preprocessing in the devices firmware program of course?

Just to confirm I did get it right...

Re: [PATCH] IDE 58

[Eric W. Biederman]

Benjamin Herrenschmidt <benh@kernel.crashing.org> writes:

> >And done properly with per-controller (or drive - you may want to
> >virtualize at the drive level just because you could separate out
> >different kinds of drive accesses that way too) function pointers you can
> >then _mix_ access methods, without getting completely idiotic run-time
> >checks inside "ide_out()".
> 
> Which ends up basically into having function pointers in the
> ata_channel (or ata_drive, but I doubt that would be really
> necessary) a set of 4 access functions: taskfile_in/out for
> access to taskfile registers (8 bits), and data_in/out for
> steaming datas in/out of the data reg (16 bits).
> 
> That would cleanly solve my problem of mixing MMIO and PIO
> controllers in the same machine, that would solve the crazy
> byteswapping needed by some controllers for PIO at least,
> etc...
> 
> I would even suggest not caring about the taskfile register
> address at all (that is kill the array of port addresses) but
> just pass the taskfile_in/out functions the register number
> (cyl_hi, cyl_lo, select, ....) as a nice symbolic constant,
> and let the channel specific implementation figure it out.
> I haven't checked if you already killed all of the request/release
> region crap done by the common ide code, that is matter is completely
> internal to the host controller driver, etc...
> 
> Now, andre may tell us we need one more set for "slow IO"
> versions for some HW, I don't know the details for these so
> I'll let the old man speak up here.


I'd suggest pointers in the ata_channel that abstract out the
functions of the host controllers.  For most controllers we
can have a common PCI IDE library that implements them, and provides
a reference implementation for the weird cases.  

>From the ata-6 draft there are the following protocols, that should
be implementable on an IDE host controller.

- Software reset protocol
- Non-data command protocol
- PIO data-in command protocol
- PIO data-out command protocol
- DMA command protocol
- PACKET command protocol
- READ/WRITE DMA QUEUED command protocol
- EXECUTE DEVICE DIAGNOSTIC command protocol
- DEVICE RESET command protocol
- Ultra DMA data-in commands
- Ultra DMA data-out commands

Given the high level of the protocol abstraction we aren't
likely to beat ourselves to death with extra cpu or io overhead.
Nor is this an insane number of things to implement.

Perhaps more can be factored out (controllers being so similiar) but
that is the abstraction we need for the layer sending commands to ATA
devices.  This allows the higher layers to focus on sending commands
to ATA devices.

Eric

Re: [PATCH] IDE 58

[Ian Molton]

On Wed, 8 May 2002 21:10:54 +0200
Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> 
> That would cleanly solve my problem of mixing MMIO and PIO
> controllers in the same machine, that would solve the crazy
> byteswapping needed by some controllers for PIO at least,
> etc...

Its good here on the old and slightly odd Acorn Podule bus interface...

Re: [PATCH] IDE 58

[Benjamin Herrenschmidt]

>I don't see why all IDE-interfaces in the world have to be I/O-mapped just 
>because the first PC implementations used that. Sure it was an extended 
>ISA-bus but the ISA bus is long gone and we don't all run PC's anymore 
>either.
>
>So the simple abstraction we need to hit IDE-bus registers is a macro or 
>inline, instead of a call of an I/O-primitive. It was too much work to 
>abstract this when I inserted the CRIS-arch IDE-driver in the first place 
>so I found a workaround but now seems like a better time..

No, not a macro. There are cases where you want different access methods
on the same machine. For example, pmacs can have the "mac-io" (ide-pmac)
controller, which is MMIO based, _and_ a PCI-based legacy IDE controller
using inx/outx like IOs. (A typical example is the Blue&White G3 who has
both on the motherboard).

Ultimately, you want the hwif (or what it becomes in 2.5) provide a set
of functions for accessing taskfile registers and doing the PIO data
stream read/writes (that is replace inb/outb and insw/outsw).

>Similarily, there is no reason at all why the CPU has to do _polling_ just 
>because the IDE _bus_ is using a PIO-mode. It probably does that on legacy 
>PC's but HW designed, hrm, more optimally can use DMA. Hence the hooks for 
>the ide_func_t.
>
>So I'd figure the software side really would be _easier_ to implement with 
>those assumptions about how an IDE-interface is supposed to work gone.
>
>> This makes my  cleanup of the portability layer a bit hard
>> to finish on the software side.
>
>I understand that, so lets keep the discussion going and I'll check over 
>your current cleanup.
>
>/Bjorn
>
>
>-
>To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
>Please read the FAQ at  http://www.tux.org/lkml/