Skip to contentSkip to conversation

pdc202xx_old: rewrite mode programming code

PATCH In review Bartlomiej Zolnierkiewicz 2 participants 6 messages Mar 4, 2007 – Mar 5, 2007 libata

lore
Share & tools
lore

Bartlomiej Zolnierkiewicz

author
#0 lore raw

PATCH 1/1 pdc202xx_old: rewrite mode programming code

[PATCH] pdc202xx_old: rewrite mode programming code

This patch is based on the documentation (I would like to thank Promise for it) and also partially on the older vendor driver.

Rewrite mode programming code:

* disable 66MHz clock in pdc202xx_tune_chipset() so it is correctly disabled even if both devices on the channel are not DMA capable and after reset

* enable/disable IORDY and PREFETCH bits in pdc202xx_tune_chipset() as they need to be setup correctly also for PIO only devices, plus IORDY wasn't disabled for non-IORDY devices and PREFETCH wasn't disabled for ATAPI devices

* remove dead code for setting SYNC_ERDDY_EN bits from config_chipset_for_dma() (driver sets ->autotune to 1 so PIO modes are always programmed => lower nibble of register A never equals 4 => "chipset_is_set" is always true)

* enable PIO mode programming for all ATAPI devices (it was limited to ->media == ide_cdrom devices)

* remove extra reads of registers A/B/C, don't read register D et all

* do clearing / programming of registers A/B/C in one go (gets rid of extra PCI config space read/write cycle)

* set initial values of drive_conf/AP/BP/CP variables to zero (paranoia for the case when PCI reads fail)

* remove XFER_UDMA6 to XFER_UDMA5 remapping case - it can't happen (ide_rate_filter() takes care of it)

* fix XFER_MW_DMA0 timings (they were overclocked, use the official ones)

* fix bitmasks for clearing bits of register B:

- when programming DMA mode bit 0x10 of register B was cleared which resulted in overclocked PIO timing setting (iff PIO0 was used)

- when programming PIO mode bits 0x18 weren't cleared so suboptimal

    timings were used for PIO1-4 if PIO0 was previously set (bit 0x10)
    and for PIO0/3/4 if PIO1/2 was previously set (bit 0x08)

* add FIXME comment about missing locking for 66MHz clock register

Also while at it:

* remove unused defines

* do a few cosmetic / CodingStyle fixes

* bump driver version

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

---

 drivers/ide/pci/pdc202xx_old.c |  176 +++++++++--------------------------------
 1 file changed, 42 insertions(+), 134 deletions(-)
drivers/ide/pci/pdc202xx_old.c +42 -134
Index: b/drivers/ide/pci/pdc202xx_old.c
===================================================================
@@ -1,8 +1,9 @@
 /*
- *  linux/drivers/ide/pci/pdc202xx_old.c	Version 0.36	Sept 11, 2002
+ *  linux/drivers/ide/pci/pdc202xx_old.c	Version 0.50	Mar 3, 2007
  *
  *  Copyright (C) 1998-2002		Andre Hedrick <[email protected]>
  *  Copyright (C) 2006-2007		MontaVista Software, Inc.
+ *  Copyright (C) 2007			Bartlomiej Zolnierkiewicz
  *
  *  Promise Ultra33 cards with BIOS v1.20 through 1.28 will need this
  *  compiled into the kernel if you have more than one card installed.

@@ -60,45 +61,7 @@ static const char *pdc_quirk_drives[] = 
 	NULL
 };
 
-/* A Register */
-#define	SYNC_ERRDY_EN	0xC0
-
-#define	SYNC_IN		0x80	/* control bit, different for master vs. slave drives */
-#define	ERRDY_EN	0x40	/* control bit, different for master vs. slave drives */
-#define	IORDY_EN	0x20	/* PIO: IOREADY */
-#define	PREFETCH_EN	0x10	/* PIO: PREFETCH */
-
-#define	PA3		0x08	/* PIO"A" timing */
-#define	PA2		0x04	/* PIO"A" timing */
-#define	PA1		0x02	/* PIO"A" timing */
-#define	PA0		0x01	/* PIO"A" timing */
-
-/* B Register */
-
-#define	MB2		0x80	/* DMA"B" timing */
-#define	MB1		0x40	/* DMA"B" timing */
-#define	MB0		0x20	/* DMA"B" timing */
-
-#define	PB4		0x10	/* PIO_FORCE 1:0 */
-
-#define	PB3		0x08	/* PIO"B" timing */	/* PIO flow Control mode */
-#define	PB2		0x04	/* PIO"B" timing */	/* PIO 4 */
-#define	PB1		0x02	/* PIO"B" timing */	/* PIO 3 half */
-#define	PB0		0x01	/* PIO"B" timing */	/* PIO 3 other half */
-
-/* C Register */
-#define	IORDYp_NO_SPEED	0x4F
-#define	SPEED_DIS	0x0F
-
-#define	DMARQp		0x80
-#define	IORDYp		0x40
-#define	DMAR_EN		0x20
-#define	DMAW_EN		0x10
-
-#define	MC3		0x08	/* DMA"C" timing */
-#define	MC2		0x04	/* DMA"C" timing */
-#define	MC1		0x02	/* DMA"C" timing */
-#define	MC0		0x01	/* DMA"C" timing */
+static void pdc_old_disable_66MHz_clock(ide_hwif_t *);
 
 static int pdc202xx_tune_chipset (ide_drive_t *drive, u8 xferspeed)
 {

@@ -107,52 +70,23 @@ static int pdc202xx_tune_chipset (ide_dr
 	u8 drive_pci		= 0x60 + (drive->dn << 2);
 	u8 speed		= ide_rate_filter(drive, xferspeed);
 
-	u32			drive_conf;
-	u8			AP, BP, CP, DP;
+	u32			drive_conf = 0;
+	u8			AP = 0, BP = 0, CP = 0;
 	u8			TA = 0, TB = 0, TC = 0;
 
-	if (drive->media != ide_disk &&
-		drive->media != ide_cdrom && speed < XFER_SW_DMA_0)
-		return -1;
+	/*
+	 * TODO: do this once per channel
+	 */
+	if (dev->device != PCI_DEVICE_ID_PROMISE_20246)
+		pdc_old_disable_66MHz_clock(hwif);
 
 	pci_read_config_dword(dev, drive_pci, &drive_conf);
-	pci_read_config_byte(dev, (drive_pci), &AP);
-	pci_read_config_byte(dev, (drive_pci)|0x01, &BP);
-	pci_read_config_byte(dev, (drive_pci)|0x02, &CP);
-	pci_read_config_byte(dev, (drive_pci)|0x03, &DP);
 
-	if (speed < XFER_SW_DMA_0) {
-		if ((AP & 0x0F) || (BP & 0x07)) {
-			/* clear PIO modes of lower 8421 bits of A Register */
-			pci_write_config_byte(dev, (drive_pci), AP &~0x0F);
-			pci_read_config_byte(dev, (drive_pci), &AP);
-
-			/* clear PIO modes of lower 421 bits of B Register */
-			pci_write_config_byte(dev, (drive_pci)|0x01, BP &~0x07);
-			pci_read_config_byte(dev, (drive_pci)|0x01, &BP);
-
-			pci_read_config_byte(dev, (drive_pci), &AP);
-			pci_read_config_byte(dev, (drive_pci)|0x01, &BP);
-		}
-	} else {
-		if ((BP & 0xF0) && (CP & 0x0F)) {
-			/* clear DMA modes of upper 842 bits of B Register */
-			/* clear PIO forced mode upper 1 bit of B Register */
-			pci_write_config_byte(dev, (drive_pci)|0x01, BP &~0xF0);
-			pci_read_config_byte(dev, (drive_pci)|0x01, &BP);
-
-			/* clear DMA modes of lower 8421 bits of C Register */
-			pci_write_config_byte(dev, (drive_pci)|0x02, CP &~0x0F);
-			pci_read_config_byte(dev, (drive_pci)|0x02, &CP);
-		}
-	}
-
-	pci_read_config_byte(dev, (drive_pci), &AP);
-	pci_read_config_byte(dev, (drive_pci)|0x01, &BP);
-	pci_read_config_byte(dev, (drive_pci)|0x02, &CP);
+	pci_read_config_byte(dev, drive_pci,     &AP);
+	pci_read_config_byte(dev, drive_pci + 1, &BP);
+	pci_read_config_byte(dev, drive_pci + 2, &CP);
 
 	switch(speed) {
-		case XFER_UDMA_6:	speed = XFER_UDMA_5;
 		case XFER_UDMA_5:
 		case XFER_UDMA_4:	TB = 0x20; TC = 0x01; break;
 		case XFER_UDMA_2:	TB = 0x20; TC = 0x01; break;

@@ -161,7 +95,7 @@ static int pdc202xx_tune_chipset (ide_dr
 		case XFER_UDMA_0:
 		case XFER_MW_DMA_2:	TB = 0x60; TC = 0x03; break;
 		case XFER_MW_DMA_1:	TB = 0x60; TC = 0x04; break;
-		case XFER_MW_DMA_0:
+		case XFER_MW_DMA_0:	TB = 0xE0; TC = 0x0F; break;
 		case XFER_SW_DMA_2:	TB = 0x60; TC = 0x05; break;
 		case XFER_SW_DMA_1:	TB = 0x80; TC = 0x06; break;
 		case XFER_SW_DMA_0:	TB = 0xC0; TC = 0x0B; break;

@@ -174,25 +108,39 @@ static int pdc202xx_tune_chipset (ide_dr
 	}
 
 	if (speed < XFER_SW_DMA_0) {
-		pci_write_config_byte(dev, (drive_pci), AP|TA);
-		pci_write_config_byte(dev, (drive_pci)|0x01, BP|TB);
+		/*
+		 * preserve SYNC_INT / ERDDY_EN bits while clearing
+		 * Prefetch_EN / IORDY_EN / PA[3:0] bits of register A
+		 */
+		AP &= ~0x3f;
+		if (drive->id->capability & 4)
+			AP |= 0x20;	/* set IORDY_EN bit */
+		if (drive->media == ide_disk)
+			AP |= 0x10;	/* set Prefetch_EN bit */
+		/* clear PB[4:0] bits of register B */
+		BP &= ~0x1f;
+		pci_write_config_byte(dev, drive_pci,     AP | TA);
+		pci_write_config_byte(dev, drive_pci + 1, BP | TB);
 	} else {
-		pci_write_config_byte(dev, (drive_pci)|0x01, BP|TB);
-		pci_write_config_byte(dev, (drive_pci)|0x02, CP|TC);
+		/* clear MB[2:0] bits of register B */
+		BP &= ~0xe0;
+		/* clear MC[3:0] bits of register C */
+		CP &= ~0x0f;
+		pci_write_config_byte(dev, drive_pci + 1, BP | TB);
+		pci_write_config_byte(dev, drive_pci + 2, CP | TC);
 	}
 
 #if PDC202XX_DEBUG_DRIVE_INFO
 	printk(KERN_DEBUG "%s: %s drive%d 0x%08x ",
 		drive->name, ide_xfer_verbose(speed),
 		drive->dn, drive_conf);
-		pci_read_config_dword(dev, drive_pci, &drive_conf);
+	pci_read_config_dword(dev, drive_pci, &drive_conf);
 	printk("0x%08x\n", drive_conf);
-#endif /* PDC202XX_DEBUG_DRIVE_INFO */
+#endif
 
-	return (ide_config_drive_speed(drive, speed));
+	return ide_config_drive_speed(drive, speed);
 }
 
-
 static void pdc202xx_tune_drive(ide_drive_t *drive, u8 pio)
 {
 	pio = ide_get_best_pio_mode(drive, pio, 4, NULL);

@@ -210,6 +158,8 @@ static u8 pdc202xx_old_cable_detect (ide
  * Set the control register to use the 66MHz system
  * clock for UDMA 3/4/5 mode operation when necessary.
  *
+ * FIXME: this register is shared by both channels, some locking is needed
+ *
  * It may also be possible to leave the 66MHz clock on
  * and readjust the timing parameters.
  */

@@ -231,55 +181,13 @@ static void pdc_old_disable_66MHz_clock(
 
 static int config_chipset_for_dma (ide_drive_t *drive)
 {
-	struct hd_driveid *id	= drive->id;
-	ide_hwif_t *hwif	= HWIF(drive);
-	struct pci_dev *dev	= hwif->pci_dev;
-	u32 drive_conf		= 0;
-	u8 drive_pci		= 0x60 + (drive->dn << 2);
-	u8 test1 = 0, test2 = 0, speed = -1;
-	u8 AP = 0;
-
-	if (dev->device != PCI_DEVICE_ID_PROMISE_20246)
-		pdc_old_disable_66MHz_clock(drive->hwif);
-
-	drive_pci = 0x60 + (drive->dn << 2);
-	pci_read_config_dword(dev, drive_pci, &drive_conf);
-	if ((drive_conf != 0x004ff304) && (drive_conf != 0x004ff3c4))
-		goto chipset_is_set;
+	u8 speed = ide_max_dma_mode(drive);
 
-	pci_read_config_byte(dev, drive_pci, &test1);
-	if (!(test1 & SYNC_ERRDY_EN)) {
-		if (drive->select.b.unit & 0x01) {
-			pci_read_config_byte(dev, drive_pci - 4, &test2);
-			if ((test2 & SYNC_ERRDY_EN) &&
-			    !(test1 & SYNC_ERRDY_EN)) {
-				pci_write_config_byte(dev, drive_pci,
-					test1|SYNC_ERRDY_EN);
-			}
-		} else {
-			pci_write_config_byte(dev, drive_pci,
-				test1|SYNC_ERRDY_EN);
-		}
-	}
-
-chipset_is_set:
-
-	pci_read_config_byte(dev, (drive_pci), &AP);
-	if (id->capability & 4) /* IORDY_EN */
-		pci_write_config_byte(dev, (drive_pci), AP|IORDY_EN);
-	pci_read_config_byte(dev, (drive_pci), &AP);
-	if (drive->media == ide_disk)	/* PREFETCH_EN */
-		pci_write_config_byte(dev, (drive_pci), AP|PREFETCH_EN);
-
-	speed = ide_max_dma_mode(drive);
-
-	if (!(speed)) {
-		/* restore original pci-config space */
-		pci_write_config_dword(dev, drive_pci, drive_conf);
+	if (!speed)
 		return 0;
-	}
 
-	(void) hwif->speedproc(drive, speed);
+	(void)pdc202xx_tune_chipset(drive, speed);
+
 	return ide_dma_enable(drive);
 }
#1 lore raw

Hello.

Quoted 8 lines · Bartlomiej Zolnierkiewicz

[PATCH] pdc202xx_old: rewrite mode programming code

This patch is based on the documentation (I would like to thank Promise for it) and also partially on the older vendor driver.

Rewrite mode programming code:

* fix XFER_MW_DMA0 timings (they were overclocked, use the official ones)

Erm, those look a bit doubtful...

Quoted 8 lines

* fix bitmasks for clearing bits of register B:

- when programming DMA mode bit 0x10 of register B was cleared which resulted in overclocked PIO timing setting (iff PIO0 was used)

- when programming PIO mode bits 0x18 weren't cleared so suboptimal

    timings were used for PIO1-4 if PIO0 was previously set (bit 0x10)
    and for PIO0/3/4 if PIO1/2 was previously set (bit 0x08)

I'm glad that somebody fixed those pesky masks at last. :-)
I've noticed that issue more than a year ago but lacking time,
documentation and access to hardware, have never got to really fixing it... :-(

Quoted 4 lines

Index: b/drivers/ide/pci/pdc202xx_old.c
===================================================================

drivers/ide/pci/pdc202xx_old.c +0 -0

[...]

Quoted 10 lines
drivers/ide/pci/pdc202xx_old.c +2 -2
@@ -107,52 +70,23 @@ static int pdc202xx_tune_chipset (ide_dr
 	u8 drive_pci		= 0x60 + (drive->dn << 2);
 	u8 speed		= ide_rate_filter(drive, xferspeed);
 
-	u32			drive_conf;
-	u8			AP, BP, CP, DP;
+	u32			drive_conf = 0;
+	u8			AP = 0, BP = 0, CP = 0;
 	u8			TA = 0, TB = 0, TC = 0;

drivers/ide/pci/pdc202xx_old.c static int pdc202xx_tune_chipset (ide_dr · line 83

Quoted 10 lines
drivers/ide/pci/pdc202xx_old.c +5 -3
-	if (drive->media != ide_disk &&
-		drive->media != ide_cdrom && speed < XFER_SW_DMA_0)
-		return -1;
+	/*
+	 * TODO: do this once per channel
+	 */
+	if (dev->device != PCI_DEVICE_ID_PROMISE_20246)
+		pdc_old_disable_66MHz_clock(hwif);
 
 	pci_read_config_dword(dev, drive_pci, &drive_conf);

This function never uses it as u32 entity, I wonder why read it? Just to
hush a warning? :-)

drivers/ide/pci/pdc202xx_old.c static int pdc202xx_tune_chipset (ide_dr · line 91

Quoted 4 lines
drivers/ide/pci/pdc202xx_old.c +0 -1
 	switch(speed) {
-		case XFER_UDMA_6:	speed = XFER_UDMA_5;
 		case XFER_UDMA_5:
 		case XFER_UDMA_4:	TB = 0x20; TC = 0x01; break;

The same clocks for UDMA4/5... I wonder if PDC20265/7 indeed supported
UDMA5 (as I'm not seeing any extra clock switching for this mode)?

drivers/ide/pci/pdc202xx_old.c static int pdc202xx_tune_chipset (ide_dr · line 98

Quoted 7 lines
drivers/ide/pci/pdc202xx_old.c +1 -1
 		case XFER_UDMA_2:	TB = 0x20; TC = 0x01; break;

@@ -161,7 +95,7 @@ static int pdc202xx_tune_chipset (ide_dr
 		case XFER_UDMA_0:
 		case XFER_MW_DMA_2:	TB = 0x60; TC = 0x03; break;
 		case XFER_MW_DMA_1:	TB = 0x60; TC = 0x04; break;
-		case XFER_MW_DMA_0:
+		case XFER_MW_DMA_0:	TB = 0xE0; TC = 0x0F; break;

This seems even slower than SWDMA0!
Let's assume that means 7 active cycles and 15 recovery cycles (MWDMA1/2
settings seem to confirm this hypothesis) -- this would give us 720 ns vs the specified 480. Could you shed some light on what these fields mean? :-/

drivers/ide/pci/pdc202xx_old.c static int pdc202xx_tune_chipset (ide_dr · line 99

Quoted 1 line
drivers/ide/pci/pdc202xx_old.c +0 -0
 		case XFER_SW_DMA_2:	TB = 0x60; TC = 0x05; break;

Well, this don't look right to me -- we need longer active time (given
that my hypothesis is true)

drivers/ide/pci/pdc202xx_old.c static int pdc202xx_tune_chipset (ide_dr · line 100

Quoted 1 line
drivers/ide/pci/pdc202xx_old.c +0 -0
 		case XFER_SW_DMA_1:	TB = 0x80; TC = 0x06; break;

This looks more fitting for SWDMA1 -- however, the recovery time seems to
be overly long. It certainly doesn't look like SWDMA1 unless the active/recover times are not in clock cycles (should be 8 cycles, not 4 or 6).

drivers/ide/pci/pdc202xx_old.c static int pdc202xx_tune_chipset (ide_dr · line 101

Quoted 1 line
drivers/ide/pci/pdc202xx_old.c +0 -0
 		case XFER_SW_DMA_0:	TB = 0xC0; TC = 0x0B; break;

Same here -- should be 16 cycles both for active and recovery...

MBR, Sergei

author
#2 lore raw

Hi,

Quoted 11 lines · Sergei Shtylyov

Hello.

Quoted 8 lines · Bartlomiej Zolnierkiewicz

[PATCH] pdc202xx_old: rewrite mode programming code

This patch is based on the documentation (I would like to thank Promise for it) and also partially on the older vendor driver.

Rewrite mode programming code:

* fix XFER_MW_DMA0 timings (they were overclocked, use the official ones)

official == same as in the docs and vendor driver :-)

Quoted 1 line

Erm, those look a bit doubtful...

I believe that they are correct - please see explanations below.

Quoted 37 lines
Quoted 8 lines

* fix bitmasks for clearing bits of register B:

- when programming DMA mode bit 0x10 of register B was cleared which resulted in overclocked PIO timing setting (iff PIO0 was used)

- when programming PIO mode bits 0x18 weren't cleared so suboptimal

    timings were used for PIO1-4 if PIO0 was previously set (bit 0x10)
    and for PIO0/3/4 if PIO1/2 was previously set (bit 0x08)

I'm glad that somebody fixed those pesky masks at last. :-) I've noticed that issue more than a year ago but lacking time,
documentation and access to hardware, have never got to really fixing it... :-(

Quoted 4 lines

Index: b/drivers/ide/pci/pdc202xx_old.c
===================================================================

drivers/ide/pci/pdc202xx_old.c +0 -0

[...]

Quoted 19 lines
drivers/ide/pci/pdc202xx_old.c +7 -5
@@ -107,52 +70,23 @@ static int pdc202xx_tune_chipset (ide_dr
 	u8 drive_pci		= 0x60 + (drive->dn << 2);
 	u8 speed		= ide_rate_filter(drive, xferspeed);
 
-	u32			drive_conf;
-	u8			AP, BP, CP, DP;
+	u32			drive_conf = 0;
+	u8			AP = 0, BP = 0, CP = 0;
 	u8			TA = 0, TB = 0, TC = 0;
 
-	if (drive->media != ide_disk &&
-		drive->media != ide_cdrom && speed < XFER_SW_DMA_0)
-		return -1;
+	/*
+	 * TODO: do this once per channel
+	 */
+	if (dev->device != PCI_DEVICE_ID_PROMISE_20246)
+		pdc_old_disable_66MHz_clock(hwif);
Quoted 4 lines

drivers/ide/pci/pdc202xx_old.c static int pdc202xx_tune_chipset (ide_dr · line 83

Quoted 1 line
drivers/ide/pci/pdc202xx_old.c +0 -0
 	pci_read_config_dword(dev, drive_pci, &drive_conf);

This function never uses it as u32 entity, I wonder why read it? Just to
hush a warning? :-)

It is used for debugging purposes by PDC202XX_DEBUG_DRIVE_INFO (it prints old/new content of drive configuration registers).

I think that I'll cover it by #if PDC202XX_DEBUG_DRIVE_INFO to make the aforementioned fact clear and to optimize non-debug case a bit...

Quoted 7 lines

drivers/ide/pci/pdc202xx_old.c static int pdc202xx_tune_chipset (ide_dr · line 91

Quoted 4 lines
drivers/ide/pci/pdc202xx_old.c +0 -1
 	switch(speed) {
-		case XFER_UDMA_6:	speed = XFER_UDMA_5;
 		case XFER_UDMA_5:
 		case XFER_UDMA_4:	TB = 0x20; TC = 0x01; break;

The same clocks for UDMA4/5... I wonder if PDC20265/7 indeed supported
UDMA5 (as I'm not seeing any extra clock switching for this mode)?

Probably chipset snoops WIN_SETFEATURES (w/ SETFEATURES_XFER subcommand) and sets the appropriate timings internally. It might be possible to drop the timing setup completely for UDMA modes but the vendor driver actually does it so I left it alone for now.

Quoted 4 lines
Quoted 4 lines
drivers/ide/pci/pdc202xx_old.c +0 -0
 		case XFER_UDMA_2:	TB = 0x20; TC = 0x01; break;

@@ -161,7 +95,7 @@ static int pdc202xx_tune_chipset (ide_dr
 		case XFER_UDMA_0:
 		case XFER_MW_DMA_2:	TB = 0x60; TC = 0x03; break;
Quoted 8 lines

drivers/ide/pci/pdc202xx_old.c static int pdc202xx_tune_chipset (ide_dr · line 98

Quoted 3 lines
drivers/ide/pci/pdc202xx_old.c +1 -1
 		case XFER_MW_DMA_1:	TB = 0x60; TC = 0x04; break;
-		case XFER_MW_DMA_0:
+		case XFER_MW_DMA_0:	TB = 0xE0; TC = 0x0F; break;

This seems even slower than SWDMA0!
Let's assume that means 7 active cycles and 15 recovery cycles (MWDMA1/2
settings seem to confirm this hypothesis) -- this would give us 720 ns vs the specified 480. Could you shed some light on what these fields mean? :-/

The calculations are done in a different way so we get the correct timings:

7 cycles (== 210 ns) are used for active time
16 cycles (== 480 ns) are used for cycle time

These timings are the maximum possible ones using MB[2:0] and MC[3:0] (please refer to the comments in the code to see how MB/MC map to TB/TC).

Quoted 4 lines

drivers/ide/pci/pdc202xx_old.c static int pdc202xx_tune_chipset (ide_dr · line 99

Quoted 1 line
drivers/ide/pci/pdc202xx_old.c +0 -0
 		case XFER_SW_DMA_2:	TB = 0x60; TC = 0x05; break;

Well, this don't look right to me -- we need longer active time (given
that my hypothesis is true)

MB[2:0] and MC[3:0] are for MWDMA/UDMA timings only (it is impossible to set SWDMA0/1 timings using them).

I suppose that PA[3:0] and PB[4:0] (PIO timings) should be used for SWDMA.

Quoted 9 lines

drivers/ide/pci/pdc202xx_old.c static int pdc202xx_tune_chipset (ide_dr · line 100

Quoted 1 line
drivers/ide/pci/pdc202xx_old.c +0 -0
 		case XFER_SW_DMA_1:	TB = 0x80; TC = 0x06; break;

This looks more fitting for SWDMA1 -- however, the recovery time seems to
be overly long. It certainly doesn't look like SWDMA1 unless the active/recover times are not in clock cycles (should be 8 cycles, not 4 or 6).

drivers/ide/pci/pdc202xx_old.c static int pdc202xx_tune_chipset (ide_dr · line 101

Quoted 1 line
drivers/ide/pci/pdc202xx_old.c +0 -0
 		case XFER_SW_DMA_0:	TB = 0xC0; TC = 0x0B; break;

Same here -- should be 16 cycles both for active and recovery...

Fixing SWDMA was not a goal of my changes (my patch is already quite overloaded) but I would happily welcome the incremental patch doing it.

[ I'm also aware that it may difficult without docs so it still on my
personal TODO if nobody beats my to it earlier. ]

Thanks,
Bart

#3 lore raw

Hello.

Quoted 7 lines · Bartlomiej Zolnierkiewicz
Quoted 6 lines
Quoted 6 lines

[PATCH] pdc202xx_old: rewrite mode programming code

This patch is based on the documentation (I would like to thank Promise for it) and also partially on the older vendor driver.

Rewrite mode programming code:

Quoted 7 lines
Quoted 1 line
Quoted 1 line

* fix XFER_MW_DMA0 timings (they were overclocked, use the official ones)

official == same as in the docs and vendor driver :-)

Quoted 1 line

Erm, those look a bit doubtful...

I believe that they are correct - please see explanations below.

Yeah, sorry about that. Only SWDMA timings are suspicious.

Quoted 24 lines
Quoted 23 lines
Quoted 4 lines

Index: b/drivers/ide/pci/pdc202xx_old.c
===================================================================

drivers/ide/pci/pdc202xx_old.c +0 -0

[...]

Quoted 18 lines
drivers/ide/pci/pdc202xx_old.c +7 -5
@@ -107,52 +70,23 @@ static int pdc202xx_tune_chipset (ide_dr
 	u8 drive_pci		= 0x60 + (drive->dn << 2);
 	u8 speed		= ide_rate_filter(drive, xferspeed);
 
-	u32			drive_conf;
-	u8			AP, BP, CP, DP;
+	u32			drive_conf = 0;
+	u8			AP = 0, BP = 0, CP = 0;
 	u8			TA = 0, TB = 0, TC = 0;
 
-	if (drive->media != ide_disk &&
-		drive->media != ide_cdrom && speed < XFER_SW_DMA_0)
-		return -1;
+	/*
+	 * TODO: do this once per channel
+	 */
+	if (dev->device != PCI_DEVICE_ID_PROMISE_20246)
+		pdc_old_disable_66MHz_clock(hwif);
Quoted 7 lines
Quoted 4 lines

drivers/ide/pci/pdc202xx_old.c static int pdc202xx_tune_chipset (ide_dr · line 83

Quoted 1 line
drivers/ide/pci/pdc202xx_old.c +0 -0
 	pci_read_config_dword(dev, drive_pci, &drive_conf);

This function never uses it as u32 entity, I wonder why read it? Just to
hush a warning? :-)

It is used for debugging purposes by PDC202XX_DEBUG_DRIVE_INFO (it prints old/new content of drive configuration registers).

Ah, indeed. That printk() didn't look obvious... :-)

Quoted 11 lines

I think that I'll cover it by #if PDC202XX_DEBUG_DRIVE_INFO to make the aforementioned fact clear and to optimize non-debug case a bit...

Quoted 7 lines

drivers/ide/pci/pdc202xx_old.c static int pdc202xx_tune_chipset (ide_dr · line 91

Quoted 4 lines
drivers/ide/pci/pdc202xx_old.c +0 -1
 	switch(speed) {
-		case XFER_UDMA_6:	speed = XFER_UDMA_5;
 		case XFER_UDMA_5:
 		case XFER_UDMA_4:	TB = 0x20; TC = 0x01; break;

The same clocks for UDMA4/5... I wonder if PDC20265/7 indeed supported
UDMA5 (as I'm not seeing any extra clock switching for this mode)?

Quoted 4 lines

Probably chipset snoops WIN_SETFEATURES (w/ SETFEATURES_XFER subcommand) and sets the appropriate timings internally. It might be possible to drop the timing setup completely for UDMA modes but the vendor driver actually does it so I left it alone for now.

Actually, their BIOSes also do it and even do some fixups to the table
with timings (I forgot on which condition), sooo.. snooping doesn't seem likely (although possible, judging on the UDMA/MWDMA issue).

Quoted 12 lines
Quoted 11 lines

drivers/ide/pci/pdc202xx_old.c static int pdc202xx_tune_chipset (ide_dr · line 98

Quoted 7 lines
drivers/ide/pci/pdc202xx_old.c +1 -1
 		case XFER_UDMA_2:	TB = 0x20; TC = 0x01; break;

@@ -161,7 +95,7 @@ static int pdc202xx_tune_chipset (ide_dr
 		case XFER_UDMA_0:
 		case XFER_MW_DMA_2:	TB = 0x60; TC = 0x03; break;
 		case XFER_MW_DMA_1:	TB = 0x60; TC = 0x04; break;
-		case XFER_MW_DMA_0:
+		case XFER_MW_DMA_0:	TB = 0xE0; TC = 0x0F; break;

This seems even slower than SWDMA0!
Let's assume that means 7 active cycles and 15 recovery cycles (MWDMA1/2
settings seem to confirm this hypothesis) -- this would give us 720 ns vs the specified 480. Could you shed some light on what these fields mean? :-/

Quoted 4 lines

The calculations are done in a different way so we get the correct timings:

7 cycles (== 210 ns) are used for active time
16 cycles (== 480 ns) are used for cycle time

Ah, indeed, I've erred in MWDMA1/2 calculations. This makes sense then.

Quoted 2 lines

These timings are the maximum possible ones using MB[2:0] and MC[3:0] (please refer to the comments in the code to see how MB/MC map to TB/TC).

Yeah, I've taken that into account of course.

Quoted 7 lines
Quoted 4 lines

drivers/ide/pci/pdc202xx_old.c static int pdc202xx_tune_chipset (ide_dr · line 99

Quoted 1 line
drivers/ide/pci/pdc202xx_old.c +0 -0
 		case XFER_SW_DMA_2:	TB = 0x60; TC = 0x05; break;

Well, this don't look right to me -- we need longer active time (given
that my hypothesis is true)

MB[2:0] and MC[3:0] are for MWDMA/UDMA timings only (it is impossible to set SWDMA0/1 timings using them).

That only strenghtens my belief that SWDMA was never intended to be
working on these chips, and should just be dropped (or only SWDMA2 supported).

Quoted 1 line

I suppose that PA[3:0] and PB[4:0] (PIO timings) should be used for SWDMA.

This seems doubtful bit of course you can never predict what the broken-
minded chip designers could come up with... :-)

Quoted 7 lines
Quoted 7 lines

drivers/ide/pci/pdc202xx_old.c static int pdc202xx_tune_chipset (ide_dr · line 100

Quoted 1 line
drivers/ide/pci/pdc202xx_old.c +0 -0
 		case XFER_SW_DMA_1:	TB = 0x80; TC = 0x06; break;

This looks more fitting for SWDMA1 -- however, the recovery time seems to
be overly long. It certainly doesn't look like SWDMA1 unless the active/recover times are not in clock cycles (should be 8 cycles, not 4 or 6).

Quoted 1 line
drivers/ide/pci/pdc202xx_old.c +0 -0
 		case XFER_SW_DMA_0:	TB = 0xC0; TC = 0x0B; break;
Quoted 7 lines
Quoted 1 line

Same here -- should be 16 cycles both for active and recovery...

Fixing SWDMA was not a goal of my changes (my patch is already quite overloaded) but I would happily welcome the incremental patch doing it.

[ I'm also aware that it may difficult without docs so it still on my
personal TODO if nobody beats my to it earlier. ]

I'd need to find time and get to the real hardware as a mimumum (which I
haven't been able to do in the past months... :-)

Quoted 2 lines

Thanks,
Bart

MBR, Sergei

#4 lore raw
Quoted 7 lines · Hello, I wrote:
Quoted 5 lines

official == same as in the docs and vendor driver :-)

Quoted 1 line

Erm, those look a bit doubtful...

I believe that they are correct - please see explanations below.

Yeah, sorry about that. Only SWDMA timings are suspicious.

Hm, too early to say sorry. I was hasty/distacted and forgot what I was
going to write... :-)

Quoted 18 lines
Quoted 17 lines
Quoted 17 lines
Quoted 4 lines

Index: b/drivers/ide/pci/pdc202xx_old.c
===================================================================

drivers/ide/pci/pdc202xx_old.c +0 -0

[...]

drivers/ide/pci/pdc202xx_old.c static int pdc202xx_tune_chipset (ide_dr · line 98

Quoted 6 lines
drivers/ide/pci/pdc202xx_old.c +1 -1
@@ -161,7 +95,7 @@ static int pdc202xx_tune_chipset (ide_dr
        case XFER_UDMA_0:
        case XFER_MW_DMA_2:    TB = 0x60; TC = 0x03; break;
        case XFER_MW_DMA_1:    TB = 0x60; TC = 0x04; break;
-        case XFER_MW_DMA_0:
+        case XFER_MW_DMA_0:    TB = 0xE0; TC = 0x0F; break;

This seems even slower than SWDMA0!
Let's assume that means 7 active cycles and 15 recovery cycles
(MWDMA1/2 settings seem to confirm this hypothesis) -- this would give us 720 ns vs the specified 480. Could you shed some light on what these fields mean? :-/

Quoted 3 lines
Quoted 3 lines

The calculations are done in a different way so we get the correct timings:

7 cycles (== 210 ns) are used for active time

Ugh, forgot to say: this is overclocked, 215 ns is the minimum active time
for this mode.

Quoted 3 lines
Quoted 1 line

16 cycles (== 480 ns) are used for cycle time

Ah, indeed, I've erred in MWDMA1/2 calculations. This makes sense then.

MBR, Sergei

author
#5 lore raw
Quoted 33 lines · Sergei Shtylyov
Quoted 7 lines · Hello, I wrote:
Quoted 5 lines

official == same as in the docs and vendor driver :-)

Quoted 1 line

Erm, those look a bit doubtful...

I believe that they are correct - please see explanations below.

Yeah, sorry about that. Only SWDMA timings are suspicious.

Hm, too early to say sorry. I was hasty/distacted and forgot what I was
going to write... :-)

Quoted 19 lines
Quoted 19 lines
Quoted 19 lines
Quoted 4 lines

Index: b/drivers/ide/pci/pdc202xx_old.c
===================================================================

drivers/ide/pci/pdc202xx_old.c +0 -0

[...]

drivers/ide/pci/pdc202xx_old.c static int pdc202xx_tune_chipset (ide_dr · line 98

Quoted 6 lines
drivers/ide/pci/pdc202xx_old.c +1 -1
@@ -161,7 +95,7 @@ static int pdc202xx_tune_chipset (ide_dr
        case XFER_UDMA_0:
        case XFER_MW_DMA_2:    TB = 0x60; TC = 0x03; break;
        case XFER_MW_DMA_1:    TB = 0x60; TC = 0x04; break;
-        case XFER_MW_DMA_0:
+        case XFER_MW_DMA_0:    TB = 0xE0; TC = 0x0F; break;

This seems even slower than SWDMA0!
Let's assume that means 7 active cycles and 15 recovery cycles
(MWDMA1/2 settings seem to confirm this hypothesis) -- this would give us 720 ns vs the specified 480. Could you shed some light on what these fields mean? :-/

Quoted 7 lines
Quoted 4 lines
Quoted 4 lines

The calculations are done in a different way so we get the correct timings:

7 cycles (== 210 ns) are used for active time

Ugh, forgot to say: this is overclocked, 215 ns is the minimum active time
for this mode.

I know that is why I wrote in my previous mail:

"These timings are the maximum possible ones using MB[2:0] and MC[3:0]"

Driver can't do better than hardware and hopefully these 5 ns are compensated somehow by the chipset (or not :-).

Bart