Re: [PATCH] ide: move ide_config_drive_speed() calls to upper layers
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 [EMAIL PROTECTED]
Acked-by: Sergei Shtylyov [EMAIL PROTECTED]
MBR, Sergei
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ide: move ide_config_drive_speed() calls to upper layers
Hello. Bartlomiej Zolnierkiewicz wrote: I have a feeling that only pdc202xx_new and jmicron drivers actually need this flag (and the latter one actually doesn't care as its methods are empty anyway). Well, AMD/VIA chips enable UltraDMA mode by snooping Set Features command (as the drivers tell them to do so), so the order seems important unless that is changed. I've mixed it up (again :-): setting the MSB of the UltraDMA registers should be disabling Set Features snooping, so still the flag doesn't seem needed... Since changing of order affects the way in which hardware is accessed I prefer to keep such changes out of this patch (which is just a cleanup). I understand. Could you please handle them in pre or post patch(es)? More likely in post-patches -- I'll probably be out-of-office for the next several weeks (only appearing at weekends). Bart WBR, Sergei - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ide: move ide_config_drive_speed() calls to upper layers
Hi,
On Saturday 28 July 2007, Sergei Shtylyov wrote:
Hello.
Bartlomiej Zolnierkiewicz wrote:
On Fri, 27 Jul 2007 02:22:27 +0200
Bartlomiej Zolnierkiewicz [EMAIL PROTECTED] wrote:
* 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/cs5530/cs5535/pdc202xx_new/sc1200/via82cxxx host drivers.
The CS5530 at least shouldn't care what order changes are done. I don't
And neither CS5535. And Au1200 static bus controller shouldn't care about
the order too, so au1xxx-ide hardly needs that.
Here's the datasheet, BTW:
http://www.razamicroelectronics.com/documents/32798e_Au1200_db.pdf
think the SC1200 does either but I don't have the docs to hand.
It seems pretty much alike CS553x except it's accessed via PCI config.
space, not MSRs... Here's the datasheet:
http://www.amd.com/files/connectivitysolutions/geode/32579B_sc1200_ds.pdf
Thanks for links to specs.
I have a feeling that only pdc202xx_new and jmicron drivers actually need
this flag (and the latter one actually doesn't care as its methods are empty
anyway). Well, AMD/VIA chips enable UltraDMA mode by snooping Set Features
command (as the drivers tell them to do so), so the order seems important
unless that is changed.
Since changing of order affects the way in which hardware is accessed
I prefer to keep such changes out of this patch (which is just a cleanup).
Could you please handle them in pre or post patch(es)?
Bart
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ide: move ide_config_drive_speed() calls to upper layers
Hello again. :-)
Bartlomiej Zolnierkiewicz wrote:
* 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/cs5530/cs5535/pdc202xx_new/sc1200/via82cxxx host drivers.
Well, I've already commented about the necessity of this flag...
* 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.
Now this flag seems completely artificial to me. I suggest instead to
just not install the set_{pio|dma}_mode() methods at the driver startup when
the chips are in smart mode.
* Handle -set_pio_mode abuse wrt to setting DMA modes in do_special()
instead of sc1200.c::sc1200_set_pio_mode().
I'm not sure it was a great idea to carry the code specific to only one
driver into the generic code. Why not leave it where it was?
* 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
PIO0 or even slower mode which isn't quite correct.
Signed-off-by: Bartlomiej Zolnierkiewicz [EMAIL PROTECTED]
Index: b/drivers/ide/ide-io.c
===
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -834,6 +834,26 @@ static ide_startstop_t do_special (ide_d
s-b.set_tune = 0;
if (set_pio_mode_abuse(drive-hwif, req_pio)) {
+ int mode = -1;
+
+ switch (req_pio) {
+ case 200: mode = XFER_UDMA_0; break;
+ case 201: mode = XFER_UDMA_1; break;
+ case 202: mode = XFER_UDMA_2; break;
+ case 100: mode = XFER_MW_DMA_0; break;
+ case 101: mode = XFER_MW_DMA_1; break;
+ case 102: mode = XFER_MW_DMA_2; break;
+ }
+
+ if (mode != -1) {
+ printk(KERN_INFO %s: changing (U)DMA mode\n,
+drive-name);
+ hwif-dma_off_quietly(drive);
+ if (ide_set_dma_mode(drive, mode) == 0)
+ hwif-dma_host_on(drive);
+ return ide_stopped;
+ }
+
So, I'm doubtful about this part. Could you clarify why/whether you deem
it necassary?
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
drive-name, host_pio, req_pio,
req_pio == 255 ? (auto-tune) : , pio);
- hwif-set_pio_mode(drive, pio);
+ (void)ide_set_pio_mode(drive, XFER_PIO_0 + pio);
}
EXPORT_SYMBOL_GPL(ide_set_pio);
@@ -378,39 +378,85 @@ void ide_toggle_bounce(ide_drive_t *driv
blk_queue_bounce_limit(drive-queue, addr);
}
+int ide_set_pio_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 think that we should not a lie to a user if we can *not* set the
transfer mode that he requested. So, I suggest that this be replaced by:
if (hwif-set_pio_mode == NULL)
return -1;
+
+ /*
+* 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()?
+
+ 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 -
Re: [PATCH] ide: move ide_config_drive_speed() calls to upper layers
On Friday 27 July 2007, Alan Cox wrote:
On Fri, 27 Jul 2007 02:22:27 +0200
Bartlomiej Zolnierkiewicz [EMAIL PROTECTED] wrote:
* 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/cs5530/cs5535/pdc202xx_new/sc1200/via82cxxx host drivers.
The CS5530 at least shouldn't care what order changes are done. I don't
think the SC1200 does either but I don't have the docs to hand.
Thanks, I will make a follow up patch to remove this flag from
cs5530 host driver (unless of course somebody beats me to it).
For libata we went the post_set_mode way and then junked it. Instead
there is a -set_mode method which defaults to the usual order and
actions. IT821x and similar override it to do very little, post_set_mode
people call the default method and then do their stuff, and some of the
crazier cases do stuff both before and after the default.
Much more flexible and it can do anything unlike flags for the cases you
have.
I view this patch as an intermediate step in -set_mode direction.
Thanks,
Bart
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ide: move ide_config_drive_speed() calls to upper layers
Hello.
Bartlomiej Zolnierkiewicz wrote:
On Fri, 27 Jul 2007 02:22:27 +0200
Bartlomiej Zolnierkiewicz [EMAIL PROTECTED] wrote:
* 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/cs5530/cs5535/pdc202xx_new/sc1200/via82cxxx host drivers.
The CS5530 at least shouldn't care what order changes are done. I don't
And neither CS5535. And Au1200 static bus controller shouldn't care about
the order too, so au1xxx-ide hardly needs that.
Here's the datasheet, BTW:
http://www.razamicroelectronics.com/documents/32798e_Au1200_db.pdf
think the SC1200 does either but I don't have the docs to hand.
It seems pretty much alike CS553x except it's accessed via PCI config.
space, not MSRs... Here's the datasheet:
http://www.amd.com/files/connectivitysolutions/geode/32579B_sc1200_ds.pdf
I have a feeling that only pdc202xx_new and jmicron drivers actually need
this flag (and the latter one actually doesn't care as its methods are empty
anyway). Well, AMD/VIA chips enable UltraDMA mode by snooping Set Features
command (as the drivers tell them to do so), so the order seems important
unless that is changed.
Thanks,
Bart
MBR, Sergei
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ide: move ide_config_drive_speed() calls to upper layers
On Fri, 27 Jul 2007 02:22:27 +0200
Bartlomiej Zolnierkiewicz [EMAIL PROTECTED] wrote:
* 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/cs5530/cs5535/pdc202xx_new/sc1200/via82cxxx host drivers.
The CS5530 at least shouldn't care what order changes are done. I don't
think the SC1200 does either but I don't have the docs to hand.
For libata we went the post_set_mode way and then junked it. Instead
there is a -set_mode method which defaults to the usual order and
actions. IT821x and similar override it to do very little, post_set_mode
people call the default method and then do their stuff, and some of the
crazier cases do stuff both before and after the default.
Much more flexible and it can do anything unlike flags for the cases you
have.
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ide: move ide_config_drive_speed() calls to upper layers
On Friday 27 July 2007, Bartlomiej Zolnierkiewicz wrote: * 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/cs5530/cs5535/pdc202xx_new/sc1200/via82cxxx host drivers. and amd74xx :) - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ide: move ide_config_drive_speed() calls to upper layers
On Friday 27 July 2007, Bartlomiej Zolnierkiewicz wrote: On Friday 27 July 2007, Bartlomiej Zolnierkiewicz wrote: * 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/cs5530/cs5535/pdc202xx_new/sc1200/via82cxxx host drivers. and amd74xx :) and pmac :) :) now time for some sleep... Bart - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
