Hello,
On Sunday 24 June 2007, Sergei Shtylyov wrote:
> >>>Index: b/drivers/ide/pci/sl82c105.c
> >>>===================================================================
> >>>--- a/drivers/ide/pci/sl82c105.c
> >>>+++ b/drivers/ide/pci/sl82c105.c
> >>>@@ -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;
>
> >>>- 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 &&. :-<
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.
> >>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.
> > 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.
> >>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()
>
> 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 <bzolnier@xxxxxxxxx>
Acked-by: Sergei Shtylyov <sshtylyov@xxxxxxxxxxxxx>
---
drivers/ide/ide-lib.c | 7 +------
drivers/ide/pci/sl82c105.c | 10 +++++++---
include/linux/ide.h | 6 +++++-
3 files changed, 13 insertions(+), 10 deletions(-)
Index: b/drivers/ide/ide-lib.c
===================================================================
--- a/drivers/ide/ide-lib.c
+++ b/drivers/ide/ide-lib.c
@@ -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 */
@@ -286,8 +283,7 @@ u8 ide_get_best_pio_mode (ide_drive_t *d
overridden = 1;
}
if (id->field_valid & 2) { /* drive implements ATA2? */
- if (id->capability & 8) { /* drive supports use_iordy? */
- use_iordy = 1;
+ if (id->capability & 8) { /* IORDY supported? */
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
===================================================================
--- a/drivers/ide/pci/sl82c105.c
+++ b/drivers/ide/pci/sl82c105.c
@@ -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;
- 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;
+
+ 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
===================================================================
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -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 {
@@ -1374,7 +1379,6 @@ typedef struct ide_pio_timings_s {
typedef struct ide_pio_data_s {
u8 pio_mode;
- u8 use_iordy;
unsigned int cycle_time;
} ide_pio_data_t;
-
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
