[PATCH 3/6] ide-pmac: PIO fixes

[PATCH 3/6] ide-pmac: PIO fixes

[Bartlomiej Zolnierkiewicz]

* Add { 0, 0 } entry to {kauai,shasta}_pio_timings[] so kauai_lookup_timing()
  always returns a valid PIO timing (fixes PIO timing not being set for devices
  with minimum PIO cycle <= 120ns).

* Add setting transfer mode on the device to pmac_ide_set_pio_mode().

* Fix pmac_ide_set_pio() to always program chipset for given PIO timing instead
  of only when the device we want to program PIO timing for is the currently
  selected one.

* Now that pmac_ide_set_pio() is fixed there is no need to set transfer mode
  on the device and program chipset for PIO in pmac_ide_tune_chipset()
  (returning 0 == success is not entirely correct but is OK for now since
  the upper layers are only checking ->speedproc return value for DMA modes).

This patch should have no effect on the default kernel behavior because
IDE pmac driver doesn't enable ->autotune (this would also explain why some
of the above bugs remained unfixed for so long).

Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Kyle McMartin <kyle@mcmartin.ca>
---
 drivers/ide/ppc/pmac.c |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

Index: b/drivers/ide/ppc/pmac.c
===================================================================
--- a/drivers/ide/ppc/pmac.c
+++ b/drivers/ide/ppc/pmac.c
@@ -311,7 +311,8 @@ static struct kauai_timing	kauai_pio_tim
 	{ 240	, 0x0800038b },
 	{ 239	, 0x0800030c },
 	{ 180	, 0x05000249 },
-	{ 120	, 0x04000148 }
+	{ 120	, 0x04000148 },
+	{ 0	, 0 },
 };
 
 static struct kauai_timing	kauai_mdma_timings[] =
@@ -351,7 +352,8 @@ static struct kauai_timing	shasta_pio_ti
 	{ 240	, 0x040003cd },
 	{ 239	, 0x040003cd },
 	{ 180	, 0x0400028b },
-	{ 120	, 0x0400010a }
+	{ 120	, 0x0400010a },
+	{ 0	, 0 },
 };
 
 static struct kauai_timing	shasta_mdma_timings[] =
@@ -631,8 +633,6 @@ 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);
-		if (tr == 0)
-			return;
 		*timings = ((*timings) & ~TR_133_PIOREG_PIO_MASK) | tr;
 		break;
 		}
@@ -640,8 +640,6 @@ pmac_ide_set_pio_mode(ide_drive_t *drive
 	case controller_k2_ata6: {
 		/* 100Mhz cell */
 		u32 tr = kauai_lookup_timing(kauai_pio_timings, cycle_time);
-		if (tr == 0)
-			return;
 		*timings = ((*timings) & ~TR_100_PIOREG_PIO_MASK) | tr;
 		break;
 		}
@@ -692,8 +690,11 @@ pmac_ide_set_pio_mode(ide_drive_t *drive
 		drive->name, pio,  *timings);
 #endif	
 
-	if (drive->select.all == HWIF(drive)->INB(IDE_SELECT_REG))
-		pmac_ide_do_update_timings(drive);
+	if (pmac_ide_do_setfeature(drive, speed))
+		return;
+
+	pmac_ide_do_update_timings(drive);
+	drive->current_speed = speed;
 }
 
 #ifdef CONFIG_BLK_DEV_IDEDMA_PMAC
@@ -962,7 +963,7 @@ static int pmac_ide_tune_chipset(ide_dri
 		case XFER_PIO_1:
 		case XFER_PIO_0:
 			pmac_ide_set_pio_mode(drive, speed & 0x07);
-			break;
+			return 0;
 		default:
 			ret = 1;
 	}

Re: [PATCH 3/6] ide-pmac: PIO fixes

[Bartlomiej Zolnierkiewicz]

On Wednesday 11 July 2007, Bartlomiej Zolnierkiewicz wrote:
> 
> * Add { 0, 0 } entry to {kauai,shasta}_pio_timings[] so kauai_lookup_timing()
>   always returns a valid PIO timing (fixes PIO timing not being set for devices
>   with minimum PIO cycle <= 120ns).
> 
> * Add setting transfer mode on the device to pmac_ide_set_pio_mode().
> 
> * Fix pmac_ide_set_pio() to always program chipset for given PIO timing instead
>   of only when the device we want to program PIO timing for is the currently
>   selected one.
> 
> * Now that pmac_ide_set_pio() is fixed there is no need to set transfer mode
>   on the device and program chipset for PIO in pmac_ide_tune_chipset()
>   (returning 0 == success is not entirely correct but is OK for now since
>   the upper layers are only checking ->speedproc return value for DMA modes).


> This patch should have no effect on the default kernel behavior because
> IDE pmac driver doesn't enable ->autotune (this would also explain why some
> of the above bugs remained unfixed for so long).

This part of the patch description is actually incorrect: ->tuneproc is used
unconditionally during suspend so the patch has the real effect (hopefully
positive :) on the default kernel behavior.

> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Kyle McMartin <kyle@mcmartin.ca>
> ---
>  drivers/ide/ppc/pmac.c |   19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> Index: b/drivers/ide/ppc/pmac.c
> ===================================================================
> --- a/drivers/ide/ppc/pmac.c
> +++ b/drivers/ide/ppc/pmac.c
> @@ -311,7 +311,8 @@ static struct kauai_timing	kauai_pio_tim
>  	{ 240	, 0x0800038b },
>  	{ 239	, 0x0800030c },
>  	{ 180	, 0x05000249 },
> -	{ 120	, 0x04000148 }
> +	{ 120	, 0x04000148 },
> +	{ 0	, 0 },
>  };
>  
>  static struct kauai_timing	kauai_mdma_timings[] =
> @@ -351,7 +352,8 @@ static struct kauai_timing	shasta_pio_ti
>  	{ 240	, 0x040003cd },
>  	{ 239	, 0x040003cd },
>  	{ 180	, 0x0400028b },
> -	{ 120	, 0x0400010a }
> +	{ 120	, 0x0400010a },
> +	{ 0	, 0 },
>  };
>  
>  static struct kauai_timing	shasta_mdma_timings[] =
> @@ -631,8 +633,6 @@ 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);
> -		if (tr == 0)
> -			return;
>  		*timings = ((*timings) & ~TR_133_PIOREG_PIO_MASK) | tr;
>  		break;
>  		}
> @@ -640,8 +640,6 @@ pmac_ide_set_pio_mode(ide_drive_t *drive
>  	case controller_k2_ata6: {
>  		/* 100Mhz cell */
>  		u32 tr = kauai_lookup_timing(kauai_pio_timings, cycle_time);
> -		if (tr == 0)
> -			return;
>  		*timings = ((*timings) & ~TR_100_PIOREG_PIO_MASK) | tr;
>  		break;
>  		}
> @@ -692,8 +690,11 @@ pmac_ide_set_pio_mode(ide_drive_t *drive
>  		drive->name, pio,  *timings);
>  #endif	
>  
> -	if (drive->select.all == HWIF(drive)->INB(IDE_SELECT_REG))
> -		pmac_ide_do_update_timings(drive);
> +	if (pmac_ide_do_setfeature(drive, speed))
> +		return;
> +
> +	pmac_ide_do_update_timings(drive);
> +	drive->current_speed = speed;
>  }
>  
>  #ifdef CONFIG_BLK_DEV_IDEDMA_PMAC
> @@ -962,7 +963,7 @@ static int pmac_ide_tune_chipset(ide_dri
>  		case XFER_PIO_1:
>  		case XFER_PIO_0:
>  			pmac_ide_set_pio_mode(drive, speed & 0x07);
> -			break;
> +			return 0;
>  		default:
>  			ret = 1;
>  	}
> 

Re: [PATCH 3/6] ide-pmac: PIO fixes

[Benjamin Herrenschmidt]

> > This patch should have no effect on the default kernel behavior because
> > IDE pmac driver doesn't enable ->autotune (this would also explain why some
> > of the above bugs remained unfixed for so long).
> 
> This part of the patch description is actually incorrect: ->tuneproc is used
> unconditionally during suspend so the patch has the real effect (hopefully
> positive :) on the default kernel behavior.

I need to spend some time looking in details and testing. Thanks for
doing it though. I'll try to get through it next week. Please don't
hesitate to ping me if you don't hear back from me soon.

Ben.

Re: [PATCH 3/6] ide-pmac: PIO fixes

[Sergei Shtylyov]

Hello.

Bartlomiej Zolnierkiewicz wrote:

> * Add { 0, 0 } entry to {kauai,shasta}_pio_timings[] so kauai_lookup_timing()
>   always returns a valid PIO timing (fixes PIO timing not being set for devices
>   with minimum PIO cycle <= 120ns).

    Ugh... the way those tables are following each other, the driver should be 
programming MWDMA2 timings instead. :-/

> * Add setting transfer mode on the device to pmac_ide_set_pio_mode().

> * Fix pmac_ide_set_pio() to always program chipset for given PIO timing instead
>   of only when the device we want to program PIO timing for is the currently
>   selected one.

    Hm, why this was necessary?
    AFAIU, pmac_ide_do_setfeature() will cause selectproc() to be called 
anyway, via SELECT_DRIVE()...

> * Now that pmac_ide_set_pio() is fixed there is no need to set transfer mode
>   on the device and program chipset for PIO in pmac_ide_tune_chipset()

    BTW, I'm also not seeing much sense in calling 
pmac_ide_do_update_timings() from there as well since pmac_ide_do_setfeature() 
is called before that anyway.

>   (returning 0 == success is not entirely correct but is OK for now since
>   the upper layers are only checking ->speedproc return value for DMA modes).

> This patch should have no effect on the default kernel behavior because
> IDE pmac driver doesn't enable ->autotune (this would also explain why some
> of the above bugs remained unfixed for so long).

> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

Acked-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

MBR, Sergei

Re: [PATCH 3/6] ide-pmac: PIO fixes

[Bartlomiej Zolnierkiewicz]

On Friday 13 July 2007, Sergei Shtylyov wrote:
> Hello.
> 
> Bartlomiej Zolnierkiewicz wrote:
> 
> > * Add { 0, 0 } entry to {kauai,shasta}_pio_timings[] so kauai_lookup_timing()
> >   always returns a valid PIO timing (fixes PIO timing not being set for devices
> >   with minimum PIO cycle <= 120ns).
> 
>     Ugh... the way those tables are following each other, the driver should be 
> programming MWDMA2 timings instead. :-/

Indeed... nasty...

> > * Add setting transfer mode on the device to pmac_ide_set_pio_mode().
> 
> > * Fix pmac_ide_set_pio() to always program chipset for given PIO timing instead
> >   of only when the device we want to program PIO timing for is the currently
> >   selected one.
> 
>     Hm, why this was necessary?

No idea...

...(mis)optimization? :)

>     AFAIU, pmac_ide_do_setfeature() will cause selectproc() to be called 
> anyway, via SELECT_DRIVE()...

Yes, but pmac_ide_do_setfeature() wasn't called et all in the old version
of pmac_ide_set_pio().

> > * Now that pmac_ide_set_pio() is fixed there is no need to set transfer mode
> >   on the device and program chipset for PIO in pmac_ide_tune_chipset()
> 
>     BTW, I'm also not seeing much sense in calling 
> pmac_ide_do_update_timings() from there as well since pmac_ide_do_setfeature() 
> is called before that anyway.

The patch only intended to make pmac_ide_set_pio() match the code in
pmac_ide_tune_chipset().  Thanks to your analysis I see now that more
fixups/cleanups are possible in this driver but I'm not up to it...

> >   (returning 0 == success is not entirely correct but is OK for now since
> >   the upper layers are only checking ->speedproc return value for DMA modes).
> 
> > This patch should have no effect on the default kernel behavior because
> > IDE pmac driver doesn't enable ->autotune (this would also explain why some
> > of the above bugs remained unfixed for so long).
> 
> > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> 
> Acked-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

added

Re: [PATCH 3/6] ide-pmac: PIO fixes

[Sergei Shtylyov]

Hello.

Bartlomiej Zolnierkiewicz wrote:

>>>* Add setting transfer mode on the device to pmac_ide_set_pio_mode().

>>>* Fix pmac_ide_set_pio() to always program chipset for given PIO timing instead
>>>  of only when the device we want to program PIO timing for is the currently
>>>  selected one.

>>    Hm, why this was necessary?

> No idea...

    I may understand why this was needed beore the patch but why keep it?

> ...(mis)optimization? :)

>>    AFAIU, pmac_ide_do_setfeature() will cause selectproc() to be called 
>>anyway, via SELECT_DRIVE()...

> Yes, but pmac_ide_do_setfeature() wasn't called et all in the old version
> of pmac_ide_set_pio().

    But now it is.

>>>* Now that pmac_ide_set_pio() is fixed there is no need to set transfer mode
>>>  on the device and program chipset for PIO in pmac_ide_tune_chipset()

>>    BTW, I'm also not seeing much sense in calling 
>>pmac_ide_do_update_timings() from there as well since pmac_ide_do_setfeature() 
>>is called before that anyway.

> The patch only intended to make pmac_ide_set_pio() match the code in
> pmac_ide_tune_chipset().  Thanks to your analysis I see now that more
> fixups/cleanups are possible in this driver but I'm not up to it...

    :-)

MBR, Sergei