Hello. Bartlomiej Zolnierkiewicz 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).
Indeed, it may... BUT it may not support PIO > 2 (since this also requires the "IORDY supported" bit set).
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>)
Nah, that wouldn't match to the ATA definition of these values.
- check for speed == XFER_PIO_[0,6] in ide-lib.c::ide_config_drive_speed()
It's in ide-iops.c. ;-)
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.
This should be done together with fixing these host drivers that don't handle IORDY properly.
Erm, not necessarily...
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.
Sigh, I'm trying to get some time (more like time slices :-) off to deal with my own issues...
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".
Thanks.
[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> MBR, Sergei - 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
