[PATCH] ide-pmac: fix drive->init_speed reporting

[PATCH] ide-pmac: fix drive->init_speed reporting

[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}().

Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
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
===================================================================
--- a/drivers/ide/ppc/pmac.c
+++ b/drivers/ide/ppc/pmac.c
@@ -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;
 }
 

Re: [PATCH] ide-pmac: fix drive->init_speed reporting

[Benjamin Herrenschmidt]

On Sun, 2007-07-22 at 20:19 +0200, Bartlomiej Zolnierkiewicz wrote:
> 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 <benh@kernel.crashing.org>

> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> ---
> 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
> ===================================================================
> --- a/drivers/ide/ppc/pmac.c
> +++ b/drivers/ide/ppc/pmac.c
> @@ -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;
>  }
>  

Re: [PATCH] ide-pmac: fix drive->init_speed reporting

[Bartlomiej Zolnierkiewicz]

On Sunday 22 July 2007, Benjamin Herrenschmidt wrote:
> On Sun, 2007-07-22 at 20:19 +0200, Bartlomiej Zolnierkiewicz wrote:
> > 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 <benh@kernel.crashing.org>

added

ide patches

[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...

Ben.

Re: ide patches

[Benjamin Herrenschmidt]

On Mon, 2007-07-23 at 09:55 +1000, Benjamin Herrenschmidt wrote:
> 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.

Cheers,
Ben.

Re: ide patches

[Benjamin Herrenschmidt]

On Mon, 2007-07-23 at 11:21 +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2007-07-23 at 09:55 +1000, Benjamin Herrenschmidt wrote:
> > 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.

I'm investigating what's broken with hdparm

Ben.

Re: ide patches

[Benjamin Herrenschmidt]

> 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
===================================================================
--- linux-work.orig/drivers/ide/ppc/pmac.c	2007-07-23 10:21:07.000000000 +1000
+++ linux-work/drivers/ide/ppc/pmac.c	2007-07-23 11:58:50.000000000 +1000
@@ -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

Re: ide patches

[Sergei Shtylyov]

Hello.

Benjamin Herrenschmidt wrote:

> 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.

    We do too. :-)

> Index: linux-work/drivers/ide/ppc/pmac.c
> ===================================================================
> --- linux-work.orig/drivers/ide/ppc/pmac.c	2007-07-23 10:21:07.000000000 +1000
> +++ linux-work/drivers/ide/ppc/pmac.c	2007-07-23 11:58:50.000000000 +1000
> @@ -535,7 +535,7 @@ pmac_outbsync(ide_drive_t *drive, u8 val
[...]
> @@ -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;

    Hm, indeed, most of the other drivers are just using 1.

>  	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
>  }

    Gah! Why the debug messages might be using KERN_ERR?

MBR, Sergei

Re: ide patches

[Benjamin Herrenschmidt]

>     Gah! Why the debug messages might be using KERN_ERR?

Blah, other debug messages where like that and I copy/pasted stupidly. I
suppose at one point, somebody (maybe me) decided that it was a PITA to
have to add "debug" on the kernel command line :-)

I will change all of these to pr_debug anyway.

Cheers,
Ben.

Re: ide patches

[Bartlomiej Zolnierkiewicz]

Hi,

Thanks for reviewing and testing this patch series.

On Monday 23 July 2007, Benjamin Herrenschmidt wrote:
> 
> > 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.

> > 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.

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

> >  - 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.

> 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.

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

Agreed.

Thanks,
Bart

> Index: linux-work/drivers/ide/ppc/pmac.c
> ===================================================================
> --- linux-work.orig/drivers/ide/ppc/pmac.c	2007-07-23 10:21:07.000000000 +1000
> +++ linux-work/drivers/ide/ppc/pmac.c	2007-07-23 11:58:50.000000000 +1000
> @@ -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

Re: ide patches

[Benjamin Herrenschmidt]

On Mon, 2007-07-23 at 23:22 +0200, Bartlomiej Zolnierkiewicz wrote:
> Hi,
> 
> Thanks for reviewing and testing this patch series.
> 
> On Monday 23 July 2007, Benjamin Herrenschmidt wrote:
> > 
> > > 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.

Disabling while programming is fine with me. The problem is that we
don't restore DMA after programming the PIO timings.

Yes, but we still need to address it as it's a regression for ide-pmac
when moving to the generic ide_config_drive_speed.

> > >  - 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.

Yup. All hdparm's around do that, I need to find out when it actually
broke (if it did, but I -think- that early versions used to autotune
with just -p).

> > >  - 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?

Sure.

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

I've been using that weird machine without problems at boot, but I still
need to test actual hotplug of the media bay, this is usually when the
shit hits the fan. It's quite possible that some of the fixes you did to
ide_config_drive_speed() (or other changes around) are also fixing that
problem, it used to look a lot like a heisenbug back then. However, I'm
still annoyed that with all current debian or ubuntu distro, you'll end
up with DMA disabled at boot because of those pbbuttons scripts.

> > 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.

Yea, please.

> I will respin patch #11 with necessary changes if needed.
> 
> > I really believe that the kernel should keep track separately of PIO and
> > DMA speeds.
> 
> Agreed.

Excellent, we all agree :-)

Cheers,
Ben.

Re: ide patches

[Sergei Shtylyov]

Bartlomiej Zolnierkiewicz wrote:

>>>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)?

MBR, Sergei

Re: ide patches

[Bartlomiej Zolnierkiewicz]

On Sunday 29 July 2007, Sergei Shtylyov wrote:
> Bartlomiej Zolnierkiewicz wrote:
> 
> >>>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)?

Yes, in particular I meant cases like cmd64x (shared MWDMA/PIO timing
registers) so doing "hdparm -p" on a drive currently using DMA without
ide_config_drive_speed() disabling DMA would result in DMA operations
using PIO timings and possible data corruptions.

BTW all PIO to (U)DMA "couplings" should be fixed now

Bart

Re: ide patches

[Sergei Shtylyov]

Hello.

Benjamin Herrenschmidt wrote:

>>>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 :-)

    Yeah, it's #define'd as 0 in drivers/ide/ide-timings.h... :-/

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

    I think Bart will just recast his patch (as usual).

>>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.

    That's the way ide_config_drive_speed() works.

> 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.

> 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.

> 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.

MBR, Sergei

Re: ide patches

[Benjamin Herrenschmidt]

> > 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.

> > 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.

> > 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.

> > 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.

Ah good :-)

Cheers,
Ben.

Re: ide patches

[Sergei Shtylyov]

Hello.

Benjamin Herrenschmidt wrote:

>>>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.

    Bart has just named one reason for that -- many chips have shared PIO/DMA 
timing regs -- not all driver actually care about that issue and that's *not* 
the safest way to do this via ide_dma_{on|off}() calls anyway...

>>>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.

    Hm... "it's the way the cookie crumbles" for all the other drivers. :-)

>>>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.

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

MBR, Sergei

Re: ide patches

[Benjamin Herrenschmidt]

On Tue, 2007-07-24 at 02:06 +0400, Sergei Shtylyov wrote:
> > 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.
> 
>     No. DMA is still diabled for the IDE core at this point. You need
> a real 
> ide_dma_on() method call to do it. 

Ah right... even worse :-)

Ben.