Skip to contentSkip to conversation

ide: use only ->set_pio_mode method for programming PIO modes

PATCH In review Bartlomiej Zolnierkiewicz 2 participants 5 messages Jul 20, 2007 – Jul 23, 2007

Subsystems (2)
PowerPC libata
lore
Share & tools
lore

Bartlomiej Zolnierkiewicz

author
#0 lore raw

PATCH 4/4 ide: use only ->set_pio_mode method for programming PIO modes

Use ->set_pio_mode method to program PIO modes in ide_set_xfer_rate() (the only place which used ->speedproc to program PIO modes) and remove handling of PIO modes from all ->speedproc implementations.

There should be no functionality changes caused by this patch.

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---
 drivers/ide/cris/ide-cris.c   |    5 -----
 drivers/ide/ide-lib.c         |   12 ++++++++++++
 drivers/ide/mips/au1xxx-ide.c |    5 -----
 drivers/ide/pci/alim15x3.c    |    5 -----
 drivers/ide/pci/atiixp.c      |    5 -----
 drivers/ide/pci/cmd64x.c      |    8 --------
 drivers/ide/pci/cs5520.c      |   36 ++++++++++++++----------------------
 drivers/ide/pci/cs5530.c      |    7 -------
 drivers/ide/pci/it8213.c      |    5 -----
 drivers/ide/pci/it821x.c      |    9 ---------
 drivers/ide/pci/piix.c        |    5 -----
 drivers/ide/pci/sc1200.c      |   10 ----------
 drivers/ide/pci/scc_pata.c    |    7 -------
 drivers/ide/pci/serverworks.c |    5 -----
 drivers/ide/pci/siimage.c     |    7 -------
 drivers/ide/pci/sis5513.c     |   12 ++----------
 drivers/ide/pci/sl82c105.c    |    8 --------
 drivers/ide/pci/slc90e66.c    |    5 -----
 drivers/ide/ppc/pmac.c        |    7 -------
 19 files changed, 28 insertions(+), 135 deletions(-)
drivers/ide/cris/ide-cris.c +0 -5
Index: b/drivers/ide/cris/ide-cris.c
===================================================================
@@ -724,11 +724,6 @@ static int speed_cris_ide(ide_drive_t *d
 {
 	int cyc = 0, dvs = 0, strobe = 0, hold = 0;
 
-	if (speed >= XFER_PIO_0 && speed <= XFER_PIO_4) {
-		cris_set_pio_mode(drive, speed - XFER_PIO_0);
-		return ide_config_drive_speed(drive, speed);
-	}
-
 	switch(speed)
 	{
 		case XFER_UDMA_0:
drivers/ide/ide-lib.c +12 -0
Index: b/drivers/ide/ide-lib.c
===================================================================
@@ -398,6 +398,18 @@ int ide_set_xfer_rate(ide_drive_t *drive
 
 	rate = ide_rate_filter(drive, rate);
 
+	if (rate >= XFER_PIO_0 && rate <= XFER_PIO_5) {
+		if (hwif->set_pio_mode)
+			hwif->set_pio_mode(drive, rate - XFER_PIO_0);
+
+		/*
+		 * FIXME: this incorrect to return zero here but
+		 * since all users of ide_set_xfer_rate() ignore
+		 * the return value it is not a problem currently
+		 */
+		return 0;
+	}
+
 	return hwif->speedproc(drive, rate);
 }
drivers/ide/mips/au1xxx-ide.c +0 -5
Index: b/drivers/ide/mips/au1xxx-ide.c
===================================================================
@@ -177,11 +177,6 @@ static int auide_tune_chipset(ide_drive_
 	mem_sttime = 0;
 	mem_stcfg  = au_readl(MEM_STCFG2);
 
-	if (speed >= XFER_PIO_0 && speed <= XFER_PIO_4) {
-		au1xxx_set_pio_mode(drive, speed - XFER_PIO_0);
-		return 0;
-	}
-
 	switch(speed) {
 #ifdef CONFIG_BLK_DEV_IDE_AU1XXX_MDMA2_DBDMA
 	case XFER_MW_DMA_2:
drivers/ide/pci/alim15x3.c +0 -5
Index: b/drivers/ide/pci/alim15x3.c
===================================================================
@@ -421,11 +421,6 @@ static int ali15x3_tune_chipset(ide_driv
 	if (speed < XFER_PIO_0)
 		return 1;
 
-	if (speed >= XFER_PIO_0 && speed <= XFER_PIO_5) {
-		ali_tune_pio(drive, speed - XFER_PIO_0);
-		return ide_config_drive_speed(drive, speed);
-	}
-
 	if (speed == XFER_UDMA_6)
 		speed1 = 0x47;
drivers/ide/pci/atiixp.c +0 -5
Index: b/drivers/ide/pci/atiixp.c
===================================================================
@@ -178,11 +178,6 @@ static int atiixp_speedproc(ide_drive_t 
 	u16 tmp16;
 	u8 pio;
 
-	if (speed >= XFER_PIO_0 && speed <= XFER_PIO_4) {
-		atiixp_tune_pio(drive, speed - XFER_PIO_0);
-		return ide_config_drive_speed(drive, speed);
-	}
-
 	spin_lock_irqsave(&atiixp_lock, flags);
 
 	save_mdma_mode[drive->dn] = 0;
drivers/ide/pci/cmd64x.c +0 -8
Index: b/drivers/ide/pci/cmd64x.c
===================================================================
@@ -323,14 +323,6 @@ static int cmd64x_tune_chipset(ide_drive
 	case XFER_MW_DMA_0:
 		program_cycle_times(drive, 480, 215);
 		break;
-	case XFER_PIO_5:
-	case XFER_PIO_4:
-	case XFER_PIO_3:
-	case XFER_PIO_2:
-	case XFER_PIO_1:
-	case XFER_PIO_0:
-		cmd64x_tune_pio(drive, speed - XFER_PIO_0);
-		break;
 	default:
 		return 1;
 	}
drivers/ide/pci/cs5520.c +14 -22
Index: b/drivers/ide/pci/cs5520.c
===================================================================
@@ -66,30 +66,13 @@ static struct pio_clocks cs5520_pio_cloc
 	{1, 2, 1}
 };
 
-static int cs5520_tune_chipset(ide_drive_t *drive, const u8 speed)
+static void cs5520_set_pio_mode(ide_drive_t *drive, const u8 pio)
 {
 	ide_hwif_t *hwif = HWIF(drive);
 	struct pci_dev *pdev = hwif->pci_dev;
-	int pio = speed;
-	u8 reg;
 	int controller = drive->dn > 1 ? 1 : 0;
+	u8 reg;
 
-	switch(speed)
-	{
-		case XFER_PIO_4:
-		case XFER_PIO_3:
-		case XFER_PIO_2:
-		case XFER_PIO_1:
-		case XFER_PIO_0:
-			pio -= XFER_PIO_0;
-			break;
-		default:
-			pio = 0;
-			printk(KERN_ERR "cs55x0: bad ide timing.\n");
-	}
-	
-	printk("PIO clocking = %d\n", pio);
-	
 	/* FIXME: if DMA = 1 do we need to set the DMA bit here ? */
 
 	/* 8bit CAT/CRT - 8bit command timing for channel */

@@ -114,12 +97,21 @@ static int cs5520_tune_chipset(ide_drive
 	reg |= 1<<((drive->dn&1)+5);
 	outb(reg, hwif->dma_base + 0x02 + 8*controller);
 
-	return ide_config_drive_speed(drive, speed);
+	(void)ide_config_drive_speed(drive, XFER_PIO_0 + pio);
 }
 
-static void cs5520_set_pio_mode(ide_drive_t *drive, const u8 pio)
+static int cs5520_tune_chipset(ide_drive_t *drive, const u8 speed)
 {
-	cs5520_tune_chipset(drive, XFER_PIO_0 + pio);
+	printk(KERN_ERR "cs55x0: bad ide timing.\n");
+
+	cs5520_set_pio_mode(drive, 0);
+
+	/*
+	 * FIXME: this incorrect to return zero here but
+	 * since all users of ide_set_xfer_rate() ignore
+	 * the return value it is not a problem currently
+	 */
+	return 0;
 }
 
 static int cs5520_config_drive_xfer_rate(ide_drive_t *drive)
drivers/ide/pci/cs5530.c +0 -7
Index: b/drivers/ide/pci/cs5530.c
===================================================================
@@ -162,13 +162,6 @@ static int cs5530_tune_chipset(ide_drive
 		case XFER_MW_DMA_0:	timings = 0x00077771; break;
 		case XFER_MW_DMA_1:	timings = 0x00012121; break;
 		case XFER_MW_DMA_2:	timings = 0x00002020; break;
-		case XFER_PIO_4:
-		case XFER_PIO_3:
-		case XFER_PIO_2:
-		case XFER_PIO_1:
-		case XFER_PIO_0:
-			cs5530_tunepio(drive, mode - XFER_PIO_0);
-			return 0;
 		default:
 			BUG();
 			break;
drivers/ide/pci/it8213.c +0 -5
Index: b/drivers/ide/pci/it8213.c
===================================================================
@@ -134,11 +134,6 @@ static int it8213_tune_chipset(ide_drive
 	u16			reg4042, reg4a;
 	u8			reg48, reg54, reg55;
 
-	if (speed >= XFER_PIO_0 && speed <= XFER_PIO_4) {
-		it8213_tune_pio(drive, speed - XFER_PIO_0);
-		return ide_config_drive_speed(drive, speed);
-	}
-
 	pci_read_config_word(dev, maslave, &reg4042);
 	pci_read_config_byte(dev, 0x48, &reg48);
 	pci_read_config_word(dev, 0x4a, &reg4a);
drivers/ide/pci/it821x.c +0 -9
Index: b/drivers/ide/pci/it821x.c
===================================================================
@@ -418,15 +418,6 @@ static int it821x_tune_chipset(ide_drive
 	ide_hwif_t *hwif	= drive->hwif;
 	struct it821x_dev *itdev = ide_get_hwifdata(hwif);
 
-	switch (speed) {
-	case XFER_PIO_4:
-	case XFER_PIO_3:
-	case XFER_PIO_2:
-	case XFER_PIO_1:
-	case XFER_PIO_0:
-		return it821x_tunepio(drive, speed - XFER_PIO_0);
-	}
-
 	if (itdev->smart == 0) {
 		switch (speed) {
 			/* MWDMA tuning is really hard because our MWDMA and PIO
drivers/ide/pci/piix.c +0 -5
Index: b/drivers/ide/pci/piix.c
===================================================================
@@ -243,11 +243,6 @@ static int piix_tune_chipset(ide_drive_t
 	u16			reg4042, reg4a;
 	u8			reg48, reg54, reg55;
 
-	if (speed >= XFER_PIO_0 && speed <= XFER_PIO_4) {
-		piix_tune_pio(drive, speed - XFER_PIO_0);
-		return ide_config_drive_speed(drive, speed);
-	}
-
 	pci_read_config_word(dev, maslave, &reg4042);
 	sitre = (reg4042 & 0x4000) ? 1 : 0;
 	pci_read_config_byte(dev, 0x48, &reg48);
drivers/ide/pci/sc1200.c +0 -10
Index: b/drivers/ide/pci/sc1200.c
===================================================================
@@ -152,16 +152,6 @@ static int sc1200_tune_chipset(ide_drive
 	if (sc1200_set_xfer_mode(drive, mode))
 		return 1;	/* failure */
 
-	switch (mode) {
-	case XFER_PIO_4:
-	case XFER_PIO_3:
-	case XFER_PIO_2:
-	case XFER_PIO_1:
-	case XFER_PIO_0:
-		sc1200_tunepio(drive, mode - XFER_PIO_0);
-		return 0;
-	}
-
 	pci_clock = sc1200_get_pci_clock();
 
 	/*
drivers/ide/pci/scc_pata.c +0 -7
Index: b/drivers/ide/pci/scc_pata.c
===================================================================
@@ -270,13 +270,6 @@ static int scc_tune_chipset(ide_drive_t 
 	case XFER_UDMA_0:
 		idx = speed - XFER_UDMA_0;
 		break;
-	case XFER_PIO_4:
-	case XFER_PIO_3:
-	case XFER_PIO_2:
-	case XFER_PIO_1:
-	case XFER_PIO_0:
-		scc_tune_pio(drive, speed - XFER_PIO_0);
-		return ide_config_drive_speed(drive, speed);
 	default:
 		return 1;
 	}
drivers/ide/pci/serverworks.c +0 -5
Index: b/drivers/ide/pci/serverworks.c
===================================================================
@@ -157,11 +157,6 @@ static int svwks_tune_chipset(ide_drive_
 
 	u8 ultra_enable	 = 0, ultra_timing = 0, dma_timing = 0;
 
-	if (speed >= XFER_PIO_0 && speed <= XFER_PIO_4) {
-		svwks_tune_pio(drive, speed - XFER_PIO_0);
-		return ide_config_drive_speed(drive, speed);
-	}
-
 	/* If we are about to put a disk into UDMA mode we screwed up.
 	   Our code assumes we never _ever_ do this on an OSB4 */
drivers/ide/pci/siimage.c +0 -7
Index: b/drivers/ide/pci/siimage.c
===================================================================
@@ -284,13 +284,6 @@ static int siimage_tune_chipset(ide_driv
 	scsc = is_sata(hwif) ? 1 : scsc;
 
 	switch(speed) {
-		case XFER_PIO_4:
-		case XFER_PIO_3:
-		case XFER_PIO_2:
-		case XFER_PIO_1:
-		case XFER_PIO_0:
-			sil_tune_pio(drive, speed - XFER_PIO_0);
-			return ide_config_drive_speed(drive, speed);
 		case XFER_MW_DMA_2:
 		case XFER_MW_DMA_1:
 		case XFER_MW_DMA_0:
drivers/ide/pci/sis5513.c +2 -10
Index: b/drivers/ide/pci/sis5513.c
===================================================================
@@ -519,15 +519,10 @@ static void config_art_rwp_pio (ide_driv
 	}
 }
 
-static int sis5513_tune_drive(ide_drive_t *drive, const u8 pio)
-{
-	config_art_rwp_pio(drive, pio);
-	return ide_config_drive_speed(drive, XFER_PIO_0 + pio);
-}
-
 static void sis_set_pio_mode(ide_drive_t *drive, const u8 pio)
 {
-	(void)sis5513_tune_drive(drive, pio);
+	config_art_rwp_pio(drive, pio);
+	(void)ide_config_drive_speed(drive, XFER_PIO_0 + pio);
 }
 
 static int sis5513_tune_chipset(ide_drive_t *drive, const u8 speed)

@@ -537,9 +532,6 @@ static int sis5513_tune_chipset(ide_driv
 	u32 regdw;
 	u8 drive_pci, reg;
 
-	if (speed >= XFER_PIO_0 && speed <= XFER_PIO_4)
-		return sis5513_tune_drive(drive, speed - XFER_PIO_0);
-
 	/* See config_art_rwp_pio for drive pci config registers */
 	drive_pci = 0x40;
 	if (chipset_family >= ATA_133) {
drivers/ide/pci/sl82c105.c +0 -8
Index: b/drivers/ide/pci/sl82c105.c
===================================================================
@@ -139,14 +139,6 @@ static int sl82c105_tune_chipset(ide_dri
 			pci_write_config_word(dev, reg, drv_ctrl);
 		}
 		break;
-	case XFER_PIO_5:
-	case XFER_PIO_4:
-	case XFER_PIO_3:
-	case XFER_PIO_2:
-	case XFER_PIO_1:
-	case XFER_PIO_0:
-		sl82c105_tune_pio(drive, speed - XFER_PIO_0);
-		break;
 	default:
 		return -1;
 	}
drivers/ide/pci/slc90e66.c +0 -5
Index: b/drivers/ide/pci/slc90e66.c
===================================================================
@@ -110,11 +110,6 @@ static int slc90e66_tune_chipset(ide_dri
 	int u_speed = 0, u_flag = 1 << drive->dn;
 	u16			reg4042, reg44, reg48, reg4a;
 
-	if (speed >= XFER_PIO_0 && speed <= XFER_PIO_4) {
-		slc90e66_tune_pio(drive, speed - XFER_PIO_0);
-		return ide_config_drive_speed(drive, speed);
-	}
-
 	pci_read_config_word(dev, maslave, &reg4042);
 	sitre = (reg4042 & 0x4000) ? 1 : 0;
 	pci_read_config_word(dev, 0x44, &reg44);
drivers/ide/ppc/pmac.c +0 -7
Index: b/drivers/ide/ppc/pmac.c
===================================================================
@@ -957,13 +957,6 @@ static int pmac_ide_tune_chipset(ide_dri
 		case XFER_SW_DMA_0:
 			return 1;
 #endif /* CONFIG_BLK_DEV_IDEDMA_PMAC */
-		case XFER_PIO_4:
-		case XFER_PIO_3:
-		case XFER_PIO_2:
-		case XFER_PIO_1:
-		case XFER_PIO_0:
-			pmac_ide_set_pio_mode(drive, speed & 0x07);
-			return 0;
 		default:
 			ret = 1;
 	}
#1 lore raw
Quoted 5 lines · Bartlomiej Zolnierkiewicz

Use ->set_pio_mode method to program PIO modes in ide_set_xfer_rate() (the only place which used ->speedproc to program PIO modes) and remove handling of PIO modes from all ->speedproc implementations.

There should be no functionality changes caused by this patch.

Does a missing printk(KERN_ERR, ...) in the cs5520.c count as a
functionality change? ;-)

Quoted 17 lines
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---
 drivers/ide/cris/ide-cris.c   |    5 -----
 drivers/ide/pci/alim15x3.c    |    5 -----
 drivers/ide/pci/atiixp.c      |    5 -----
 drivers/ide/pci/cmd64x.c      |    8 --------
 drivers/ide/pci/cs5530.c      |    7 -------
 drivers/ide/pci/it8213.c      |    5 -----
 drivers/ide/pci/it821x.c      |    9 ---------
 drivers/ide/pci/piix.c        |    5 -----
 drivers/ide/pci/sc1200.c      |   10 ----------
 drivers/ide/pci/scc_pata.c    |    7 -------
 drivers/ide/pci/serverworks.c |    5 -----
 drivers/ide/pci/siimage.c     |    7 -------
 drivers/ide/pci/sis5513.c     |   12 ++----------
 drivers/ide/pci/sl82c105.c    |    8 --------
 drivers/ide/pci/slc90e66.c    |    5 -----

These don't need their *_tune_pio() as a separate item anymore, however it
should be OK as gcc will inline them...

Quoted 5 lines

Index: b/drivers/ide/ide-lib.c
===================================================================

drivers/ide/ide-lib.c int ide_set_xfer_rate(ide_drive_t *drive · line 406

Quoted 9 lines
drivers/ide/ide-lib.c +6 -0
@@ -398,6 +398,18 @@ int ide_set_xfer_rate(ide_drive_t *drive
 
 	rate = ide_rate_filter(drive, rate);
 
+	if (rate >= XFER_PIO_0 && rate <= XFER_PIO_5) {
+		if (hwif->set_pio_mode)
+			hwif->set_pio_mode(drive, rate - XFER_PIO_0);
+
+		/*
+		 * FIXME: this incorrect to return zero here but

Missing "is" before "this"...

drivers/ide/ide-lib.c int ide_set_xfer_rate(ide_drive_t *drive · line 414

Quoted 8 lines
drivers/ide/ide-lib.c +6 -0
+		 * since all users of ide_set_xfer_rate() ignore
+		 * the return value it is not a problem currently
+		 */
+		return 0;
+	}
+
 	return hwif->speedproc(drive, rate);
 }

Erm, what if there's no DMA mode support an therefore no speedproc()
method, only set_pio_mode()? I guess that moving the (speedproc() == NULL) check in a prior patch was somewhat rash...

Quoted 6 lines

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

drivers/ide/pci/cs5520.c +0 -0
@@ -66,30 +66,13 @@ static struct pio_clocks cs5520_pio_cloc
 	{1, 2, 1}

drivers/ide/pci/cs5520.c static struct pio_clocks cs5520_pio_cloc · line 73

Quoted 10 lines
drivers/ide/pci/cs5520.c +1 -3
 };
 
-static int cs5520_tune_chipset(ide_drive_t *drive, const u8 speed)
+static void cs5520_set_pio_mode(ide_drive_t *drive, const u8 pio)
 {
 	ide_hwif_t *hwif = HWIF(drive);
 	struct pci_dev *pdev = hwif->pci_dev;
-	int pio = speed;
-	u8 reg;
 	int controller = drive->dn > 1 ? 1 : 0;

Wouldn't hwif->channel be enough?

Quoted 9 lines
drivers/ide/pci/cs5520.c +1 -7
+	u8 reg;
 
-	switch(speed)
-	{
-		case XFER_PIO_4:
-		case XFER_PIO_3:
-		case XFER_PIO_2:
-		case XFER_PIO_1:
-		case XFER_PIO_0:

drivers/ide/pci/cs5520.c static struct pio_clocks cs5520_pio_cloc · line 76

Quoted 10 lines
drivers/ide/pci/cs5520.c +0 -9
-			pio -= XFER_PIO_0;
-			break;
-		default:
-			pio = 0;
-			printk(KERN_ERR "cs55x0: bad ide timing.\n");
-	}
-	
-	printk("PIO clocking = %d\n", pio);
-	
 	/* FIXME: if DMA = 1 do we need to set the DMA bit here ? */

Indeed? Guess not. :-)

drivers/ide/pci/cs5520.c static int cs5520_tune_chipset(ide_drive · line 98

Quoted 4 lines
drivers/ide/pci/cs5520.c +0 -0
 	/* 8bit CAT/CRT - 8bit command timing for channel */

@@ -114,12 +97,21 @@ static int cs5520_tune_chipset(ide_drive
 	reg |= 1<<((drive->dn&1)+5);
 	outb(reg, hwif->dma_base + 0x02 + 8*controller);

That should go...

drivers/ide/pci/cs5520.c static int cs5520_tune_chipset(ide_drive · line 107

Quoted 11 lines
drivers/ide/pci/cs5520.c +5 -3
-	return ide_config_drive_speed(drive, speed);
+	(void)ide_config_drive_speed(drive, XFER_PIO_0 + pio);
 }
 
-static void cs5520_set_pio_mode(ide_drive_t *drive, const u8 pio)
+static int cs5520_tune_chipset(ide_drive_t *drive, const u8 speed)
 {
-	cs5520_tune_chipset(drive, XFER_PIO_0 + pio);
+	printk(KERN_ERR "cs55x0: bad ide timing.\n");
+
+	cs5520_set_pio_mode(drive, 0);

Huh?

drivers/ide/pci/cs5520.c static int cs5520_tune_chipset(ide_drive · line 109

Quoted 3 lines
drivers/ide/pci/cs5520.c +3 -0
+
+	/*
+	 * FIXME: this incorrect to return zero here but

Again, no "is" before "this"...

drivers/ide/pci/cs5520.c static int cs5520_tune_chipset(ide_drive · line 114

Quoted 5 lines
drivers/ide/pci/cs5520.c +4 -0
+	 * since all users of ide_set_xfer_rate() ignore
+	 * the return value it is not a problem currently
+	 */
+	return 0;
 }

The question is why we have to leave the speedproc() handler to be in this
driver that doesn't support DMA modes per se?

MBR, Sergei

author
#2 lore raw

Hi,

Quoted 10 lines · Sergei Shtylyov
Quoted 5 lines · Bartlomiej Zolnierkiewicz

Use ->set_pio_mode method to program PIO modes in ide_set_xfer_rate() (the only place which used ->speedproc to program PIO modes) and remove handling of PIO modes from all ->speedproc implementations.

There should be no functionality changes caused by this patch.

Does a missing printk(KERN_ERR, ...) in the cs5520.c count as a
functionality change? ;-)

Psst. ;-)

Quoted 18 lines
Quoted 17 lines
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
---
 drivers/ide/cris/ide-cris.c   |    5 -----
 drivers/ide/pci/alim15x3.c    |    5 -----
 drivers/ide/pci/atiixp.c      |    5 -----
 drivers/ide/pci/cmd64x.c      |    8 --------
 drivers/ide/pci/cs5530.c      |    7 -------
 drivers/ide/pci/it8213.c      |    5 -----
 drivers/ide/pci/it821x.c      |    9 ---------
 drivers/ide/pci/piix.c        |    5 -----
 drivers/ide/pci/sc1200.c      |   10 ----------
 drivers/ide/pci/scc_pata.c    |    7 -------
 drivers/ide/pci/serverworks.c |    5 -----
 drivers/ide/pci/siimage.c     |    7 -------
 drivers/ide/pci/sis5513.c     |   12 ++----------
 drivers/ide/pci/sl82c105.c    |    8 --------
 drivers/ide/pci/slc90e66.c    |    5 -----
Quoted 2 lines

These don't need their *_tune_pio() as a separate item anymore, however it
should be OK as gcc will inline them...

Removal of redundant *_tune_pio() is WIP.

Quoted 8 lines
Quoted 8 lines

Index: b/drivers/ide/ide-lib.c
===================================================================

drivers/ide/ide-lib.c +0 -0
@@ -398,6 +398,18 @@ int ide_set_xfer_rate(ide_drive_t *drive
 
 	rate = ide_rate_filter(drive, rate);
Quoted 8 lines

drivers/ide/ide-lib.c int ide_set_xfer_rate(ide_drive_t *drive · line 406

Quoted 6 lines
drivers/ide/ide-lib.c +6 -0
+	if (rate >= XFER_PIO_0 && rate <= XFER_PIO_5) {
+		if (hwif->set_pio_mode)
+			hwif->set_pio_mode(drive, rate - XFER_PIO_0);
+
+		/*
+		 * FIXME: this incorrect to return zero here but

Missing "is" before "this"...

Fixed, thanks.

Quoted 4 lines
Quoted 4 lines
drivers/ide/ide-lib.c +4 -0
+		 * since all users of ide_set_xfer_rate() ignore
+		 * the return value it is not a problem currently
+		 */
+		return 0;
Quoted 8 lines

drivers/ide/ide-lib.c int ide_set_xfer_rate(ide_drive_t *drive · line 414

Quoted 4 lines
drivers/ide/ide-lib.c +2 -0
+	}
+
 	return hwif->speedproc(drive, rate);
 }

Erm, what if there's no DMA mode support an therefore no speedproc()
method, only set_pio_mode()? I guess that moving the (speedproc() == NULL) check in a prior patch was somewhat rash...

Quite the contrary, it was intended... :)

Few old host drivers have only ->set_pio_mode and they don't set ->autotune so not checking for ->speedproc would be a major change in behavior for them while this patch was meant to be as non-intrusive and simple as possible.

Quoted 8 lines
Quoted 8 lines

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

drivers/ide/pci/cs5520.c +0 -0
@@ -66,30 +66,13 @@ static struct pio_clocks cs5520_pio_cloc
 	{1, 2, 1}
 };
Quoted 10 lines

drivers/ide/pci/cs5520.c static struct pio_clocks cs5520_pio_cloc · line 73

Quoted 8 lines
drivers/ide/pci/cs5520.c +1 -3
-static int cs5520_tune_chipset(ide_drive_t *drive, const u8 speed)
+static void cs5520_set_pio_mode(ide_drive_t *drive, const u8 pio)
 {
 	ide_hwif_t *hwif = HWIF(drive);
 	struct pci_dev *pdev = hwif->pci_dev;
-	int pio = speed;
-	u8 reg;
 	int controller = drive->dn > 1 ? 1 : 0;

    Wouldn't hwif->channel be enough?

Agreed.

Quoted 22 lines
Quoted 9 lines
drivers/ide/pci/cs5520.c +1 -7
+	u8 reg;
 
-	switch(speed)
-	{
-		case XFER_PIO_4:
-		case XFER_PIO_3:
-		case XFER_PIO_2:
-		case XFER_PIO_1:
-		case XFER_PIO_0:

drivers/ide/pci/cs5520.c static struct pio_clocks cs5520_pio_cloc · line 76

Quoted 10 lines
drivers/ide/pci/cs5520.c +0 -9
-			pio -= XFER_PIO_0;
-			break;
-		default:
-			pio = 0;
-			printk(KERN_ERR "cs55x0: bad ide timing.\n");
-	}
-	
-	printk("PIO clocking = %d\n", pio);
-	
 	/* FIXME: if DMA = 1 do we need to set the DMA bit here ? */

Indeed? Guess not. :-)

Quoted 6 lines

drivers/ide/pci/cs5520.c static int cs5520_tune_chipset(ide_drive · line 98

Quoted 4 lines
drivers/ide/pci/cs5520.c +0 -0
 	/* 8bit CAT/CRT - 8bit command timing for channel */

@@ -114,12 +97,21 @@ static int cs5520_tune_chipset(ide_drive
 	reg |= 1<<((drive->dn&1)+5);
 	outb(reg, hwif->dma_base + 0x02 + 8*controller);

That should go...

-ENOPATCH :)

Quoted 4 lines
Quoted 4 lines
drivers/ide/pci/cs5520.c +1 -1
-	return ide_config_drive_speed(drive, speed);
+	(void)ide_config_drive_speed(drive, XFER_PIO_0 + pio);
 }
Quoted 9 lines

drivers/ide/pci/cs5520.c static int cs5520_tune_chipset(ide_drive · line 107

Quoted 7 lines
drivers/ide/pci/cs5520.c +4 -2
-static void cs5520_set_pio_mode(ide_drive_t *drive, const u8 pio)
+static int cs5520_tune_chipset(ide_drive_t *drive, const u8 speed)
 {
-	cs5520_tune_chipset(drive, XFER_PIO_0 + pio);
+	printk(KERN_ERR "cs55x0: bad ide timing.\n");
+
+	cs5520_set_pio_mode(drive, 0);

Huh?

Yes, this needs fixing but not in this patch.

"There should be no functionality changes caused by this patch."

Quoted 5 lines

drivers/ide/pci/cs5520.c static int cs5520_tune_chipset(ide_drive · line 109

Quoted 3 lines
drivers/ide/pci/cs5520.c +3 -0
+
+	/*
+	 * FIXME: this incorrect to return zero here but

    Again, no "is" before "this"...

Fixed.

Quoted 8 lines

drivers/ide/pci/cs5520.c static int cs5520_tune_chipset(ide_drive · line 114

Quoted 5 lines
drivers/ide/pci/cs5520.c +4 -0
+	 * since all users of ide_set_xfer_rate() ignore
+	 * the return value it is not a problem currently
+	 */
+	return 0;
 }

The question is why we have to leave the speedproc() handler to be in this
driver that doesn't support DMA modes per se?

"There should be no functionality changes caused by this patch."

However prior or incremental patches are of course welcomed.

Thanks,
Bart

#3 lore raw
In reply to Bartlomiej Zolnierkiewicz

Hello.

Quoted 29 lines · Bartlomiej Zolnierkiewicz

Use ->set_pio_mode method to program PIO modes in ide_set_xfer_rate() (the only place which used ->speedproc to program PIO modes) and remove handling of PIO modes from all ->speedproc implementations.

There should be no functionality changes caused by this patch.

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

Index: b/drivers/ide/cris/ide-cris.c
===================================================================

drivers/ide/cris/ide-cris.c +0 -5
@@ -724,11 +724,6 @@ static int speed_cris_ide(ide_drive_t *d
 {
 	int cyc = 0, dvs = 0, strobe = 0, hold = 0;
 
-	if (speed >= XFER_PIO_0 && speed <= XFER_PIO_4) {
-		cris_set_pio_mode(drive, speed - XFER_PIO_0);
-		return ide_config_drive_speed(drive, speed);
-	}
-
 	switch(speed)
 	{
 		case XFER_UDMA_0:

Index: b/drivers/ide/ide-lib.c
===================================================================

drivers/ide/ide-lib.c int ide_set_xfer_rate(ide_drive_t *drive · line 403

Quoted 6 lines
drivers/ide/ide-lib.c +3 -0
@@ -398,6 +398,18 @@ int ide_set_xfer_rate(ide_drive_t *drive
 
 	rate = ide_rate_filter(drive, rate);
 
+	if (rate >= XFER_PIO_0 && rate <= XFER_PIO_5) {
+		if (hwif->set_pio_mode)
+			hwif->set_pio_mode(drive, rate - XFER_PIO_0);

BTW, why doesn it pass CF-specific PIO6? Not an issue now, but what the
hell? I'd suggest instead:

	if (rate & XFER_PIO_0) {

MBR, Sergei

author
#4 lore raw
Quoted 36 lines · Sergei Shtylyov

Hello.

Quoted 32 lines · Bartlomiej Zolnierkiewicz

Use ->set_pio_mode method to program PIO modes in ide_set_xfer_rate() (the only place which used ->speedproc to program PIO modes) and remove handling of PIO modes from all ->speedproc implementations.

There should be no functionality changes caused by this patch.

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

Index: b/drivers/ide/cris/ide-cris.c
===================================================================

drivers/ide/cris/ide-cris.c +0 -5
@@ -724,11 +724,6 @@ static int speed_cris_ide(ide_drive_t *d
 {
 	int cyc = 0, dvs = 0, strobe = 0, hold = 0;
 
-	if (speed >= XFER_PIO_0 && speed <= XFER_PIO_4) {
-		cris_set_pio_mode(drive, speed - XFER_PIO_0);
-		return ide_config_drive_speed(drive, speed);
-	}
-
 	switch(speed)
 	{
 		case XFER_UDMA_0:

Index: b/drivers/ide/ide-lib.c
===================================================================

drivers/ide/ide-lib.c +0 -0
@@ -398,6 +398,18 @@ int ide_set_xfer_rate(ide_drive_t *drive
 
 	rate = ide_rate_filter(drive, rate);
Quoted 8 lines

drivers/ide/ide-lib.c int ide_set_xfer_rate(ide_drive_t *drive · line 403

Quoted 3 lines
drivers/ide/ide-lib.c +3 -0
+	if (rate >= XFER_PIO_0 && rate <= XFER_PIO_5) {
+		if (hwif->set_pio_mode)
+			hwif->set_pio_mode(drive, rate - XFER_PIO_0);

BTW, why doesn it pass CF-specific PIO6? Not an issue now, but what the
hell? I'd suggest instead:

	if (rate & XFER_PIO_0) {

Like you've said not an issue now and moreover the current code serves as a documentation that PIO6 is not supported. ;)

Thanks,
Bart