Re: ide patches

[Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>]

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