ide: add ata_dev_has_iordy() helper

* Add ata_dev_has_iordy() helper.

* Use it in sl82c105 host driver so it always gets the correct info (use_iordy was incorrectly set for "pio" != 255 cases).

* Remove no longer needed ide_pio_data_t.use_iordy field.

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

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

drivers/ide/ide-lib.c +0 -3

@@ -267,18 +267,15 @@ u8 ide_get_best_pio_mode (ide_drive_t *d
 {
 	int pio_mode;
 	int cycle_time = 0;
-	int use_iordy = 0;
 	struct hd_driveid* id = drive->id;
 	int overridden  = 0;
 
 	if (mode_wanted != 255) {
 		pio_mode = mode_wanted;
-		use_iordy = (pio_mode > 2);
 	} else if (!drive->id) {
 		pio_mode = 0;
 	} else if ((pio_mode = ide_scan_pio_blacklist(id->model)) != -1) {
 		printk(KERN_INFO "%s: is on PIO blacklist\n", drive->name);
-		use_iordy = (pio_mode > 2);
 	} else {
 		pio_mode = id->tPIO;
 		if (pio_mode > 2) {	/* 2 is maximum allowed tPIO value */

drivers/ide/ide-lib.c +0 -2

-				use_iordy = 1;
 				cycle_time = id->eide_pio_iordy;
 				if (id->eide_pio_modes & 7) {
 					overridden = 0;

@@ -325,7 +321,6 @@ u8 ide_get_best_pio_mode (ide_drive_t *d
 	if (d) {
 		d->pio_mode = pio_mode;
 		d->cycle_time = cycle_time ? cycle_time : ide_pio_timings[pio_mode].cycle_time;
-		d->use_iordy = use_iordy;
 	}
 	return pio_mode;
 }

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

drivers/ide/pci/sl82c105.c +2 -1

@@ -52,9 +52,10 @@
  * Convert a PIO mode and cycle time to the required on/off times
  * for the interface.  This has protection against runaway timings.
  */
-static unsigned int get_pio_timings(ide_pio_data_t *p)
+static unsigned int get_pio_timings(ide_drive_t *drive, ide_pio_data_t *p)
 {
 	unsigned int cmd_on, cmd_off;
+	u8 iordy = 0;
 
 	cmd_on  = (ide_pio_timings[p->pio_mode].active_time + 29) / 30;
 	cmd_off = (p->cycle_time - 30 * cmd_on + 29) / 30;

drivers/ide/pci/sl82c105.c +2 -1

+	return (cmd_on - 1) << 8 | (cmd_off - 1) | iordy;
 }
 
 /*

@@ -82,7 +86,7 @@ static u8 sl82c105_tune_pio(ide_drive_t 
 
 	pio = ide_get_best_pio_mode(drive, pio, 5, &p);
 
-	drv_ctrl = get_pio_timings(&p);
+	drv_ctrl = get_pio_timings(drive, &p);
 
 	/*
 	 * Store the PIO timings so that we can restore them

Index: b/include/linux/ide.h
===================================================================

include/linux/ide.h +5 -0

@@ -1363,6 +1363,11 @@ extern void ide_toggle_bounce(ide_drive_
 extern int ide_set_xfer_rate(ide_drive_t *drive, u8 rate);
 int ide_use_fast_pio(ide_drive_t *);
 
+static inline int ata_dev_has_iordy(struct hd_driveid *id)
+{
+	return ((id->field_valid & 2) && (id->capability & 8)) ? 1 : 0;
+}
+
 u8 ide_dump_status(ide_drive_t *, const char *, u8);
 
 typedef struct ide_pio_timings_s {

Bartlomiej Zolnierkiewicz wrote:

This one is actually arguable...

Quoted 1 line

* Add ata_dev_has_iordy() helper.

I thought IDE subsys make use of only 'ide_' prefixes. :-)

Quoted 2 lines

* Use it in sl82c105 host driver so it always gets the correct info (use_iordy was incorrectly set for "pio" != 255 cases).

Hm, why incorrectly? ATA specs say PIO3/4 *must* use IORDY flow control,
IIRC... :-/
Maybe you meant the mode is being later adjusted by max_mode thereshold --
well, that's a common issue in ide_get_best_pio_mode().

Quoted 25 lines

* Remove no longer needed ide_pio_data_t.use_iordy field.

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

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

drivers/ide/ide-lib.c +0 -3

@@ -267,18 +267,15 @@ u8 ide_get_best_pio_mode (ide_drive_t *d
 {
 	int pio_mode;
 	int cycle_time = 0;
-	int use_iordy = 0;
 	struct hd_driveid* id = drive->id;
 	int overridden  = 0;
 
 	if (mode_wanted != 255) {
 		pio_mode = mode_wanted;
-		use_iordy = (pio_mode > 2);
 	} else if (!drive->id) {
 		pio_mode = 0;
 	} else if ((pio_mode = ide_scan_pio_blacklist(id->model)) != -1) {
 		printk(KERN_INFO "%s: is on PIO blacklist\n", drive->name);
-		use_iordy = (pio_mode > 2);
 	} else {

drivers/ide/ide-lib.c u8 ide_get_best_pio_mode (ide_drive_t *d · line 286

Quoted 6 lines

drivers/ide/ide-lib.c +0 -0

 		pio_mode = id->tPIO;
 		if (pio_mode > 2) {	/* 2 is maximum allowed tPIO value */

@@ -287,7 +284,6 @@ u8 ide_get_best_pio_mode (ide_drive_t *d
 		}
 		if (id->field_valid & 2) {	  /* drive implements ATA2? */
 			if (id->capability & 8) { /* drive supports use_iordy? */

The comment above needs fixing too -- kill reference to use_iordy.

Quoted 32 lines

drivers/ide/ide-lib.c +0 -2

-				use_iordy = 1;
 				cycle_time = id->eide_pio_iordy;
 				if (id->eide_pio_modes & 7) {
 					overridden = 0;

@@ -325,7 +321,6 @@ u8 ide_get_best_pio_mode (ide_drive_t *d
 	if (d) {
 		d->pio_mode = pio_mode;
 		d->cycle_time = cycle_time ? cycle_time : ide_pio_timings[pio_mode].cycle_time;
-		d->use_iordy = use_iordy;
 	}
 	return pio_mode;
 }

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

drivers/ide/pci/sl82c105.c +2 -1

@@ -52,9 +52,10 @@
  * Convert a PIO mode and cycle time to the required on/off times
  * for the interface.  This has protection against runaway timings.
  */
-static unsigned int get_pio_timings(ide_pio_data_t *p)
+static unsigned int get_pio_timings(ide_drive_t *drive, ide_pio_data_t *p)
 {
 	unsigned int cmd_on, cmd_off;
+	u8 iordy = 0;
 
 	cmd_on  = (ide_pio_timings[p->pio_mode].active_time + 29) / 30;
 	cmd_off = (p->cycle_time - 30 * cmd_on + 29) / 30;

@@ -65,7 +66,10 @@ static unsigned int get_pio_timings(ide_
 	if (cmd_off == 0)
 		cmd_off = 1;
 

drivers/ide/pci/sl82c105.c static unsigned int get_pio_timings(ide_ · line 70

Quoted 3 lines

drivers/ide/pci/sl82c105.c +2 -1

-	return (cmd_on - 1) << 8 | (cmd_off - 1) | (p->use_iordy ? 0x40 : 0x00);
+	if (p->pio_mode > 2 || ata_dev_has_iordy(drive->id))
+		iordy = 0x40;

This logic, although mimicking the old one from ide_get_best_pio_mode(),
is not quite correct. As have been noted before, when you set a PIO mode using Set Transfer Mode subcode of the Set Features command, you're always selecting the flow control mode, i.e. using IORDY. So, the last condition in this if

stmt should probably be the first, if not the sole one...

Quoted 5 lines

Quoted 2 lines

* Use it in sl82c105 host driver so it always gets the correct info (use_iordy was incorrectly set for "pio" != 255 cases).

Hm, why incorrectly? ATA specs say PIO3/4 *must* use IORDY flow control,
IIRC... :-/

Quoted 19 lines

Quoted 19 lines

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

drivers/ide/pci/sl82c105.c +2 -1

@@ -52,9 +52,10 @@
 * Convert a PIO mode and cycle time to the required on/off times
 * for the interface.  This has protection against runaway timings.
 */
-static unsigned int get_pio_timings(ide_pio_data_t *p)
+static unsigned int get_pio_timings(ide_drive_t *drive, ide_pio_data_t *p)
 {
 	unsigned int cmd_on, cmd_off;
+	u8 iordy = 0;
 
 	cmd_on  = (ide_pio_timings[p->pio_mode].active_time + 29) / 30;
 	cmd_off = (p->cycle_time - 30 * cmd_on + 29) / 30;

@@ -65,7 +66,10 @@ static unsigned int get_pio_timings(ide_
 	if (cmd_off == 0)
 		cmd_off = 1;

Quoted 6 lines

drivers/ide/pci/sl82c105.c static unsigned int get_pio_timings(ide_ · line 70

Quoted 3 lines

drivers/ide/pci/sl82c105.c +2 -1

-	return (cmd_on - 1) << 8 | (cmd_off - 1) | (p->use_iordy ? 0x40 : 0x00);
+	if (p->pio_mode > 2 || ata_dev_has_iordy(drive->id))
+		iordy = 0x40;

This logic, although mimicking the old one from ide_get_best_pio_mode(),
is not quite correct. As have been noted before, when you set a PIO mode using

Quoted 1 line

Set Transfer Mode subcode of the Set Features command, you're always selecting the flow control mode, i.e. using IORDY. So, the last condition in this if

Oh yes, I keep forgetting about it - some nice FIXME comment in <linux/ata.h> would be of a great help. :-)

Quoted 1 line

stmt should probably be the first, if not the sole one...

Fixed, new patch below.

[PATCH] ide: add ata_dev_has_iordy() helper (take 2)

* Add ata_dev_has_iordy() helper and use it sl82c105 host driver.

* Remove no longer needed ide_pio_data_t.use_iordy field.

v2:
* Fix issues noticed by Sergei:
- correct patch description
- remove stale comment from ide_get_best_pio_mode()

- use only ata_dev_has_iordy() in sl82c105 host driver

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

Quoted 20 lines

Quoted 19 lines

Quoted 19 lines

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

drivers/ide/pci/sl82c105.c +2 -1

@@ -52,9 +52,10 @@
 * Convert a PIO mode and cycle time to the required on/off times
 * for the interface.  This has protection against runaway timings.
 */
-static unsigned int get_pio_timings(ide_pio_data_t *p)
+static unsigned int get_pio_timings(ide_drive_t *drive, ide_pio_data_t *p)
 {
 	unsigned int cmd_on, cmd_off;
+	u8 iordy = 0;
 
 	cmd_on  = (ide_pio_timings[p->pio_mode].active_time + 29) / 30;
 	cmd_off = (p->cycle_time - 30 * cmd_on + 29) / 30;

@@ -65,7 +66,10 @@ static unsigned int get_pio_timings(ide_
 	if (cmd_off == 0)
 		cmd_off = 1;

Quoted 6 lines

Quoted 6 lines

drivers/ide/pci/sl82c105.c static unsigned int get_pio_timings(ide_ · line 69

Quoted 3 lines

drivers/ide/pci/sl82c105.c +2 -1

-	return (cmd_on - 1) << 8 | (cmd_off - 1) | (p->use_iordy ? 0x40 : 0x00);
+	if (p->pio_mode > 2 || ata_dev_has_iordy(drive->id))
+		iordy = 0x40;

This logic, although mimicking the old one from ide_get_best_pio_mode(),
is not quite correct. As have been noted before, when you set a PIO mode using

It was actully correct enough, just superfluous -- it was me who was
incorrect, mistaking || for &&. :-<

Quoted 2 lines

Quoted 2 lines

Set Transfer Mode subcode of the Set Features command, you're always selecting the flow control mode, i.e. using IORDY. So, the last condition in this if

So, what actually would need fixing in *all* the drivers if one was aiming
at ATA-1 compatibility is *not* issuing that subcommand to such drives...

Quoted 2 lines

Oh yes, I keep forgetting about it - some nice FIXME comment in <linux/ata.h> would be of a great help. :-)

Well, some drivers (like pdc202xx_*) don't do the IORDY thing right for
PIO modes < 3 as well...

Quoted 7 lines

Quoted 1 line

stmt should probably be the first, if not the sole one...

Fixed, new patch below.

[PATCH] ide: add ata_dev_has_iordy() helper (take 2)

* Add ata_dev_has_iordy() helper and use it sl82c105 host driver.

Quoted 6 lines

* Remove no longer needed ide_pio_data_t.use_iordy field.

v2:
* Fix issues noticed by Sergei:
- correct patch description
- remove stale comment from ide_get_best_pio_mode()

I meant something like changing use_iordy to IORDY but it's good as is now...

Quoted 26 lines

Quoted 26 lines

Quoted 26 lines

Quoted 17 lines

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

drivers/ide/pci/sl82c105.c +2 -1

@@ -52,9 +52,10 @@
 * Convert a PIO mode and cycle time to the required on/off times
 * for the interface.  This has protection against runaway timings.
 */
-static unsigned int get_pio_timings(ide_pio_data_t *p)
+static unsigned int get_pio_timings(ide_drive_t *drive, ide_pio_data_t *p)
 {
 	unsigned int cmd_on, cmd_off;
+	u8 iordy = 0;
 
 	cmd_on  = (ide_pio_timings[p->pio_mode].active_time + 29) / 30;
 	cmd_off = (p->cycle_time - 30 * cmd_on + 29) / 30;

drivers/ide/pci/sl82c105.c static unsigned int get_pio_timings(ide_ · line 70

Quoted 6 lines

drivers/ide/pci/sl82c105.c +2 -1

@@ -65,7 +66,10 @@ static unsigned int get_pio_timings(ide_
 	if (cmd_off == 0)
 		cmd_off = 1;
 
-	return (cmd_on - 1) << 8 | (cmd_off - 1) | (p->use_iordy ? 0x40 : 0x00);
+	if (p->pio_mode > 2 || ata_dev_has_iordy(drive->id))
+		iordy = 0x40;

This logic, although mimicking the old one from ide_get_best_pio_mode(),
is not quite correct. As have been noted before, when you set a PIO mode using

Quoted 2 lines

It was actully correct enough, just superfluous -- it was me who was
incorrect, mistaking || for &&. :-<

After checking with ATA-2 spec I would prefer to leave extra p->pio_mode > 2 check (if ata_dev_has_iordy() fails device still _may_ support IORDY).

Fixed in "take 3" of the patch.

Quoted 4 lines

Quoted 1 line

Quoted 1 line

Set Transfer Mode subcode of the Set Features command, you're always selecting the flow control mode, i.e. using IORDY. So, the last condition in this if

So, what actually would need fixing in *all* the drivers if one was aiming
at ATA-1 compatibility is *not* issuing that subcommand to such drives...

I was actually thinking about a different way of fixing this:

- remove 0x08 bit from XFER_PIO_[0,6] defines and add new XFER_PIO_IORDY define (<linux/ata.h>)

- check for speed == XFER_PIO_[0,6] in ide-lib.c::ide_config_drive_speed()

and pmac.c::pmac_ide_do_setfeature(), add XFER_PIO_IORDY if needed

This should be done together with fixing these host drivers that don't handle IORDY properly.

Quoted 5 lines

Quoted 2 lines

Oh yes, I keep forgetting about it - some nice FIXME comment in <linux/ata.h> would be of a great help. :-)

Well, some drivers (like pdc202xx_*) don't do the IORDY thing right for
PIO modes < 3 as well...

Added to the existing IDE TODO at

http://kernel.org/pub/linux/kernel/people/bart/pata-2.6/TODO

Patches adding/removing items are welcomed.

Patches fixing actual issues are welcomed even more.

Quoted 9 lines

Quoted 9 lines

Quoted 1 line

stmt should probably be the first, if not the sole one...

Fixed, new patch below.

[PATCH] ide: add ata_dev_has_iordy() helper (take 2)

* Add ata_dev_has_iordy() helper and use it sl82c105 host driver.

* Remove no longer needed ide_pio_data_t.use_iordy field.

Quoted 6 lines

Quoted 4 lines

v2:
* Fix issues noticed by Sergei:
- correct patch description
- remove stale comment from ide_get_best_pio_mode()

I meant something like changing use_iordy to IORDY but it's good as is now...

Corrected in "take 3".

[PATCH] ide: add ata_dev_has_iordy() helper (take 3)

* Add ata_dev_has_iordy() helper and use it sl82c105 host driver.

* Remove no longer needed ide_pio_data_t.use_iordy field.

v2/v3:
* Fix issues noticed by Sergei:
- correct patch description
- fix comment in ide_get_best_pio_mode()

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

Quoted 10 lines

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

Set Transfer Mode subcode of the Set Features command, you're always selecting the flow control mode, i.e. using IORDY. So, the last condition in this if

So, what actually would need fixing in *all* the drivers if one was aiming
at ATA-1 compatibility is *not* issuing that subcommand to such drives...

I was actually thinking about a different way of fixing this:

- remove 0x08 bit from XFER_PIO_[0,6] defines and add new XFER_PIO_IORDY define (<linux/ata.h>)

Nah, that wouldn't match to the ATA definition of these values.

Quoted 1 line

- check for speed == XFER_PIO_[0,6] in ide-lib.c::ide_config_drive_speed()

    It's in ide-iops.c. ;-)

Quoted 1 line

and pmac.c::pmac_ide_do_setfeature(), add XFER_PIO_IORDY if needed

And what, just pass the mode thru to the Set Features if there's no IORDY
support? That would be bogus since there are just *no* subcodes to set the specific mode below 0x08 -- the only defined subcodes are 0x00 (set default mode), and 0x01 (set default mode w/o IORDY).

I was thinking of checking if the drive really supports IORDY before
issuing a command to set PIO mode (and just skipping the command if there's no IORDY -- well, maybe adding an extra check that the passed mode is acceptable to the drive, i.e. <= its default one). Should be quite simple to do.

Quoted 2 lines

This should be done together with fixing these host drivers that don't handle IORDY properly.

Erm, not necessarily...

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

Oh yes, I keep forgetting about it - some nice FIXME comment in <linux/ata.h> would be of a great help. :-)

Well, some drivers (like pdc202xx_*) don't do the IORDY thing right for
PIO modes < 3 as well...

Added to the existing IDE TODO at

Quoted 5 lines

http://kernel.org/pub/linux/kernel/people/bart/pata-2.6/TODO

Patches adding/removing items are welcomed.

Patches fixing actual issues are welcomed even more.

Sigh, I'm trying to get some time (more like time slices :-) off to deal
with my own issues...

Damn, I should have re-check ATA specs before writing this. :)

Quoted 2 lines

I was thinking of checking if the drive really supports IORDY before
issuing a command to set PIO mode (and just skipping the command if there's no IORDY -- well, maybe adding an extra check that the passed mode is acceptable to the drive, i.e. <= its default one). Should be quite simple to do.

Sounds fine.

Quoted 4 lines

Quoted 2 lines

This should be done together with fixing these host drivers that don't handle IORDY properly.

Erm, not necessarily...

Hmm, yes.

I'll just count on you with fixing all this IORDY stuff (as you have much more expertise in this field) and concentrate on other things.

Quoted 9 lines

Quoted 9 lines

Quoted 5 lines

Quoted 2 lines

Oh yes, I keep forgetting about it - some nice FIXME comment in <linux/ata.h> would be of a great help. :-)

Well, some drivers (like pdc202xx_*) don't do the IORDY thing right for
PIO modes < 3 as well...

Added to the existing IDE TODO at

http://kernel.org/pub/linux/kernel/people/bart/pata-2.6/TODO

Quoted 6 lines

Quoted 3 lines

Patches adding/removing items are welcomed.

Patches fixing actual issues are welcomed even more.

Sigh, I'm trying to get some time (more like time slices :-) off to deal
with my own issues...

Which reminds me about some HPT IDE patches... 8)

Bart

Hello.

Bartlomiej Zolnierkiewicz wrote:

[...]

Quoted 1 line

Damn, I should have re-check ATA specs before writing this. :)

Yeah, never hurts... but takes time. ;-)

Quoted 6 lines

Quoted 4 lines

I was thinking of checking if the drive really supports IORDY before
issuing a command to set PIO mode (and just skipping the command if there's no IORDY -- well, maybe adding an extra check that the passed mode is acceptable to the drive, i.e. <= its default one). Should be quite simple to do.

Sounds fine.

I'll look into doing this some time... :-)

Quoted 9 lines

Quoted 4 lines

Quoted 2 lines

This should be done together with fixing these host drivers that don't handle IORDY properly.

Erm, not necessarily...

Hmm, yes.

I'll just count on you with fixing all this IORDY stuff (as you have much more expertise in this field) and concentrate on other things.

I'm afraid this compliment is not well deserved. :-<

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

Quoted 2 lines

Oh yes, I keep forgetting about it - some nice FIXME comment in <linux/ata.h> would be of a great help. :-)

Well, some drivers (like pdc202xx_*) don't do the IORDY thing right for
PIO modes < 3 as well...

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Added to the existing IDE TODO at

http://kernel.org/pub/linux/kernel/people/bart/pata-2.6/TODO

I think this list already needs cleaning -- e.g. at least cmd64x.c and
hpt366.c have been deatl with.

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

Patches adding/removing items are welcomed.

Patches fixing actual issues are welcomed even more.

Sigh, I'm trying to get some time (more like time slices :-) off to deal
with my own issues...

By by "my own issues" I don't mean patches -- patches don't help me, it's
rather injections and pills... +:-)

Quoted 1 line

Which reminds me about some HPT IDE patches... 8)

Done now -- at last. However, I've had troubles with SMPT sending them. :-/

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

I was thinking of checking if the drive really supports IORDY before
issuing a command to set PIO mode (and just skipping the command if there's no IORDY -- well, maybe adding an extra check that the passed mode is acceptable to the drive, i.e. <= its default one). Should be quite simple to do.

Sounds fine.

I'll look into doing this some time... :-)

Cool.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 3 lines

Patches adding/removing items are welcomed.

Patches fixing actual issues are welcomed even more.

Sigh, I'm trying to get some time (more like time slices :-) off to deal
with my own issues...

Quoted 2 lines

By by "my own issues" I don't mean patches -- patches don't help me, it's
rather injections and pills... +:-)

Heh, sorry I didn't know about that.

I hope that it is nothing serious and wish you quick recovery...

So you could hack some more... :-)