Hello again. :-)
Bartlomiej Zolnierkiewicz wrote:
* Add ide_set_pio_mode()/ide_set_dma_mode() helpers and convert all
direct ->set_pio_mode/->speedproc users to use these helpers.
* Move ide_config_drive_speed() calls from ->set_pio_mode/->speedproc
methods to callers.
* Rename ->speedproc method to ->set_dma_mode, make it void and update
all implementations accordingly.
* Update ide_set_xfer_rate() comments.
Except removal of two debugging printk-s (from cs5530.c and sc1200.c)
and the fact that transfer modes 0x00-0x07 passed from user space may
be programmed twice on the device (not really an issue since 0x00-0x01
are handled by very few host drivers and 0x02-0x07 are simply invalid)
there should be no other functionality changes caused by this patch.
Haven't see any driver handling 0x01. 0x00 is usually handled by setting
Maybe I haven't looked too hard though... :-)
Index: b/drivers/ide/ide-lib.c
===================================================================
--- a/drivers/ide/ide-lib.c
+++ b/drivers/ide/ide-lib.c
@@ -349,7 +349,7 @@ void ide_set_pio(ide_drive_t *drive, u8
[...]
+
+ /*
+ * TODO: temporary hack for some legacy host drivers that didn't
+ * set transfer mode on the device in ->set_pio_mode method...
+ */
+ if (hwif->set_dma_mode == NULL) {
+ hwif->set_pio_mode(drive, mode - XFER_PIO_0);
+ return 0;
+ }
Er... I didn't quite get it. :-/
You mean those that are still unfixed WRT not calling
ide_config_drive_speed()?
Yes.
Ah, so you're emulating the current behaviour with this...
+
+ if (hwif->host_flags & IDE_HFLAG_POST_SET_MODE) {
+ if (ide_config_drive_speed(drive, mode))
+ return -1;
+ hwif->set_pio_mode(drive, mode - XFER_PIO_0);
+ return 0;
+ } else {
+ hwif->set_pio_mode(drive, mode - XFER_PIO_0);
+ return ide_config_drive_speed(drive, mode);
+ }
+}
+
+int ide_set_dma_mode(ide_drive_t *drive, const u8 mode)
+{
+ ide_hwif_t *hwif = drive->hwif;
+
+ if (hwif->host_flags & IDE_HFLAG_NO_SET_MODE)
+ return 0;
I suggest that this be replaced by:
if (hwif->set_dma_mode == NULL)
return -1;
Done.
This alone would make ide_tune_dma() fail on it821x in smart mode
but moving IDE_HFLAG_NO_SET_MODE checking there fixes the issue.
Ah, I hadn't thought about all the consequencies... :-)
+ if (hwif->host_flags & IDE_HFLAG_POST_SET_MODE) {
+ if (ide_config_drive_speed(drive, mode))
+ return -1;
+ hwif->set_dma_mode(drive, mode);
+ return 0;
+ } else {
+ hwif->set_dma_mode(drive, mode);
+ return ide_config_drive_speed(drive, mode);
+ }
+}
+
/**
* ide_set_xfer_rate - set transfer rate
* @drive: drive to set
- * @speed: speed to attempt to set
+ * @rate: speed to attempt to set
*
* General helper for setting the speed of an IDE device. This
* function knows about user enforced limits from the configuration
- * which speedproc() does not. High level drivers should never
- * invoke speedproc() directly.
+ * which ->set_pio_mode/->set_dma_mode does not.
*/
-
+
int ide_set_xfer_rate(ide_drive_t *drive, u8 rate)
{
ide_hwif_t *hwif = drive->hwif;
- if (hwif->speedproc == NULL)
+ if (hwif->set_dma_mode == NULL)
return -1;
rate = ide_rate_filter(drive, rate);
if (rate >= XFER_PIO_0 && rate <= XFER_PIO_5) {
if (hwif->set_pio_mode)
Won't be needed if we'll check it inside ide_set_pio_mode().
- hwif->set_pio_mode(drive, rate - XFER_PIO_0);
+ return ide_set_pio_mode(drive, rate);
- /*
- * FIXME: this is incorrect to return zero here but
- * since all users of ide_set_xfer_rate() ignore
- * the return value it is not a problem currently
- */
- return 0;
+ return -1;
}
- return hwif->speedproc(drive, rate);
+ /*
+ * TODO: transfer modes 0x00-0x07 passed from the user-space are
+ * currently handled here which needs fixing (please note that such
+ * case could happen iff the transfer mode has already been set on
+ * the device by ide-proc.c::set_xfer_rate()).
+ */
Would be quite easy to hook and *properly* handle mode 0x00 here, however
mode 0x01 would certainly be much trickier -- unless we'd want to delegate it
to set_pio_mode() itself (I'm not suggesting it though :-)...
We do want (patches are welcomed). :-)
Erm, actually the preferrable way would be to handle 0x00/0x01 in some
generic manner -- and I was talking about the drivers' set_pio_mode() methods.
Handling 0x00 properly in ide_set_pio()/ide_set_pio_mode() would allow us to
handle non-IORDY devices correctly without resorting to special hacks.
Handling 0x01 correctly would be quite a hack in itself. :-)
Index: b/drivers/ide/pci/piix.c
===================================================================
--- a/drivers/ide/pci/piix.c
+++ b/drivers/ide/pci/piix.c
[...]
@@ -288,9 +273,7 @@ static int piix_tune_chipset(ide_drive_t
pci_write_config_byte(dev, 0x55, (u8) reg55 & ~w_flag);
}
- piix_tune_pio(drive, piix_dma_2_pio(speed));
-
- return ide_config_drive_speed(drive, speed);
+ piix_set_pio_mode(drive, piix_dma_2_pio(speed));
Hm, I remember some earlier patches which have changed this code...
Earlier patches removed *_dma_2_pio() calls for PIO modes.
I'll post patches to completely remove *_ dma_2_pio() shortly.
Well, you have, and that patch looked half-baked to me. ;-)
Index: b/drivers/ide/ppc/pmac.c
===================================================================
--- a/drivers/ide/ppc/pmac.c
+++ b/drivers/ide/ppc/pmac.c
[...]
@@ -1143,7 +1130,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->host_flags = IDE_HFLAG_SET_PIO_MODE_KEEP_DMA;
+ hwif->host_flags = IDE_HFLAG_SET_PIO_MODE_KEEP_DMA |
+ IDE_HFLAG_POST_SET_MODE,
Er... have you tried to compile this before posting? ;-)
This? No. ;-)
Fixed.
[PATCH] ide: move ide_config_drive_speed() calls to upper layers (take 2)
* Convert {ide_hwif_t,ide_pci_device_t}->host_flag to be u16.
* Add IDE_HFLAG_POST_SET_MODE host to indicate the need to program the
host for the transfer mode after programming the device. Set it in
au1xxx-ide, amd74xx, cs5530, cs5535, pdc202xx_new, sc1200, pmac and
via82cxxx host drivers.
* Add IDE_HFLAG_NO_SET_MODE host flags to indicate the need to completely
skip programming of host/device for the transfer mode ("smart" hosts).
Set it in it821x host driver and check it in ide_tune_dma().
* Add ide_set_pio_mode()/ide_set_dma_mode() helpers and convert all
direct ->set_pio_mode/->speedproc users to use these helpers.
* Move ide_config_drive_speed() calls from ->set_pio_mode/->speedproc
methods to callers.
* Rename ->speedproc method to ->set_dma_mode, make it void and update
all implementations accordingly.
* Update ide_set_xfer_rate() comments.
* Unexport ide_config_drive_speed().
v2:
* Fix issues noticed by Sergei:
- export ide_set_dma_mode() instead of moving ->set_pio_mode abuse wrt
to setting DMA modes from sc1200_set_pio_mode() to do_special()
- check IDE_HFLAG_NO_SET_MODE in ide_tune_dma()
- check for (hwif->set_pio_mode) == NULL in ide_set_pio_mode()
- check for (hwif->set_dma_mode) == NULL in ide_set_dma_mode()
- return -1 from ide_set_{pio,dma}_mode() if ->set_{pio,dma}_mode == NULL
- don't set ->set_{pio,dma}_mode on it821x in "smart" mode
- fix build problem in pmac.c
- minor fixes in au1xxx-ide.c/cs5530.c/siimage.c
- improve patch description
Changes in behavior caused by this patch:
- HDIO_SET_PIO_MODE ioctl would now return -ENOSYS for attempts to change
PIO mode if it821x controller is in "smart" mode
- removal of two debugging printk-s (from cs5530.c and sc1200.c)
- transfer modes 0x00-0x07 passed from user space may be programmed twice on
the device (not really an issue since 0x00 is not supported correctly by
any host driver ATM, 0x01 is not supported et all and 0x02-0x07 are invalid)
Erm, may I insert a grammatical nit here? :-)
It's either "at all" or "et al." (meaning something completely different)
and your hybrid variant would mean "and all" if we treat "et" as a Latin word
(well, it's certainly not an Emglish word anyway ;-)...
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
