ide-pmac: fix drive->init_speed reporting

pmac_ide_tune_chipset() don't set drive->init_speed.

Fix it by setting drive->{current,init}_speed in pmac_ide_do_setfeature() and clean up pmac_ide_{tune_chipset,mdma_enable,udma_enable}().

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

---
Goes before "[PATCH] ide-pmac: PIO mode setup fixes (take 2)" patch in the IDE quilt tree.

 drivers/ide/ppc/pmac.c |   14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

Index: b/drivers/ide/ppc/pmac.c
===================================================================

drivers/ide/ppc/pmac.c +3 -11

@@ -602,6 +602,9 @@ out:
 				drive->id->dma_1word |= 0x0101; break;
 			default: break;
 		}
+		if (!drive->init_speed)
+			drive->init_speed = command;
+		drive->current_speed = command;
 	}
 	enable_irq(hwif->irq);
 	return result;

@@ -974,7 +977,6 @@ static int pmac_ide_tune_chipset(ide_dri
 		return ret;
 
 	pmac_ide_do_update_timings(drive);	
-	drive->current_speed = speed;
 
 	return 0;
 }

@@ -1725,11 +1727,6 @@ pmac_ide_mdma_enable(ide_drive_t *drive,
 	/* Apply timings to controller */
 	*timings = timing_local[0];
 	*timings2 = timing_local[1];
-	
-	/* Set speed info in drive */
-	drive->current_speed = mode;	
-	if (!drive->init_speed)
-		drive->init_speed = mode;
 
 	return 1;
 }

@@ -1781,11 +1778,6 @@ pmac_ide_udma_enable(ide_drive_t *drive,
 	*timings = timing_local[0];
 	*timings2 = timing_local[1];
 
-	/* Set speed info in drive */
-	drive->current_speed = mode;	
-	if (!drive->init_speed)
-		drive->init_speed = mode;
-
 	return 1;
 }
 

Quoted 5 lines · Bartlomiej Zolnierkiewicz

pmac_ide_tune_chipset() don't set drive->init_speed.

Fix it by setting drive->{current,init}_speed in pmac_ide_do_setfeature() and clean up pmac_ide_{tune_chipset,mdma_enable,udma_enable}().

Acked-by Benjamin Herrenschmidt <[email protected]>

Note that with all your patches applied, it doesn't seem to auto-tune the speed at boot anymore and doesn't enable DMA. I can make it do so with hdparm -d1, in which case, for example, on this wallstreet, I get MDMA2 which is correct, however, it seems to also set PIO0 which it should set PIO4...

Quoted 5 lines · Benjamin Herrenschmidt

Note that with all your patches applied, it doesn't seem to auto-tune the speed at boot anymore and doesn't enable DMA. I can make it do so with hdparm -d1, in which case, for example, on this wallstreet, I get MDMA2 which is correct, however, it seems to also set PIO0 which it should set PIO4...

One of the problems is that you do XFER_PIO + pio in pmac_ide_set_pio_mode(), which is no good. XFER_PIO is a bad constant name and causes that sort of confusion :-)

Fix is to use XFER_PIO_0 + pio. I'll send a patch fixing that plus a few other things on top of yours once I've found out what's up with DMA. I've enable autotune, I see it sending the 0x22 command, but hdparm still claims DMA isn't enabled.

Ok, there's a combination of things here:

- First, doing a set_pio from userland (hdparm -p XX) causes the kernel
to disable DMA, which I think is incorrect. It's not the case with 2.6.22 from my quick tests. The problem is that ide_config_drive_speed disables DMA, but only re-enables it when setting a DMA speed. I think the whole idea of having a "current speed" is bogus here, we should have a separate current DMA speed and current PIO speed. We should be able to set the PIO timings without stopping DMA, toggling DMA is a separate affair.

- For some reason, nowadays, hdparm -p /dev/hda will not autotune, but
will set PIO 0 (hdparm -p 255 /dev/hda will autotune). Might be a bug in whatever hdparm version I have here.

- Some userland scripts installed on debian as part of the pbbuttonsd
package are doing hdaprm -p /dev/device on all IDE devices at boot.

In the meantime, that patch applied on top of your latest series fixes the PIO setting in the driver to send the correct mode value in pmac_ide_set_pio_mode(). I still object to your patch serie at this point because hdparm -p N should not, imho, cause DMA to be disabled.

I really believe that the kernel should keep track separately of PIO and DMA speeds.

Index: linux-work/drivers/ide/ppc/pmac.c
===================================================================

drivers/ide/ppc/pmac.c +0 -0

@@ -535,7 +535,7 @@ pmac_outbsync(ide_drive_t *drive, u8 val

drivers/ide/ppc/pmac.c +1 -0

 	hwif->pio_mask = ATA_PIO4;
 	hwif->set_pio_mode = pmac_ide_set_pio_mode;
 	if (pmif->kind == controller_un_ata6

@@ -1766,10 +1770,16 @@ pmac_ide_dma_test_irq (ide_drive_t *driv
 
 static void pmac_ide_dma_host_off(ide_drive_t *drive)
 {
+#ifdef IDE_PMAC_DEBUG

Gah! Why the debug messages might be using KERN_ERR?

Quoted 7 lines

Ok, there's a combination of things here:

- First, doing a set_pio from userland (hdparm -p XX) causes the kernel
to disable DMA, which I think is incorrect. It's not the case with 2.6.22 from my quick tests. The problem is that ide_config_drive_speed disables DMA, but only re-enables it when setting a DMA speed. I think

Quoted 8 lines

the whole idea of having a "current speed" is bogus here, we should have a separate current DMA speed and current PIO speed. We should be able to set the PIO timings without stopping DMA, toggling DMA is a separate affair.

- For some reason, nowadays, hdparm -p /dev/hda will not autotune, but
will set PIO 0 (hdparm -p 255 /dev/hda will autotune). Might be a bug in whatever hdparm version I have here.

Quoted 2 lines

- Some userland scripts installed on debian as part of the pbbuttonsd
package are doing hdaprm -p /dev/device on all IDE devices at boot.

In the meantime, that patch applied on top of your latest series fixes the PIO setting in the driver to send the correct mode value in

pmac_ide_set_pio_mode(). I still object to your patch serie at this point because hdparm -p N should not, imho, cause DMA to be disabled.

I really believe that the kernel should keep track separately of PIO and DMA speeds.

Index: linux-work/drivers/ide/ppc/pmac.c
===================================================================

drivers/ide/ppc/pmac.c +19 -9

@@ -535,7 +535,7 @@ pmac_outbsync(ide_drive_t *drive, u8 val
 static void
 pmac_ide_set_pio_mode(ide_drive_t *drive, const u8 pio)
 {
-	u32 *timings;
+	u32 *timings, t;
 	unsigned accessTicks, recTicks;
 	unsigned accessTime, recTime;
 	pmac_ide_hwif_t* pmif = (pmac_ide_hwif_t *)HWIF(drive)->hwif_data;

@@ -546,6 +546,7 @@ pmac_ide_set_pio_mode(ide_drive_t *drive
 
 	/* which drive is it ? */
 	timings = &pmif->timings[drive->select.b.unit & 0x01];
+	t = *timings;
 
 	cycle_time = ide_pio_cycle_time(drive, pio);
 

@@ -553,14 +554,14 @@ pmac_ide_set_pio_mode(ide_drive_t *drive
 	case controller_sh_ata6: {
 		/* 133Mhz cell */
 		u32 tr = kauai_lookup_timing(shasta_pio_timings, cycle_time);
-		*timings = ((*timings) & ~TR_133_PIOREG_PIO_MASK) | tr;
+		t = (t & ~TR_133_PIOREG_PIO_MASK) | tr;
 		break;
 		}
 	case controller_un_ata6:
 	case controller_k2_ata6: {
 		/* 100Mhz cell */
 		u32 tr = kauai_lookup_timing(kauai_pio_timings, cycle_time);
-		*timings = ((*timings) & ~TR_100_PIOREG_PIO_MASK) | tr;
+		t = (t & ~TR_100_PIOREG_PIO_MASK) | tr;
 		break;
 		}
 	case controller_kl_ata4:

@@ -574,9 +575,9 @@ pmac_ide_set_pio_mode(ide_drive_t *drive
 		accessTicks = min(accessTicks, 0x1fU);
 		recTicks = SYSCLK_TICKS_66(recTime);
 		recTicks = min(recTicks, 0x1fU);
-		*timings = ((*timings) & ~TR_66_PIO_MASK) |
-				(accessTicks << TR_66_PIO_ACCESS_SHIFT) |
-				(recTicks << TR_66_PIO_RECOVERY_SHIFT);
+		t = (t & ~TR_66_PIO_MASK) |
+			(accessTicks << TR_66_PIO_ACCESS_SHIFT) |
+			(recTicks << TR_66_PIO_RECOVERY_SHIFT);
 		break;
 	default: {
 		/* 33Mhz cell */

@@ -596,11 +597,11 @@ pmac_ide_set_pio_mode(ide_drive_t *drive
 			recTicks--; /* guess, but it's only for PIO0, so... */
 			ebit = 1;
 		}
-		*timings = ((*timings) & ~TR_33_PIO_MASK) |
+		t = (t & ~TR_33_PIO_MASK) |
 				(accessTicks << TR_33_PIO_ACCESS_SHIFT) |
 				(recTicks << TR_33_PIO_RECOVERY_SHIFT);
 		if (ebit)
-			*timings |= TR_33_PIO_E;
+			t |= TR_33_PIO_E;
 		break;
 		}
 	}

@@ -610,9 +611,10 @@ pmac_ide_set_pio_mode(ide_drive_t *drive
 		drive->name, pio,  *timings);
 #endif	
 
-	if (ide_config_drive_speed(drive, XFER_PIO + pio))
+	if (ide_config_drive_speed(drive, XFER_PIO_0 + pio))
 		return;
 
+	*timings = t;
 	pmac_ide_do_update_timings(drive);
 }
 

@@ -1150,6 +1152,8 @@ pmac_ide_setup_device(pmac_ide_hwif_t *p
 	hwif->cbl = pmif->cable_80 ? ATA_CBL_PATA80 : ATA_CBL_PATA40;
 	hwif->drives[0].unmask = 1;
 	hwif->drives[1].unmask = 1;
+	hwif->drives[0].autotune = IDE_TUNE_AUTO;
+	hwif->drives[1].autotune = IDE_TUNE_AUTO;
 	hwif->pio_mask = ATA_PIO4;
 	hwif->set_pio_mode = pmac_ide_set_pio_mode;
 	if (pmif->kind == controller_un_ata6

@@ -1766,10 +1770,16 @@ pmac_ide_dma_test_irq (ide_drive_t *driv
 
 static void pmac_ide_dma_host_off(ide_drive_t *drive)
 {
+#ifdef IDE_PMAC_DEBUG
+	printk(KERN_ERR "%s: dma_host_off()\n", drive->name);
+#endif
 }
 
 static void pmac_ide_dma_host_on(ide_drive_t *drive)
 {
+#ifdef IDE_PMAC_DEBUG
+	printk(KERN_ERR "%s: dma_host_on()\n", drive->name);
+#endif
 }
 
 static void

Hi,

Thanks for reviewing and testing this patch series.

Quoted 3 lines · Benjamin Herrenschmidt

Quoted 3 lines

Ok, there's a combination of things here:

Quoted 4 lines

Quoted 4 lines

- First, doing a set_pio from userland (hdparm -p XX) causes the kernel
to disable DMA, which I think is incorrect. It's not the case with 2.6.22 from my quick tests. The problem is that ide_config_drive_speed disables DMA, but only re-enables it when setting a DMA speed. I think

The problem is that _many_ chipsets don't support separate PIO and DMA timings so disabling DMA while programming chipset for PIO timings is a necessity for them.

Quoted 3 lines

Quoted 3 lines

- For some reason, nowadays, hdparm -p /dev/hda will not autotune, but
will set PIO 0 (hdparm -p 255 /dev/hda will autotune). Might be a bug in whatever hdparm version I have here.

Since -p 255 works fine it would indicate a hdparm issue.

Quoted 5 lines

Quoted 2 lines

- Some userland scripts installed on debian as part of the pbbuttonsd
package are doing hdaprm -p /dev/device on all IDE devices at boot.

In the meantime, that patch applied on top of your latest series fixes the PIO setting in the driver to send the correct mode value in

Looks fine.

Could you please rebase it on top of the fixed "ide-pmac: PIO mode setup fixes" patch, remove debugging printks and add description/Signed-off-by?

Also since the patch series has been tested please verify that your concerns wrt patches #4, #8 and #10 are still valid.

Quoted 2 lines

pmac_ide_set_pio_mode(). I still object to your patch serie at this point because hdparm -p N should not, imho, cause DMA to be disabled.

If this change indeed causes some serious problem which breaks userland on ppc (that can't be workarounded otherwise) we may add a new ->host_flags flag and special hack to ide-io.c::do_special(), something like:

...

		int keep_dma = drive->using_dma;
...
			ide_set_pio(drive, req_pio);
...
		if (hwif->host_flags & IDE_HFLAG_SET_PIO_MODE_KEEP_DMA) {
			if (keep_dma)
				hwif->ide_dma_on(drive);
		}

...

which should preserve the old behavior.

I will respin patch #11 with necessary changes if needed.

Quoted 2 lines

I really believe that the kernel should keep track separately of PIO and DMA speeds.

Agreed.

Quoted 6 lines

Quoted 6 lines

Ok, there's a combination of things here:

- First, doing a set_pio from userland (hdparm -p XX) causes the kernel to disable DMA, which I think is incorrect. It's not the case with 2.6.22 from my quick tests. The problem is that ide_config_drive_speed disables DMA, but only re-enables it when setting a DMA speed. I think

The problem is that _many_ chipsets don't support separate PIO and DMA timings so disabling DMA while programming chipset for PIO timings is a necessity for them.

Quoted 10 lines · Bartlomiej Zolnierkiewicz

Quoted 6 lines

Quoted 6 lines

Ok, there's a combination of things here:

- First, doing a set_pio from userland (hdparm -p XX) causes the kernel to disable DMA, which I think is incorrect. It's not the case with 2.6.22 from my quick tests. The problem is that ide_config_drive_speed disables DMA, but only re-enables it when setting a DMA speed. I think

The problem is that _many_ chipsets don't support separate PIO and DMA timings so disabling DMA while programming chipset for PIO timings is a necessity for them.

Actually, I didn't quite follow (as I'm afraid Ben also :-). Do you mean
the fact that the DMA timings may get overwritten (which happens due to the failure of the drivers to handle the shared PIO/DMA timing registers, and in some cases by "coupling" PIO to UltraDMA timings for no apparent reasons which leads to disabling UltraDMA when PIO is being programmed)?

Quoted 9 lines

Quoted 5 lines

Note that with all your patches applied, it doesn't seem to auto-tune the speed at boot anymore and doesn't enable DMA. I can make it do so with hdparm -d1, in which case, for example, on this wallstreet, I get MDMA2 which is correct, however, it seems to also set PIO0 which it should set PIO4...

One of the problems is that you do XFER_PIO + pio in pmac_ide_set_pio_mode(), which is no good. XFER_PIO is a bad constant name and causes that sort of confusion :-)

Quoted 1 line

Fix is to use XFER_PIO_0 + pio. I'll send a patch fixing that plus a few

Quoted 3 lines

other things on top of yours once I've found out what's up with DMA. I've enable autotune, I see it sending the 0x22 command, but hdparm still claims DMA isn't enabled.

Ok, there's a combination of things here:

- First, doing a set_pio from userland (hdparm -p XX) causes the kernel
to disable DMA, which I think is incorrect.

It's not the case with 2.6.22 from my quick tests.

The problem is that ide_config_drive_speed
disables DMA, but only re-enables it when setting a DMA speed.

I think
the whole idea of having a "current speed" is bogus here, we should have a separate current DMA speed and current PIO speed. We should be able to set the PIO timings without stopping DMA, toggling DMA is a separate affair.

Quoted 4 lines

Ok, there's a combination of things here:

- First, doing a set_pio from userland (hdparm -p XX) causes the kernel
to disable DMA, which I think is incorrect.

That's the way ide_config_drive_speed() works.

Quoted 1 line

It's not the case with 2.6.22 from my quick tests.

Which means that PIO autotuning is broken there, i.e. that
ide_config_drive_speed() not called from the driver's tuneproc() method.

Quoted 2 lines

The problem is that ide_config_drive_speed
disables DMA, but only re-enables it when setting a DMA speed.

It never "re-enables" DMA. ide_dma_host_on() method is not the same as
ide_dma_on() which actually enables DMA.

Quoted 5 lines

I think
the whole idea of having a "current speed" is bogus here, we should have a separate current DMA speed and current PIO speed. We should be able to set the PIO timings without stopping DMA, toggling DMA is a separate affair.

Agreed completely.

Quoted 6 lines

Quoted 4 lines

Ok, there's a combination of things here:

- First, doing a set_pio from userland (hdparm -p XX) causes the kernel to disable DMA, which I think is incorrect.

That's the way ide_config_drive_speed() works.

And I still think that's bad.

Quoted 4 lines

Quoted 1 line

It's not the case with 2.6.22 from my quick tests.

Which means that PIO autotuning is broken there, i.e. that
ide_config_drive_speed() not called from the driver's tuneproc() method.

Yes, the driver uses it's own function which doesn't disable DMA permanently, which is, IMHO, the way to go. I consider the current behaviour a regression.

Quoted 5 lines

Quoted 2 lines

The problem is that ide_config_drive_speed
disables DMA, but only re-enables it when setting a DMA speed.

It never "re-enables" DMA. ide_dma_host_on() method is not the same as
ide_dma_on() which actually enables DMA.

Ugh ? It re-enables DMA in the sense that if called to configure a DMA speed, it re-enables dma on the host, thus effectively leaving with DMA enabled.

Quoted 1 line

Ugh ? It re-enables DMA in the sense that if called to configure a

DMA

Quoted 1 line

speed, it re-enables dma on the host, thus effectively leaving with

DMA

Quoted 1 line

enabled.

No. DMA is still diabled for the IDE core at this point. You need
a real
ide_dma_on() method call to do it.